Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

[mka@xxxxxxxxxxxx]

On Wed, Mar 02, 2022 at 05:14:30AM +0000, Linyu Yuan (QUIC) wrote:
> > From: mka@xxxxxxxxxxxx <mka@xxxxxxxxxxxx>
> > Sent: Wednesday, March 2, 2022 12:33 AM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Tao Wang (Consultant) (QUIC)
> > <quic_wat@xxxxxxxxxxx>; balbi@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > dianders@xxxxxxxxxxxx; frowand.list@xxxxxxxxx; hadess@xxxxxxxxxx;
> > krzk@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> > mathias.nyman@xxxxxxxxx; michal.simek@xxxxxxxxxx;
> > peter.chen@xxxxxxxxxx; ravisadineni@xxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> > rogerq@xxxxxxxxxx; stern@xxxxxxxxxxxxxxxxxxx; swboyd@xxxxxxxxxxxx
> > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > onboard_usb_hub driver
> > 
> > > In my opinion, if it need update VID/PID table in this driver to support a
> > new HUB,
> > > we can parse VID/PID from device tree and create dynamic VID/PID entry
> > to id_table of onboard_hub_usbdev_driver.
> > >
> > > Hope you can understand what I said.
> > 
> > Not really.
> > 
> > I doubt that what you are suggesting would work. The easiest thing
> > to convince people would probably be to send a patch (based on this
> > one) with a working implementation of your idea.
> 
> I show my idea, but not test,
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index e280409..1811317 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -10,6 +10,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> +#include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> @@ -173,6 +174,58 @@ static void onboard_hub_remove_usbdev(struct onboard_hub *hub, const struct usb_
>  	mutex_unlock(&hub->lock);
>  }
>  
> +#define MAX_HUB_NUM		4
> +#define MAX_TABLE_SIZE		(MAX_HUB_NUM * 2)
> +static struct usb_device_id onboard_hub_id_table[MAX_TABLE_SIZE + 1];
> +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> +
> +static void onboard_hub_add_idtable(__u16 vid, __u16 pid)
> +{
> +	int i;
> +
> +	for (i = 0; i < MAX_TABLE_SIZE; i++) {
> +		if (!onboard_hub_id_table[i].idVendor)
> +			break;
> +
> +		if (onboard_hub_id_table[i].idVendor == vid &&
> +		    onboard_hub_id_table[i].idProduct == pid)
> +			return;
> +	}
> +	if (i == MAX_TABLE_SIZE)
> +		return;
> +
> +	onboard_hub_id_table[i].idVendor = vid;
> +	onboard_hub_id_table[i].idProduct = pid;
> +	onboard_hub_id_table[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
> +}
> +
> +static int onboard_hub_parse_idtable(struct device_node *np)
> +{
> +	int size = of_property_count_elems_of_size(np, "vidpid", sizeof(int));
> +	int ret, i;
> +	u16 *ids;
> +
> +	if (!size || size % 2)
> +		return -EINVAL;
> +
> +	ids = kzalloc(sizeof(u16) * size, GFP_KERNEL);
> +	if (!ids)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u16_array(np, "vidpid", ids, size);
> +	if (ret) {
> +		kfree(ids);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < size; i+=2)
> +		onboard_hub_add_idtable(ids[i], ids[i+1]);
> +
> +	kfree(ids);
> +
> +	return 0;
> +}
> +
>  static ssize_t always_powered_in_suspend_show(struct device *dev, struct device_attribute *attr,
>  			   char *buf)
>  {
> @@ -210,6 +263,10 @@ static int onboard_hub_probe(struct platform_device *pdev)
>  	struct onboard_hub *hub;
>  	int err;
>  
> +	err = onboard_hub_parse_idtable(dev->of_node);
> +	if (err)
> +		return err;
> +
>  	hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
>  	if (!hub)
>  		return -ENOMEM;
> @@ -378,15 +435,6 @@ static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
>  	onboard_hub_remove_usbdev(hub, udev);
>  }
>  
> -static const struct usb_device_id onboard_hub_id_table[] = {
> -	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
> -	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> -	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
> -	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> -	{}
> -};
> -MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> -
>  static struct usb_device_driver onboard_hub_usbdev_driver = {
>  	.name = "onboard-usb-hub",
>  	.probe = onboard_hub_usbdev_probe,

I see multiple issues with this approach:

1. The new device tree property 'vidpid'. It is (or should be) redundant
   with the compatible string, I very much doubt you could convince DT
   maintainers to add it.
2. You don't want to modify the driver to enabled support for new USB hubs.
   That means you would have to use a compatible string that is already in
   the driver, but which doesn't match the VID:PID of the hub. While this
   might work it's a hack.
3. If the USB hub is probed before the platform device it won't use this
   driver because the VID:PID isn't in the device id table.
4. Possible race conditions when changing the device id table on the fly