Re: [PATCH] usb: modalias: Added a manufacturer string to usb modalias sysfs output

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sun, Jul 06, 2025 at 02:27:26PM +0000, Daniele Barcella wrote:
> The patch improves the usb modalias sysfs output by including the manufacturer string ath the end in the format 'mnf%s' after filtering the string, like how it is done for the dmi id device.
> 
> For example usb:v0483p5740d0100dcEFdsc02dp01ic02isc02ip00in00mnfFlipperDevicesInc.
> 
> This change allows hwdb rules to target devices with the same vid+pid but different manufacturer.
> For example, the STMicroelectronics Virtual COM Port (0483:5740) is widely used in both prototyping and production devices that don't have a unique vendor ID.

Nit, please use 72 columns for your changelog.

> 
> For further context, refer to the related discussion on systemd's GitHub: https://github.com/systemd/systemd/pull/24869.

As random links can disappear at any time, and the text in a kernel
changelog will endure for forever, please put all relevant information
in the changelog itself, don't require people to go look elesewhere and
hope it is still relevant.

> 
> Signed-off-by: Daniele Barcella <daniele.barcella@pm.me>
> ---
>  drivers/usb/core/sysfs.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index 23f3cb1989f4..dc191fa94372 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -1154,20 +1154,39 @@ static ssize_t interface_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(interface);
>  
> +static void ascii_filter(char *d, const char *s)
> +{
> +	/* Filter out characters we don't want to see in the modalias string */
> +	for (; *s; s++)
> +		if (*s > ' ' && *s < 127 && *s != ':')
> +			*(d++) = *s;
> +
> +	*d = 0;
> +}

We don't have a common kernel function for this?


> +
>  static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	struct usb_interface *intf;
>  	struct usb_device *udev;
>  	struct usb_host_interface *alt;
> +	char *manufacturer;
> +	int emit;
>  
>  	intf = to_usb_interface(dev);
>  	udev = interface_to_usbdev(intf);
>  	alt = READ_ONCE(intf->cur_altsetting);
>  
> -	return sysfs_emit(buf,
> +	if (udev->manufacturer) {
> +		manufacturer = kmalloc(strlen(udev->manufacturer) + 1, GFP_KERNEL);
> +		ascii_filter(manufacturer, udev->manufacturer);
> +	} else {
> +		manufacturer = kstrdup("", GFP_KERNEL);
> +	}
> +
> +	emit = sysfs_emit(buf,
>  			"usb:v%04Xp%04Xd%04Xdc%02Xdsc%02Xdp%02X"
> -			"ic%02Xisc%02Xip%02Xin%02X\n",
> +			"ic%02Xisc%02Xip%02Xin%02Xmnf%s\n",
>  			le16_to_cpu(udev->descriptor.idVendor),
>  			le16_to_cpu(udev->descriptor.idProduct),
>  			le16_to_cpu(udev->descriptor.bcdDevice),
> @@ -1177,7 +1196,11 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>  			alt->desc.bInterfaceClass,
>  			alt->desc.bInterfaceSubClass,
>  			alt->desc.bInterfaceProtocol,
> -			alt->desc.bInterfaceNumber);
> +			alt->desc.bInterfaceNumber,
> +			manufacturer
> +		);
> +	kfree(manufacturer);
> +	return emit;

I really do not like this change, sorry.  Vendors are supposed to go and
get their own product id if they are a vendor-specific device, they
should NOT be attempting to trigger off of USB device strings as those
are not guaranteed to relate to anything at all.

This adds a new user/kernel api, just to work around a userspace tool
that can be easily modified to handle this one specific broken device
that is lying about its vendor/product id.  Please go fix this in
userspace with a simple script that udev can run to determine what the
device string contains, and don't require the kernel to do this work
instead.

thanks,

greg k-h