| 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 |
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
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 --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 */
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(-)