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

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

[mka]

On Mon, Feb 21, 2022 at 06:20:00AM +0000, Tao Wang (Consultant) (QUIC) wrote:
>    Hi,
> 
> 
> 
>    Regarding on board hub driver,
> 
>    [1]https://lore.kernel.org/linux-usb/20220119124327.v20.3.I7c9a1f1d6ced
>    41dd8310e8a03da666a32364e790@changeid/#R
> 
> 
>    I have one comment below,
> 
> 
>    +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);
> 
> 
>    Can we support read VID/PID from device tree which provide platfrom
>    device info?

As far as I understand the kernel exclusively uses the VID/PID reported by
the USB device, the compatible string in the device tree is purely
informational (though this driver uses it for the platform device).

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

[Tao Wang (Consultant) (QUIC)]

Ok, thanks your reply.

Here is my question, we must modify the driver "onboard_usb_hub.c" if we want to use it. But it's hard to complete because it's an opensource code.

My suggestion is can we use a common compatible string for onboard_hub_driver which is a platform_driver, and read compatible string from device tree for onboard_hub_usbdev_driver which is a usb_device_driver.

If so, we only need to modify our device tree if we want to use the driver.


Best regards,
Wangtao
13709202879

-----邮件原件-----
发件人: mka@chromium.org <mka@chromium.org> 
发送时间: 2022年2月23日 0:37
收件人: Tao Wang (Consultant) (QUIC) <quic_wat@quicinc.com>
抄送: balbi@kernel.org; devicetree@vger.kernel.org; dianders@chromium.org; frowand.list@gmail.com; gregkh@linuxfoundation.org; hadess@hadess.net; krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org; mathias.nyman@intel.com; michal.simek@xilinx.com; peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org; rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org; Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
主题: Re: 回复: Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

On Mon, Feb 21, 2022 at 06:20:00AM +0000, Tao Wang (Consultant) (QUIC) wrote:
>    Hi,
> 
> 
> 
>    Regarding on board hub driver,
> 
>    [1]https://lore.kernel.org/linux-usb/20220119124327.v20.3.I7c9a1f1d6ced
>    41dd8310e8a03da666a32364e790@changeid/#R
> 
> 
>    I have one comment below,
> 
> 
>    +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);
> 
> 
>    Can we support read VID/PID from device tree which provide platfrom
>    device info?

As far as I understand the kernel exclusively uses the VID/PID reported by the USB device, the compatible string in the device tree is purely informational (though this driver uses it for the platform device).

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

[gregkh]

On Wed, Feb 23, 2022 at 07:40:31AM +0000, Tao Wang (Consultant) (QUIC) wrote:
> Ok, thanks your reply.
> 
> Here is my question, we must modify the driver "onboard_usb_hub.c" if we want to use it. But it's hard to complete because it's an opensource code.

I do not understand.  We do not deal with code that is not in the kernel
source tree, as we have no idea what is out there.  Please just submit
your changes to be merged into the tree and all will be fine.

thanks,

greg k-h

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

[Linyu Yuan (QUIC)]

> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: Wednesday, February 23, 2022 3:56 PM
> To: Tao Wang (Consultant) (QUIC) <quic_wat@quicinc.com>
> Cc: mka@chromium.org; balbi@kernel.org; devicetree@vger.kernel.org;
> dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> mathias.nyman@intel.com; michal.simek@xilinx.com;
> peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org;
> Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
> 
> On Wed, Feb 23, 2022 at 07:40:31AM +0000, Tao Wang (Consultant) (QUIC)
> wrote:
> > Ok, thanks your reply.
> >
> > Here is my question, we must modify the driver "onboard_usb_hub.c" if
> we want to use it. But it's hard to complete because it's an opensource code.
> 
> I do not understand.  We do not deal with code that is not in the kernel
> source tree, as we have no idea what is out there.  Please just submit
> your changes to be merged into the tree and all will be fine.

Hi Greg and mka,

Let's make it clear that we are talking about once this driver is approved into usb tree,
If we use different USB HUB which have VID/PID not defined in this driver,
We need to update this driver.

But if we defined VID/PID in device tree(for a specific board, manufacture should know VID/PID from HUB it used),
dynamic parsed by the driver,  then we don't need to change this driver (increase VID/PID table).

> 
> thanks,
> 
> greg k-h

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

