On Wed, 3 Jul 2019 13:48:00 +0200 Marek Vasut <ma...@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 <ma...@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 <ma...@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 <ma...@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 <ma...@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 <ma...@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 <ma...@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 <msucha...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
