| Message ID | 1469007629-31757-6-git-send-email-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 20, 2016 at 05:40:28PM +0800, Peter Chen wrote: > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 69426e6..0d05812 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device *pdev) > if (!ci) > return -ENOMEM; > > + /* > + * At device tree, we have no device node for chipidea core, > + * the glue layer's node is the parent node for host and udc > + * device. But in related driver, the parent device is chipidea > + * core. So, in order to let the common driver get parent's node, > + * we let the core's device node equals glue layer's node. > + */ > + if (dev->parent && dev->parent->of_node) > + dev->of_node = dev->parent->of_node; This is a dangerous thing to do. You're changing the dev->of_node of _this_ device, which means that _this_ driver will no longer match the device if you remove and reinsert the driver module, or unbind and try to re-bind the device to this driver.
On Thu, Jul 21, 2016 at 10:14:38AM +0100, Russell King - ARM Linux wrote: > On Wed, Jul 20, 2016 at 05:40:28PM +0800, Peter Chen wrote: > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index 69426e6..0d05812 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > if (!ci) > > return -ENOMEM; > > > > + /* > > + * At device tree, we have no device node for chipidea core, > > + * the glue layer's node is the parent node for host and udc > > + * device. But in related driver, the parent device is chipidea > > + * core. So, in order to let the common driver get parent's node, > > + * we let the core's device node equals glue layer's node. > > + */ > > + if (dev->parent && dev->parent->of_node) > > + dev->of_node = dev->parent->of_node; > > This is a dangerous thing to do. You're changing the dev->of_node of > _this_ device, which means that _this_ driver will no longer match > the device if you remove and reinsert the driver module, or unbind > and try to re-bind the device to this driver. > Thanks for commenting it. I have tested load/unload, it does not show any problems. The chipidea core device is created by code at runtime, not by device node. And we have NO device node for this chipidea core device at dts.
On Thu, Jul 21, 2016 at 05:20:12PM +0800, Peter Chen wrote: > On Thu, Jul 21, 2016 at 10:14:38AM +0100, Russell King - ARM Linux wrote: > > On Wed, Jul 20, 2016 at 05:40:28PM +0800, Peter Chen wrote: > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > index 69426e6..0d05812 100644 > > > --- a/drivers/usb/chipidea/core.c > > > +++ b/drivers/usb/chipidea/core.c > > > @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > if (!ci) > > > return -ENOMEM; > > > > > > + /* > > > + * At device tree, we have no device node for chipidea core, > > > + * the glue layer's node is the parent node for host and udc > > > + * device. But in related driver, the parent device is chipidea > > > + * core. So, in order to let the common driver get parent's node, > > > + * we let the core's device node equals glue layer's node. > > > + */ > > > + if (dev->parent && dev->parent->of_node) > > > + dev->of_node = dev->parent->of_node; > > > > This is a dangerous thing to do. You're changing the dev->of_node of > > _this_ device, which means that _this_ driver will no longer match > > the device if you remove and reinsert the driver module, or unbind > > and try to re-bind the device to this driver. > > > > Thanks for commenting it. > > I have tested load/unload, it does not show any problems. > > The chipidea core device is created by code at runtime, not by device node. > And we have NO device node for this chipidea core device at dts. Okay, so we still probably have the bind/unbind problem, where "dev" can be matched by the driver which claimed "dev->parent". Remember, in an OF environment, driver matching is done by the compatible property, which is accessed via dev->of_node. Therefore, I would suggest that you NULL dev->of_node in the error cleanup paths and in the remove function, so you don't have an unbound device with a duplicated (but inappropriate) dev->of_node pointer.
On Thu, Jul 21, 2016 at 10:41:28AM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 21, 2016 at 05:20:12PM +0800, Peter Chen wrote: > > On Thu, Jul 21, 2016 at 10:14:38AM +0100, Russell King - ARM Linux wrote: > > > On Wed, Jul 20, 2016 at 05:40:28PM +0800, Peter Chen wrote: > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > > index 69426e6..0d05812 100644 > > > > --- a/drivers/usb/chipidea/core.c > > > > +++ b/drivers/usb/chipidea/core.c > > > > @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > > if (!ci) > > > > return -ENOMEM; > > > > > > > > + /* > > > > + * At device tree, we have no device node for chipidea core, > > > > + * the glue layer's node is the parent node for host and udc > > > > + * device. But in related driver, the parent device is chipidea > > > > + * core. So, in order to let the common driver get parent's node, > > > > + * we let the core's device node equals glue layer's node. > > > > + */ > > > > + if (dev->parent && dev->parent->of_node) > > > > + dev->of_node = dev->parent->of_node; > > > > > > This is a dangerous thing to do. You're changing the dev->of_node of > > > _this_ device, which means that _this_ driver will no longer match > > > the device if you remove and reinsert the driver module, or unbind > > > and try to re-bind the device to this driver. > > > > > > > Thanks for commenting it. > > > > I have tested load/unload, it does not show any problems. > > > > The chipidea core device is created by code at runtime, not by device node. > > And we have NO device node for this chipidea core device at dts. > > Okay, so we still probably have the bind/unbind problem, where "dev" > can be matched by the driver which claimed "dev->parent". Remember, > in an OF environment, driver matching is done by the compatible > property, which is accessed via dev->of_node. > > Therefore, I would suggest that you NULL dev->of_node in the error > cleanup paths and in the remove function, so you don't have an > unbound device with a duplicated (but inappropriate) dev->of_node > pointer. > Although it does no mismatch between driver and device due to the driver has no of_match_table, I find it has below re-request pinctrl error after re-bind, that's due to the parent device which has of_node and there is a pinctrl property in it. imx6sx-pinctrl 20e0000.iomuxc: pin MX6SX_PAD_GPIO1_IO10 already requested by 2184000.usb; cannot claim for ci_hdrc.0 After adding your suggestion, this error has gone, thanks.
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 69426e6..0d05812 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -914,6 +914,16 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (!ci) return -ENOMEM; + /* + * At device tree, we have no device node for chipidea core, + * the glue layer's node is the parent node for host and udc + * device. But in related driver, the parent device is chipidea + * core. So, in order to let the common driver get parent's node, + * we let the core's device node equals glue layer's node. + */ + if (dev->parent && dev->parent->of_node) + dev->of_node = dev->parent->of_node; + ci->dev = dev; ci->platdata = dev_get_platdata(dev); ci->imx28_write_fix = !!(ci->platdata->flags &