diff mbox series

[1/3] usb: hub: add infrastructure to pass onboard_dev port features

Message ID 20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9@pengutronix.de
State New
Headers show
Series External VBUS port power handling for onboard USB devices | expand

Commit Message

Marco Felsch Aug. 7, 2024, 2:36 p.m. UTC
On board devices may require special handling for en-/disable port
features due to PCB design decisions e.g. enable/disable the VBUS power
on the port. This commit adds the necessary infrastructure to prepare
the common code base for such use-cases.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/core/hub.c             | 22 ++++++++++++++++++++--
 drivers/usb/misc/onboard_usb_dev.c | 13 +++++++++++++
 include/linux/usb/onboard_dev.h    |  6 ++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

Comments

kernel test robot Aug. 8, 2024, 1:06 p.m. UTC | #1
Hi Marco,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]

url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
base:   0c3836482481200ead7b416ca80c68a29cfdaabd
patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240808/202408082050.BjhmZIt6-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408082050.BjhmZIt6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408082050.BjhmZIt6-lkp@intel.com/

All errors (new ones prefixed by >>):

   aarch64-linux-ld: Unexpected GOT/PLT entries detected!
   aarch64-linux-ld: Unexpected run-time procedure linkages detected!
   aarch64-linux-ld: drivers/usb/core/hub.o: in function `set_port_feature':
>> drivers/usb/core/hub.c:481:(.text+0x121c): undefined reference to `onboard_dev_port_feature'
   aarch64-linux-ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
   drivers/usb/core/hub.c:462:(.text+0x23b0): undefined reference to `onboard_dev_port_feature'


vim +481 drivers/usb/core/hub.c

   466	
   467	/*
   468	 * USB 2.0 spec Section 11.24.2.13
   469	 */
   470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
   471	{
   472		int ret;
   473	
   474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
   475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
   476			NULL, 0, 1000);
   477		if (ret)
   478			return ret;
   479	
   480		if (!is_root_hub(hdev))
 > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
   482	
   483		return ret;
   484	}
   485
Marco Felsch Aug. 9, 2024, 9:33 a.m. UTC | #2
Hi all,

On 24-08-08, kernel test robot wrote:
> Hi Marco,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
> base:   0c3836482481200ead7b416ca80c68a29cfdaabd
> patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
> patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
> config: i386-randconfig-141-20240808 (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202408081557.FiEe9Tzz-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    ld: drivers/usb/core/hub.o: in function `set_port_feature':
> >> drivers/usb/core/hub.c:481: undefined reference to `onboard_dev_port_feature'
>    ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
>    drivers/usb/core/hub.c:462: undefined reference to `onboard_dev_port_feature'

I understood the isse but have a few questions. Before continue the work
on this topic I would like to ask if the patchset is okay in general?
I'm open for alternatives if the patchset approach is not okay.

I have a few ideas in mind (see below) to fix the 0day build issue which
was caused by the Kconfig selection:

 - CONFIG_USB=y
 - CONFIG_USB_ONBOARD_DEV=m.

Idea-1:
-------

Dropping the module support for CONFIG_USB_ONBOARD_DEV.

Idea-2:
-------

CONFIG_USB_ONBOARD_DEV follows CONFIG_USB:

CONFIG_USB=y -> CONFIG_USB_ONBOARD_DEV=y,
CONFIG_USB=m -> CONFIG_USB_ONBOARD_DEV=m.

and exporting usb_clear_port_feature().

I don't know to add such Kconfig dependency and also this idea require
that the usbcore have to load the usb_onboard_dev module always,
regardless if used.

So this idea is rather suboptimal.

Idea-3:
-------

Adding a function to the hub.c usbcore which can be used by the
usb-onboard-dev driver to register this function as hook. This removes
the dependency from the core and the usb-onboard-dev module is only
pulled if really required. Of course this require that the hub.c usbcore
driver allows custom hooks.

Idea-X:
-------

I'm open for your input :)


Regards,
  Marco

PS: My favourite is Idea-3 followed by Idea-1.

> vim +481 drivers/usb/core/hub.c
> 
>    466	
>    467	/*
>    468	 * USB 2.0 spec Section 11.24.2.13
>    469	 */
>    470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
>    471	{
>    472		int ret;
>    473	
>    474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
>    475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
>    476			NULL, 0, 1000);
>    477		if (ret)
>    478			return ret;
>    479	
>    480		if (!is_root_hub(hdev))
>  > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
>    482	
>    483		return ret;
>    484	}
>    485	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
diff mbox series

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4b93c0bd1d4b..e639c25a729c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -450,9 +450,18 @@  static int clear_hub_feature(struct usb_device *hdev, int feature)
  */
 int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature)
 {
-	return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+	int ret;
+
+	ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
 		USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
 		NULL, 0, 1000);
+	if (ret)
+		return ret;
+
+	if (!is_root_hub(hdev))
+		ret = onboard_dev_port_feature(hdev, false, feature, port1);
+
+	return ret;
 }
 
 /*
@@ -460,9 +469,18 @@  int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature)
  */
 static int set_port_feature(struct usb_device *hdev, int port1, int feature)
 {
-	return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+	int ret;
+
+	ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
 		USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
 		NULL, 0, 1000);
+	if (ret)
+		return ret;
+
+	if (!is_root_hub(hdev))
+		ret = onboard_dev_port_feature(hdev, true, feature, port1);
+
+	return ret;
 }
 
 static char *to_led_name(int selector)
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index f2bcc1a8b95f..f61de2c353d0 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -520,6 +520,19 @@  static struct usb_device_driver onboard_dev_usbdev_driver = {
 	.id_table = onboard_dev_id_table,
 };
 
+/************************** USB control **************************/
+
+int onboard_dev_port_feature(struct usb_device *udev, bool set,
+			     int feature, int port1)
+{
+	switch (feature) {
+	default:
+		return 0;
+	}
+}
+
+/************************** Kernel module ************************/
+
 static int __init onboard_dev_init(void)
 {
 	int ret;
diff --git a/include/linux/usb/onboard_dev.h b/include/linux/usb/onboard_dev.h
index b79db6d193c8..45e1f7b844d6 100644
--- a/include/linux/usb/onboard_dev.h
+++ b/include/linux/usb/onboard_dev.h
@@ -9,10 +9,16 @@  struct list_head;
 #if IS_ENABLED(CONFIG_USB_ONBOARD_DEV)
 void onboard_dev_create_pdevs(struct usb_device *parent_dev, struct list_head *pdev_list);
 void onboard_dev_destroy_pdevs(struct list_head *pdev_list);
+int onboard_dev_port_feature(struct usb_device *udev, bool set, int feature, int port1);
 #else
 static inline void onboard_dev_create_pdevs(struct usb_device *parent_dev,
 					    struct list_head *pdev_list) {}
 static inline void onboard_dev_destroy_pdevs(struct list_head *pdev_list) {}
+static inline int onboard_dev_port_feature(struct usb_device *udev, bool set,
+					   int feature, int port1)
+{
+	return 0;
+}
 #endif
 
 #endif /* __LINUX_USB_ONBOARD_DEV_H */