Re: [PATCH] usb: cdns3: attempt to fix Kconfig dependencies

["Peter Chen (CIX)" <peter.chen@kernel.org>]

On 26-04-02 16:09:55, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The way that dependencies between host and gadget mode, as well as cdns3
> and cdnsp were handled was rather fragile before commit 6076388ca1ed
> ("usb: cdns3: Add USBSSP platform driver support").
> 
> After those changes, I get randconfig build failures:
> 
> arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `__cdnsp_gadget_init':
> cdnsp-gadget.c:(.text+0x12da): undefined reference to `cdns_drd_gadget_on'
> arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_pullup':
> cdnsp-gadget.c:(.text+0x3030): undefined reference to `cdns_clear_vbus'
> arm-linux-gnueabi-ld: cdnsp-gadget.c:(.text+0x3138): undefined reference to `cdns_set_vbus'
> arm-linux-gnueabi-ld: drivers/usb/cdns3/cdnsp-gadget.o: in function `cdnsp_gadget_exit':
> cdnsp-gadget.c:(.text+0xe0): undefined reference to `cdns_drd_gadget_off'
> 
> and I see additional configurations that are broken. The main problem
> here is that the 'common' module links against both host and gadget
> support if they are enabled, but there are insufficient protections
> agains it being built-in if only one of them is built-in and the other
> is in a loadable module, causing link failures.
> 
> The use of IS_REACHABLE() in gadget-export.h works around a similar
> problem if one of cdns3 and cdnsp is built-in but the other one is
> =m. This one is worse because instead of a clear link failure, the
> logic just makes it not work at all despite support being enabled.
> 
> To improve this mess, throw out both the Makefile hacks and the
> IS_REACHABLE() hack and replace these with regular Kconfig dependencies
> that ensure each driver is only enabled when its dependencies are there,
> as we do in most other drivers. The main downside here is that there is no
> good way to have built-in gadget support on cdn3 along with USB=m. Fixing
> this part proper would require cleaning up the code to turn the 'common'
> parts into a library module that only gets called by the other drivers
> but does not interact with either host or gadget support itself.
> 
> Another problem that is not solved by this patch is the way that
> platform specific glue logic in this driver relies on having
> a soc specific device as the parent of a generic child, instead of
> the specific driver just calling into a common helper module.
> This may be impossible to fix without breaking the DT bindings.
> 
> Fixes: 6076388ca1ed ("usb: cdns3: Add USBSSP platform driver support")

Hi Arnd,

Thanks for fixing it, I am sorry for taking your effort debug it.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> TBH, I would be more comfortable with reverting 6076388ca1ed altogether
> and asking for a new version with the proper fixups included along
> with more testing for the next merge window.

It depends on Greg, I am okay for both ways. If Greg reverts the patch,
I will do below improvements and adapts for most of your changes for v3
patch.

cdns-usb-common.ko is a libary, and no USB/GADGET dependency, could builds in.
  ├── core.o  
  └── drd.o

cdns3-host.ko     -> depends on USB/XHCI（it is m when USB = m
cdns3.ko (gadget) -> depends on USB_GADGET
cdnsp.ko (gadget) -> depends on USB_GADGET
cdns3-plat.ko     -> assign host_init/gadget_init function pointer

In that way, below combination could work:
cdns-usb-common=y
cdns3-host=m
cdns3(p)-gadget=y

> @@ -10,12 +11,24 @@ config USB_CDNS_SUPPORT
>  
>  config USB_CDNS_HOST
>  	bool
> +	depends on USB=y || USB=USB_CDNS_SUPPORT
> +
> +config CONFIG_USB_CDNS_PLATFORM

%s/CONFIG_USB_CDNS_PLATFORM/USB_CDNS_PLATFORM

Peter

