| Message ID | 20200818143419.117637-2-jason.wessel@windriver.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Raspberry pi improvements usb core | expand |
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.
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. 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. Would the following be acceptable, which I can put in the next version. commit 319c75ee3d1fce8a3389d1de857751504b5110cb (HEAD -> master) Author: Jason Wessel <jason.wessel@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 ===== With the patch, all families of the Raspberry Pi boards can use this keyboard device. Signed-off-by: Jason Wessel <jason.wessel@windriver.com> diff --git a/common/usb_kbd.c b/common/usb_kbd.c index b316807844..9ff008d5dc 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -514,18 +514,28 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) USB_KBD_BOOT_REPORT_SIZE, data->new, data->intinterval); if (!data->intq) { + printf("Failed to get keyboard state from device %04x:%04x\n", + dev->descriptor.idVendor, dev->descriptor.idProduct); + /* Abort, we don't want to use that non-functional keyboard. */ + return 0; + } #elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) if (usb_get_report(dev, iface->desc.bInterfaceNumber, 1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) { -#else - if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize, - data->intinterval, false) < 0) { -#endif printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct); /* Abort, we don't want to use that non-functional keyboard. */ return 0; } +#else + /* + * Set poll to true for initial interrupt transfer + * because some keyboard devices do not return an + * event until the first keypress. + */ + usb_int_msg(dev, data->intpipe, data->new, data->intpktsize, + data->intinterval, true); +#endif /* Success. */ return 1;
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@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 ?
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@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. 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. Jason.
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@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.
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index b316807844..3c0056e1b9 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -519,7 +519,9 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) 1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE) < 0) { #else if (usb_int_msg(dev, data->intpipe, data->new, data->intpktsize, - data->intinterval, false) < 0) { + data->intinterval, true) < 0) { + /* Read first packet if the device provides it, else pick it up later */ + return 1; #endif printf("Failed to get keyboard state from device %04x:%04x\n", dev->descriptor.idVendor, dev->descriptor.idProduct);
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. Signed-off-by: Jason Wessel <jason.wessel@windriver.com> --- common/usb_kbd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)