| 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 > > > >
Hi Marco, On Mon, 28 Oct 2024 22:49:56 +0100 Marco Felsch wrote: > 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). How's everything going? I wish you all good! It's a very useful series for various hubs, gentle ping on it. On Mon, 28 Oct 2024 22:49:56 +0100 Marco Felsch wrote: > > > 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. Use the existing onboard_hub.h header to do the hooks looks fine. I recently encountered some kind of platforms using an existing onboard hub yet their HW don't utilize the USBPE port power control feature while the hub support it. Instead, we have another GPIO for controlling the vbus of those ports to cut the cost. Wonder any idea could use this driver considering the limitation of the usb compatible set the properties of onboard_dev_pdata hard coded? Best Regards, Pan
Hi Marco, Thank you for your reply! On 2025/1/6 14:52, Marco Felsch Wrote: > On 24-12-31, Junzhong Pan wrote: >> >> I recently encountered some kind of platforms using an existing onboard >> hub yet their HW don't utilize the USBPE port power control feature >> while the hub support it. >> Instead, we have another GPIO for controlling the vbus of those ports >> to cut the cost. > > That's exactly our use-case too. > >> Wonder any idea could use this driver considering the limitation of >> the usb compatible set the properties of onboard_dev_pdata hard coded? > > Sorry but I don't get this. If the hub have 4 ports, but board only have one gpio to controll all those vbus at once, implemented as some kind of gang mode. In this case, the onboard_dev driver may not respond to the USB_PORT_FEAT_POWER, but keep the supply always on except for the suspend states. Do you have any idea how we handle this? Best Regards, Pan
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(-)