* [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear @ 2025-01-19 18:50 Dragan Simic 2025-01-27 21:55 ` Marek Vasut 0 siblings, 1 reply; 8+ messages in thread From: Dragan Simic @ 2025-01-19 18:50 UTC (permalink / raw) To: u-boot; +Cc: marex, Dang Huynh, Piotr Zalewski Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually prevents the CONFIG_USB_KEYBOARD option from working as expected, i.e. USB keyboards end up not working as console inputs when "usbkbd" is properly specified as part of the value of the "stdin" environment variable. Describe this clearly in the two relevant Kconfig files, to prevent any possible confusion. In more detail, the console_init_r() function ends up overwriting the "stdin" environment variable so it contains only the devices available at that point (which doesn't include "usbkbd"), while the "usb start" operation is performed much later, at which point the probe_usb_keyboard() function cannot assign any discovered USB keyboards to the console because "usbkbd" is no longer present in the "stdin" environment variable. Reported-by: Dang Huynh <danct12@riseup.net> Co-developed-by: Piotr Zalewski <pZ010001011111@proton.me> Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- common/Kconfig | 7 +++++++ drivers/usb/Kconfig | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/common/Kconfig b/common/Kconfig index 0e8c44f3f745..267a646e9d4e 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -297,6 +297,13 @@ config SYS_CONSOLE_ENV_OVERWRITE start-up (after relocation). This causes the environment to be updated to match the console devices actually chosen. + When selected, USB keyboards will not be registered as console + input devices regardless of the proper environment settings, i.e. + despite properly having "usbkbd" as part of the value of the + "stdin" environment variable. + + If unsure, say N. + config SYS_CONSOLE_INFO_QUIET bool "Don't display the console devices on boot" help diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig index 960b6a906ac4..4d52e382f570 100644 --- a/drivers/usb/Kconfig +++ b/drivers/usb/Kconfig @@ -106,7 +106,8 @@ config USB_KEYBOARD select SYS_STDIO_DEREGISTER ---help--- Say Y here if you want to use a USB keyboard for U-Boot command line - input. + input. Also say N for SYS_CONSOLE_ENV_OVERWRITE and see the help for + that option for more details. config USB_ONBOARD_HUB bool "Onboard USB hub support" ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-19 18:50 [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear Dragan Simic @ 2025-01-27 21:55 ` Marek Vasut 2025-01-27 22:20 ` Dragan Simic 0 siblings, 1 reply; 8+ messages in thread From: Marek Vasut @ 2025-01-27 21:55 UTC (permalink / raw) To: Dragan Simic, u-boot; +Cc: Dang Huynh, Piotr Zalewski On 1/19/25 7:50 PM, Dragan Simic wrote: > Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually prevents the > CONFIG_USB_KEYBOARD option from working as expected, i.e. USB keyboards end > up not working as console inputs when "usbkbd" is properly specified as part > of the value of the "stdin" environment variable. Describe this clearly in > the two relevant Kconfig files, to prevent any possible confusion. > > In more detail, the console_init_r() function ends up overwriting the "stdin" > environment variable so it contains only the devices available at that point > (which doesn't include "usbkbd"), while the "usb start" operation is performed > much later, at which point the probe_usb_keyboard() function cannot assign any > discovered USB keyboards to the console because "usbkbd" is no longer present > in the "stdin" environment variable. Wouldn't it be better to patch console_init_r() , check if "stdin" variable contains 'usbkbd' and if so, start USB and register the keyboard as stdin device at this point ? This is called after relocation, so starting USB should be OK to do here. The behavior should then be less confusing to users too. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-27 21:55 ` Marek Vasut @ 2025-01-27 22:20 ` Dragan Simic 2025-01-27 22:23 ` Marek Vasut 0 siblings, 1 reply; 8+ messages in thread From: Dragan Simic @ 2025-01-27 22:20 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Dang Huynh, Piotr Zalewski Hello Marek, On 2025-01-27 22:55, Marek Vasut wrote: > On 1/19/25 7:50 PM, Dragan Simic wrote: >> Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually >> prevents the >> CONFIG_USB_KEYBOARD option from working as expected, i.e. USB >> keyboards end >> up not working as console inputs when "usbkbd" is properly specified >> as part >> of the value of the "stdin" environment variable. Describe this >> clearly in >> the two relevant Kconfig files, to prevent any possible confusion. >> >> In more detail, the console_init_r() function ends up overwriting the >> "stdin" >> environment variable so it contains only the devices available at that >> point >> (which doesn't include "usbkbd"), while the "usb start" operation is >> performed >> much later, at which point the probe_usb_keyboard() function cannot >> assign any >> discovered USB keyboards to the console because "usbkbd" is no longer >> present >> in the "stdin" environment variable. > > Wouldn't it be better to patch console_init_r() , check if "stdin" > variable contains 'usbkbd' and if so, start USB and register the > keyboard as stdin device at this point ? This is called after > relocation, so starting USB should be OK to do here. The behavior > should then be less confusing to users too. Please correct me if I'm wrong, but isn't "usb start" deliberately left to be executed as desired by the supported boards, or as part of distroboot, i.e. in include/config_distro_bootcmd.h? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-27 22:20 ` Dragan Simic @ 2025-01-27 22:23 ` Marek Vasut 2025-01-27 22:42 ` Dragan Simic 0 siblings, 1 reply; 8+ messages in thread From: Marek Vasut @ 2025-01-27 22:23 UTC (permalink / raw) To: Dragan Simic; +Cc: u-boot, Dang Huynh, Piotr Zalewski On 1/27/25 11:20 PM, Dragan Simic wrote: > Hello Marek, > > On 2025-01-27 22:55, Marek Vasut wrote: >> On 1/19/25 7:50 PM, Dragan Simic wrote: >>> Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually >>> prevents the >>> CONFIG_USB_KEYBOARD option from working as expected, i.e. USB >>> keyboards end >>> up not working as console inputs when "usbkbd" is properly specified >>> as part >>> of the value of the "stdin" environment variable. Describe this >>> clearly in >>> the two relevant Kconfig files, to prevent any possible confusion. >>> >>> In more detail, the console_init_r() function ends up overwriting the >>> "stdin" >>> environment variable so it contains only the devices available at >>> that point >>> (which doesn't include "usbkbd"), while the "usb start" operation is >>> performed >>> much later, at which point the probe_usb_keyboard() function cannot >>> assign any >>> discovered USB keyboards to the console because "usbkbd" is no longer >>> present >>> in the "stdin" environment variable. >> >> Wouldn't it be better to patch console_init_r() , check if "stdin" >> variable contains 'usbkbd' and if so, start USB and register the >> keyboard as stdin device at this point ? This is called after >> relocation, so starting USB should be OK to do here. The behavior >> should then be less confusing to users too. > > Please correct me if I'm wrong, but isn't "usb start" deliberately > left to be executed as desired by the supported boards, or as part > of distroboot, i.e. in include/config_distro_bootcmd.h? It is, but if the user needs USB keyboard as input device, shouldn't we probe for that (which yes, will make the start up slower) ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-27 22:23 ` Marek Vasut @ 2025-01-27 22:42 ` Dragan Simic 2025-01-27 23:03 ` Marek Vasut 0 siblings, 1 reply; 8+ messages in thread From: Dragan Simic @ 2025-01-27 22:42 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Dang Huynh, Piotr Zalewski On 2025-01-27 23:23, Marek Vasut wrote: > On 1/27/25 11:20 PM, Dragan Simic wrote: >> On 2025-01-27 22:55, Marek Vasut wrote: >>> On 1/19/25 7:50 PM, Dragan Simic wrote: >>>> Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually >>>> prevents the >>>> CONFIG_USB_KEYBOARD option from working as expected, i.e. USB >>>> keyboards end >>>> up not working as console inputs when "usbkbd" is properly specified >>>> as part >>>> of the value of the "stdin" environment variable. Describe this >>>> clearly in >>>> the two relevant Kconfig files, to prevent any possible confusion. >>>> >>>> In more detail, the console_init_r() function ends up overwriting >>>> the "stdin" >>>> environment variable so it contains only the devices available at >>>> that point >>>> (which doesn't include "usbkbd"), while the "usb start" operation is >>>> performed >>>> much later, at which point the probe_usb_keyboard() function cannot >>>> assign any >>>> discovered USB keyboards to the console because "usbkbd" is no >>>> longer present >>>> in the "stdin" environment variable. >>> >>> Wouldn't it be better to patch console_init_r() , check if "stdin" >>> variable contains 'usbkbd' and if so, start USB and register the >>> keyboard as stdin device at this point ? This is called after >>> relocation, so starting USB should be OK to do here. The behavior >>> should then be less confusing to users too. >> >> Please correct me if I'm wrong, but isn't "usb start" deliberately >> left to be executed as desired by the supported boards, or as part >> of distroboot, i.e. in include/config_distro_bootcmd.h? > > It is, but if the user needs USB keyboard as input device, shouldn't > we probe for that (which yes, will make the start up slower) ? I agree that the way the whole thing works now, with or without this patch, leaves a fair amount of room for confusion. That's exactly how I felt while writing this patch -- I was confused a bit about the possible outcomes of different combinations of the configuration options. Though, I also felt that the way it works now also makes the whole thing as flexible as possible, so the users can choose whatever configuration they want, even the broken one. I'm also afraid that, effectively, starting USB forcedly at that point may cause some unforeseen issues or regressions. It also crossed my mind that perhaps a good solution would be to automatically deselect SYS_CONSOLE_ENV_OVERWRITE when the user selects USB_KEYBOARD, because that's needed for USB keyboards to actually work. That should also have lower potential for causing some issues or regressions. Maybe another good option would be to modify the console_init_r() function to simply leave "usbkbd" in the environment variables when USB_KEYBOARD is selected, so the "usb start" executed later can actually make USB keyboards work? I think such an approach should have the lowest potential for causing some issues or regressions, while it would pretty much eliminate any confusion. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-27 22:42 ` Dragan Simic @ 2025-01-27 23:03 ` Marek Vasut 2025-01-27 23:22 ` Dragan Simic 0 siblings, 1 reply; 8+ messages in thread From: Marek Vasut @ 2025-01-27 23:03 UTC (permalink / raw) To: Dragan Simic; +Cc: u-boot, Dang Huynh, Piotr Zalewski On 1/27/25 11:42 PM, Dragan Simic wrote: > On 2025-01-27 23:23, Marek Vasut wrote: >> On 1/27/25 11:20 PM, Dragan Simic wrote: >>> On 2025-01-27 22:55, Marek Vasut wrote: >>>> On 1/19/25 7:50 PM, Dragan Simic wrote: >>>>> Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually >>>>> prevents the >>>>> CONFIG_USB_KEYBOARD option from working as expected, i.e. USB >>>>> keyboards end >>>>> up not working as console inputs when "usbkbd" is properly >>>>> specified as part >>>>> of the value of the "stdin" environment variable. Describe this >>>>> clearly in >>>>> the two relevant Kconfig files, to prevent any possible confusion. >>>>> >>>>> In more detail, the console_init_r() function ends up overwriting >>>>> the "stdin" >>>>> environment variable so it contains only the devices available at >>>>> that point >>>>> (which doesn't include "usbkbd"), while the "usb start" operation >>>>> is performed >>>>> much later, at which point the probe_usb_keyboard() function cannot >>>>> assign any >>>>> discovered USB keyboards to the console because "usbkbd" is no >>>>> longer present >>>>> in the "stdin" environment variable. >>>> >>>> Wouldn't it be better to patch console_init_r() , check if "stdin" >>>> variable contains 'usbkbd' and if so, start USB and register the >>>> keyboard as stdin device at this point ? This is called after >>>> relocation, so starting USB should be OK to do here. The behavior >>>> should then be less confusing to users too. >>> >>> Please correct me if I'm wrong, but isn't "usb start" deliberately >>> left to be executed as desired by the supported boards, or as part >>> of distroboot, i.e. in include/config_distro_bootcmd.h? >> >> It is, but if the user needs USB keyboard as input device, shouldn't >> we probe for that (which yes, will make the start up slower) ? > > I agree that the way the whole thing works now, with or without this > patch, leaves a fair amount of room for confusion. That's exactly > how I felt while writing this patch -- I was confused a bit about > the possible outcomes of different combinations of the configuration > options. Though, I also felt that the way it works now also makes > the whole thing as flexible as possible, so the users can choose > whatever configuration they want, even the broken one. > > I'm also afraid that, effectively, starting USB forcedly at that > point may cause some unforeseen issues or regressions. > > It also crossed my mind that perhaps a good solution would be to > automatically deselect SYS_CONSOLE_ENV_OVERWRITE when the user > selects USB_KEYBOARD, because that's needed for USB keyboards to > actually work. That should also have lower potential for causing > some issues or regressions. No, that's not good, users might want env overwrite to preconfigure their input devices and USB keyboard support at the same time. > Maybe another good option would be to modify the console_init_r() > function to simply leave "usbkbd" in the environment variables when > USB_KEYBOARD is selected, so the "usb start" executed later can > actually make USB keyboards work? I think such an approach should > have the lowest potential for causing some issues or regressions, > while it would pretty much eliminate any confusion. Yes, this looks like the best option, because if 'usbkbd' is in 'stdin' environment variable, the user likely expects the USB keyboard will show up later, but it is not available this early yet and removing it from stdin variable would be unhelpful. Please also update SYS_CONSOLE_ENV_OVERWRITE Kconfig symbol description to reflect this change . ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-27 23:03 ` Marek Vasut @ 2025-01-27 23:22 ` Dragan Simic 2025-01-28 1:47 ` Marek Vasut 0 siblings, 1 reply; 8+ messages in thread From: Dragan Simic @ 2025-01-27 23:22 UTC (permalink / raw) To: Marek Vasut; +Cc: u-boot, Dang Huynh, Piotr Zalewski On 2025-01-28 00:03, Marek Vasut wrote: > On 1/27/25 11:42 PM, Dragan Simic wrote: >> On 2025-01-27 23:23, Marek Vasut wrote: >>> On 1/27/25 11:20 PM, Dragan Simic wrote: >>>> On 2025-01-27 22:55, Marek Vasut wrote: >>>>> On 1/19/25 7:50 PM, Dragan Simic wrote: >>>>>> Selecting the CONFIG_SYS_CONSOLE_ENV_OVERWRITE option actually >>>>>> prevents the >>>>>> CONFIG_USB_KEYBOARD option from working as expected, i.e. USB >>>>>> keyboards end >>>>>> up not working as console inputs when "usbkbd" is properly >>>>>> specified as part >>>>>> of the value of the "stdin" environment variable. Describe this >>>>>> clearly in >>>>>> the two relevant Kconfig files, to prevent any possible confusion. >>>>>> >>>>>> In more detail, the console_init_r() function ends up overwriting >>>>>> the "stdin" >>>>>> environment variable so it contains only the devices available at >>>>>> that point >>>>>> (which doesn't include "usbkbd"), while the "usb start" operation >>>>>> is performed >>>>>> much later, at which point the probe_usb_keyboard() function >>>>>> cannot assign any >>>>>> discovered USB keyboards to the console because "usbkbd" is no >>>>>> longer present >>>>>> in the "stdin" environment variable. >>>>> >>>>> Wouldn't it be better to patch console_init_r() , check if "stdin" >>>>> variable contains 'usbkbd' and if so, start USB and register the >>>>> keyboard as stdin device at this point ? This is called after >>>>> relocation, so starting USB should be OK to do here. The behavior >>>>> should then be less confusing to users too. >>>> >>>> Please correct me if I'm wrong, but isn't "usb start" deliberately >>>> left to be executed as desired by the supported boards, or as part >>>> of distroboot, i.e. in include/config_distro_bootcmd.h? >>> >>> It is, but if the user needs USB keyboard as input device, shouldn't >>> we probe for that (which yes, will make the start up slower) ? >> >> I agree that the way the whole thing works now, with or without this >> patch, leaves a fair amount of room for confusion. That's exactly >> how I felt while writing this patch -- I was confused a bit about >> the possible outcomes of different combinations of the configuration >> options. Though, I also felt that the way it works now also makes >> the whole thing as flexible as possible, so the users can choose >> whatever configuration they want, even the broken one. >> >> I'm also afraid that, effectively, starting USB forcedly at that >> point may cause some unforeseen issues or regressions. >> >> It also crossed my mind that perhaps a good solution would be to >> automatically deselect SYS_CONSOLE_ENV_OVERWRITE when the user >> selects USB_KEYBOARD, because that's needed for USB keyboards to >> actually work. That should also have lower potential for causing >> some issues or regressions. > > No, that's not good, users might want env overwrite to preconfigure > their input devices and USB keyboard support at the same time. Indeed, I also don't like that approach very much. It would reduce the flexibility of configuration options significantly. >> Maybe another good option would be to modify the console_init_r() >> function to simply leave "usbkbd" in the environment variables when >> USB_KEYBOARD is selected, so the "usb start" executed later can >> actually make USB keyboards work? I think such an approach should >> have the lowest potential for causing some issues or regressions, >> while it would pretty much eliminate any confusion. > > Yes, this looks like the best option, because if 'usbkbd' is in > 'stdin' environment variable, the user likely expects the USB keyboard > will show up later, but it is not available this early yet and > removing it from stdin variable would be unhelpful. > > Please also update SYS_CONSOLE_ENV_OVERWRITE Kconfig symbol > description to reflect this change . I'm glad that you like it, I'll move forward with that approach. I was also thinking about making sure that "usbkbd" gets removed from the "stdin" environment variable if there are actually no USB keyboards detected during the "usb start", but that would actually be wrong, because a USB keyboard should be detected after being plugged in (in interactive mode, of course) after "usb start" is executed, followed by executing "usb reset" on the UART console. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear 2025-01-27 23:22 ` Dragan Simic @ 2025-01-28 1:47 ` Marek Vasut 0 siblings, 0 replies; 8+ messages in thread From: Marek Vasut @ 2025-01-28 1:47 UTC (permalink / raw) To: Dragan Simic; +Cc: u-boot, Dang Huynh, Piotr Zalewski On 1/28/25 12:22 AM, Dragan Simic wrote: [...] >>> Maybe another good option would be to modify the console_init_r() >>> function to simply leave "usbkbd" in the environment variables when >>> USB_KEYBOARD is selected, so the "usb start" executed later can >>> actually make USB keyboards work? I think such an approach should >>> have the lowest potential for causing some issues or regressions, >>> while it would pretty much eliminate any confusion. >> >> Yes, this looks like the best option, because if 'usbkbd' is in >> 'stdin' environment variable, the user likely expects the USB keyboard >> will show up later, but it is not available this early yet and >> removing it from stdin variable would be unhelpful. >> >> Please also update SYS_CONSOLE_ENV_OVERWRITE Kconfig symbol >> description to reflect this change . > > I'm glad that you like it, I'll move forward with that approach. > > I was also thinking about making sure that "usbkbd" gets removed > from the "stdin" environment variable if there are actually no USB > keyboards detected during the "usb start", but that would actually > be wrong, because a USB keyboard should be detected after being > plugged in (in interactive mode, of course) after "usb start" is > executed, followed by executing "usb reset" on the UART console. That would also be problematic in case the keyboard would not be detected on first 'usb start' run, but it would be detected afterward, on the next 'usb start/reset' run. Thanks ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-01-28 1:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-19 18:50 [PATCH] usb: Make what it takes for "usbkbd" to work in "stdin" clear Dragan Simic 2025-01-27 21:55 ` Marek Vasut 2025-01-27 22:20 ` Dragan Simic 2025-01-27 22:23 ` Marek Vasut 2025-01-27 22:42 ` Dragan Simic 2025-01-27 23:03 ` Marek Vasut 2025-01-27 23:22 ` Dragan Simic 2025-01-28 1:47 ` Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.