diff mbox series

ARM: multi_v7_defconfig: Select CONFIG_USB_ONBOARD_DEV as built-in

Message ID 20240408140254.1645317-1-festevam@gmail.com (mailing list archive)
State New, archived
Headers show
Series ARM: multi_v7_defconfig: Select CONFIG_USB_ONBOARD_DEV as built-in | expand

Commit Message

Fabio Estevam April 8, 2024, 2:02 p.m. UTC 
From: Fabio Estevam <festevam@denx.de>

Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following
run-time error when probing a USB2514 hub on a imx6q-udoo board:

usb 1-1: device not accepting address 5, error -71
usb usb1-port1: unable to enumerate USB device

Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so
that the USB hub reset line and clock can be activated earlier.  

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Here is the imx6q-udoo boot log from Mark's farm that shows the problem:

https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html

 arch/arm/configs/multi_v7_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Fabio Estevam April 29, 2024, 10:09 p.m. UTC | #1 
Hi Arnd,

On Mon, Apr 8, 2024 at 11:03 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following
> run-time error when probing a USB2514 hub on a imx6q-udoo board:
>
> usb 1-1: device not accepting address 5, error -71
> usb usb1-port1: unable to enumerate USB device
>
> Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so
> that the USB hub reset line and clock can be activated earlier.
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Here is the imx6q-udoo boot log from Mark's farm that shows the problem:
>
> https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html

A gentle ping.
Alexander Stein April 30, 2024, 6:28 a.m. UTC | #2 
Hi,

Am Dienstag, 30. April 2024, 00:09:52 CEST schrieb Fabio Estevam:
> Hi Arnd,
> 
> On Mon, Apr 8, 2024 at 11:03 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > From: Fabio Estevam <festevam@denx.de>
> >
> > Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following
> > run-time error when probing a USB2514 hub on a imx6q-udoo board:
> >
> > usb 1-1: device not accepting address 5, error -71
> > usb usb1-port1: unable to enumerate USB device
> >
> > Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so
> > that the USB hub reset line and clock can be activated earlier.
> >
> > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > ---
> > Here is the imx6q-udoo boot log from Mark's farm that shows the problem:
> >
> > https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html
> 
> A gentle ping.

Despite the change to the defconfig. If this driver causes problems when
being built as module, shouldn't it be bool instead of tristate then?

Best regards,
Alexander
Arnd Bergmann April 30, 2024, 7:52 a.m. UTC | #3 
On Tue, Apr 30, 2024, at 08:28, Alexander Stein wrote:
> Am Dienstag, 30. April 2024, 00:09:52 CEST schrieb Fabio Estevam:
>> Hi Arnd,
>> 
>> On Mon, Apr 8, 2024 at 11:03 AM Fabio Estevam <festevam@gmail.com> wrote:
>> >
>> > From: Fabio Estevam <festevam@denx.de>
>> >
>> > Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following
>> > run-time error when probing a USB2514 hub on a imx6q-udoo board:
>> >
>> > usb 1-1: device not accepting address 5, error -71
>> > usb usb1-port1: unable to enumerate USB device
>> >
>> > Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so
>> > that the USB hub reset line and clock can be activated earlier.
>> >
>> > Signed-off-by: Fabio Estevam <festevam@denx.de>
>> > ---
>> > Here is the imx6q-udoo boot log from Mark's farm that shows the problem:
>> >
>> > https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html
>
> Despite the change to the defconfig. If this driver causes problems when
> being built as module, shouldn't it be bool instead of tristate then?

It does sound like this is something the kernel should be able to
get to work properly in some form, but I don't think making it
a 'bool' symbol is the correct answer here: if CONFIG_USB is
set to =m, it would be impossible to include USB_ONBOARD_DEV
in this case.

Fabio, can you explain how making it built-in addresses the
problem here? I assume this is related to probe order, so I
wonder if it's just a matter of making the usb hub driver
properly handle -EPROBE_DEFER until the onboard dev has been
initialized.

     Arnd
Fabio Estevam April 30, 2024, 7:53 p.m. UTC | #4 
On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote:

> It does sound like this is something the kernel should be able to
> get to work properly in some form, but I don't think making it
> a 'bool' symbol is the correct answer here: if CONFIG_USB is
> set to =m, it would be impossible to include USB_ONBOARD_DEV
> in this case.
>
> Fabio, can you explain how making it built-in addresses the
> problem here? I assume this is related to probe order, so I
> wonder if it's just a matter of making the usb hub driver
> properly handle -EPROBE_DEFER until the onboard dev has been
> initialized.

From drivers/usb/misc/Kconfig:

