Re: [PATCH v2] of: Keep track of populated platform devices

[Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>]

On Wed, 2014-05-14 at 11:56 +0100, Grant Likely wrote:
> > Agreed, but, unless I'm missing some fundamental issue, I believe we can
> > solve the flag clearing problem in a generic way:
> > 
> > --8<-----------
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 3cf61a1..0f489fb 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/amba/bus.h>
> >  #include <linux/sizes.h>
> > +#include <linux/of_device.h>
> >  
> >  #include <asm/irq.h>
> >  
> > @@ -268,6 +269,7 @@ static void amba_device_release(struct device *dev)
> >  {
> >  	struct amba_device *d = to_amba_device(dev);
> >  
> > +	of_device_node_put(dev);
> >  	if (d->res.parent)
> >  		release_resource(&d->res);
> >  	kfree(d);
> > diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> > index ef37021..fd14d46 100644
> > --- a/include/linux/of_device.h
> > +++ b/include/linux/of_device.h
> > @@ -41,6 +41,8 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env
> >  
> >  static inline void of_device_node_put(struct device *dev)
> >  {
> > +	if (dev->of_node)
> > +		of_node_clear_flag(dev->of_node, OF_POPULATED);
> >  	of_node_put(dev->of_node);
> >  }
> > --8<-----------
> > 
> > This will work even if one manually unregisters a DT-originating device,
> > because of_device_node_put() would be called in both amba and platform
> > device (kobj) release path.
> 
> Doing it that way will also catch platform_devices that were created
> apart from of_platform_populate() and manually attached to an of_node,
> which is done some times. It will also break whenever multiple devices
> reference the same node if they call of_device_node_put().
> 
> For example, a platform device providing an i2c bus will have a child
> device that represents the i2c bus, and that i2c device will point to
> the same node.

Yes, can I see it doing this. Without 

> The flag clear really does need to be kept in the
> platform_device_unpopulate path because it is only to be used internally
> by of_platform_{populate,depopulate}

Ok, will move it there.
 
> > By the way, the fact that today amba_device_release() doesn't do
> > of_device_node_put() seems like a bug to me? (node's reference counter
> > won't be decremented)
> 
> Yes, that is a bug. I want to get some unittests added to the DT code to
> verify that reference counts are being tracked correctly. Right now it
> is an utter mess.

Ok. If I move the flag, this bit is not longer an integral part of the
patch so will split it into a separate one, but merge the _depopulate()
one instead.

Thanks!

Paweł

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html