[mka]

On Mon, Feb 28, 2022 at 03:08:34AM +0000, Linyu Yuan (QUIC) wrote:
> > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > Sent: Wednesday, February 23, 2022 3:56 PM
> > To: Tao Wang (Consultant) (QUIC) <quic_wat@quicinc.com>
> > Cc: mka@chromium.org; balbi@kernel.org; devicetree@vger.kernel.org;
> > dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> > krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> > mathias.nyman@intel.com; michal.simek@xilinx.com;
> > peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> > rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org;
> > Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > onboard_usb_hub driver
> > 
> > On Wed, Feb 23, 2022 at 07:40:31AM +0000, Tao Wang (Consultant) (QUIC)
> > wrote:
> > > Ok, thanks your reply.
> > >
> > > Here is my question, we must modify the driver "onboard_usb_hub.c" if
> > we want to use it. But it's hard to complete because it's an opensource code.
> > 
> > I do not understand.  We do not deal with code that is not in the kernel
> > source tree, as we have no idea what is out there.  Please just submit
> > your changes to be merged into the tree and all will be fine.
> 
> Hi Greg and mka,
> 
> Let's make it clear that we are talking about once this driver is approved into usb tree,
> If we use different USB HUB which have VID/PID not defined in this driver,
> We need to update this driver.
> 
> But if we defined VID/PID in device tree(for a specific board, manufacture should know VID/PID from HUB it used),
> dynamic parsed by the driver,  then we don't need to change this driver (increase VID/PID table).

As per my earlier reply, the kernel/USB core uses the VID:PID reported
by the USB device, the compatible string in the device tree is purely
informational. That's not something that could be changed by this
driver.

And even if the VID:PID from the device tree was used: how is the
kernel supposed to know that the onboard_hub driver should be
probed for a given VID:PID from the device tree, without listing
the VID:PID (or compatible string) in the driver (which is what
you seem to seek to avoid)?

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

[Linyu Yuan (QUIC)]

> From: mka@chromium.org <mka@chromium.org>
> Sent: Tuesday, March 1, 2022 2:29 AM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: gregkh@linuxfoundation.org; Tao Wang (Consultant) (QUIC)
> <quic_wat@quicinc.com>; balbi@kernel.org; devicetree@vger.kernel.org;
> dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> mathias.nyman@intel.com; michal.simek@xilinx.com;
> peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org
> Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
> 
> >
> > Hi Greg and mka,
> >
> > Let's make it clear that we are talking about once this driver is approved
> into usb tree,
> > If we use different USB HUB which have VID/PID not defined in this driver,
> > We need to update this driver.
> >
> > But if we defined VID/PID in device tree(for a specific board, manufacture
> should know VID/PID from HUB it used),
> > dynamic parsed by the driver,  then we don't need to change this driver
> (increase VID/PID table).
> 
> As per my earlier reply, the kernel/USB core uses the VID:PID reported
> by the USB device, the compatible string in the device tree is purely
> informational. That's not something that could be changed by this
> driver.
I can't fully understand this comment,  could you please share step if we want to add a new HUB support, what should we do ? nothing ?

If do nothing, can we remove id_table from  onboard_hub_usbdev_driver  ?
> 
> And even if the VID:PID from the device tree was used: how is the
> kernel supposed to know that the onboard_hub driver should be
> probed for a given VID:PID from the device tree, without listing
> the VID:PID (or compatible string) in the driver (which is what
> you seem to seek to avoid)?
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.

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

[mka]

On Tue, Mar 01, 2022 at 02:30:00AM +0000, Linyu Yuan (QUIC) wrote:
> > From: mka@chromium.org <mka@chromium.org>
> > Sent: Tuesday, March 1, 2022 2:29 AM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: gregkh@linuxfoundation.org; Tao Wang (Consultant) (QUIC)
> > <quic_wat@quicinc.com>; balbi@kernel.org; devicetree@vger.kernel.org;
> > dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> > krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> > mathias.nyman@intel.com; michal.simek@xilinx.com;
> > peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> > rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org
> > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > onboard_usb_hub driver
> > 
> > >
> > > Hi Greg and mka,
> > >
> > > Let's make it clear that we are talking about once this driver is approved
> > into usb tree,
> > > If we use different USB HUB which have VID/PID not defined in this driver,
> > > We need to update this driver.
> > >
> > > But if we defined VID/PID in device tree(for a specific board, manufacture
> > should know VID/PID from HUB it used),
> > > dynamic parsed by the driver,  then we don't need to change this driver
> > (increase VID/PID table).
> > 
> > As per my earlier reply, the kernel/USB core uses the VID:PID reported
> > by the USB device, the compatible string in the device tree is purely
> > informational. That's not something that could be changed by this
> > driver.
> I can't fully understand this comment,  could you please share step if we want to add a new HUB support, what should we do ? nothing ?