"config USB_ONBOARD_DEV
tristate "Onboard USB device support"
.....
  This driver can be used as a module but its state (module vs
  builtin) must match the state of the USB subsystem. Enabling
  this config will enable the driver and it will automatically
  match the state of the USB subsystem. If this driver is a
  module it will be called onboard_usb_dev."

In multi_v7_defconfig:
CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch.

My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching.

Is there any other way to solve this?
Arnd Bergmann April 30, 2024, 8:24 p.m. UTC | #5 
On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote:
> On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
>> It does sound like this is something the kernel should be able to
>> get to work properly in some form, but I don't think making it
>> a 'bool' symbol is the correct answer here: if CONFIG_USB is
>> set to =m, it would be impossible to include USB_ONBOARD_DEV
>> in this case.
>>
>> Fabio, can you explain how making it built-in addresses the
>> problem here? I assume this is related to probe order, so I
>> wonder if it's just a matter of making the usb hub driver
>> properly handle -EPROBE_DEFER until the onboard dev has been
>> initialized.
>
> From drivers/usb/misc/Kconfig:
>
> "config USB_ONBOARD_DEV
> tristate "Onboard USB device support"
> .....
>   This driver can be used as a module but its state (module vs
>   builtin) must match the state of the USB subsystem. Enabling
>   this config will enable the driver and it will automatically
>   match the state of the USB subsystem. If this driver is a
>   module it will be called onboard_usb_dev."

Ok, so there is some kind of design mistake here that this
is trying to paper over. Kbuild should always be powerful
enough to enforce these things without having a person read
a comment though.

> In multi_v7_defconfig:
> CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch.
>
> My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching.
>
> Is there any other way to solve this?

I can think of multiple ways to enforce this in Kbuild:

a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add
   a hidden symbol that duplicates the state of CONFIG_USB when
   USB_ONBOARD_DEV is enabled:

config CONFIG_USB_ONBOARD_DEV_MODULE
     def_tristate USB
     depends on CONFIG_USB_ONBOARD_DEV

b) Do the same thing but use Makefile syntax instead of Kconfig
   syntax:

usb-onboard-dev-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o 
obj-$(CONFIG_USB) += usb-onboard-dev.o

(or something close to this, need to try).


However, I still feel there should be a better way to solve
the problem at the code level rather than the Kbuild level.

I don't know exactly how a "usb_device_driver" (as opposed
to a "usb_driver") works here, but from what I can tell,
the idea here is that the USB bus probe finds a device, matches
the driver and then continues probing it. If the onboard-dev
driver gets loaded after the initial bus probe, that driver
won't exist yet and it then runs into some error that
prevents it from being retried when after the onboard-dev
driver is there. If you can pinpoint exactly what error it
runs into, try to make it retry the same thing after
-EPROBE_DEFER.

    Arnd
Fabio Estevam May 1, 2024, 11:04 p.m. UTC | #6 
Adding Matthias, the onboard usb hub driver maintainer.

