Re: [PATCH] usb: host: ehci-tegra: fix Kconfig depencies

[Dmitry Osipenko]

26.01.2021 00:10, Arnd Bergmann пишет:
> On Mon, Jan 25, 2021 at 5:22 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>>
>> On Mon, Jan 25, 2021 at 12:32:30PM +0100, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@xxxxxxxx>
>>>
>>> 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@xxxxxxxx>
>>> ---
>>>  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.