Re: [PATCH v4 0/6] USB keyboard improvements for asahi / desktop systems

[Janne Grunau <j@jannau.net>]

[-- Attachment #1: Type: text/plain, Size: 3510 bytes --]

On Sun, Apr 07, 2024 at 03:05:59AM +0200, Marek Vasut wrote:
> On 4/6/24 10:04 PM, Janne Grunau wrote:
> > On Sat, Apr 06, 2024 at 08:52:17PM +0200, Marek Vasut wrote:
> >> On 4/5/24 9:05 PM, Janne Grunau wrote:
> >>> On Fri, Apr 05, 2024 at 04:52:32PM +0200, Marek Vasut wrote:
> >>>> On 4/4/24 8:25 AM, Janne Grunau via B4 Relay wrote:
> >>>>> Apple USB Keyboards from 2021 need quirks to be useable. The boot HID
> >>>>> keyboard protocol is unfortunately not described in the first interface
> >>>>> descriptor but the second. This needs several changes. The USB keyboard
> >>>>> driver has to look at all (2) interface descriptors during probing.
> >>>>> Since I didn't want to rebuild the USB driver probe code the Apple
> >>>>> keyboards are bound to the keyboard driver via USB vendor and product
> >>>>> IDs.
> >>>>> To make the keyboards useable on Apple silicon devices the xhci driver
> >>>>> needs to initializes rings for the endpoints of the first two interface
> >>>>> descriptors. If this is causes concerns regarding regressions or memory
> >>>>> use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG
> >>>>> option.
> >>>>> Even after this changes the keyboards still do not probe successfully
> >>>>> since they apparently do not behave HID standard compliant. They only
> >>>>> generate reports on key events. This leads the final check whether the
> >>>>> keyboard is operational to fail unless the user presses keys during the
> >>>>> probe. Skip this check for known keyboards.
> >>>>> Keychron seems to emulate Apple keyboards (some models even "re-use"
> >>>>> Apple's USB vendor ID) so apply this quirk as well.
> >>>>>
> >>>>> Some devices like Yubikeys emulate a keyboard. since u-boot only binds a
> >>>>> single keyboard block this kind of devices from the USB keyboard driver.
> >>>>>
> >>>>> Signed-off-by: Janne Grunau <j@jannau.net>
> >>>>
> >>>> I picked the series, but CI indicates build errors, can you have a look ?
> >>>>
> >>>> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20215
> >>>
> >>> The issue seems to be that the field dev in struct usb_device exists
> >>> only for DM_USB. That means we can't use dev_dbg.
> >>> Either take the following fixup patch or I can resend the series.
> >>
> >> I squashed the extra patch in, but I think the CI still complains:
> >>
> >> Pipeline #20236 (
> >> https://source.denx.de/u-boot/custodians/u-boot-usb/-/pipelines/20236 )
> >> triggered by Marek Vasut ( https://source.denx.de/marex )
> >> had 1 failed job.
> >>
> >> Job #812215 (
> >> https://source.denx.de/u-boot/custodians/u-boot-usb/-/jobs/812215/raw )
> > 
> > env_get() missing without CONFIG_ENV_SUPPORT. I'm too accustomed to the
> > kernel's stub functions. Best option seems to to just #if the
> > functionality out in this case. See attached fixup patch.
> > 
> > Sorry,
> 
> No worries.
> 
> Does it work if you do this instead ?
> 
>   static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
>   {
>          ulong vid, pid;
> +       /* No env support, nothing can be ignored */
> +       if (CONFIG_IS_ENABLED(ENV_SUPPORT))
> +           return 0;
> 
> That way, the function is always compiled and if it is unreachable, then 
> compiler throws it out. This should improve code compile coverage.

Seems to work here with a broken imx8 config from the CI. Is it ok to
rely on dead code elimination? Apparently it is, build with KCFLAGS=-O0
has already several other missing symbols.

See attached fixup

Janne

[-- Attachment #2: 0001-fixup-usb-Add-environment-based-device-ignorelist.patch --]
[-- Type: text/plain, Size: 831 bytes --]

From ab3b825d7bc0571cfb38eb80a1e449e51d5d2f6d Mon Sep 17 00:00:00 2001
From: Janne Grunau <j@jannau.net>
Date: Mon, 8 Apr 2024 09:44:54 +0200
Subject: [PATCH 1/1] fixup! usb: Add environment based device ignorelist

---
 common/usb.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/common/usb.c b/common/usb.c
index 8bc85c58b28c..99e6b857c74c 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1089,7 +1089,13 @@ static int usb_device_is_ignored(u16 id_vendor, u16 id_product)
 {
 	ulong vid, pid;
 	char *end;
-	const char *cur = env_get("usb_ignorelist");
+	const char *cur = NULL;
+
+	/* ignore list depends on env support */
+	if (!CONFIG_IS_ENABLED(ENV_SUPPORT))
+		return 0;
+
+	cur = env_get("usb_ignorelist");
 
 	/* parse "usb_ignorelist" strictly */
 	while (cur && cur[0] != '\0') {
-- 
2.43.2