Re: [PATCH v2] usb: dwc2: add pci_device_id driver_data parse support

[Greg Kroah-Hartman]

On Fri, Jun 09, 2023 at 05:07:03PM +0800, zhuyinbo wrote:
> 
> 
> 在 2023/6/9 下午2:13, Greg Kroah-Hartman 写道:
> > On Fri, Jun 09, 2023 at 10:50:47AM +0800, Yinbo Zhu wrote:
> > > The dwc2 driver has everything we need to run in PCI mode except
> > > for pci_device_id driver_data parse.  With that to set Loongson
> > > dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON
> > > and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson
> > > dwc2 controller will work.
> > > 
> > > Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
> > > ---
> > > Change in v2:
> > > 		1. Move the dwc2 pci ID from pci_ids.h to params.c.
> > > 		2. Add some code logic to ensure that the current device is
> > > 		   a PCI device.
> > > 		3. Fix the compile issue when dwc2 pci driver as module.
> > > 
> > >   drivers/usb/dwc2/core.h   |  1 +
> > >   drivers/usb/dwc2/params.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > >   drivers/usb/dwc2/pci.c    | 14 +-------------
> > >   3 files changed, 40 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > > index 0bb4c0c845bf..c92a1da46a01 100644
> > > --- a/drivers/usb/dwc2/core.h
> > > +++ b/drivers/usb/dwc2/core.h
> > > @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev);
> > >   /* The device ID match table */
> > >   extern const struct of_device_id dwc2_of_match_table[];
> > >   extern const struct acpi_device_id dwc2_acpi_match[];
> > > +extern const struct pci_device_id dwc2_pci_ids[];
> > >   int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg);
> > >   int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg);
> > > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> > > index 21d16533bd2f..6b68a8830781 100644
> > > --- a/drivers/usb/dwc2/params.c
> > > +++ b/drivers/usb/dwc2/params.c
> > > @@ -7,9 +7,14 @@
> > >   #include <linux/module.h>
> > >   #include <linux/of_device.h>
> > >   #include <linux/usb/of.h>
> > > +#include <linux/pci_ids.h>
> > > +#include <linux/pci.h>
> > >   #include "core.h"
> > > +#define PCI_PRODUCT_ID_HAPS_HSOTG	0xabc0
> > > +#define PCI_DEVICE_ID_LOONGSON_DWC2	0x7a04
> > > +
> > >   static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg)
> > >   {
> > >   	struct dwc2_core_params *p = &hsotg->params;
> > > @@ -55,6 +60,14 @@ static void dwc2_set_jz4775_params(struct dwc2_hsotg *hsotg)
> > >   		!device_property_read_bool(hsotg->dev, "disable-over-current");
> > >   }
> > > +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg)
> > > +{
> > > +	struct dwc2_core_params *p = &hsotg->params;
> > > +
> > > +	p->phy_utmi_width = 8;
> > > +	p->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL;
> > > +}
> > > +
> > >   static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg)
> > >   {
> > >   	struct dwc2_core_params *p = &hsotg->params;
> > > @@ -281,6 +294,23 @@ const struct acpi_device_id dwc2_acpi_match[] = {
> > >   };
> > >   MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match);
> > > +const struct pci_device_id dwc2_pci_ids[] = {
> > > +	{
> > > +		PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG),
> > > +	},
> > > +	{
> > > +		PCI_DEVICE(PCI_VENDOR_ID_STMICRO,
> > > +			   PCI_DEVICE_ID_STMICRO_USB_OTG),
> > > +	},
> > > +	{
> > > +		PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DWC2),
> > > +		.driver_data = (unsigned long)dwc2_set_loongson_params,
> > > +	},
> > > +	{ /* end: all zeroes */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids);
> > > +EXPORT_SYMBOL_GPL(dwc2_pci_ids);
> > > +
> > >   static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg)
> > >   {
> > >   	switch (hsotg->hw_params.op_mode) {
> > > @@ -927,13 +957,20 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg)
> > >   	if (match && match->data) {
> > >   		set_params = match->data;
> > >   		set_params(hsotg);
> > > -	} else {
> > > +	} else if (!match) {
> > >   		const struct acpi_device_id *amatch;
> > > +		const struct pci_device_id *pmatch = NULL;
> > >   		amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev);
> > >   		if (amatch && amatch->driver_data) {
> > >   			set_params = (set_params_cb)amatch->driver_data;
> > >   			set_params(hsotg);
> > > +		} else if (!amatch)
> > > +			pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent));
> > 
> > At this point in time, how can you guarantee that the parent device
> > really is a PCI one?  This function is being called from a platform
> > device callback, and platform devices should NEVER be a child of a PCI
> > device, as that's not how PCI or platform devices work.
> > 
> > So how is this even possible?
> > 
> > confused,
> > 
> 
> 
> Hi greg k-h,
> 
> My current considerations are based on that the dwc2 drivers support
> three types of devices, they are Platform device / ACPI device / PCI
> device, and The dwc2/platform.c can all cover the three type dwc2
> device, no matter how it is registered.
> 
> So, when a dwc2 device wasn't platform device and acpi device and It
> will be a PCI device, and the the dwc2/pci.c will register a dwc2
> platform device that dwc2 device is a device that in dwc2/platform.c's
> dwc2_driver_probe.  the "&dwc2->dev->parent" is the "hsotg->dev->parent"
> and it was also the &pci->dev.

That's wrong, a PCI device should NEVER register a platform device under
it, as obviously it's not a platform device at all.

So please work to fix that layering violation here, as that's not
correct and will cause problems as you are showing here.

> So, We can use "to_pci_dev(hsotg->dev->parent)" to gain a dwc2 pci
> device.

No, not if the parent is something else as you have no way of knowing
this at this point in time.

> 
> 1)  DWC2 PCI device driver (drivers/usb/dwc2/pci.c) :
> 
> static int dwc2_pci_probe(struct pci_dev *pci,
>                           const struct pci_device_id *id)
> {
> ...
>         struct platform_device  *dwc2;
> ...
>         struct device           *dev = &pci->dev;
> ...
> 	dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO);
> 	              //"dwc2" was used to match dwc2 platform driver

This needs to be fixed, it's not ok at all.  Just use the real PCI
device here please.

> ...
>         dwc2->dev.parent = dev;
> ...
> 
> }
> 
> 2)   DWC2 PLATFROM driver (drivers/usb/dwc2/platform.c) :
> // This driver cover pci type device.
> 
> static const char dwc2_driver_name[] = "dwc2";
> static struct platform_driver dwc2_platform_driver = {
>         .driver = {
>                 .name = dwc2_driver_name,
> 			//"dwc2" was used to match pci type dwc2 device
> ...
>         },
>         .probe = dwc2_driver_probe,
>         .remove = dwc2_driver_remove,
>         .shutdown = dwc2_driver_shutdown,
> };
> static int dwc2_driver_probe(struct platform_device *dev)
> {
> // The dev is the "dwc2" platform device in dwc2/pci.c driver.
>         struct dwc2_hsotg *hsotg;
> ...
>         hsotg->dev = &dev->dev;

Yes, and if this was a real platform device, the parent would NOT be a
PCI device, and so your check will cause all sorts of problems, right?

thanks,

greg k-h