| Message ID | 20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9@pengutronix.de (mailing list archive) |
|---|---|
| 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: 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' 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 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 >
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 > >
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(-)