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

[Marek Vasut <marex@denx.de>]

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 ?

>>> 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 ?

[...]