[U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received

[Michal Suchánek]

On Tue, 2 Jul 2019 23:20:28 +0200
Marek Vasut <marex at denx.de> wrote:

> On 7/2/19 9:31 PM, Michal Suchánek wrote:
> > On Tue, 2 Jul 2019 20:38:27 +0200
> > Marek Vasut <marex at denx.de> wrote:
> >   
> >> On 7/2/19 7:50 PM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 18:58:54 +0200
> >>> Marek Vasut <marex at denx.de> wrote:
> >>>     
> >>>> On 7/2/19 4:22 PM, Michal Suchánek wrote:    
> >>>>> On Tue, 2 Jul 2019 15:11:07 +0200
> >>>>> Marek Vasut <marex at denx.de> wrote:
> >>>>>       
> >>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote:      
> >>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200
> >>>>>>> Marek Vasut <marex at denx.de> wrote:
> >>>>>>>         
> >>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote:        
> >>>>>>>>> Causes unbound key repeat on error otherwise.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Michal Suchanek <msuchanek at suse.de>
> >>>>>>>>> ---
> >>>>>>>>>  common/usb_kbd.c | 7 +++----
> >>>>>>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> >>>>>>>>> index cc99c6be0720..948f9fd68490 100644
> >>>>>>>>> --- a/common/usb_kbd.c
> >>>>>>>>> +++ b/common/usb_kbd.c
> >>>>>>>>> @@ -339,10 +339,9 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
> >>>>>>>>>  	struct usb_kbd_pdata *data = dev->privptr;
> >>>>>>>>>  
> >>>>>>>>>  	/* Submit a interrupt transfer request */
> >>>>>>>>> -	usb_submit_int_msg(dev, data->intpipe, &data->new[0], data->intpktsize,
> >>>>>>>>> -			   data->intinterval);
> >>>>>>>>> -
> >>>>>>>>> -	usb_kbd_irq_worker(dev);
> >>>>>>>>> +	if (!usb_submit_int_msg(dev, data->intpipe, &data->new[0],          
> >>>>>>>>
> >>>>>>>> Shouldn't you propagate return value from this function ? It can return
> >>>>>>>> ENOTSUPP.
> >>>>>>>>        
> >>>>>>>
> >>>>>>> If it did then probing keyboard would fail and we would not get here.        
> >>>>>>
> >>>>>> So there is no chance this function could return an error here, ever ?
> >>>>>> E.g. what if it's implemented and someone yanks the keyboard cable out
> >>>>>> just at the right time ?      
> >>>>>
> >>>>> It returns errors all the time with dwc2. That's why we need to check
> >>>>> for the error condition. We should not get here if probing the keyboard
> >>>>> failed, though. So if the function is not supported we will not get
> >>>>> here. Anyway, if it's not supported or the keyboard is missing it by
> >>>>> definition cannot provide useful result so we should not process it.      
> >>>>
> >>>> Except you start ignoring the error value from e.g. malfunctioning
> >>>> keyboard here, instead of propagating it, correct ?    
> >>>
> >>> It was never propagated to start with. The return value was not checked
> >>> at all. What I do here is check the return value and not process the
> >>> data on error whatever it contains (like the keypress returned last
> >>> time valid data was received).    
> >>
> >> I can see a patch which checks usb_kbd_poll_for_event() return value.
> >> Can you add one ?  
> > 
> > What for? Apparently the keypress is processed in usb_kbd_irq_worker.
> > So checking the return value is needed to decide if the worker should
> > run, and is not particularly useful outside usb_kbd_poll_for_event. We
> > could signal a getc() failure but do we have any code handling getc()
> > failures?  
> 
> I presume getc() might signal EOF if the underlying hardware fails.
> But in general, it's a good practice to not ignore errors.
> 

It is not such a great idea. You might have multiple input hardware (ie
serial and usb keyboard). What does it mean that usb keyboard failed in
this context?

So in my view the ultimate consumer of getc() has no use for the error
so there is no point in propagating it.

Thanks

Michal