[RESEND][PATCH 1/3] usb_kbd: succeed even if no interrupt is pending

[Marek Vasut]

On 8/18/20 8:47 PM, Jason Wessel wrote:
> 
> 
> On 8/18/20 12:20 PM, Marek Vasut wrote:
>> On 8/18/20 6:54 PM, Jason Wessel wrote:
>>>
>>>
>>> On 8/18/20 10:05 AM, Marek Vasut wrote:
>>>> On 8/18/20 4:34 PM, Jason Wessel wrote:
>>>>> After the initial configuration some USB keyboard+mouse devices never
>>>>> return any kind of event on the interrupt line.  In particular, the
>>>>> device identified by "Cypress Cypress USB Keyboard / PS2 Mouse as
>>>>> 3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:04B4:0101.0001/input/input0"
>>>>> never returns a data packet until the first external input event.
>>>>>
>>>>> I found this was also true with some newer model Dell keyboards.
>>>>>
>>>>> When the device is plugged into a xhci controller there is also no
>>>>> point in waiting 5 seconds for a device that is never going to present
>>>>> data, so the call to the interrupt service was changed to a
>>>>> nonblocking operation for the controllers that support this.
>>>>>
>>>>> With the patch applied, the rpi3 and rpi4 work well with the more
>>>>> complex keyboard devices.
>>>>
>>>> Please extend the comment in the code, it is not clear from it or from
>>>> the commit message what the problem really is that this patch tries to fix.
>>>>
>>>> Also, the printf() below the return 1 is never reached.
>>>>
>>>
>>>
>>> The printf() is only reached in the case of the #ifdef above it being true. 
>>
>> That's pretty awful and confusing code then. The original code wasn't
>> stellar either, but can this be somehow improved now ?
>>
>>> The compiler in theory should optimize it away, but for clarity it can be moved 
>>> with in the #ifdef but that also requires fixing it in two places because 
>>> there are multiple levels to the #ifdef.
>>
>> I think it's better to make it more readable if possible.
>>
>>> Would the following be acceptable, which I can put in the next version.
>>>
>>>
>>> commit 319c75ee3d1fce8a3389d1de857751504b5110cb (HEAD -> master)
>>> Author: Jason Wessel <jason.wessel at windriver.com>
>>> Date:   Thu Jun 25 05:31:25 2020 -0700
>>>
>>>     usb_kbd: succeed even if no interrupt is pending
>>>     
>>>     After the initial configuration some USB keyboard+mouse devices never
>>>     return any kind of event on the interrupt line.  In particular, the
>>>     device identified by "Cypress Cypress USB Keyboard / PS2 Mouse as
>>>     3f980000.usb/usb1/1-1/1-1.3/1-1.3:1.0/0003:04B4:0101.0001/input/input0"
>>>     never returns a data packet until the first external input event.
>>>     
>>>     I found this was also true with some newer model Dell keyboards.
>>>     
>>>     Without the patch u-boot prints the following on one of keyboards and
>>>     leave it unable to accept input events.
>>>     
>>>     =====
>>>     scanning bus xhci_pci for devices...
>>>       Failed to get keyboard state from device 14dd:1009
>>
>> So I have to wonder, if the keyboard never returns a data packet until
>> you press a key (that makes sense), how does Linux deal with this ?
>>
> 
> 
> As far as I can tell the usb_kbd_probe() probe function in the Linux kernel 
> sets up the configuration and exits right away.  The keyboard drivers state 
> was zeroed out in the probe function and the kernel later processes callbacks
> from the interrupt handler.  

Why can't we return right away and why do we poll/wait then ?

> Does that imply the other 2 code paths should also just return 1 and we get
> rid of the printf() entirely? 
> 
> I don't have any boards that wanted either of these two paths through code,
> so I wasn't inclined to change it.

I'd say, change it, it'd go in ~October and the next release is in 3
months from October anyway.