[RFC PATCH 0/8] component helper improvements

[RFC PATCH 0/8] component helper improvements

[Russell King - ARM Linux]

A while back, Laurent raised some comments about the component helper,
which this patch set starts to address.

The first point it addresses is the repeated parsing inefficiency when
deferred probing occurs.  When DT is used, the structure of the
component helper today means that masters end up parsing the device
tree for each attempt to re-bind the driver.

We remove this inefficiency by creating an array of matching data and
functions, which the component helper can use internally to match up
components to their master.

The second point was the inefficiency of destroying the list of
components each time we saw a failure.  We did this to ensure that
we kept things correctly ordered: component bind order matters.
As we have an array instead, the array is already ordered, so we
use this array to store the component pointers instead of a list,
and remember which are duplicates (and thus should be avoided.)
Avoiding the right duplicates matters as we walk the array in the
opposite direction at tear down.

I would like to see patches 1-5 scheduled for the next merge window,
with 6-8 for the following window - this gives us grace of one kernel
cycle to ensure that any new component helper users are properly
converted.

 drivers/base/component.c               | 248 ++++++++++++++++++++++-----------
 drivers/gpu/drm/msm/msm_drv.c          |  81 +++++------
 drivers/staging/imx-drm/imx-drm-core.c |  57 +-------
 include/linux/component.h              |   8 +-
 4 files changed, 206 insertions(+), 188 deletions(-)

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[RFC PATCH 0/8] component helper improvements

[Russell King - ARM Linux]

A while back, Laurent raised some comments about the component helper,
which this patch set starts to address.

The first point it addresses is the repeated parsing inefficiency when
deferred probing occurs.  When DT is used, the structure of the
component helper today means that masters end up parsing the device
tree for each attempt to re-bind the driver.

We remove this inefficiency by creating an array of matching data and
functions, which the component helper can use internally to match up
components to their master.

The second point was the inefficiency of destroying the list of
components each time we saw a failure.  We did this to ensure that
we kept things correctly ordered: component bind order matters.
As we have an array instead, the array is already ordered, so we
use this array to store the component pointers instead of a list,
and remember which are duplicates (and thus should be avoided.)
Avoiding the right duplicates matters as we walk the array in the
opposite direction at tear down.

I would like to see patches 1-5 scheduled for the next merge window,
with 6-8 for the following window - this gives us grace of one kernel
cycle to ensure that any new component helper users are properly
converted.

 drivers/base/component.c               | 248 ++++++++++++++++++++++-----------
 drivers/gpu/drm/msm/msm_drv.c          |  81 +++++------
 drivers/staging/imx-drm/imx-drm-core.c |  57 +-------
 include/linux/component.h              |   8 +-
 4 files changed, 206 insertions(+), 188 deletions(-)

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH RFC 1/8] component: fix missed cleanup in case of devres failure

[Russell King]

In try_to_bring_up_master(), we tear down the master's component list
for each error case, except for devres group failure.  Fix this
oversight by making the code less prone to such mistakes.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c | 62 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index c4778995cd72..d0ebd4431736 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -113,44 +113,44 @@ static void master_remove_components(struct master *master)
 static int try_to_bring_up_master(struct master *master,
 	struct component *component)
 {
-	int ret = 0;
+	int ret;
 
-	if (!master->bound) {
-		/*
-		 * Search the list of components, looking for components that
-		 * belong to this master, and attach them to the master.
-		 */
-		if (master->ops->add_components(master->dev, master)) {
-			/* Failed to find all components */
-			master_remove_components(master);
-			ret = 0;
-			goto out;
-		}
+	if (master->bound)
+		return 0;
 
-		if (component && component->master != master) {
-			master_remove_components(master);
-			ret = 0;
-			goto out;
-		}
+	/*
+	 * Search the list of components, looking for components that
+	 * belong to this master, and attach them to the master.
+	 */
+	if (master->ops->add_components(master->dev, master)) {
+		/* Failed to find all components */
+		ret = 0;
+		goto out;
+	}
 
-		if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) {
-			ret = -ENOMEM;
-			goto out;
-		}
+	if (component && component->master != master) {
+		ret = 0;
+		goto out;
+	}
 
-		/* Found all components */
-		ret = master->ops->bind(master->dev);
-		if (ret < 0) {
-			devres_release_group(master->dev, NULL);
-			dev_info(master->dev, "master bind failed: %d\n", ret);
-			master_remove_components(master);
-			goto out;
-		}
+	if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
-		master->bound = true;
-		ret = 1;
+	/* Found all components */
+	ret = master->ops->bind(master->dev);
+	if (ret < 0) {
+		devres_release_group(master->dev, NULL);
+		dev_info(master->dev, "master bind failed: %d\n", ret);
+		goto out;
 	}
+
+	master->bound = true;
+	return 1;
+
 out:
+	master_remove_components(master);
 
 	return ret;
 }
-- 
1.8.3.1

[PATCH RFC 2/8] component: ignore multiple additions of the same component

[Russell King]

Permit masters to call component_master_add_child() and match the same
child multiple times.  This may happen if there's multiple connections
to a single component device from other devices.  In such scenarios,
we should not return a failure, but instead ignore the attempt.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index d0ebd4431736..55813e91bf0d 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -69,6 +69,11 @@ static void component_detach_master(struct master *master, struct component *c)
 	c->master = NULL;
 }
 
