Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1447728 > unrolled thread

Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node equal's glue layer device of_node

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node  equal's glue layer device of_node Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-07-21 11:20 +0200
    Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node  equal's glue layer device of_node Peter Chen <hzpeterchen@gmail.com> - 2016-07-21 11:30 +0200
      Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node  equal's glue layer device of_node Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-07-21 11:50 +0200
        Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node  equal's glue layer device of_node Peter Chen <hzpeterchen@gmail.com> - 2016-07-21 12:30 +0200

#1447728 — Re: [PATCH v3 5/6] usb: chipidea: let chipidea core device of_node equal's glue layer device of_node

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [next] | [standalone]

#1447741

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.

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1447747

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1447770

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.


-- 

Best Regards,
Peter Chen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web