[v2] of: Keep track of populated platform devices

On Thu, 2014-05-01 at 10:43 +0100, Grant Likely wrote:
> > That doesn't work in the case where drivers use of_platform_populate().
> > MFD devices in particular make use of it. If the driver does not get
> > cleared on removal of the devices, then all MFD users will be broken on
> > driver unbind/rebind. *
> >
> > We really need to have the inverse of of_platform_populate which will
> > remove all child and child's child devices and clear all flags and such.
> > Right now moany drivers are open-coding the removal.

Agreed, but, unless I'm missing some fundamental issue, I believe we can
solve the flag clearing problem in a generic way:

--8<-----------
8<-----------

> static int __of_platform_unpopulate_device(struct device *dev, void *c)
> {
>     if (!dev->of_node || !of_node_get_flag(dev->of_node, OF_POPULATED)
>         return 0;
> 
>     // recursively remove the children too --- I'd like to find a way
> to do this non-recursively
>     device_for_each_child(dev, NULL, __of_platform_unpopulate_device);

As of_platform_populate() is recursive itself, I don't see much problem
with this.

>     // Need to check if the device should be explicitly unbound from
> it's driver before removing it. However, I think the driver core takes
> care of this for us.

Yep, removing either a driver or a device unbinds all existing pairs.

>     // Remove based on the bus type
>     switch (dev->bus) {
>         case &platform_bus_type:

To my surprise gcc said "error: pointers are not permitted as case
values". One learns every day ;-)

>             platform_device_remove(to_platform_device(dev));
>             break;
>         case &amba_bus_type:
>             amba_device_remove(to_platform_device(dev));
>             break;
>     }
> 
>     // Should check here if there are any other children to the
> device. It is probably bad to remove a device that still has children.
> Need to check what the driver core will do.
> }

The reference count will be still be greater than 0, so the parent won't
be harmed. Nevertheless it's better to consciously handle such case. At
least I tried.

Pawel

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

On Wed, 07 May 2014 15:37:17 +0100, Pawel Moll <pawel.moll@arm.com> wrote:
> On Thu, 2014-05-01 at 10:43 +0100, Grant Likely wrote:
> > > That doesn't work in the case where drivers use of_platform_populate().
> > > MFD devices in particular make use of it. If the driver does not get
> > > cleared on removal of the devices, then all MFD users will be broken on
> > > driver unbind/rebind. *
> > >
> > > We really need to have the inverse of of_platform_populate which will
> > > remove all child and child's child devices and clear all flags and such.
> > > Right now moany drivers are open-coding the removal.
> 
> 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.

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}

> 
> 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.

> 
> > The function shouldn't be too difficult. I would expect it to look
> > something like this. You'll need to check the details.
> 
> 8<-----------
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index dd4328c..b6b5a2b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -493,4 +493,49 @@ int of_platform_populate(struct device_node *root,
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +static int of_platform_device_destroy(struct device *dev, void *data)
> +{
> +	int *parents_children_left = data;
> +	int my_children_left = 0;
> +
> +	/* Do not touch devices not populated from the device tree */
> +	if (!dev->of_node || !of_node_check_flag(dev->of_node, OF_POPULATED)) {
> +		*parents_children_left++;
> +		return 0;
> +	}
> +
> +	device_for_each_child(dev, &my_children_left,
> +			of_platform_device_destroy);
> +	if (my_children_left) {
> +		*parents_children_left++;
> +		return 0;
> +	}
> +
> +	if (dev->bus == &platform_bus_type)
> +		platform_device_unregister(to_platform_device(dev));
> +	else if (dev->bus == &amba_bustype)
> +		amba_device_unregister(to_amba_device(dev));
> +
> +	return 0;
> +}

This new code looks good to me. I haven't done any testing though.

Acked-by: Grant Likely <grant.likely@linaro.org>

> +
> +/**
> + * of_platform_depopulate() - Remove devices populated from device tree
> + * @parent: device which childred will be removed
> + *
> + * Complementary to of_platform_populate(), this function removes children
> + * of the given device (and, recurrently, their children) that have been
> + * created from their respective device tree nodes (and only those,
> + * leaving others - eg. manually created - unharmed). */
> + */
> +int of_platform_depopulate(struct device *parent)
> +{
> +	int children_left = 0;
> +
> +	device_for_each_child(dev, &children_left, of_platform_device_destroy);
> +
> +	return children_left ? -EBUSY : 0;
> +}
> +EXPORT_SYMBOL_GPL(of_platform_depopulate);
>  #endif /* CONFIG_OF_ADDRESS */
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..ef4f4ad 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -72,6 +72,7 @@ extern int of_platform_populate(struct device_node *root,
>  				const struct of_device_id *matches,
>  				const struct of_dev_auxdata *lookup,
>  				struct device *parent);
> +static inline int of_platform_depopulate(struct device *parent);
>  #else
>  static inline int of_platform_populate(struct device_node *root,
>  					const struct of_device_id *matches,
> @@ -80,6 +81,10 @@ static inline int of_platform_populate(struct device_node *root,
>  {
>  	return -ENODEV;
>  }
> +static inline int of_platform_depopulate(struct device *parent)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  #endif	/* _LINUX_OF_PLATFORM_H */
> 8<-----------
> 
> > static int __of_platform_unpopulate_device(struct device *dev, void *c)
> > {
> >     if (!dev->of_node || !of_node_get_flag(dev->of_node, OF_POPULATED)
> >         return 0;
> > 
> >     // recursively remove the children too --- I'd like to find a way
> > to do this non-recursively
> >     device_for_each_child(dev, NULL, __of_platform_unpopulate_device);
> 
> As of_platform_populate() is recursive itself, I don't see much problem
> with this

Right, that was just a side comment. I like to avoid recursion as much
as possible in kernel code, but I'm certainly not going to NAK on that
basis.

> 
> >     // Need to check if the device should be explicitly unbound from
> > it's driver before removing it. However, I think the driver core takes
> > care of this for us.
> 
> Yep, removing either a driver or a device unbinds all existing pairs.
> 
> >     // Remove based on the bus type
> >     switch (dev->bus) {
> >         case &platform_bus_type:
> 
> To my surprise gcc said "error: pointers are not permitted as case
> values". One learns every day ;-)

:-) Good to know.

> 
> >             platform_device_remove(to_platform_device(dev));
> >             break;
> >         case &amba_bus_type:
> >             amba_device_remove(to_platform_device(dev));
> >             break;
> >     }
> > 
> >     // Should check here if there are any other children to the
> > device. It is probably bad to remove a device that still has children.
> > Need to check what the driver core will do.
> > }
> 
> The reference count will be still be greater than 0, so the parent won't
> be harmed. Nevertheless it's better to consciously handle such case. At
> least I tried.
> 
> Pawel
> 

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html