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@xxxxxxxxx/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@xxxxxxxxx/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@xxxxxxxxx> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202408081557.FiEe9Tzz-lkp@xxxxxxxxx/ > > > > > > 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 > > > >