+/*
+ * Add a component to a master, finding the component via the compare
+ * function and compare data.  This is safe to call for duplicate matches
+ * and will not result in the same component being added multiple times.
+ */
 int component_master_add_child(struct master *master,
 	int (*compare)(struct device *, void *), void *compare_data)
 {
@@ -76,11 +81,12 @@ int component_master_add_child(struct master *master,
 	int ret = -ENXIO;
 
 	list_for_each_entry(c, &component_list, node) {
-		if (c->master)
+		if (c->master && c->master != master)
 			continue;
 
 		if (compare(c->dev, compare_data)) {
-			component_attach_master(master, c);
+			if (!c->master)
+				component_attach_master(master, c);
 			ret = 0;
 			break;
 		}
-- 
1.8.3.1

[PATCH RFC 3/8] component: add support for component match array

[Russell King]

Add support for generating a set of component matches at master probe
time, and submitting them to the component layer.  This allows the
component layer to perform the matches internally without needing to
call into the master driver, and allows for further restructuring of
the component helper.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c  | 118 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/component.h |   7 +++
 2 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 55813e91bf0d..66990bb3adfe 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -18,6 +18,15 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 
+struct component_match {
+	size_t alloc;
+	size_t num;
+	struct {
+		void *data;
+		int (*fn)(struct device *, void *);
+	} compare[0];
+};
+
 struct master {
 	struct list_head node;
 	struct list_head components;
@@ -25,6 +34,7 @@ struct master {
 
 	const struct component_master_ops *ops;
 	struct device *dev;
+	struct component_match *match;
 };
 
 struct component {
@@ -96,6 +106,34 @@ int component_master_add_child(struct master *master,
 }
 EXPORT_SYMBOL_GPL(component_master_add_child);
 
+static int find_components(struct master *master)
+{
+	struct component_match *match = master->match;
+	size_t i;
+	int ret = 0;
+
+	if (!match) {
+		/*
+		 * Search the list of components, looking for components that
+		 * belong to this master, and attach them to the master.
+		 */
+		return master->ops->add_components(master->dev, master);
+	}
+
+	/*
+	 * Scan the array of match functions and attach
+	 * any components which are found to this master.
+	 */
+	for (i = 0; i < match->num; i++) {
+		ret = component_master_add_child(master,
+						 match->compare[i].fn,
+						 match->compare[i].data);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
 /* Detach all attached components from this master */
 static void master_remove_components(struct master *master)
 {
@@ -128,7 +166,7 @@ static int try_to_bring_up_master(struct master *master,
 	 * Search the list of components, looking for components that
 	 * belong to this master, and attach them to the master.
 	 */
-	if (master->ops->add_components(master->dev, master)) {
+	if (find_components(master)) {
 		/* Failed to find all components */
 		ret = 0;
 		goto out;
@@ -186,18 +224,85 @@ static void take_down_master(struct master *master)
 	master_remove_components(master);
 }
 
-int component_master_add(struct device *dev,
-	const struct component_master_ops *ops)
+static size_t component_match_size(size_t num)
+{
+	return offsetof(struct component_match, compare[num]);
+}
+
+static struct component_match *component_match_realloc(struct device *dev,
+	struct component_match *match, size_t num)
+{
+	struct component_match *new;
+
+	if (match && match->alloc == num)
+		return match;
+
+	new = devm_kmalloc(dev, component_match_size(num), GFP_KERNEL);
+	if (!new)
+		return ERR_PTR(-ENOMEM);
+
+	if (match) {
+		memcpy(new, match, component_match_size(min(match->num, num)));
+		devm_kfree(dev, match);
+	} else {
+		new->num = 0;
+	}
+
+	new->alloc = num;
+
+	return new;
+}
+
+/*
+ * Add a component to be matched.
+ *
+ * The match array is first created or extended if necessary.
+ */
+void component_match_add(struct device *dev, struct component_match **matchptr,
+	int (*compare)(struct device *, void *), void *compare_data)
+{
+	struct component_match *match = *matchptr;
+
+	if (IS_ERR(match))
+		return;
+
+	if (!match || ++match->num == match->alloc) {
+		size_t new_size = match ? match->alloc + 16 : 15;
+
+		match = component_match_realloc(dev, match, new_size);
+
+		*matchptr = match;
+
+		if (IS_ERR(match))
+			return;
+	}
+
+	match->compare[match->num].fn = compare;
+	match->compare[match->num].data = compare_data;
+}
+
+int component_master_add_with_match(struct device *dev,
+	const struct component_master_ops *ops,
+	struct component_match *match)
 {
 	struct master *master;
 	int ret;
 
+	if (ops->add_components && match)
+		return -EINVAL;
+
+	/* Reallocate the match array for its true size */
+	match = component_match_realloc(dev, match, match->num);
+	if (IS_ERR(match))
+		return PTR_ERR(match);
+
 	master = kzalloc(sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
 
 	master->dev = dev;
 	master->ops = ops;
+	master->match = match;
 	INIT_LIST_HEAD(&master->components);
 
 	/* Add to the list of available masters. */
@@ -215,6 +320,13 @@ int component_master_add(struct device *dev,
 
 	return ret < 0 ? ret : 0;
 }
+EXPORT_SYMBOL_GPL(component_master_add_with_match);
+
+int component_master_add(struct device *dev,
+	const struct component_master_ops *ops)
+{
+	return component_master_add_with_match(dev, ops, NULL);
+}
 EXPORT_SYMBOL_GPL(component_master_add);
 
 void component_master_del(struct device *dev,
diff --git a/include/linux/component.h b/include/linux/component.h
index 68870182ca1e..c00dcc302611 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -29,4 +29,11 @@ void component_master_del(struct device *,
 int component_master_add_child(struct master *master,
 	int (*compare)(struct device *, void *), void *compare_data);
 
+struct component_match;
+
+int component_master_add_with_match(struct device *,
+	const struct component_master_ops *, struct component_match *);
+void component_match_add(struct device *, struct component_match **,
+	int (*compare)(struct device *, void *), void *compare_data);
+
 #endif
-- 
1.8.3.1

[PATCH RFC 3/8] component: add support for component match array

[Thierry Reding]

On Sun, Apr 27, 2014 at 12:01:58AM +0100, Russell King wrote:
> Add support for generating a set of component matches at master probe
> time, and submitting them to the component layer.  This allows the
> component layer to perform the matches internally without needing to
> call into the master driver, and allows for further restructuring of
> the component helper.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/component.c  | 118 ++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/component.h |   7 +++
>  2 files changed, 122 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
[...]
> +void component_match_add(struct device *dev, struct component_match **matchptr,
> +	int (*compare)(struct device *, void *), void *compare_data)
> +{
> +	struct component_match *match = *matchptr;
> +
> +	if (IS_ERR(match))
> +		return;
> +
> +	if (!match || ++match->num == match->alloc) {
> +		size_t new_size = match ? match->alloc + 16 : 15;

Doesn't this allocate prematurely? If component_match_add() is called on
the final component of a master, then match->num will be incremented to
it's final value. If that matches match->alloc, then things are just
fine, aren't they? No need to allocate another 16 entries.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/44ea3b9b/attachment.sig>

[PATCH RFC 3/8] component: add support for component match array

[Russell King - ARM Linux]

On Mon, Apr 28, 2014 at 11:21:32AM +0200, Thierry Reding wrote:
> On Sun, Apr 27, 2014 at 12:01:58AM +0100, Russell King wrote:
> > Add support for generating a set of component matches at master probe
> > time, and submitting them to the component layer.  This allows the
> > component layer to perform the matches internally without needing to
> > call into the master driver, and allows for further restructuring of
> > the component helper.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  drivers/base/component.c  | 118 ++++++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/component.h |   7 +++
> >  2 files changed, 122 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/base/component.c b/drivers/base/component.c
> [...]
> > +void component_match_add(struct device *dev, struct component_match **matchptr,
> > +	int (*compare)(struct device *, void *), void *compare_data)
> > +{
> > +	struct component_match *match = *matchptr;
> > +
> > +	if (IS_ERR(match))
> > +		return;
> > +
> > +	if (!match || ++match->num == match->alloc) {
> > +		size_t new_size = match ? match->alloc + 16 : 15;
> 
> Doesn't this allocate prematurely? If component_match_add() is called on
> the final component of a master, then match->num will be incremented to
> it's final value. If that matches match->alloc, then things are just
> fine, aren't they? No need to allocate another 16 entries.

This code appears to be correct - `num' is actually one less than the
number of components.

However, that makes code elsewhere wrong... v2 coming up soon with this
fixed.  Good catch.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH RFC 4/8] drm: msm: update to use component match support

[Russell King]

Update MSM's DRM driver to use the component match support rather than
add_components.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f9de156b9e65..26be58bd611d 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -865,12 +865,41 @@ static int compare_of(struct device *dev, void *data)
 {
 	return dev->of_node == data;
 }
+#else
+static int compare_dev(struct device *dev, void *data)
+{
+	return dev == data;
+}
+#endif
+
+static int msm_drm_bind(struct device *dev)
+{
+	return drm_platform_init(&msm_driver, to_platform_device(dev));
+}
+
+static void msm_drm_unbind(struct device *dev)
+{
+	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
+}
+
+static const struct component_master_ops msm_drm_ops = {
+	.bind = msm_drm_bind,
+	.unbind = msm_drm_unbind,
+};
+
+/*
+ * Platform driver:
+ */
 
-static int msm_drm_add_components(struct device *master, struct master *m)
+static int msm_pdev_probe(struct platform_device *pdev)
 {
+	struct component_match *match = NULL;
+#ifdef CONFIG_OF
+	/* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx
+	 * (or probably any other).. so probably some room for some helpers
+	 */
 	struct device_node *np = master->of_node;
 	unsigned i;
-	int ret;
 
 	for (i = 0; ; i++) {
 		struct device_node *node;
@@ -879,22 +908,9 @@ static int msm_drm_add_components(struct device *master, struct master *m)
 		if (!node)
 			break;
 
-		ret = component_master_add_child(m, compare_of, node);
-		of_node_put(node);
-
-		if (ret)
-			return ret;
+		component_match_add(&pdev->dev, &match, compare_of, node);
 	}
-	return 0;
-}
 #else
-static int compare_dev(struct device *dev, void *data)
-{
-	return dev == data;
-}
-
-static int msm_drm_add_components(struct device *master, struct master *m)
-{
 	/* For non-DT case, it kinda sucks.  We don't actually have a way
 	 * to know whether or not we are waiting for certain devices (or if
 	 * they are simply not present).  But for non-DT we only need to
@@ -918,41 +934,12 @@ static int msm_drm_add_components(struct device *master, struct master *m)
 			return -EPROBE_DEFER;
 		}
 
-		ret = component_master_add_child(m, compare_dev, dev);
-		if (ret) {
-			DBG("could not add child: %d", ret);
-			return ret;
-		}
+		component_match_add(&pdev->dev, &match, compare_dev, dev);
 	}
-
-	return 0;
-}
 #endif
 
-static int msm_drm_bind(struct device *dev)
-{
-	return drm_platform_init(&msm_driver, to_platform_device(dev));
-}
-
-static void msm_drm_unbind(struct device *dev)
-{
-	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
-}
-
-static const struct component_master_ops msm_drm_ops = {
-		.add_components = msm_drm_add_components,
-		.bind = msm_drm_bind,
-		.unbind = msm_drm_unbind,
-};
-
-/*
- * Platform driver:
- */
-
-static int msm_pdev_probe(struct platform_device *pdev)
-{
 	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	return component_master_add(&pdev->dev, &msm_drm_ops);
+	return component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
 }
 
 static int msm_pdev_remove(struct platform_device *pdev)
-- 
1.8.3.1

[PATCH RFC 4/8] drm: msm: update to use component match support

[Russell King]

Update MSM's DRM driver to use the component match support rather than
add_components.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f9de156b9e65..26be58bd611d 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -865,12 +865,41 @@ static int compare_of(struct device *dev, void *data)
 {
 	return dev->of_node == data;
 }
+#else
+static int compare_dev(struct device *dev, void *data)
+{
+	return dev == data;
+}
+#endif
+
+static int msm_drm_bind(struct device *dev)
+{
+	return drm_platform_init(&msm_driver, to_platform_device(dev));
+}
+
+static void msm_drm_unbind(struct device *dev)
+{
+	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
+}
+
+static const struct component_master_ops msm_drm_ops = {
+	.bind = msm_drm_bind,
+	.unbind = msm_drm_unbind,
+};
+
+/*
+ * Platform driver:
+ */
 
-static int msm_drm_add_components(struct device *master, struct master *m)
+static int msm_pdev_probe(struct platform_device *pdev)
 {
+	struct component_match *match = NULL;
+#ifdef CONFIG_OF
+	/* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx
+	 * (or probably any other).. so probably some room for some helpers
+	 */
 	struct device_node *np = master->of_node;
 	unsigned i;
-	int ret;
 
 	for (i = 0; ; i++) {
 		struct device_node *node;
@@ -879,22 +908,9 @@ static int msm_drm_add_components(struct device *master, struct master *m)
 		if (!node)
 			break;
 
-		ret = component_master_add_child(m, compare_of, node);
-		of_node_put(node);
-
-		if (ret)
-			return ret;
+		component_match_add(&pdev->dev, &match, compare_of, node);
 	}
-	return 0;
-}
 #else
-static int compare_dev(struct device *dev, void *data)
-{
-	return dev == data;
-}
-
-static int msm_drm_add_components(struct device *master, struct master *m)
-{
 	/* For non-DT case, it kinda sucks.  We don't actually have a way
 	 * to know whether or not we are waiting for certain devices (or if
 	 * they are simply not present).  But for non-DT we only need to
@@ -918,41 +934,12 @@ static int msm_drm_add_components(struct device *master, struct master *m)
 			return -EPROBE_DEFER;
 		}
 
-		ret = component_master_add_child(m, compare_dev, dev);
-		if (ret) {
-			DBG("could not add child: %d", ret);
-			return ret;
-		}
+		component_match_add(&pdev->dev, &match, compare_dev, dev);
 	}
-
-	return 0;
-}
 #endif
 
-static int msm_drm_bind(struct device *dev)
-{
-	return drm_platform_init(&msm_driver, to_platform_device(dev));
-}
-
-static void msm_drm_unbind(struct device *dev)
-{
-	drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
-}
-
-static const struct component_master_ops msm_drm_ops = {
-		.add_components = msm_drm_add_components,
-		.bind = msm_drm_bind,
-		.unbind = msm_drm_unbind,
-};
-
-/*
- * Platform driver:
- */
-
-static int msm_pdev_probe(struct platform_device *pdev)
-{
 	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	return component_master_add(&pdev->dev, &msm_drm_ops);
+	return component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
 }
 
 static int msm_pdev_remove(struct platform_device *pdev)
-- 
1.8.3.1

[PATCH RFC 4/8] drm: msm: update to use component match support

[Rob Clark]

On Sat, Apr 26, 2014 at 7:02 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:
> Update MSM's DRM driver to use the component match support rather than
> add_components.

Looks like the component helper improvements make things nicer for the
driver.  Let me know when you are ready to merge the set, if you want
me to take this one via my tree or not.

Reviewed-by: Rob Clark <robdclark@gmail.com>

> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index f9de156b9e65..26be58bd611d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -865,12 +865,41 @@ static int compare_of(struct device *dev, void *data)
>  {
>         return dev->of_node == data;
>  }
> +#else
> +static int compare_dev(struct device *dev, void *data)
> +{
> +       return dev == data;
> +}
> +#endif
> +
> +static int msm_drm_bind(struct device *dev)
> +{
> +       return drm_platform_init(&msm_driver, to_platform_device(dev));
> +}
> +
> +static void msm_drm_unbind(struct device *dev)
> +{
> +       drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
> +}
> +
> +static const struct component_master_ops msm_drm_ops = {
> +       .bind = msm_drm_bind,
> +       .unbind = msm_drm_unbind,
> +};
> +
> +/*
> + * Platform driver:
> + */
>
> -static int msm_drm_add_components(struct device *master, struct master *m)
> +static int msm_pdev_probe(struct platform_device *pdev)
>  {
> +       struct component_match *match = NULL;
> +#ifdef CONFIG_OF
> +       /* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx
> +        * (or probably any other).. so probably some room for some helpers
> +        */
>         struct device_node *np = master->of_node;
>         unsigned i;
> -       int ret;
>
>         for (i = 0; ; i++) {
>                 struct device_node *node;
> @@ -879,22 +908,9 @@ static int msm_drm_add_components(struct device *master, struct master *m)
>                 if (!node)
>                         break;
>
> -               ret = component_master_add_child(m, compare_of, node);
> -               of_node_put(node);
> -
> -               if (ret)
> -                       return ret;
> +               component_match_add(&pdev->dev, &match, compare_of, node);
>         }
> -       return 0;
> -}
>  #else
> -static int compare_dev(struct device *dev, void *data)
> -{
> -       return dev == data;
> -}
> -
> -static int msm_drm_add_components(struct device *master, struct master *m)
> -{
>         /* For non-DT case, it kinda sucks.  We don't actually have a way
>          * to know whether or not we are waiting for certain devices (or if
>          * they are simply not present).  But for non-DT we only need to
> @@ -918,41 +934,12 @@ static int msm_drm_add_components(struct device *master, struct master *m)
>                         return -EPROBE_DEFER;
>                 }
>
> -               ret = component_master_add_child(m, compare_dev, dev);
> -               if (ret) {
> -                       DBG("could not add child: %d", ret);
> -                       return ret;
> -               }
> +               component_match_add(&pdev->dev, &match, compare_dev, dev);
>         }
> -
> -       return 0;
> -}
>  #endif
>
> -static int msm_drm_bind(struct device *dev)
> -{
> -       return drm_platform_init(&msm_driver, to_platform_device(dev));
> -}
> -
> -static void msm_drm_unbind(struct device *dev)
> -{
> -       drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
> -}
> -
> -static const struct component_master_ops msm_drm_ops = {
> -               .add_components = msm_drm_add_components,
> -               .bind = msm_drm_bind,
> -               .unbind = msm_drm_unbind,
> -};
> -
> -/*
> - * Platform driver:
> - */
> -
> -static int msm_pdev_probe(struct platform_device *pdev)
> -{
>         pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> -       return component_master_add(&pdev->dev, &msm_drm_ops);
> +       return component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
>  }
>
>  static int msm_pdev_remove(struct platform_device *pdev)
> --
> 1.8.3.1
>

Re: [PATCH RFC 4/8] drm: msm: update to use component match support

[Rob Clark]

On Sat, Apr 26, 2014 at 7:02 PM, Russell King
<rmk+kernel@arm.linux.org.uk> wrote:
> Update MSM's DRM driver to use the component match support rather than
> add_components.

Looks like the component helper improvements make things nicer for the
driver.  Let me know when you are ready to merge the set, if you want
me to take this one via my tree or not.

Reviewed-by: Rob Clark <robdclark@gmail.com>

> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 81 ++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index f9de156b9e65..26be58bd611d 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -865,12 +865,41 @@ static int compare_of(struct device *dev, void *data)
>  {
>         return dev->of_node == data;
>  }
> +#else
> +static int compare_dev(struct device *dev, void *data)
> +{
> +       return dev == data;
> +}
> +#endif
> +
> +static int msm_drm_bind(struct device *dev)
> +{
> +       return drm_platform_init(&msm_driver, to_platform_device(dev));
> +}
> +
> +static void msm_drm_unbind(struct device *dev)
> +{
> +       drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
> +}
> +
> +static const struct component_master_ops msm_drm_ops = {
> +       .bind = msm_drm_bind,
> +       .unbind = msm_drm_unbind,
> +};
> +
> +/*
> + * Platform driver:
> + */
>
> -static int msm_drm_add_components(struct device *master, struct master *m)
> +static int msm_pdev_probe(struct platform_device *pdev)
>  {
> +       struct component_match *match = NULL;
> +#ifdef CONFIG_OF
> +       /* NOTE: the CONFIG_OF case duplicates the same code as exynos or imx
> +        * (or probably any other).. so probably some room for some helpers
> +        */
>         struct device_node *np = master->of_node;
>         unsigned i;
> -       int ret;
>
>         for (i = 0; ; i++) {
>                 struct device_node *node;
> @@ -879,22 +908,9 @@ static int msm_drm_add_components(struct device *master, struct master *m)
>                 if (!node)
>                         break;
>
> -               ret = component_master_add_child(m, compare_of, node);
> -               of_node_put(node);
> -
> -               if (ret)
> -                       return ret;
> +               component_match_add(&pdev->dev, &match, compare_of, node);
>         }
> -       return 0;
> -}
>  #else
> -static int compare_dev(struct device *dev, void *data)
> -{
> -       return dev == data;
> -}
> -
> -static int msm_drm_add_components(struct device *master, struct master *m)
> -{
>         /* For non-DT case, it kinda sucks.  We don't actually have a way
>          * to know whether or not we are waiting for certain devices (or if
>          * they are simply not present).  But for non-DT we only need to
> @@ -918,41 +934,12 @@ static int msm_drm_add_components(struct device *master, struct master *m)
>                         return -EPROBE_DEFER;
>                 }
>
> -               ret = component_master_add_child(m, compare_dev, dev);
> -               if (ret) {
> -                       DBG("could not add child: %d", ret);
> -                       return ret;
> -               }
> +               component_match_add(&pdev->dev, &match, compare_dev, dev);
>         }
> -
> -       return 0;
> -}
>  #endif
>
> -static int msm_drm_bind(struct device *dev)
> -{
> -       return drm_platform_init(&msm_driver, to_platform_device(dev));
> -}
> -
> -static void msm_drm_unbind(struct device *dev)
> -{
> -       drm_put_dev(platform_get_drvdata(to_platform_device(dev)));
> -}
> -
> -static const struct component_master_ops msm_drm_ops = {
> -               .add_components = msm_drm_add_components,
> -               .bind = msm_drm_bind,
> -               .unbind = msm_drm_unbind,
> -};
> -
> -/*
> - * Platform driver:
> - */
> -
> -static int msm_pdev_probe(struct platform_device *pdev)
> -{
>         pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> -       return component_master_add(&pdev->dev, &msm_drm_ops);
> +       return component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
>  }
>
>  static int msm_pdev_remove(struct platform_device *pdev)
> --
> 1.8.3.1
>

[PATCH RFC 5/8] imx-drm: update to use component match support

[Russell King]

Update the imx-drm driver to use the component match support rather than
add_components.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/staging/imx-drm/imx-drm-core.c | 57 +++-------------------------------
 1 file changed, 4 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 4144a75e5f71..84cd915bba44 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -577,22 +577,6 @@ static int compare_of(struct device *dev, void *data)
 	return dev->of_node == np;
 }
 
-static LIST_HEAD(imx_drm_components);
-
-static int imx_drm_add_components(struct device *master, struct master *m)
-{
-	struct imx_drm_component *component;
-	int ret;
-
-	list_for_each_entry(component, &imx_drm_components, list) {
-		ret = component_master_add_child(m, compare_of,
-						 component->of_node);
-		if (ret)
-			return ret;
-	}
-	return 0;
-}
-
 static int imx_drm_bind(struct device *dev)
 {
 	return drm_platform_init(&imx_drm_driver, to_platform_device(dev));
@@ -604,43 +588,14 @@ static void imx_drm_unbind(struct device *dev)
 }
 
 static const struct component_master_ops imx_drm_ops = {
-	.add_components = imx_drm_add_components,
 	.bind = imx_drm_bind,
 	.unbind = imx_drm_unbind,
 };
 
-static struct imx_drm_component *imx_drm_find_component(struct device *dev,
-		struct device_node *node)
-{
-	struct imx_drm_component *component;
-
-	list_for_each_entry(component, &imx_drm_components, list)
-		if (component->of_node == node)
-			return component;
-
-	return NULL;
-}
-
-static int imx_drm_add_component(struct device *dev, struct device_node *node)
-{
-	struct imx_drm_component *component;
-
-	if (imx_drm_find_component(dev, node))
-		return 0;
-
-	component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL);
-	if (!component)
-		return -ENOMEM;
-
-	component->of_node = node;
-	list_add_tail(&component->list, &imx_drm_components);
-
-	return 0;
-}
-
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
 	struct device_node *ep, *port, *remote;
+	struct component_match *match = NULL;
 	int ret;
 	int i;
 
@@ -654,9 +609,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)
 		if (!port)
 			break;
 
-		ret = imx_drm_add_component(&pdev->dev, port);
-		if (ret < 0)
-			return ret;
+		component_match_add(&pdev->dev, &match, compare_of, port);
 	}
 
 	if (i == 0) {
@@ -677,10 +630,8 @@ static int imx_drm_platform_probe(struct platform_device *pdev)
 				continue;
 			}
 
-			ret = imx_drm_add_component(&pdev->dev, remote);
+			component_match_add(&pdev->dev, &match, compare_of, remote);
 			of_node_put(remote);
-			if (ret < 0)
-				return ret;
 		}
 		of_node_put(port);
 	}
@@ -689,7 +640,7 @@ static int imx_drm_platform_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return component_master_add(&pdev->dev, &imx_drm_ops);
+	return component_master_add_with_match(&pdev->dev, &imx_drm_ops, match);
 }
 
 static int imx_drm_platform_remove(struct platform_device *pdev)
-- 
1.8.3.1

[PATCH RFC 6/8] component: remove old add_components method

[Russell King]

Now that drivers create an array of component matches at probe time, we
can retire the old methods.  This involves removing the add_components
master method, and removing component_master_add_child() from public
view.  We also remove component_add_master() as that interface is no
longer useful.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c  | 21 +--------------------
 include/linux/component.h |  5 -----
 2 files changed, 1 insertion(+), 25 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 66990bb3adfe..0f7679830cf1 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -84,7 +84,7 @@ static void component_detach_master(struct master *master, struct component *c)
  * function and compare data.  This is safe to call for duplicate matches
  * and will not result in the same component being added multiple times.
  */
-int component_master_add_child(struct master *master,
+static int component_master_add_child(struct master *master,
 	int (*compare)(struct device *, void *), void *compare_data)
 {
 	struct component *c;
@@ -104,7 +104,6 @@ int component_master_add_child(struct master *master,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(component_master_add_child);
 
 static int find_components(struct master *master)
 {
@@ -112,14 +111,6 @@ static int find_components(struct master *master)
 	size_t i;
 	int ret = 0;
 
-	if (!match) {
-		/*
-		 * Search the list of components, looking for components that
-		 * belong to this master, and attach them to the master.
-		 */
-		return master->ops->add_components(master->dev, master);
-	}
-
 	/*
 	 * Scan the array of match functions and attach
 	 * any components which are found to this master.
@@ -288,9 +279,6 @@ int component_master_add_with_match(struct device *dev,
 	struct master *master;
 	int ret;
 
-	if (ops->add_components && match)
-		return -EINVAL;
-
 	/* Reallocate the match array for its true size */
 	match = component_match_realloc(dev, match, match->num);
 	if (IS_ERR(match))
@@ -322,13 +310,6 @@ int component_master_add_with_match(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
-int component_master_add(struct device *dev,
-	const struct component_master_ops *ops)
-{
-	return component_master_add_with_match(dev, ops, NULL);
-}
-EXPORT_SYMBOL_GPL(component_master_add);
-
 void component_master_del(struct device *dev,
 	const struct component_master_ops *ops)
 {
diff --git a/include/linux/component.h b/include/linux/component.h
index c00dcc302611..71c434a6a5ee 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -17,18 +17,13 @@ void component_unbind_all(struct device *, void *);
 struct master;
 
 struct component_master_ops {
-	int (*add_components)(struct device *, struct master *);
 	int (*bind)(struct device *);
 	void (*unbind)(struct device *);
 };
 
-int component_master_add(struct device *, const struct component_master_ops *);
 void component_master_del(struct device *,
 	const struct component_master_ops *);
 
-int component_master_add_child(struct master *master,
-	int (*compare)(struct device *, void *), void *compare_data);
-
 struct component_match;
 
 int component_master_add_with_match(struct device *,
-- 
1.8.3.1

[PATCH RFC 6/8] component: remove old add_components method

[Thierry Reding]

On Sun, Apr 27, 2014 at 12:02:13AM +0100, Russell King wrote:
> Now that drivers create an array of component matches at probe time, we
> can retire the old methods.  This involves removing the add_components
> master method, and removing component_master_add_child() from public
> view.  We also remove component_add_master() as that interface is no
> longer useful.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/component.c  | 21 +--------------------
>  include/linux/component.h |  5 -----
>  2 files changed, 1 insertion(+), 25 deletions(-)

I'm wondering if there may be an advantage to keeping both interfaces.
Even if currently what all implementations do is essentially creating
the match table at probe time there may be use-cases where that doesn't
work so well.

There's also some elegance to the .add_components() interface. I have a
set of patches to convert Tegra DRM to use the component framework and
it works out rather nicely. I'm sure I can get it to work with your
proposed match array changes too, but it seems like it will require more
code.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/ce62653f/attachment.sig>

[PATCH RFC 6/8] component: remove old add_components method

[Russell King - ARM Linux]

On Mon, Apr 28, 2014 at 09:07:28AM +0200, Thierry Reding wrote:
> I'm wondering if there may be an advantage to keeping both interfaces.
> Even if currently what all implementations do is essentially creating
> the match table at probe time there may be use-cases where that doesn't
> work so well.

Keeping both interfaces makes this whole change is pointless, because
then there's no way to avoid having to rebuild the tracking of which
components belong to which master - and we might as well stick with
what we have.

Moreover, one of the other points Laurent raised is that we need to
be able to do partial binds for some subsystems, and for that to work
we need more information held within the component helpers and the
teardown/rebuild of the master/component relationships to be eliminated.

The last point is that the repeated teardown/rebuild is already being
used as a justification to go off and write a completely different
infrastructure instead... it seems some people deem this to be far
too wasteful of our billion cycles per second CPUs.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH RFC 6/8] component: remove old add_components method

[Thierry Reding]

On Mon, Apr 28, 2014 at 11:28:06AM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 28, 2014 at 09:07:28AM +0200, Thierry Reding wrote:
> > I'm wondering if there may be an advantage to keeping both interfaces.
> > Even if currently what all implementations do is essentially creating
> > the match table at probe time there may be use-cases where that doesn't
> > work so well.
> 
> Keeping both interfaces makes this whole change is pointless, because
> then there's no way to avoid having to rebuild the tracking of which
> components belong to which master - and we might as well stick with
> what we have.
> 
> Moreover, one of the other points Laurent raised is that we need to
> be able to do partial binds for some subsystems, and for that to work
> we need more information held within the component helpers and the
> teardown/rebuild of the master/component relationships to be eliminated.
> 
> The last point is that the repeated teardown/rebuild is already being
> used as a justification to go off and write a completely different
> infrastructure instead... it seems some people deem this to be far
> too wasteful of our billion cycles per second CPUs.

FWIW, I do like the simplicity of the ->add_components() variant, and I
don't mind the few extra cycles wasted, but if this change makes sense
for everybody else I'll go convert my Tegra patches on top of this.

By the way, any chance I could get you to look at two earlier patches[0]
I did to make component/master work better for my use-case? The patches
are part of a series to convert the Tegra DRM driver to use the
component framework. The final patch[1] has the complete conversion with
a diff that I really like, although it requires yet another patch to
make it work (because Tegra has a somewhat unusual architecture).

Thierry

[0]: https://lkml.org/lkml/2014/4/22/535
     https://lkml.org/lkml/2014/4/22/1123
[1]: https://lkml.org/lkml/2014/4/22/1099
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/5ef864d2/attachment-0001.sig>

[PATCH RFC 7/8] component: move check for unbound master into try_to_bring_up_masters()

[Russell King]

Clean up the code a little; we don't need to check that the master is
unbound for every invocation of try_to_bring_up_master(), so let's move
it to where it's really needed - try_to_bring_up_masters(), where we may
encounter already bound masters.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 0f7679830cf1..34e88e23d395 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -150,13 +150,6 @@ static int try_to_bring_up_master(struct master *master,
 {
 	int ret;
 
-	if (master->bound)
-		return 0;
-
-	/*
-	 * Search the list of components, looking for components that
-	 * belong to this master, and attach them to the master.
-	 */
 	if (find_components(master)) {
 		/* Failed to find all components */
 		ret = 0;
@@ -196,9 +189,11 @@ static int try_to_bring_up_masters(struct component *component)
 	int ret = 0;
 
 	list_for_each_entry(m, &masters, node) {
-		ret = try_to_bring_up_master(m, component);
-		if (ret != 0)
-			break;
+		if (!m->bound) {
+			ret = try_to_bring_up_master(m, component);
+			if (ret != 0)
+				break;
+		}
 	}
 
 	return ret;
-- 
1.8.3.1

[PATCH RFC 7/8] component: move check for unbound master into try_to_bring_up_masters()

[Thierry Reding]

On Sun, Apr 27, 2014 at 12:02:18AM +0100, Russell King wrote:
> Clean up the code a little; we don't need to check that the master is
> unbound for every invocation of try_to_bring_up_master(), so let's move
> it to where it's really needed - try_to_bring_up_masters(), where we may
> encounter already bound masters.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/base/component.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140428/60a0e5e5/attachment.sig>

[PATCH RFC 8/8] component: track components via array rather than list

[Russell King]

Since we now have an array which defines each component, maintain the
components to be bound in the array rather than a separate list.  We
also need duplicate tracking so we can eliminate multiple bind calls
for the same component: we preserve the list-based component order in
that the first match which adds the component determines its position.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c | 140 +++++++++++++++++++++--------------------------
 1 file changed, 62 insertions(+), 78 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 34e88e23d395..5ca32c60147a 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -18,18 +18,21 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 
+struct component;
+
 struct component_match {
 	size_t alloc;
 	size_t num;
 	struct {
 		void *data;
 		int (*fn)(struct device *, void *);
+		struct component *component;
+		bool duplicate;
 	} compare[0];
 };
 
 struct master {
 	struct list_head node;
-	struct list_head components;
 	bool bound;
 
 	const struct component_master_ops *ops;
@@ -39,7 +42,6 @@ struct master {
 
 struct component {
 	struct list_head node;
-	struct list_head master_node;
 	struct master *master;
 	bool bound;
 
@@ -63,46 +65,20 @@ static struct master *__master_find(struct device *dev,
 	return NULL;
 }
 
-/* Attach an unattached component to a master. */
-static void component_attach_master(struct master *master, struct component *c)
-{
-	c->master = master;
-
-	list_add_tail(&c->master_node, &master->components);
-}
-
-/* Detach a component from a master. */
-static void component_detach_master(struct master *master, struct component *c)
-{
-	list_del(&c->master_node);
-
-	c->master = NULL;
-}
-
-/*
- * Add a component to a master, finding the component via the compare
- * function and compare data.  This is safe to call for duplicate matches
- * and will not result in the same component being added multiple times.
- */
-static int component_master_add_child(struct master *master,
+static struct component *find_component(struct master *master,
 	int (*compare)(struct device *, void *), void *compare_data)
 {
 	struct component *c;
-	int ret = -ENXIO;
 
 	list_for_each_entry(c, &component_list, node) {
 		if (c->master && c->master != master)
 			continue;
 
-		if (compare(c->dev, compare_data)) {
-			if (!c->master)
-				component_attach_master(master, c);
-			ret = 0;
-			break;
-		}
+		if (compare(c->dev, compare_data))
+			return c;
 	}
 
-	return ret;
+	return NULL;
 }
 
 static int find_components(struct master *master)
@@ -116,26 +92,35 @@ static int find_components(struct master *master)
 	 * any components which are found to this master.
 	 */
 	for (i = 0; i < match->num; i++) {
-		ret = component_master_add_child(master,
-						 match->compare[i].fn,
-						 match->compare[i].data);
-		if (ret)
+		struct component *c;
+
+		if (match->compare[i].component)
+			continue;
+
+		c = find_component(master, match->compare[i].fn,
+				   match->compare[i].data);
+		if (!c) {
+			ret = -ENXIO;
 			break;
+		}
+
+		/* Attach this component to the master */
+		match->compare[i].duplicate = !!c->master;
+		match->compare[i].component = c;
+		c->master = master;
 	}
 	return ret;
 }
 
-/* Detach all attached components from this master */
-static void master_remove_components(struct master *master)
+/* Detach component from associated master */
+static void remove_component(struct master *master, struct component *c)
 {
-	while (!list_empty(&master->components)) {
-		struct component *c = list_first_entry(&master->components,
-					struct component, master_node);
-
-		WARN_ON(c->master != master);
+	size_t i;
 
-		component_detach_master(master, c);
-	}
+	/* Detach the component from this master. */
+	for (i = 0; i < master->match->num; i++)
+		if (master->match->compare[i].component == c)
+			master->match->compare[i].component = NULL;
 }
 
 /*
@@ -150,37 +135,25 @@ static int try_to_bring_up_master(struct master *master,
 {
 	int ret;
 
-	if (find_components(master)) {
-		/* Failed to find all components */
-		ret = 0;
-		goto out;
-	}
+	if (find_components(master))
+		return 0;
 
-	if (component && component->master != master) {
-		ret = 0;
-		goto out;
-	}
+	if (component && component->master != master)
+		return 0;
 
-	if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
+		return -ENOMEM;
 
 	/* Found all components */
 	ret = master->ops->bind(master->dev);
 	if (ret < 0) {
 		devres_release_group(master->dev, NULL);
 		dev_info(master->dev, "master bind failed: %d\n", ret);
-		goto out;
+		return ret;
 	}
 
 	master->bound = true;
 	return 1;
-
-out:
-	master_remove_components(master);
-
-	return ret;
 }
 
 static int try_to_bring_up_masters(struct component *component)
@@ -206,8 +179,6 @@ static void take_down_master(struct master *master)
 		devres_release_group(master->dev, NULL);
 		master->bound = false;
 	}
-
-	master_remove_components(master);
 }
 
 static size_t component_match_size(size_t num)
@@ -265,6 +236,7 @@ void component_match_add(struct device *dev, struct component_match **matchptr,
 
 	match->compare[match->num].fn = compare;
 	match->compare[match->num].data = compare_data;
+	match->compare[match->num].component = NULL;
 }
 
 int component_master_add_with_match(struct device *dev,
@@ -286,7 +258,6 @@ int component_master_add_with_match(struct device *dev,
 	master->dev = dev;
 	master->ops = ops;
 	master->match = match;
-	INIT_LIST_HEAD(&master->components);
 
 	/* Add to the list of available masters. */
 	mutex_lock(&component_mutex);
@@ -338,6 +309,7 @@ void component_unbind_all(struct device *master_dev, void *data)
 {
 	struct master *master;
 	struct component *c;
+	size_t i;
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
@@ -345,8 +317,12 @@ void component_unbind_all(struct device *master_dev, void *data)
 	if (!master)
 		return;
 
-	list_for_each_entry_reverse(c, &master->components, master_node)
-		component_unbind(c, master, data);
+	/* Unbind components in reverse order */
+	for (i = master->match->num; i--; )
+		if (!master->match->compare[i].duplicate) {
+			c = master->match->compare[i].component;
+			component_unbind(c, master, data);
+		}
 }
 EXPORT_SYMBOL_GPL(component_unbind_all);
 
@@ -406,6 +382,7 @@ int component_bind_all(struct device *master_dev, void *data)
 {
 	struct master *master;
 	struct component *c;
+	size_t i;
 	int ret = 0;
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
@@ -414,16 +391,21 @@ int component_bind_all(struct device *master_dev, void *data)
 	if (!master)
 		return -EINVAL;
 
-	list_for_each_entry(c, &master->components, master_node) {
-		ret = component_bind(c, master, data);
-		if (ret)
-			break;
-	}
+	/* Bind components in match order */
+	for (i = 0; i < master->match->num; i++)
+		if (!master->match->compare[i].duplicate) {
+			c = master->match->compare[i].component;
+			ret = component_bind(c, master, data);
+			if (ret)
+				break;
+		}
 
 	if (ret != 0) {
-		list_for_each_entry_continue_reverse(c, &master->components,
-						     master_node)
-			component_unbind(c, master, data);
+		for (; i--; )
+			if (!master->match->compare[i].duplicate) {
+				c = master->match->compare[i].component;
+				component_unbind(c, master, data);
+			}
 	}
 
 	return ret;
@@ -471,8 +453,10 @@ void component_del(struct device *dev, const struct component_ops *ops)
 			break;
 		}
 
-	if (component && component->master)
+	if (component && component->master) {
 		take_down_master(component->master);
+		remove_component(component->master, component);
+	}
 
 	mutex_unlock(&component_mutex);
 
-- 
1.8.3.1

[PATCH RFC 1/8] component: fix missed cleanup in case of devres failure

[Russell King]

In try_to_bring_up_master(), we tear down the master's component list
for each error case, except for devres group failure.  Fix this
oversight by making the code less prone to such mistakes.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/base/component.c | 62 ++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index c4778995cd72..d0ebd4431736 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -113,44 +113,44 @@ static void master_remove_components(struct master *master)
 static int try_to_bring_up_master(struct master *master,
 	struct component *component)
 {
-	int ret = 0;
+	int ret;
 
-	if (!master->bound) {
-		/*
-		 * Search the list of components, looking for components that
-		 * belong to this master, and attach them to the master.
-		 */
-		if (master->ops->add_components(master->dev, master)) {
-			/* Failed to find all components */
-			master_remove_components(master);
-			ret = 0;
-			goto out;
-		}
+	if (master->bound)
+		return 0;
 
-		if (component && component->master != master) {
-			master_remove_components(master);
-			ret = 0;
-			goto out;
-		}
+	/*
+	 * Search the list of components, looking for components that
+	 * belong to this master, and attach them to the master.
+	 */
+	if (master->ops->add_components(master->dev, master)) {
+		/* Failed to find all components */
+		ret = 0;
+		goto out;
+	}
 
-		if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) {
-			ret = -ENOMEM;
-			goto out;
-		}
+	if (component && component->master != master) {
+		ret = 0;
+		goto out;
+	}
 
-		/* Found all components */
-		ret = master->ops->bind(master->dev);
-		if (ret < 0) {
-			devres_release_group(master->dev, NULL);
-			dev_info(master->dev, "master bind failed: %d\n", ret);
-			master_remove_components(master);
-			goto out;
-		}
+	if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
-		master->bound = true;
-		ret = 1;
+	/* Found all components */
+	ret = master->ops->bind(master->dev);
+	if (ret < 0) {
+		devres_release_group(master->dev, NULL);
+		dev_info(master->dev, "master bind failed: %d\n", ret);
+		goto out;
 	}
+
+	master->bound = true;
+	return 1;
+
 out:
+	master_remove_components(master);
 
 	return ret;
 }
-- 
1.8.3.1

[RFC PATCH 0/8] component helper improvements

[Daniel Vetter]

On Sun, Apr 27, 2014 at 1:00 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> A while back, Laurent raised some comments about the component helper,
> which this patch set starts to address.
>
> The first point it addresses is the repeated parsing inefficiency when
> deferred probing occurs.  When DT is used, the structure of the
> component helper today means that masters end up parsing the device
> tree for each attempt to re-bind the driver.
>
> We remove this inefficiency by creating an array of matching data and
> functions, which the component helper can use internally to match up
> components to their master.
>
> The second point was the inefficiency of destroying the list of
> components each time we saw a failure.  We did this to ensure that
> we kept things correctly ordered: component bind order matters.
> As we have an array instead, the array is already ordered, so we
> use this array to store the component pointers instead of a list,
> and remember which are duplicates (and thus should be avoided.)
> Avoiding the right duplicates matters as we walk the array in the
> opposite direction at tear down.
>
> I would like to see patches 1-5 scheduled for the next merge window,
> with 6-8 for the following window - this gives us grace of one kernel
> cycle to ensure that any new component helper users are properly
> converted.

Afaict the actual patches haven't made it to dri-devel, only to
linux-arm-kernel. Are they stuck somewhere?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH 0/8] component helper improvements

[Daniel Vetter]

On Sun, Apr 27, 2014 at 1:00 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> A while back, Laurent raised some comments about the component helper,
> which this patch set starts to address.
>
> The first point it addresses is the repeated parsing inefficiency when
> deferred probing occurs.  When DT is used, the structure of the
> component helper today means that masters end up parsing the device
> tree for each attempt to re-bind the driver.
>
> We remove this inefficiency by creating an array of matching data and
> functions, which the component helper can use internally to match up
> components to their master.
>
> The second point was the inefficiency of destroying the list of
> components each time we saw a failure.  We did this to ensure that
> we kept things correctly ordered: component bind order matters.
> As we have an array instead, the array is already ordered, so we
> use this array to store the component pointers instead of a list,
> and remember which are duplicates (and thus should be avoided.)
> Avoiding the right duplicates matters as we walk the array in the
> opposite direction at tear down.
>
> I would like to see patches 1-5 scheduled for the next merge window,
> with 6-8 for the following window - this gives us grace of one kernel
> cycle to ensure that any new component helper users are properly
> converted.

Afaict the actual patches haven't made it to dri-devel, only to
linux-arm-kernel. Are they stuck somewhere?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC PATCH 0/8] component helper improvements

[Russell King - ARM Linux]

On Sun, Apr 27, 2014 at 02:51:30PM +0200, Daniel Vetter wrote:
> On Sun, Apr 27, 2014 at 1:00 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > A while back, Laurent raised some comments about the component helper,
> > which this patch set starts to address.
> >
> > The first point it addresses is the repeated parsing inefficiency when
> > deferred probing occurs.  When DT is used, the structure of the
> > component helper today means that masters end up parsing the device
> > tree for each attempt to re-bind the driver.
> >
> > We remove this inefficiency by creating an array of matching data and
> > functions, which the component helper can use internally to match up
> > components to their master.
> >
> > The second point was the inefficiency of destroying the list of
> > components each time we saw a failure.  We did this to ensure that
> > we kept things correctly ordered: component bind order matters.
> > As we have an array instead, the array is already ordered, so we
> > use this array to store the component pointers instead of a list,
> > and remember which are duplicates (and thus should be avoided.)
> > Avoiding the right duplicates matters as we walk the array in the
> > opposite direction at tear down.
> >
> > I would like to see patches 1-5 scheduled for the next merge window,
> > with 6-8 for the following window - this gives us grace of one kernel
> > cycle to ensure that any new component helper users are properly
> > converted.
> 
> Afaict the actual patches haven't made it to dri-devel, only to
> linux-arm-kernel. Are they stuck somewhere?

The patches themselves end up being Cc'd depending on their content and
the contents of the MAINTAINERS file, unless I specifically tell my
scripts that the patches are to be sent to/cc people - generally I do
that for the primary recipients of the series.

That means only the patch(es) which touch DRM stuff were copied to
dri-devel, in this case, that being the MSM one.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [RFC PATCH 0/8] component helper improvements

[Russell King - ARM Linux]

On Sun, Apr 27, 2014 at 02:51:30PM +0200, Daniel Vetter wrote:
> On Sun, Apr 27, 2014 at 1:00 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > A while back, Laurent raised some comments about the component helper,
> > which this patch set starts to address.
> >
> > The first point it addresses is the repeated parsing inefficiency when
> > deferred probing occurs.  When DT is used, the structure of the
> > component helper today means that masters end up parsing the device
> > tree for each attempt to re-bind the driver.
> >
> > We remove this inefficiency by creating an array of matching data and
> > functions, which the component helper can use internally to match up
> > components to their master.
> >
> > The second point was the inefficiency of destroying the list of
> > components each time we saw a failure.  We did this to ensure that
> > we kept things correctly ordered: component bind order matters.
> > As we have an array instead, the array is already ordered, so we
> > use this array to store the component pointers instead of a list,
> > and remember which are duplicates (and thus should be avoided.)
> > Avoiding the right duplicates matters as we walk the array in the
> > opposite direction at tear down.
> >
> > I would like to see patches 1-5 scheduled for the next merge window,
> > with 6-8 for the following window - this gives us grace of one kernel
> > cycle to ensure that any new component helper users are properly
> > converted.
> 
> Afaict the actual patches haven't made it to dri-devel, only to
> linux-arm-kernel. Are they stuck somewhere?

The patches themselves end up being Cc'd depending on their content and
the contents of the MAINTAINERS file, unless I specifically tell my
scripts that the patches are to be sent to/cc people - generally I do
that for the primary recipients of the series.

That means only the patch(es) which touch DRM stuff were copied to
dri-devel, in this case, that being the MSM one.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[RFC PATCH 0/8] component helper improvements

[Thierry Reding]

On Sun, Apr 27, 2014 at 12:00:25AM +0100, Russell King - ARM Linux wrote:
> A while back, Laurent raised some comments about the component helper,
> which this patch set starts to address.
> 
> The first point it addresses is the repeated parsing inefficiency when
> deferred probing occurs.  When DT is used, the structure of the
> component helper today means that masters end up parsing the device
> tree for each attempt to re-bind the driver.
> 
> We remove this inefficiency by creating an array of matching data and
> functions, which the component helper can use internally to match up
> components to their master.
> 
> The second point was the inefficiency of destroying the list of
> components each time we saw a failure.  We did this to ensure that
> we kept things correctly ordered: component bind order matters.
> As we have an array instead, the array is already ordered, so we
> use this array to store the component pointers instead of a list,
> and remember which are duplicates (and thus should be avoided.)
> Avoiding the right duplicates matters as we walk the array in the
> opposite direction at tear down.

I've been looking at converting the Tegra DRM driver to the component
helpers for a while now and had to make some changes to make it work for
that particular use-case. While updating the imx-drm and msm DRM drivers
for those changes I noticed an oddity. Both of the existing drivers use
the following pattern:

	static int driver_component_bind(struct device *dev,
					 struct device *master,
					 void *data)
	{
		allocate memory
		request resources
		...
		hook up to subsystem
		...
		enable hardware
	}

	static const struct component_ops driver_component_ops = {
		.bind = driver_component_bind,
	};

	static int driver_probe(struct platform_device *pdev)
	{
		return component_add(&pdev->dev, &driver_component_ops);
	}

While converting Tegra DRM, what I intuitively did (I didn't actually
look at the other drivers for inspiration) was something more along the
lines of the following:

	static int driver_component_bind(struct device *dev,
					 struct device *master,
					 void *data)
	{
		hook up to subsystem
		...
		enable hardware
	}

	static const struct component_ops driver_component_ops = {
		.bind = driver_component_bind,
	};

	static int driver_probe(struct platform_device *pdev)
	{
		allocate memory
		request resources
		...
		return component_add(&pdev->dev, &driver_component_ops);
	}

Since usually deferred probing is caused by resource allocations failing
this has the side-effect of handling deferred probing before the master
device is even bound (the component_add() happens as the very last step)
and therefore there is less risk for component_bind_all() to fail. I've
actually never seen it fail at all. Failure at that point is almost
certainly irrecoverable anyway.

It would seem to me that if other drivers followed the same pattern, the
second point above is solved by moving deferred probe handling one level
up and reduce the work of the component helpers to gluing together the
components on a subsystem level.

Another advantage to that pattern is that probe failure happens on a
much more granular level. It's handled by each component device rather
than all at once when the master is bound. By that time all components
will be ready and the heavy work of building the subsystem device will
usually not have to be undone as opposed to the former pattern where
that process is bound to be interrupted possibly many times be deferred
probing.

But perhaps I'm missing something. Was there another reason for choosing
this particular pattern?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140514/2cd86462/attachment.sig>

Re: [RFC PATCH 0/8] component helper improvements

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3728 bytes --]

On Sun, Apr 27, 2014 at 12:00:25AM +0100, Russell King - ARM Linux wrote:
> A while back, Laurent raised some comments about the component helper,
> which this patch set starts to address.
> 
> The first point it addresses is the repeated parsing inefficiency when
> deferred probing occurs.  When DT is used, the structure of the
> component helper today means that masters end up parsing the device
> tree for each attempt to re-bind the driver.
> 
> We remove this inefficiency by creating an array of matching data and
> functions, which the component helper can use internally to match up
> components to their master.
> 
> The second point was the inefficiency of destroying the list of
> components each time we saw a failure.  We did this to ensure that
> we kept things correctly ordered: component bind order matters.
> As we have an array instead, the array is already ordered, so we
> use this array to store the component pointers instead of a list,
> and remember which are duplicates (and thus should be avoided.)
> Avoiding the right duplicates matters as we walk the array in the
> opposite direction at tear down.

I've been looking at converting the Tegra DRM driver to the component
helpers for a while now and had to make some changes to make it work for
that particular use-case. While updating the imx-drm and msm DRM drivers
for those changes I noticed an oddity. Both of the existing drivers use
the following pattern:

	static int driver_component_bind(struct device *dev,
					 struct device *master,
					 void *data)
	{
		allocate memory
		request resources
		...
		hook up to subsystem
		...
		enable hardware
	}

	static const struct component_ops driver_component_ops = {
		.bind = driver_component_bind,
	};

	static int driver_probe(struct platform_device *pdev)
	{
		return component_add(&pdev->dev, &driver_component_ops);
	}

While converting Tegra DRM, what I intuitively did (I didn't actually
look at the other drivers for inspiration) was something more along the
lines of the following:

	static int driver_component_bind(struct device *dev,
					 struct device *master,
					 void *data)
	{
		hook up to subsystem
		...
		enable hardware
	}

	static const struct component_ops driver_component_ops = {
		.bind = driver_component_bind,
	};

	static int driver_probe(struct platform_device *pdev)
	{
		allocate memory
		request resources
		...
		return component_add(&pdev->dev, &driver_component_ops);
	}

Since usually deferred probing is caused by resource allocations failing
this has the side-effect of handling deferred probing before the master
device is even bound (the component_add() happens as the very last step)
and therefore there is less risk for component_bind_all() to fail. I've
actually never seen it fail at all. Failure at that point is almost
certainly irrecoverable anyway.

It would seem to me that if other drivers followed the same pattern, the
second point above is solved by moving deferred probe handling one level
up and reduce the work of the component helpers to gluing together the
components on a subsystem level.

Another advantage to that pattern is that probe failure happens on a
much more granular level. It's handled by each component device rather
than all at once when the master is bound. By that time all components
will be ready and the heavy work of building the subsystem device will
usually not have to be undone as opposed to the former pattern where
that process is bound to be interrupted possibly many times be deferred
probing.

But perhaps I'm missing something. Was there another reason for choosing
this particular pattern?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFC PATCH 0/8] component helper improvements

[Russell King - ARM Linux]

On Wed, May 14, 2014 at 08:42:17PM +0200, Thierry Reding wrote:
> I've been looking at converting the Tegra DRM driver to the component
> helpers for a while now and had to make some changes to make it work for
> that particular use-case. While updating the imx-drm and msm DRM drivers
> for those changes I noticed an oddity. Both of the existing drivers use
> the following pattern:
> 
> 	static int driver_component_bind(struct device *dev,
> 					 struct device *master,
> 					 void *data)
> 	{
> 		allocate memory
> 		request resources
> 		...
> 		hook up to subsystem
> 		...
> 		enable hardware
> 	}
> 
> 	static const struct component_ops driver_component_ops = {
> 		.bind = driver_component_bind,
> 	};
> 
> 	static int driver_probe(struct platform_device *pdev)
> 	{
> 		return component_add(&pdev->dev, &driver_component_ops);
> 	}
> 
> While converting Tegra DRM, what I intuitively did (I didn't actually
> look at the other drivers for inspiration) was something more along the
> lines of the following:
> 
> 	static int driver_component_bind(struct device *dev,
> 					 struct device *master,
> 					 void *data)
> 	{
> 		hook up to subsystem
> 		...
> 		enable hardware
> 	}
> 
> 	static const struct component_ops driver_component_ops = {
> 		.bind = driver_component_bind,
> 	};
> 
> 	static int driver_probe(struct platform_device *pdev)
> 	{
> 		allocate memory
> 		request resources
> 		...
> 		return component_add(&pdev->dev, &driver_component_ops);
> 	}
> 
> Since usually deferred probing is caused by resource allocations failing
> this has the side-effect of handling deferred probing before the master
> device is even bound (the component_add() happens as the very last step)
> and therefore there is less risk for component_bind_all() to fail. I've
> actually never seen it fail at all. Failure at that point is almost
> certainly irrecoverable anyway.

It isn't irrecoverable - that case is handled.

I really don't like two-stage driver initialisation - it increases the
chances of bugs creeping in.  Take for example this code:

probe()
{
	priv = devm_kzalloc(dev, whatever);
	priv->mem = devm_ioremap_resource(dev, res);
	dev_set_drvdata(dev, priv);
	return component_add(dev, &ops);
}

So far so good, not much can go wrong at that point - we know exactly
what state the 'priv' structure is at the point where the component_add
call is made.

Now, when the ops' bind method is called, we retrieve the private data.
At this point, we can no longer rely on the initialisation state of
many of the members.  We can't assume that they were zero when we're
called, because we can have this sequence of events:

- driver is probed
- component is bound
- component is unbound
- component is bound

At this point, the private data will be dirty.  This actually makes the
use of devm_kzalloc() a joke in the probe function - although it does
initialise all members to zero, we can't rely on that at all when the
component is bound.

While the driver itself may be coded for this to be safe, can we say the
same for any structures which are embedded into the private data, which
may be private to other subsystems?

By way of illustration, ASoC can also have this two stage approach.  I'll
draw your attention to SGTL5000, and the recent patch I submitted (which
I don't think will be taken.)  This driver suffers badly if the ASoC
"card" is bound, then unbound, and an attempt to rebind it again.  That's
because the driver gets some managed resources in both the first stage and
the second stage, and expects them to be automatically released in when
the second stage is torn down.  This bug has existed for a very long
time, and has gone unnoticed (it will be unnoticed until you try to debug
by removing modules and trying to load replacements, which is how I found
it.)

That exact bug can't happen with the component helpers, because I
explicitly thought about the handling of managed resources, and added
the necessary support to deal with these correctly.  However, it serves
as an example that, despite comments from people saying that my fear
is unlikely to happen, we already have code which suffers from issues
with two-stage initialisation.

The unfortunate thing is that validation testing for Linux tends not to
venture much past "does it boot", "are my devices present" and "can I run
some programs".  It doesn't cover system shutdown/reboot very often
(we've had bugs which have been present for ages there - my test farm
explicitly does a power off after boot testing now) and it hardly ever
covers drivers being unbound or module removal.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [RFC PATCH 0/8] component helper improvements

[Russell King - ARM Linux]

On Wed, May 14, 2014 at 08:42:17PM +0200, Thierry Reding wrote:
> I've been looking at converting the Tegra DRM driver to the component
> helpers for a while now and had to make some changes to make it work for
> that particular use-case. While updating the imx-drm and msm DRM drivers
> for those changes I noticed an oddity. Both of the existing drivers use
> the following pattern:
> 
> 	static int driver_component_bind(struct device *dev,
> 					 struct device *master,
> 					 void *data)
> 	{
> 		allocate memory
> 		request resources
> 		...
> 		hook up to subsystem
> 		...
> 		enable hardware
> 	}
> 
> 	static const struct component_ops driver_component_ops = {
> 		.bind = driver_component_bind,
> 	};
> 
> 	static int driver_probe(struct platform_device *pdev)
> 	{
> 		return component_add(&pdev->dev, &driver_component_ops);
> 	}
> 
> While converting Tegra DRM, what I intuitively did (I didn't actually
> look at the other drivers for inspiration) was something more along the
> lines of the following:
> 
> 	static int driver_component_bind(struct device *dev,
> 					 struct device *master,
> 					 void *data)
> 	{
> 		hook up to subsystem
> 		...
> 		enable hardware
> 	}
> 
> 	static const struct component_ops driver_component_ops = {
> 		.bind = driver_component_bind,
> 	};
> 
> 	static int driver_probe(struct platform_device *pdev)
> 	{
> 		allocate memory
> 		request resources
> 		...
> 		return component_add(&pdev->dev, &driver_component_ops);
> 	}
> 
> Since usually deferred probing is caused by resource allocations failing
> this has the side-effect of handling deferred probing before the master
> device is even bound (the component_add() happens as the very last step)
> and therefore there is less risk for component_bind_all() to fail. I've
> actually never seen it fail at all. Failure at that point is almost
> certainly irrecoverable anyway.

It isn't irrecoverable - that case is handled.

I really don't like two-stage driver initialisation - it increases the
chances of bugs creeping in.  Take for example this code:

probe()
{
	priv = devm_kzalloc(dev, whatever);
	priv->mem = devm_ioremap_resource(dev, res);
	dev_set_drvdata(dev, priv);
	return component_add(dev, &ops);
}

So far so good, not much can go wrong at that point - we know exactly
what state the 'priv' structure is at the point where the component_add
call is made.

Now, when the ops' bind method is called, we retrieve the private data.
At this point, we can no longer rely on the initialisation state of
many of the members.  We can't assume that they were zero when we're
called, because we can have this sequence of events:

- driver is probed
- component is bound
- component is unbound
- component is bound

At this point, the private data will be dirty.  This actually makes the
use of devm_kzalloc() a joke in the probe function - although it does
initialise all members to zero, we can't rely on that at all when the
component is bound.

While the driver itself may be coded for this to be safe, can we say the
same for any structures which are embedded into the private data, which
may be private to other subsystems?

By way of illustration, ASoC can also have this two stage approach.  I'll
draw your attention to SGTL5000, and the recent patch I submitted (which
I don't think will be taken.)  This driver suffers badly if the ASoC
"card" is bound, then unbound, and an attempt to rebind it again.  That's
because the driver gets some managed resources in both the first stage and
the second stage, and expects them to be automatically released in when
the second stage is torn down.  This bug has existed for a very long
time, and has gone unnoticed (it will be unnoticed until you try to debug
by removing modules and trying to load replacements, which is how I found
it.)

That exact bug can't happen with the component helpers, because I
explicitly thought about the handling of managed resources, and added
the necessary support to deal with these correctly.  However, it serves
as an example that, despite comments from people saying that my fear
is unlikely to happen, we already have code which suffers from issues
with two-stage initialisation.

The unfortunate thing is that validation testing for Linux tends not to
venture much past "does it boot", "are my devices present" and "can I run
some programs".  It doesn't cover system shutdown/reboot very often
(we've had bugs which have been present for ages there - my test farm
explicitly does a power off after boot testing now) and it hardly ever
covers drivers being unbound or module removal.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.