[PATCH] ARM: multi_v7_defconfig: Select CONFIG_USB_ONBOARD_DEV as built-in

Thu May 2 14:45:24 PDT 2024

On Thu, May 2, 2024 at 11:37 AM Arnd Bergmann <arnd at 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 at gmail.com> wrote:
> >> On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd at 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 at 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