From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Thu, 4 Jul 2019 17:19:03 +0200 Subject: [U-Boot] [PATCH 3/8] usb_kdb: only process events succesfully received In-Reply-To: <20190703184141.07cc3a2d@kitsune.suse.cz> References: <5dfc00046742db978f1ae046d8855663ccf7c281.1561996556.git.msuchanek@suse.de> <4fd1fdcc59394509236814a45d7cd384794acaae.1561996556.git.msuchanek@suse.de> <00f68615-155e-d513-1bae-c03f2d6b8097@denx.de> <20190702150400.090f85ba@kitsune.suse.cz> <41ce804f-1a82-29d8-ffdf-f1b95032016a@denx.de> <20190702162252.153a725b@kitsune.suse.cz> <20190702195005.46823451@kitsune.suse.cz> <9422e86f-7b4b-d56e-123c-ac059abdd89b@denx.de> <20190702213113.66acd70c@naga> <2a188bb4-95bc-d69e-50f2-1d247888c005@denx.de> <20190703114628.21c861a5@naga> <66cc8d26-778a-c3b2-4577-6e02dcf43f7c@denx.de> <20190703134312.0733e1b3@naga> <20190703184141.07cc3a2d@kitsune.suse.cz> Message-ID: <209fdd2f-ec6b-70fa-128f-75db0e0a596d@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On 7/3/19 6:41 PM, Michal Suchánek wrote: > On Wed, 3 Jul 2019 13:48:00 +0200 > Marek Vasut wrote: > >> On 7/3/19 1:43 PM, Michal Suchánek wrote: >>> On Wed, 3 Jul 2019 13:26:50 +0200 >>> Marek Vasut wrote: >>> >>>> On 7/3/19 11:46 AM, Michal Suchánek wrote: >>>>> On Tue, 2 Jul 2019 23:20:28 +0200 >>>>> Marek Vasut wrote: >>>>> >>>>>> On 7/2/19 9:31 PM, Michal Suchánek wrote: >>>>>>> On Tue, 2 Jul 2019 20:38:27 +0200 >>>>>>> Marek Vasut wrote: >>>>>>> >>>>>>>> On 7/2/19 7:50 PM, Michal Suchánek wrote: >>>>>>>>> On Tue, 2 Jul 2019 18:58:54 +0200 >>>>>>>>> Marek Vasut wrote: >>>>>>>>> >>>>>>>>>> On 7/2/19 4:22 PM, Michal Suchánek wrote: >>>>>>>>>>> On Tue, 2 Jul 2019 15:11:07 +0200 >>>>>>>>>>> Marek Vasut wrote: >>>>>>>>>>> >>>>>>>>>>>> On 7/2/19 3:04 PM, Michal Suchánek wrote: >>>>>>>>>>>>> On Tue, 2 Jul 2019 13:58:30 +0200 >>>>>>>>>>>>> Marek Vasut wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> On 7/1/19 5:56 PM, Michal Suchanek wrote: >>>>>>>>>>>>>>> Causes unbound key repeat on error otherwise. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Michal Suchanek >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> 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. It's literally 3-line change to make usb_kbd_poll_for_event() return the error code. >>>>> 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. Uh, would you be willing to debug that ? [...]