Re: [PATCH] usb: core: notify unrecognized usb device

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Feb 21, 2025 at 10:29:49AM +0000, Akshay Gujar wrote:
> Description: To send uevent for unrecognized device connected on system.

Odd format here, have you read the documentation of the kernel process
in how to write a changelog?  I recommend a quick glance at the section
"The canonical patch format" in the kernel file,
Documentation/process/submitting-patches.rst for details.

> As per the usb compliance, USB-IF enforces a "no silent failure" rule.
> This means that an implementation of USB must not appear broken to the
> consumer. In configurations where the consumer's expectations are not
> met, either the peripheral or host must provide appropriate and useful
> feedback to the consumer regarding the problem.
> 
> Link: https://compliance.usb.org/index.asp?UpdateFile=Embedded%20Host&Format=Standard#10

Odd, many Linux devices have passed usb-if testing since 2005 when this
was made a "rule", how did that happen?  What recently changed to
suddenly require this be a kernel issue?

And does usb-if even matter these days?  You do know what they think
about Linux overall, right (hint, they kicked us out from
participating...) so why should we follow their "requirements" when they
do not allow us to even participate or provide feedback when they create
them?

> Signed-off-by: Akshay Gujar <Akshay.Gujar@harman.com>
> ---
>  drivers/usb/core/hub.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c3f839637..d00129b59 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5343,6 +5343,26 @@ static int descriptors_changed(struct usb_device *udev,
>  	return changed;
>  }
>  
> +static void unrecognized_usb_device_notify(struct usb_port *port_dev)
> +{
> +	char *envp[2] = { NULL, NULL };
> +	struct device *hub_dev;
> +
> +	hub_dev = port_dev->dev.parent;
> +
> +	if (!hub_dev)
> +		return;

How can this be true?

> +
> +	envp[0] = kasprintf(GFP_KERNEL, "UNRECOGNIZED_USB_DEVICE_ON_PORT=%s",
> +				kobject_name(&port_dev->dev.kobj));

Hint, if a driver ever starts calling into kobject or sysfs functions,
usually something is wrong.  This should just use dev_name(), right?

> +	if (!envp[0])
> +		return;
> +
> +	kobject_uevent_env(&hub_dev->kobj, KOBJ_CHANGE, envp);

Where is this new uevent documented?  What userspace tool will see this
and do something about it?  How was this tested?

> +
> +	kfree(envp[0]);
> +}
> +
>  static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>  		u16 portchange)
>  {
> @@ -5569,9 +5589,11 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>  	if (hub->hdev->parent ||
>  			!hcd->driver->port_handed_over ||
>  			!(hcd->driver->port_handed_over)(hcd, port1)) {
> -		if (status != -ENOTCONN && status != -ENODEV)
> +		if (status != -ENOTCONN && status != -ENODEV) {
>  			dev_err(&port_dev->dev,
>  					"unable to enumerate USB device\n");
> +			unrecognized_usb_device_notify(port_dev);

This is only if a hub acts up with talking to a device, it does not mean
the device was not supported at all.  So this isn't going to meet the
standard that you describe above.  Userspace is really the only thing
that can know if a device is "supported" or not, not the kernel.

thanks,

greg k-h