| 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 >
Hi, I found two mistakes I made in my v1. I would send a v2 if this series is interesting for upstream. The remaining open question is how the driver dependencies should be handled (see idea-1,2,3). Regards, Marco On 24-09-23, Matthias Kaehlcke wrote: > El Fri, Aug 09, 2024 at 11:33:13AM GMT Marco Felsch ha dit: > > > 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. > > From the perspective of the onboard_usb_dev driver it seems sound to me. > > So far the USB maintainers haven't raised objections, so I would say move > forward and we'll see if concerns arise and deal with them if needed. > > > 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. > > With that CONFIG_USB could not be 'm' when CONFIG_USB_ONBOARD_DEV > is set, which wouldn't be great. > > > 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. > > This seems like the best approach IMO, if USB maintainers are onboard with > it. > > Since this is about port features (only applicable to hubs) the function > should be associated with struct usb_hub, not struct usb_device. And we > probably want two functions, onboard_hub_set_port_feature() and > onboard_hub_clear_port_feature(), whose implementations may use shared > code. > > > 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(-)