On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote:
> > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> >> It does sound like this is something the kernel should be able to
> >> get to work properly in some form, but I don't think making it
> >> a 'bool' symbol is the correct answer here: if CONFIG_USB is
> >> set to =m, it would be impossible to include USB_ONBOARD_DEV
> >> in this case.
> >>
> >> Fabio, can you explain how making it built-in addresses the
> >> problem here? I assume this is related to probe order, so I
> >> wonder if it's just a matter of making the usb hub driver
> >> properly handle -EPROBE_DEFER until the onboard dev has been
> >> initialized.
> >
> > From drivers/usb/misc/Kconfig:
> >
> > "config USB_ONBOARD_DEV
> > tristate "Onboard USB device support"
> > .....
> >   This driver can be used as a module but its state (module vs
> >   builtin) must match the state of the USB subsystem. Enabling
> >   this config will enable the driver and it will automatically
> >   match the state of the USB subsystem. If this driver is a
> >   module it will be called onboard_usb_dev."
>
> Ok, so there is some kind of design mistake here that this
> is trying to paper over. Kbuild should always be powerful
> enough to enforce these things without having a person read
> a comment though.
>
> > In multi_v7_defconfig:
> > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch.
> >
> > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching.
> >
> > Is there any other way to solve this?
>
> I can think of multiple ways to enforce this in Kbuild:
>
> a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add
>    a hidden symbol that duplicates the state of CONFIG_USB when
>    USB_ONBOARD_DEV is enabled:
>
> config CONFIG_USB_ONBOARD_DEV_MODULE
>      def_tristate USB
>      depends on CONFIG_USB_ONBOARD_DEV
>
> b) Do the same thing but use Makefile syntax instead of Kconfig
>    syntax:
>
> usb-onboard-dev-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o
> obj-$(CONFIG_USB) += usb-onboard-dev.o
>
> (or something close to this, need to try).
>
>
> However, I still feel there should be a better way to solve
> the problem at the code level rather than the Kbuild level.
>
> I don't know exactly how a "usb_device_driver" (as opposed
> to a "usb_driver") works here, but from what I can tell,
> the idea here is that the USB bus probe finds a device, matches
> the driver and then continues probing it. If the onboard-dev
> driver gets loaded after the initial bus probe, that driver
> won't exist yet and it then runs into some error that
> prevents it from being retried when after the onboard-dev
> driver is there. If you can pinpoint exactly what error it
> runs into, try to make it retry the same thing after
> -EPROBE_DEFER.
>
>     Arnd
Matthias Kaehlcke May 2, 2024, 2:49 p.m. UTC | #7 
On Wed, May 1, 2024 at 4:04 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Adding Matthias, the onboard usb hub driver maintainer.
>
> On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote:
> > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > >> It does sound like this is something the kernel should be able to
> > >> get to work properly in some form, but I don't think making it
> > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is
> > >> set to =m, it would be impossible to include USB_ONBOARD_DEV
> > >> in this case.
> > >>
> > >> Fabio, can you explain how making it built-in addresses the
> > >> problem here? I assume this is related to probe order, so I
> > >> wonder if it's just a matter of making the usb hub driver
> > >> properly handle -EPROBE_DEFER until the onboard dev has been
> > >> initialized.
> > >
> > > From drivers/usb/misc/Kconfig:
> > >
> > > "config USB_ONBOARD_DEV
> > > tristate "Onboard USB device support"
> > > .....
> > >   This driver can be used as a module but its state (module vs
> > >   builtin) must match the state of the USB subsystem. Enabling
> > >   this config will enable the driver and it will automatically
> > >   match the state of the USB subsystem. If this driver is a
> > >   module it will be called onboard_usb_dev."
> >
> > Ok, so there is some kind of design mistake here that this
> > is trying to paper over. Kbuild should always be powerful
> > enough to enforce these things without having a person read
> > a comment though.

Agreed that it would be preferable to enforce this at Kbuild level (or
address it otherwise without having a person to read the comment).

Kconfig *build* dependencies (mutual dependencies between the USB core
and the onboard_hub/dev driver) were a major headache that stalled
landing the driver for a long time, with at least one revert after
landing.

The change log of the final version that landed reflects some of that ordeal:

https://lore.kernel.org/linux-usb/20220630123445.v24.3.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/

