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

On Thu, 2014-05-01 at 10:43 +0100, Grant Likely wrote:

quoted

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.

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)

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;
+}
+
+/**
+ * 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.

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

On Wed, 07 May 2014 15:37:17 +0100, Pawel Moll [off-list ref] wrote:

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.

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

Acked-by: Grant Likely <redacted>

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.

quoted

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

quoted

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

quoted

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