lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241028214956.gmefpvcvm3zrfout@pengutronix.de>
Date: Mon, 28 Oct 2024 22:49:56 +0100
From: Marco Felsch <m.felsch@...gutronix.de>
To: Matthias Kaehlcke <matthias@...hlcke.net>,
	kernel test robot <lkp@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Matthias Kaehlcke <mka@...omium.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Fabio Estevam <festevam@...il.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, oe-kbuild-all@...ts.linux.dev,
	kernel@...gutronix.de, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev
 port features

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@...el.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
> > > 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