Eventually the build dependencies were fixed, but apparently there is
still a runtime issue :(

> > > In multi_v7_defconfig:
> > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch.
> > >
> > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching.
> > >
> > > Is there any other way to solve this?
> >
> > I can think of multiple ways to enforce this in Kbuild:
> >
> > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add
> >    a hidden symbol that duplicates the state of CONFIG_USB when
> >    USB_ONBOARD_DEV is enabled:
> >
> > config CONFIG_USB_ONBOARD_DEV_MODULE
> >      def_tristate USB
> >      depends on CONFIG_USB_ONBOARD_DEV
> >

An intermediate version of the driver had something similar:

config USB_ONBOARD_HUB
  bool "Onboard USB hub support"

  if USB_ONBOARD_HUB
  config USB_ONBOARD_HUB_ACTUAL
  tristate
  default m if USB=m
  default y if USB=y
  endif

https://lore.kernel.org/linux-usb/20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/

But it was removed later since the *build* dependency issues were
resolved otherwise.

We could add a construct like that again if we can't come up with a
better solution.

> > b) Do the same thing but use Makefile syntax instead of Kconfig
> >    syntax:
> >
> > usb-onboard-dev-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o
> > obj-$(CONFIG_USB) += usb-onboard-dev.o
> >
> > (or something close to this, need to try).
> >
> >
> > However, I still feel there should be a better way to solve
> > the problem at the code level rather than the Kbuild level.
> >
> > I don't know exactly how a "usb_device_driver" (as opposed
> > to a "usb_driver") works here, but from what I can tell,
> > the idea here is that the USB bus probe finds a device, matches
> > the driver and then continues probing it. If the onboard-dev
> > driver gets loaded after the initial bus probe, that driver
> > won't exist yet and it then runs into some error that
> > prevents it from being retried when after the onboard-dev
> > driver is there. If you can pinpoint exactly what error it
> > runs into, try to make it retry the same thing after
> > -EPROBE_DEFER.
> >
> >     Arnd
Arnd Bergmann May 2, 2024, 6:37 p.m. UTC | #8 
On Thu, May 2, 2024, at 16:49, Matthias Kaehlcke wrote:
> On Wed, May 1, 2024 at 4:04 PM Fabio Estevam <festevam@gmail.com> wrote:
>> On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> >
>> > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote:
>> > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
>> > >
>> > >> It does sound like this is something the kernel should be able to
>> > >> get to work properly in some form, but I don't think making it
>> > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is
>> > >> set to =m, it would be impossible to include USB_ONBOARD_DEV
>> > >> in this case.
>> > >>
>> > >> Fabio, can you explain how making it built-in addresses the
>> > >> problem here? I assume this is related to probe order, so I
>> > >> wonder if it's just a matter of making the usb hub driver
>> > >> properly handle -EPROBE_DEFER until the onboard dev has been
>> > >> initialized.
>> > >
>> > > From drivers/usb/misc/Kconfig:
>> > >
>> > > "config USB_ONBOARD_DEV
>> > > tristate "Onboard USB device support"
>> > > .....
>> > >   This driver can be used as a module but its state (module vs
>> > >   builtin) must match the state of the USB subsystem. Enabling
>> > >   this config will enable the driver and it will automatically
>> > >   match the state of the USB subsystem. If this driver is a
>> > >   module it will be called onboard_usb_dev."
>> >
>> > Ok, so there is some kind of design mistake here that this
>> > is trying to paper over. Kbuild should always be powerful
>> > enough to enforce these things without having a person read
>> > a comment though.
>
> Agreed that it would be preferable to enforce this at Kbuild level (or
> address it otherwise without having a person to read the comment).
>
> Kconfig *build* dependencies (mutual dependencies between the USB core
> and the onboard_hub/dev driver) were a major headache that stalled
> landing the driver for a long time, with at least one revert after
> landing.
>
> The change log of the final version that landed reflects some of that ordeal:
>
> https://lore.kernel.org/linux-usb/20220630123445.v24.3.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/
>
> Eventually the build dependencies were fixed, but apparently there is
> still a runtime issue :(

I can see what the link time problem was and how it's currently
being worked around. It would be nice to have something better
than the

ifdef CONFIG_USB_ONBOARD_DEV
usbcore-y                       += ../misc/onboard_usb_dev_pdevs.o
endif

bit, but this bit is obviously not the problem here.

>> > > In multi_v7_defconfig:
>> > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch.
>> > >
>> > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching.
>> > >
>> > > Is there any other way to solve this?
>> >
>> > I can think of multiple ways to enforce this in Kbuild:
>> >
>> > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add
>> >    a hidden symbol that duplicates the state of CONFIG_USB when
>> >    USB_ONBOARD_DEV is enabled:
>> >
>> > config CONFIG_USB_ONBOARD_DEV_MODULE
>> >      def_tristate USB
>> >      depends on CONFIG_USB_ONBOARD_DEV
>> >
>
> An intermediate version of the driver had something similar:
>
> config USB_ONBOARD_HUB
>   bool "Onboard USB hub support"
>
>   if USB_ONBOARD_HUB
>   config USB_ONBOARD_HUB_ACTUAL
>   tristate
>   default m if USB=m
>   default y if USB=y
>   endif

I think this is exactly the same think I wrote above, just written
in a more verbose way.

> https://lore.kernel.org/linux-usb/20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/
>
> But it was removed later since the *build* dependency issues were
> resolved otherwise.
>
> We could add a construct like that again if we can't come up with a
> better solution.

The bit I still don't understand is why the current code is
broken with CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m but not
with CONFIG_USB=m.

Most likely there is a runtime dependency that requires the
usb_onboard_dev module to be initialized before probing
any USB host controller that is a parent of an onboard hub.

This can work if you load the modules in the right order, but
I don't see this being actually enforced, so why doesn't
CONFIG_USB=m CONFIG_USB_ONBOARD_DEV=m fail if you load
xhci first. If the probe deferral for the hub works correctly,
why doesn't it work with USB=y?

     Arnd
Matthias Kaehlcke May 2, 2024, 9:45 p.m. UTC | #9 
On Thu, May 2, 2024 at 11:37 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, May 2, 2024, at 16:49, Matthias Kaehlcke wrote:
> > On Wed, May 1, 2024 at 4:04 PM Fabio Estevam <festevam@gmail.com> wrote:
> >> On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> >
> >> > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote:
> >> > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >> > >
> >> > >> It does sound like this is something the kernel should be able to
> >> > >> get to work properly in some form, but I don't think making it
> >> > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is
> >> > >> set to =m, it would be impossible to include USB_ONBOARD_DEV
> >> > >> in this case.
> >> > >>
> >> > >> Fabio, can you explain how making it built-in addresses the
> >> > >> problem here? I assume this is related to probe order, so I
> >> > >> wonder if it's just a matter of making the usb hub driver
> >> > >> properly handle -EPROBE_DEFER until the onboard dev has been
> >> > >> initialized.
> >> > >
> >> > > From drivers/usb/misc/Kconfig:
> >> > >
> >> > > "config USB_ONBOARD_DEV
> >> > > tristate "Onboard USB device support"
> >> > > .....
> >> > >   This driver can be used as a module but its state (module vs
> >> > >   builtin) must match the state of the USB subsystem. Enabling
> >> > >   this config will enable the driver and it will automatically
> >> > >   match the state of the USB subsystem. If this driver is a
> >> > >   module it will be called onboard_usb_dev."
> >> >
> >> > Ok, so there is some kind of design mistake here that this
> >> > is trying to paper over. Kbuild should always be powerful
> >> > enough to enforce these things without having a person read
> >> > a comment though.
> >
> > Agreed that it would be preferable to enforce this at Kbuild level (or
> > address it otherwise without having a person to read the comment).
> >
> > Kconfig *build* dependencies (mutual dependencies between the USB core
> > and the onboard_hub/dev driver) were a major headache that stalled
> > landing the driver for a long time, with at least one revert after
> > landing.
> >
> > The change log of the final version that landed reflects some of that ordeal:
> >
> > https://lore.kernel.org/linux-usb/20220630123445.v24.3.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/
> >
> > Eventually the build dependencies were fixed, but apparently there is
> > still a runtime issue :(
>
> I can see what the link time problem was and how it's currently
> being worked around. It would be nice to have something better
> than the
>
> ifdef CONFIG_USB_ONBOARD_DEV
> usbcore-y                       += ../misc/onboard_usb_dev_pdevs.o
> endif
>
> bit, but this bit is obviously not the problem here.
>
> >> > > In multi_v7_defconfig:
> >> > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch.
> >> > >
> >> > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching.
> >> > >
> >> > > Is there any other way to solve this?
> >> >
> >> > I can think of multiple ways to enforce this in Kbuild:
> >> >
> >> > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add
> >> >    a hidden symbol that duplicates the state of CONFIG_USB when
> >> >    USB_ONBOARD_DEV is enabled:
> >> >
> >> > config CONFIG_USB_ONBOARD_DEV_MODULE
> >> >      def_tristate USB
> >> >      depends on CONFIG_USB_ONBOARD_DEV
> >> >
> >
> > An intermediate version of the driver had something similar:
> >
> > config USB_ONBOARD_HUB
> >   bool "Onboard USB hub support"
> >
> >   if USB_ONBOARD_HUB
> >   config USB_ONBOARD_HUB_ACTUAL
> >   tristate
> >   default m if USB=m
> >   default y if USB=y
> >   endif
>
> I think this is exactly the same think I wrote above, just written
> in a more verbose way.

Could be, I tried a few variants and recall one less verbose one
didn't work as intended, but it could have been different from your
suggestion.

> > https://lore.kernel.org/linux-usb/20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/
> >
> > But it was removed later since the *build* dependency issues were
> > resolved otherwise.
> >
> > We could add a construct like that again if we can't come up with a
> > better solution.
>
> The bit I still don't understand is why the current code is
> broken with CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m but not
> with CONFIG_USB=m.
>
> Most likely there is a runtime dependency that requires the
> usb_onboard_dev module to be initialized before probing
> any USB host controller that is a parent of an onboard hub.

Indeed, there is a dependency:

Even though the onboard_usb_dev driver lives under drivers/usb its
core component is not a USB driver but a platform driver. The platform
driver is in charge of powering the onboard USB device on, so that the
parent hub can probe it. hub_probe() calls onboard_dev_create_pdevs(),
which creates platform devices for eligible USB devices with a device
tree node. onboard_dev_create_pdevs() is linked into the USB core to
avoid the aforementioned build dependency issues. IIUC the creation of
the platform device(s) should trigger the onboard_usb_dev driver being
loaded (if it wasn't already), which in turn would result in an
invocation of onboard_dev_probe(), which would power the onboard USB
device on.

It is also not clear to me why things would be broken with
CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It
doesn't seem to be an universal issue, I can't repro it locally.



> This can work if you load the modules in the right order, but
> I don't see this being actually enforced, so why doesn't
> CONFIG_USB=m CONFIG_USB_ONBOARD_DEV=m fail if you load
> xhci first. If the probe deferral for the hub works correctly,
> why doesn't it work with USB=y?
>
>      Arnd
Fabio Estevam May 2, 2024, 10:58 p.m. UTC | #10 
Hi Matthias,

On Thu, May 2, 2024 at 6:45 PM Matthias Kaehlcke <mka@chromium.org> wrote:

> It is also not clear to me why things would be broken with
> CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It
> doesn't seem to be an universal issue, I can't repro it locally.

This issue happens on an imx6q-udoo board.

multi_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m:
https://storage.kernelci.org/next/master/next-20240502/arm/multi_v7_defconfig+CONFIG_THUMB2_KERNEL=y/gcc-10/lab-broonie/baseline-imx6q-udoo.html

usb 1-1: device descriptor read/64, error -71
usb 1-1: device descriptor read/64, error -71
usb 1-1: new full-speed USB device number 3 using ci_hdrc
usb 1-1: device descriptor read/64, error -71
usb 1-1: device descriptor read/64, error -71
usb usb1-port1: attempt power cycle
usb 1-1: new full-speed USB device number 4 using ci_hdrc
usb 1-1: device not accepting address 4, error -71
usb 1-1: new full-speed USB device number 5 using ci_hdrc
usb 1-1: device not accepting address 5, error -71
usb usb1-port1: unable to enumerate USB device

imx_v6_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=y:
https://storage.kernelci.org/next/master/next-20240502/arm/imx_v6_v7_defconfig/gcc-10/lab-broonie/baseline-imx6q-udoo.html

In this case, the USB Wifi chip connected to the USB2514 hub is
correctly detected:

usb 1-1.3: new high-speed USB device number 3 using ci_hdrc
usb 1-1.3: New USB device found, idVendor=148f, idProduct=5370, bcdDevice= 1.01
usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 1-1.3: Product: 802.11 n WLAN
usb 1-1.3: Manufacturer: Ralink
usb 1-1.3: SerialNumber: 1.0
Matthias Kaehlcke May 3, 2024, 5:16 p.m. UTC | #11 
On Thu, May 2, 2024 at 3:59 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Matthias,
>
> On Thu, May 2, 2024 at 6:45 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> > It is also not clear to me why things would be broken with
> > CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It
> > doesn't seem to be an universal issue, I can't repro it locally.
>
> This issue happens on an imx6q-udoo board.
>
> multi_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m:
> https://storage.kernelci.org/next/master/next-20240502/arm/multi_v7_defconfig+CONFIG_THUMB2_KERNEL=y/gcc-10/lab-broonie/baseline-imx6q-udoo.html
>
> usb 1-1: device descriptor read/64, error -71
> usb 1-1: device descriptor read/64, error -71
> usb 1-1: new full-speed USB device number 3 using ci_hdrc
> usb 1-1: device descriptor read/64, error -71
> usb 1-1: device descriptor read/64, error -71
> usb usb1-port1: attempt power cycle
> usb 1-1: new full-speed USB device number 4 using ci_hdrc
> usb 1-1: device not accepting address 4, error -71
> usb 1-1: new full-speed USB device number 5 using ci_hdrc
> usb 1-1: device not accepting address 5, error -71
> usb usb1-port1: unable to enumerate USB device
>
> imx_v6_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=y:
> https://storage.kernelci.org/next/master/next-20240502/arm/imx_v6_v7_defconfig/gcc-10/lab-broonie/baseline-imx6q-udoo.html
>
> In this case, the USB Wifi chip connected to the USB2514 hub is
> correctly detected:
>
> usb 1-1.3: new high-speed USB device number 3 using ci_hdrc
> usb 1-1.3: New USB device found, idVendor=148f, idProduct=5370, bcdDevice= 1.01
> usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> usb 1-1.3: Product: 802.11 n WLAN
> usb 1-1.3: Manufacturer: Ralink
> usb 1-1.3: SerialNumber: 1.0

Ah, from the discussion my impression was that CONFIG_USB=y and
CONFIG_USB_ONBOARD_DEV=m works, but not  CONFIG_USB=m and
CONFIG_USB_ONBOARD_DEV=m, good we clarified that.

Is the rootfs by chance on a USB device and that prevents the
onboard_usb_dev module from being loaded?

You could add debug logs to hub_probe(), onboard_dev_init() and
onboard_dev_probe() to get more information.
Fabio Estevam May 3, 2024, 5:51 p.m. UTC | #12 
On Fri, May 3, 2024 at 2:16 PM Matthias Kaehlcke <mka@chromium.org> wrote:

> Ah, from the discussion my impression was that CONFIG_USB=y and
> CONFIG_USB_ONBOARD_DEV=m works, but not  CONFIG_USB=m and
> CONFIG_USB_ONBOARD_DEV=m, good we clarified that.
>
> Is the rootfs by chance on a USB device and that prevents the
> onboard_usb_dev module from being loaded?

No, the logs show the rootfs come from a ramdisk: root=/dev/ram0.
Matthias Kaehlcke May 3, 2024, 8:11 p.m. UTC | #13 
On Thu, May 02, 2024 at 07:58:57PM -0300, Fabio Estevam wrote:
> Hi Matthias,
> 
> On Thu, May 2, 2024 at 6:45 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > It is also not clear to me why things would be broken with
> > CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It
> > doesn't seem to be an universal issue, I can't repro it locally.
> 
> This issue happens on an imx6q-udoo board.
> 
> multi_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m:
> https://storage.kernelci.org/next/master/next-20240502/arm/multi_v7_defconfig+CONFIG_THUMB2_KERNEL=y/gcc-10/lab-broonie/baseline-imx6q-udoo.html
> 
> usb 1-1: device descriptor read/64, error -71
> usb 1-1: device descriptor read/64, error -71
> usb 1-1: new full-speed USB device number 3 using ci_hdrc
> usb 1-1: device descriptor read/64, error -71
> usb 1-1: device descriptor read/64, error -71
> usb usb1-port1: attempt power cycle
> usb 1-1: new full-speed USB device number 4 using ci_hdrc
> usb 1-1: device not accepting address 4, error -71
> usb 1-1: new full-speed USB device number 5 using ci_hdrc
> usb 1-1: device not accepting address 5, error -71
> usb usb1-port1: unable to enumerate USB device
> 
> imx_v6_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=y:
> https://storage.kernelci.org/next/master/next-20240502/arm/imx_v6_v7_defconfig/gcc-10/lab-broonie/baseline-imx6q-udoo.html
> 
> In this case, the USB Wifi chip connected to the USB2514 hub is
> correctly detected:
> 
> usb 1-1.3: new high-speed USB device number 3 using ci_hdrc
> usb 1-1.3: New USB device found, idVendor=148f, idProduct=5370, bcdDevice= 1.01
> usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> usb 1-1.3: Product: 802.11 n WLAN
> usb 1-1.3: Manufacturer: Ralink
> usb 1-1.3: SerialNumber: 1.0


Here are some debug logs from my side with CONFIG_USB_ONBOARD_DEV=m:

[    0.755965] DBG: hub_probe: adding onboard pdevs
[    0.756204] DBG: hub_probe: done
[    0.756618] DBG: hub_probe: adding onboard pdevs
[    0.756621] DBG: hub_probe: done
[    8.094539] DBG: onboard_dev_init
[    9.141729] DBG: onboard_dev_probe
[    9.142237] DBG: onboard_dev_probe (done)
[    9.142428] DBG: onboard_dev_init (done)

The root hub adds the onboard pdev at 0.75..., but the onboard_dev
module is only loaded more than 7s later (and probed even later). In
the meantime there are no errors of failed enumerations as seen on
the imx6q-udoo.

I wonder if the problem could be that the sense resistors of the hub
on the imx6q-udoo are always active and not only when the hub is
powered, indicating the controller the presence of a device, which
results in an attempt to enumerate it.
Fabio Estevam May 3, 2024, 9:13 p.m. UTC | #14 
On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote:

> Here are some debug logs from my side with CONFIG_USB_ONBOARD_DEV=m:
>
> [    0.755965] DBG: hub_probe: adding onboard pdevs
> [    0.756204] DBG: hub_probe: done
> [    0.756618] DBG: hub_probe: adding onboard pdevs
> [    0.756621] DBG: hub_probe: done
> [    8.094539] DBG: onboard_dev_init
> [    9.141729] DBG: onboard_dev_probe
> [    9.142237] DBG: onboard_dev_probe (done)
> [    9.142428] DBG: onboard_dev_init (done)
>
> The root hub adds the onboard pdev at 0.75..., but the onboard_dev
> module is only loaded more than 7s later (and probed even later). In
> the meantime there are no errors of failed enumerations as seen on
> the imx6q-udoo.

Thanks for investigating.

I haven't had a chance to extract these logs on the imx6q-udoo board yet.

> I wonder if the problem could be that the sense resistors of the hub
> on the imx6q-udoo are always active and not only when the hub is
> powered, indicating the controller the presence of a device, which
> results in an attempt to enumerate it.

The  imx6q-udoo schematics are here:
https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf
Matthias Kaehlcke May 6, 2024, 2:54 p.m. UTC | #15 
On Fri, May 03, 2024 at 06:13:32PM -0300, Fabio Estevam wrote:
> On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > Here are some debug logs from my side with CONFIG_USB_ONBOARD_DEV=m:
> >
> > [    0.755965] DBG: hub_probe: adding onboard pdevs
> > [    0.756204] DBG: hub_probe: done
> > [    0.756618] DBG: hub_probe: adding onboard pdevs
> > [    0.756621] DBG: hub_probe: done
> > [    8.094539] DBG: onboard_dev_init
> > [    9.141729] DBG: onboard_dev_probe
> > [    9.142237] DBG: onboard_dev_probe (done)
> > [    9.142428] DBG: onboard_dev_init (done)
> >
> > The root hub adds the onboard pdev at 0.75..., but the onboard_dev
> > module is only loaded more than 7s later (and probed even later). In
> > the meantime there are no errors of failed enumerations as seen on
> > the imx6q-udoo.
> 
> Thanks for investigating.
> 
> I haven't had a chance to extract these logs on the imx6q-udoo board yet.
> 
> > I wonder if the problem could be that the sense resistors of the hub
> > on the imx6q-udoo are always active and not only when the hub is
> > powered, indicating the controller the presence of a device, which
> > results in an attempt to enumerate it.
> 
> The  imx6q-udoo schematics are here:
> https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf

There are no external pull-ups on USB_H1_DP/USB_H1_DN, maybe internal
pulls are enabled when the hub isn't powered?
Mark Brown May 6, 2024, 3:04 p.m. UTC | #16 
On Mon, May 06, 2024 at 02:54:12PM +0000, Matthias Kaehlcke wrote:
> On Fri, May 03, 2024 at 06:13:32PM -0300, Fabio Estevam wrote:
> > On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote:

> > > I wonder if the problem could be that the sense resistors of the hub
> > > on the imx6q-udoo are always active and not only when the hub is
> > > powered, indicating the controller the presence of a device, which
> > > results in an attempt to enumerate it.

> > The  imx6q-udoo schematics are here:
> > https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf

> There are no external pull-ups on USB_H1_DP/USB_H1_DN, maybe internal
> pulls are enabled when the hub isn't powered?

I do note that the vendor BSP worked (and I think also some older
mainline versions too?).
Matthias Kaehlcke May 6, 2024, 3:58 p.m. UTC | #17 
On Tue, May 07, 2024 at 12:04:27AM +0900, Mark Brown wrote:
> On Mon, May 06, 2024 at 02:54:12PM +0000, Matthias Kaehlcke wrote:
> > On Fri, May 03, 2024 at 06:13:32PM -0300, Fabio Estevam wrote:
> > > On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > > > I wonder if the problem could be that the sense resistors of the hub
> > > > on the imx6q-udoo are always active and not only when the hub is
> > > > powered, indicating the controller the presence of a device, which
> > > > results in an attempt to enumerate it.
> 
> > > The  imx6q-udoo schematics are here:
> > > https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf
> 
> > There are no external pull-ups on USB_H1_DP/USB_H1_DN, maybe internal
> > pulls are enabled when the hub isn't powered?
> 
> I do note that the vendor BSP worked (and I think also some older
> mainline versions too?).

The official BSP releases pre-date the onboard_usb_dev/hub driver:
https://www.udoo.org/resources-quad-dual/. I guess they use a fixed
regulator hack to power the hub on at boot.

Yup, upstream also still uses a regulator hack:

        reg_usb_h1_vbus: regulator-usb-h1-vbus {
                compatible = "regulator-fixed";
                regulator-name = "usb_h1_vbus";
                regulator-min-microvolt = <5000000>;
                regulator-max-microvolt = <5000000>;
                enable-active-high;
                startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */
                gpio = <&gpio7 12 0>;
        };

  arch/arm/boot/dts/nxp/imx/imx6qdl-udoo.dtsi
diff mbox series

Patch

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 86bf057ac366..1cc5f2c1a0c6 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -885,7 +885,7 @@  CONFIG_USB_CHIPIDEA_UDC=y
 CONFIG_USB_CHIPIDEA_HOST=y
 CONFIG_USB_ISP1760=y
 CONFIG_USB_HSIC_USB3503=y
-CONFIG_USB_ONBOARD_DEV=m
+CONFIG_USB_ONBOARD_DEV=y
 CONFIG_AB8500_USB=y
 CONFIG_KEYSTONE_USB_PHY=m
 CONFIG_NOP_USB_XCEIV=y