linux-kernel - Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <wtvhsdo2zefehkgfcp2cfdl2uht4lcrytyjyhwjhnpcyvx4kd2@iurrw554aegh>
Date: Mon, 23 Sep 2024 09:56:35 +0000
From: Matthias Kaehlcke <matthias@...hlcke.net>
To: Marco Felsch <m.felsch@...gutronix.de>
Cc: 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

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
> >