> +	tristate "Cadence USB3 generic platform support"
> +	depends on USB_CDNSP || USB_CDNS3
> +	depends on USB_CDNSP || !USB_CDNSP
> +	depends on USB_CDNS3 || !USB_CDNS3
> +	help
> +	  The platform driver support is needed on any SoC integrating
> +	  a variant of the Cadence USB3 or USBSSP dual-role controllers,
> +	  e.g. the TI K3 or NXP i.MX series of SoCs.
>  
>  if USB_CDNS_SUPPORT
>  
>  config USB_CDNS3
>  	tristate "Cadence USB3 Dual-Role Controller"
>  	depends on USB_CDNS_SUPPORT
> +	select USB_CDNS_HOST if USB_CDNS3_HOST
>  	help
>  	  Say Y here if your system has a Cadence USB3 dual-role controller.
>  	  It supports: dual-role switch, Host-only, and Peripheral-only.
> @@ -23,8 +36,9 @@ config USB_CDNS3
>  if USB_CDNS3
>  
>  config USB_CDNS3_GADGET
> -	bool "Cadence USB3 device controller"
> +	tristate "Cadence USB3 device controller"
>  	depends on USB_GADGET=y || USB_GADGET=USB_CDNS3
> +	depends on USB_CDNS_SUPPORT
>  	help
>  	  Say Y here to enable device controller functionality of the
>  	  Cadence USBSS-DEV driver.
> @@ -34,8 +48,7 @@ config USB_CDNS3_GADGET
>  
>  config USB_CDNS3_HOST
>  	bool "Cadence USB3 host controller"
> -	depends on USB=y || USB=USB_CDNS3
> -	select USB_CDNS_HOST
> +	depends on USB=y || USB=USB_CDNS_SUPPORT
>  	help
>  	  Say Y here to enable host controller functionality of the
>  	  Cadence driver.
> @@ -57,6 +70,7 @@ config USB_CDNS3_PCI_WRAP
>  config USB_CDNS3_TI
>  	tristate "Cadence USB3 support on TI platforms"
>  	depends on ARCH_K3 || COMPILE_TEST
> +	depends on USB_CDNS_PLATFORM
>  	default USB_CDNS3
>  	help
>  	  Say 'Y' or 'M' here if you are building for Texas Instruments
> @@ -67,6 +81,7 @@ config USB_CDNS3_TI
>  config USB_CDNS3_IMX
>  	tristate "Cadence USB3 support on NXP i.MX platforms"
>  	depends on ARCH_MXC || COMPILE_TEST
> +	depends on USB_CDNS_PLATFORM
>  	default USB_CDNS3
>  	help
>  	  Say 'Y' or 'M' here if you are building for NXP i.MX
> @@ -77,6 +92,7 @@ config USB_CDNS3_IMX
>  config USB_CDNS3_STARFIVE
>  	tristate "Cadence USB3 support on StarFive SoC platforms"
>  	depends on ARCH_STARFIVE || COMPILE_TEST
> +	depends on USB_CDNS_PLATFORM
>  	help
>  	  Say 'Y' or 'M' here if you are building for StarFive SoCs
>  	  platforms that contain Cadence USB3 controller core.
> @@ -91,6 +107,7 @@ endif # USB_CDNS3
>  config USB_CDNSP
>  	tristate "Cadence USBSSP Dual-Role Controller"
>  	depends on USB_CDNS_SUPPORT
> +	select USB_CDNS_HOST if USB_CDNSP_HOST
>  	help
>  	  Say Y here if your system has a Cadence USBSSP dual-role controller.
>  	  It supports: dual-role switch, Host-only, and Peripheral-only.
> @@ -115,8 +132,7 @@ config USB_CDNSP_GADGET
>  
>  config USB_CDNSP_HOST
>  	bool "Cadence USBSSP host controller"
> -	depends on USB=y || USB=USB_CDNSP
> -	select USB_CDNS_HOST
> +	depends on USB=y || USB=USB_CDNS_SUPPORT
>  	help
>  	  Say Y here to enable host controller functionality of the
>  	  Cadence driver.
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index 63484f145bb9..f1e00ab3d729 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -5,32 +5,25 @@ CFLAGS_cdnsp-trace.o				:= -I$(src)
>  
>  cdns-usb-common-y				:= core.o drd.o
>  
> -ifeq ($(CONFIG_USB),m)
> -obj-m						+= cdns-usb-common.o
> -obj-m						+= cdns3-plat.o
> -else
>  obj-$(CONFIG_USB_CDNS_SUPPORT)			+= cdns-usb-common.o
> -obj-$(CONFIG_USB_CDNS_SUPPORT)			+= cdns3-plat.o
> -endif
> +
> +obj-$(CONFIG_USB_CDNS_PLATFORM)			+= cdns3-plat.o
>  
>  cdns-usb-common-$(CONFIG_USB_CDNS_HOST) 	+= host.o
>  
>  # For CDNS3 gadget
> -ifneq ($(CONFIG_USB_CDNS3_GADGET),)
>  cdns3-y						:= cdns3-gadget.o cdns3-ep0.o
>  cdns3-$(CONFIG_TRACING)				+= cdns3-trace.o
> -obj-$(CONFIG_USB_CDNS3)				+= cdns3.o
> -endif
> +obj-$(CONFIG_USB_CDNS3_GADGET)			+= cdns3.o
> +
>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)		+= cdns3-pci-wrap.o
>  obj-$(CONFIG_USB_CDNS3_TI)			+= cdns3-ti.o
>  obj-$(CONFIG_USB_CDNS3_IMX)			+= cdns3-imx.o
>  obj-$(CONFIG_USB_CDNS3_STARFIVE)		+= cdns3-starfive.o
>  
>  # For CDNSP gadget
> -ifneq ($(CONFIG_USB_CDNSP_GADGET),)
>  cdnsp-y						:= cdnsp-ring.o cdnsp-gadget.o \
>  						   cdnsp-mem.o cdnsp-ep0.o
>  cdnsp-$(CONFIG_TRACING)				+= cdnsp-trace.o
> -obj-$(CONFIG_USB_CDNSP)				+= cdnsp.o
> -endif
> +obj-$(CONFIG_USB_CDNSP_GADGET)			+= cdnsp.o
>  obj-$(CONFIG_USB_CDNSP_PCI)			+= cdnsp-pci.o
> diff --git a/drivers/usb/cdns3/gadget-export.h b/drivers/usb/cdns3/gadget-export.h
> index 0cb600e2b5d2..c37b6269b001 100644
> --- a/drivers/usb/cdns3/gadget-export.h
> +++ b/drivers/usb/cdns3/gadget-export.h
> @@ -10,7 +10,7 @@
>  #ifndef __LINUX_CDNS3_GADGET_EXPORT
>  #define __LINUX_CDNS3_GADGET_EXPORT
>  
> -#if defined(CONFIG_USB_CDNSP_GADGET) && IS_REACHABLE(CONFIG_USB_CDNSP)
> +#if IS_ENABLED(CONFIG_USB_CDNSP_GADGET)
>  
>  int cdnsp_gadget_init(struct cdns *cdns);
>  #else
> @@ -22,7 +22,7 @@ static inline int cdnsp_gadget_init(struct cdns *cdns)
>  
>  #endif /* CONFIG_USB_CDNSP_GADGET */
>  
> -#if defined(CONFIG_USB_CDNS3_GADGET) && IS_REACHABLE(CONFIG_USB_CDNS3)
> +#if IS_ENABLED(CONFIG_USB_CDNS3_GADGET)
>  
>  int cdns3_gadget_init(struct cdns *cdns);
>  #else
> -- 
> 2.39.5
> 

-- 

Best regards,
Peter