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

["Michal Suchánek" <msuchanek@suse.de>]

On Wed, 3 Jul 2019 13:48:00 +0200
Marek Vasut <marex@denx.de> wrote:

> On 7/3/19 1:43 PM, Michal Suchánek wrote:
> > On Wed, 3 Jul 2019 13:26:50 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> On 7/3/19 11:46 AM, Michal Suchánek wrote:  
> >>> On Tue, 2 Jul 2019 23:20:28 +0200
> >>> Marek Vasut <marex@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@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@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@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@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@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?    
> >>
> >> I'd say, the behavior is undefined ?  
> > 
> > But we need to define it which the code does by ignoring the
> > device-specific error and relying on devices that are still working
> > (like a serial port) or for which error detection is not available
> > (like most serial ports).  
> 
> Maybe the error should still be propagated to the input layer , and not
> ignored at the USB layer ?

Maybe. I would leave the discussion of handling errors in the input
layer for a separate patchset.

> 
> >>> So in my view the ultimate consumer of getc() has no use for the error
> >>> so there is no point in propagating it.    
> >>
> >> Ignoring errors and not reporting them isn't nice either, so what other
> >> option(s) do we have here ?  
> > 
> > Ignoring the errors is exactly the desirable behavior when facing
> > broken hardware like dwc2. On non-broken hardware you will get fewer
> > errors to ignore. It is up to the device driver to report device
> > failure with a message when the error condition could be informative to
> > the user (such as previously working device going away completely).  
> 
> I thought this error is a keyboard failure though , and has nothing to
> do with the USB controller ?

As far as I know the keyboard is working fine but the controller is
failing to deliver some messages.

Thanks

Michal