Add the VID:PID and compatible strings to onboard_usb_hub.c, analogous
to those for the RTS5411 and RTS5414. More work will be needed if the
hub needs a special power up or power down sequence (multiple regulators,
GPIOs, ...)

> If do nothing, can we remove id_table from  onboard_hub_usbdev_driver  ?
> > 
> > And even if the VID:PID from the device tree was used: how is the
> > kernel supposed to know that the onboard_hub driver should be
> > probed for a given VID:PID from the device tree, without listing
> > the VID:PID (or compatible string) in the driver (which is what
> > you seem to seek to avoid)?
> 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.

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

[Linyu Yuan (QUIC)]

> From: mka@chromium.org <mka@chromium.org>
> Sent: Wednesday, March 2, 2022 12:33 AM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: gregkh@linuxfoundation.org; Tao Wang (Consultant) (QUIC)
> <quic_wat@quicinc.com>; balbi@kernel.org; devicetree@vger.kernel.org;
> dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> mathias.nyman@intel.com; michal.simek@xilinx.com;
> peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org
> 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,

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

[mka]

On Wed, Mar 02, 2022 at 05:14:30AM +0000, Linyu Yuan (QUIC) wrote:
> > From: mka@chromium.org <mka@chromium.org>
> > Sent: Wednesday, March 2, 2022 12:33 AM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: gregkh@linuxfoundation.org; Tao Wang (Consultant) (QUIC)
> > <quic_wat@quicinc.com>; balbi@kernel.org; devicetree@vger.kernel.org;
> > dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> > krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> > mathias.nyman@intel.com; michal.simek@xilinx.com;
> > peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> > rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org
> > 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

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

[Linyu Yuan (QUIC)]

> From: mka@chromium.org <mka@chromium.org>
> Sent: Saturday, March 5, 2022 12:48 AM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: gregkh@linuxfoundation.org; Tao Wang (Consultant) (QUIC)
> <quic_wat@quicinc.com>; balbi@kernel.org; devicetree@vger.kernel.org;
> dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> krzk@kernel.org; linux-kernel@vger.kernel.org; linux-usb@vger.kernel.org;
> mathias.nyman@intel.com; michal.simek@xilinx.com;
> peter.chen@kernel.org; ravisadineni@chromium.org; robh+dt@kernel.org;
> rogerq@kernel.org; stern@rowland.harvard.edu; swboyd@chromium.org
> Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
> 
> On Wed, Mar 02, 2022 at 05:14:30AM +0000, Linyu Yuan (QUIC) wrote:
> > > From: mka@chromium.org <mka@chromium.org>
> > > Sent: Wednesday, March 2, 2022 12:33 AM
> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > Cc: gregkh@linuxfoundation.org; Tao Wang (Consultant) (QUIC)
> > > <quic_wat@quicinc.com>; balbi@kernel.org;
> devicetree@vger.kernel.org;
> > > dianders@chromium.org; frowand.list@gmail.com; hadess@hadess.net;
> > > krzk@kernel.org; linux-kernel@vger.kernel.org; linux-
> usb@vger.kernel.org;
> > > mathias.nyman@intel.com; michal.simek@xilinx.com;
> > > peter.chen@kernel.org; ravisadineni@chromium.org;
> robh+dt@kernel.org;
> > > rogerq@kernel.org; stern@rowland.harvard.edu;
> swboyd@chromium.org
> > > 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.
This is another topic which I don't know if discuss or not,

What is power state requirement of hub for this driver ?
Do it expect hub power off when system enter linux kernel stage ?
If so, all this driver may not work as hub may enumerated before this driver load.

> 4. Possible race conditions when changing the device id table on the fly
If hub is default power off, there is no race.