| Message ID | 20210125113248.2268952-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: host: ehci-tegra: fix Kconfig depencies | expand |
On Mon, Jan 25, 2021 at 12:32:30PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Selecting the chipidea driver from the old Kconfig symbol > can lead to a missing dependency: Arnd: I found this whole patch a little confusing. For example, in the sentence above, what does "the old Kconfig symbol" refer to? Comparing the various Kconfig files, I see what the problem is. The commit which this one fixes made CONFIG_EHCI_TEGRA select CONFIG_USB_CHIPIDEA, but it didn't make EHCI_TEGRA depend on the things that USB_CHIPIDEA depends on. Can you please state this more explicitly in the patch description? > WARNING: unmet direct dependencies detected for USB_CHIPIDEA > Depends on [m]: USB_SUPPORT [=y] && (USB_EHCI_HCD [=y] && USB_GADGET [=m] || USB_EHCI_HCD [=y] && !USB_GADGET [=m] || !USB_EHCI_HCD [=y] && USB_GADGET [=m]) && HAS_DMA [=y] > Selected by [y]: > - USB_EHCI_TEGRA [=y] && USB_SUPPORT [=y] && USB [=y] && USB_EHCI_HCD [=y] && ARCH_TEGRA [=y] > aarch64-linux-ld: drivers/usb/chipidea/otg.o: in function `ci_handle_vbus_change': > otg.c:(.text+0x3c8): undefined reference to `usb_gadget_vbus_connect' > aarch64-linux-ld: otg.c:(.text+0x42c): undefined reference to `usb_gadget_vbus_disconnect' > aarch64-linux-ld: drivers/usb/chipidea/otg.o: in function `ci_otg_work': > otg.c:(.text+0x5d4): undefined reference to `usb_gadget_vbus_disconnect' > ... > > Duplicate the dependency to ensure that this driver can > only be a loadable module if one of its dependencies is. > > Fixes: c3590c7656fb ("usb: host: ehci-tegra: Remove the driver") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/usb/host/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 160e5d3927e2..66b01b619ecd 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -269,6 +269,7 @@ config USB_EHCI_HCD_AT91 > config USB_EHCI_TEGRA > tristate "NVIDIA Tegra HCD support" > depends on ARCH_TEGRA > + depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA > select USB_CHIPIDEA > select USB_CHIPIDEA_HOST > select USB_CHIPIDEA_TEGRA Isn't there at least one other missing dependency? This entry selects USB_CHIPIDEA_TEGRA, which depends on OF. So shouldn't this entry also depend on OF? Or does the Kconfig system detect that for us? Also, while I'm no expert on the Kconfig language, it seems that the new "depends" line could be a lot easier to understand if it was refactored with some comments added. Yes, I realize you just copied the existing dependency from the USB_CHIPIDEA entry -- that one could stand to be cleaned up as well. For instance, how about putting the HAS_DMA part into a separate line, since it's unrelated to the other stuff? And the rest looks like it could be changed to: depends on USB_EHCI_HCD || USB_GADGET although that probably isn't quite valid. Still, can't it be changed to something simpler than (USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET) ? Alan Stern
On Mon, Jan 25, 2021 at 5:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Jan 25, 2021 at 12:32:30PM +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > Selecting the chipidea driver from the old Kconfig symbol > > can lead to a missing dependency: > > Arnd: > > I found this whole patch a little confusing. For example, in the > sentence above, what does "the old Kconfig symbol" refer to? > > Comparing the various Kconfig files, I see what the problem is. The > commit which this one fixes made CONFIG_EHCI_TEGRA select > CONFIG_USB_CHIPIDEA, but it didn't make EHCI_TEGRA depend on the things > that USB_CHIPIDEA depends on. Can you please state this more explicitly > in the patch description? Sure, I'll resend in a bit. > > WARNING: unmet direct dependencies detected for USB_CHIPIDEA > > Depends on [m]: USB_SUPPORT [=y] && (USB_EHCI_HCD [=y] && USB_GADGET [=m] || USB_EHCI_HCD [=y] && !USB_GADGET [=m] || !USB_EHCI_HCD [=y] && USB_GADGET [=m]) && HAS_DMA [=y] > > Selected by [y]: > > - USB_EHCI_TEGRA [=y] && USB_SUPPORT [=y] && USB [=y] && USB_EHCI_HCD [=y] && ARCH_TEGRA [=y] > > aarch64-linux-ld: drivers/usb/chipidea/otg.o: in function `ci_handle_vbus_change': > > otg.c:(.text+0x3c8): undefined reference to `usb_gadget_vbus_connect' > > aarch64-linux-ld: otg.c:(.text+0x42c): undefined reference to `usb_gadget_vbus_disconnect' > > aarch64-linux-ld: drivers/usb/chipidea/otg.o: in function `ci_otg_work': > > otg.c:(.text+0x5d4): undefined reference to `usb_gadget_vbus_disconnect' > > ... > > > > Duplicate the dependency to ensure that this driver can > > only be a loadable module if one of its dependencies is. > > > > Fixes: c3590c7656fb ("usb: host: ehci-tegra: Remove the driver") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/usb/host/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > > index 160e5d3927e2..66b01b619ecd 100644 > > --- a/drivers/usb/host/Kconfig > > +++ b/drivers/usb/host/Kconfig > > @@ -269,6 +269,7 @@ config USB_EHCI_HCD_AT91 > > config USB_EHCI_TEGRA > > tristate "NVIDIA Tegra HCD support" > > depends on ARCH_TEGRA > > + depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA > > select USB_CHIPIDEA > > select USB_CHIPIDEA_HOST > > select USB_CHIPIDEA_TEGRA > > Isn't there at least one other missing dependency? This entry selects > USB_CHIPIDEA_TEGRA, which depends on OF. So shouldn't this entry also > depend on OF? Or does the Kconfig system detect that for us? Yes, there is a hard dependency on ARCH_TEGRA, which implies OF. > Also, while I'm no expert on the Kconfig language, it seems that the new > "depends" line could be a lot easier to understand if it was refactored > with some comments added. Yes, I realize you just copied the existing > dependency from the USB_CHIPIDEA entry -- that one could stand to be > cleaned up as well. > > For instance, how about putting the HAS_DMA part into a separate line, > since it's unrelated to the other stuff? Actually it's probably best to just drop the HAS_DMA here, as it is also implied by ARCH_TEGRA. > And the rest looks like it > could be changed to: > > depends on USB_EHCI_HCD || USB_GADGET > > although that probably isn't quite valid. Still, can't it be changed to > something simpler than > > (USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || > (!USB_EHCI_HCD && USB_GADGET) The problem is that if either USB_EHCI_HCD or USB_GADGET are loadable modules, then USB_CHIPIDEA must not be built-in, but if one of the two is disabled, we must still have a dependency on the other. I guess it could be rewritten into depends on USB_EHCI_HCD || USB_GADGET # needs at least one of the two depends on m || USB_EHCI_HCD!=m # must be =m if USB_EHCI_HCD=m depends on m || USB_GADGET!=m # must be =m if USB_GADGET=m I see that USB_EHCI_TEGRA already depends on USB_EHCI_HCD, so I think this collapses into a much simpler depends on USB_GADGET || USB_GADGET=n # for USB_CHIPIDEA Arnd
26.01.2021 00:10, Arnd Bergmann пишет: > On Mon, Jan 25, 2021 at 5:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: >> >> On Mon, Jan 25, 2021 at 12:32:30PM +0100, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> Selecting the chipidea driver from the old Kconfig symbol >>> can lead to a missing dependency: >> >> Arnd: >> >> I found this whole patch a little confusing. For example, in the >> sentence above, what does "the old Kconfig symbol" refer to? >> >> Comparing the various Kconfig files, I see what the problem is. The >> commit which this one fixes made CONFIG_EHCI_TEGRA select >> CONFIG_USB_CHIPIDEA, but it didn't make EHCI_TEGRA depend on the things >> that USB_CHIPIDEA depends on. Can you please state this more explicitly >> in the patch description? > > Sure, I'll resend in a bit. > >>> WARNING: unmet direct dependencies detected for USB_CHIPIDEA >>> Depends on [m]: USB_SUPPORT [=y] && (USB_EHCI_HCD [=y] && USB_GADGET [=m] || USB_EHCI_HCD [=y] && !USB_GADGET [=m] || !USB_EHCI_HCD [=y] && USB_GADGET [=m]) && HAS_DMA [=y] >>> Selected by [y]: >>> - USB_EHCI_TEGRA [=y] && USB_SUPPORT [=y] && USB [=y] && USB_EHCI_HCD [=y] && ARCH_TEGRA [=y] >>> aarch64-linux-ld: drivers/usb/chipidea/otg.o: in function `ci_handle_vbus_change': >>> otg.c:(.text+0x3c8): undefined reference to `usb_gadget_vbus_connect' >>> aarch64-linux-ld: otg.c:(.text+0x42c): undefined reference to `usb_gadget_vbus_disconnect' >>> aarch64-linux-ld: drivers/usb/chipidea/otg.o: in function `ci_otg_work': >>> otg.c:(.text+0x5d4): undefined reference to `usb_gadget_vbus_disconnect' >>> ... >>> >>> Duplicate the dependency to ensure that this driver can >>> only be a loadable module if one of its dependencies is. >>> >>> Fixes: c3590c7656fb ("usb: host: ehci-tegra: Remove the driver") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> --- >>> drivers/usb/host/Kconfig | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig >>> index 160e5d3927e2..66b01b619ecd 100644 >>> --- a/drivers/usb/host/Kconfig >>> +++ b/drivers/usb/host/Kconfig >>> @@ -269,6 +269,7 @@ config USB_EHCI_HCD_AT91 >>> config USB_EHCI_TEGRA >>> tristate "NVIDIA Tegra HCD support" >>> depends on ARCH_TEGRA >>> + depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA >>> select USB_CHIPIDEA >>> select USB_CHIPIDEA_HOST >>> select USB_CHIPIDEA_TEGRA >> >> Isn't there at least one other missing dependency? This entry selects >> USB_CHIPIDEA_TEGRA, which depends on OF. So shouldn't this entry also >> depend on OF? Or does the Kconfig system detect that for us? > > Yes, there is a hard dependency on ARCH_TEGRA, which implies OF. > >> Also, while I'm no expert on the Kconfig language, it seems that the new >> "depends" line could be a lot easier to understand if it was refactored >> with some comments added. Yes, I realize you just copied the existing >> dependency from the USB_CHIPIDEA entry -- that one could stand to be >> cleaned up as well. >> >> For instance, how about putting the HAS_DMA part into a separate line, >> since it's unrelated to the other stuff? > > Actually it's probably best to just drop the HAS_DMA here, as it is also > implied by ARCH_TEGRA. > >> And the rest looks like it >> could be changed to: >> >> depends on USB_EHCI_HCD || USB_GADGET >> >> although that probably isn't quite valid. Still, can't it be changed to >> something simpler than >> >> (USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || >> (!USB_EHCI_HCD && USB_GADGET) > > The problem is that if either USB_EHCI_HCD or USB_GADGET are loadable > modules, then USB_CHIPIDEA must not be built-in, but if one of the two is > disabled, we must still have a dependency on the other. I guess it could be > rewritten into > > depends on USB_EHCI_HCD || USB_GADGET # needs at least one of the two > depends on m || USB_EHCI_HCD!=m # must be =m if USB_EHCI_HCD=m > depends on m || USB_GADGET!=m # must be =m if USB_GADGET=m > > I see that USB_EHCI_TEGRA already depends on USB_EHCI_HCD, > so I think this collapses into a much simpler > > depends on USB_GADGET || USB_GADGET=n # for USB_CHIPIDEA > > Arnd > It looks like the Chipidea's Kconfig is the problem here because it unconditionally compiles otg.c which uses usb_gadget_* API, but the Kconfig doesn't select nor depend on the USB_GADGET properly.
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 160e5d3927e2..66b01b619ecd 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -269,6 +269,7 @@ config USB_EHCI_HCD_AT91 config USB_EHCI_TEGRA tristate "NVIDIA Tegra HCD support" depends on ARCH_TEGRA + depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA select USB_CHIPIDEA select USB_CHIPIDEA_HOST select USB_CHIPIDEA_TEGRA