All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] component: Make into an aggregate bus
@ 2021-05-20  0:25 ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

This series is from discussion we had on reordering the device lists for
drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
the aggregate device onto and then we probe the device once all the
components are probed and call component_add(). The probe/remove hooks
are where the bind/unbind calls go, and then a shutdown hook is added
that can be used to shutdown the drm display pipeline at the right time.

This works for me on my sc7180 board, but I'm currently struggling with
the last patch where we migrate the msm driver. It runs into a runtime
PM problem where the parent device isn't runtime PM enabled yet. I'm
still trying to figure out a clean solution there. Moving runtime PM
around breaks boot and I think that's because the power domain is off.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>

[1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org

Stephen Boyd (7):
  component: Drop 'dev' argument to component_match_realloc()
  component: Rename 'dev' to 'parent'
  component: Introduce struct aggregate_device
  component: Introduce the aggregate bus_type
  component: Use dev.parent instead of adev->parent
  component: Move struct aggregate_device out to header file
  drm/msm: Migrate to aggregate driver

 drivers/base/component.c      | 614 ++++++++++++++++++++++------------
 drivers/gpu/drm/msm/msm_drv.c |  47 +--
 include/linux/component.h     |  73 +++-
 3 files changed, 487 insertions(+), 247 deletions(-)


base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
-- 
https://chromeos.dev


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 0/7] component: Make into an aggregate bus
@ 2021-05-20  0:25 ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

This series is from discussion we had on reordering the device lists for
drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
the aggregate device onto and then we probe the device once all the
components are probed and call component_add(). The probe/remove hooks
are where the bind/unbind calls go, and then a shutdown hook is added
that can be used to shutdown the drm display pipeline at the right time.

This works for me on my sc7180 board, but I'm currently struggling with
the last patch where we migrate the msm driver. It runs into a runtime
PM problem where the parent device isn't runtime PM enabled yet. I'm
still trying to figure out a clean solution there. Moving runtime PM
around breaks boot and I think that's because the power domain is off.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>

[1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org

Stephen Boyd (7):
  component: Drop 'dev' argument to component_match_realloc()
  component: Rename 'dev' to 'parent'
  component: Introduce struct aggregate_device
  component: Introduce the aggregate bus_type
  component: Use dev.parent instead of adev->parent
  component: Move struct aggregate_device out to header file
  drm/msm: Migrate to aggregate driver

 drivers/base/component.c      | 614 ++++++++++++++++++++++------------
 drivers/gpu/drm/msm/msm_drv.c |  47 +--
 include/linux/component.h     |  73 +++-
 3 files changed, 487 insertions(+), 247 deletions(-)


base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
-- 
https://chromeos.dev


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 1/7] component: Drop 'dev' argument to component_match_realloc()
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

This argument isn't used. Drop it.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 272ba42392f0..bbe1757dfa89 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -307,8 +307,7 @@ static void devm_component_match_release(struct device *dev, void *res)
 	component_match_release(dev, res);
 }
 
-static int component_match_realloc(struct device *dev,
-	struct component_match *match, size_t num)
+static int component_match_realloc(struct component_match *match, size_t num)
 {
 	struct component_match_array *new;
 
@@ -359,7 +358,7 @@ static void __component_match_add(struct device *master,
 		size_t new_size = match->alloc + 16;
 		int ret;
 
-		ret = component_match_realloc(master, match, new_size);
+		ret = component_match_realloc(match, new_size);
 		if (ret) {
 			*matchptr = ERR_PTR(ret);
 			return;
@@ -469,7 +468,7 @@ int component_master_add_with_match(struct device *dev,
 	int ret;
 
 	/* Reallocate the match array for its true size */
-	ret = component_match_realloc(dev, match, match->num);
+	ret = component_match_realloc(match, match->num);
 	if (ret)
 		return ret;
 
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 1/7] component: Drop 'dev' argument to component_match_realloc()
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

This argument isn't used. Drop it.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 272ba42392f0..bbe1757dfa89 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -307,8 +307,7 @@ static void devm_component_match_release(struct device *dev, void *res)
 	component_match_release(dev, res);
 }
 
-static int component_match_realloc(struct device *dev,
-	struct component_match *match, size_t num)
+static int component_match_realloc(struct component_match *match, size_t num)
 {
 	struct component_match_array *new;
 
@@ -359,7 +358,7 @@ static void __component_match_add(struct device *master,
 		size_t new_size = match->alloc + 16;
 		int ret;
 
-		ret = component_match_realloc(master, match, new_size);
+		ret = component_match_realloc(match, new_size);
 		if (ret) {
 			*matchptr = ERR_PTR(ret);
 			return;
@@ -469,7 +468,7 @@ int component_master_add_with_match(struct device *dev,
 	int ret;
 
 	/* Reallocate the match array for its true size */
-	ret = component_match_realloc(dev, match, match->num);
+	ret = component_match_realloc(match, match->num);
 	if (ret)
 		return ret;
 
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 2/7] component: Rename 'dev' to 'parent'
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

Let's rename this struct member to 'parent' to better reflect the
reality that it's the parent device of this psuedo-device. In the next
patch we'll put a 'struct device' inside of this struct so moving this
away simplifies that patch by reducing the number of places that 'dev'
is modified.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c | 89 +++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index bbe1757dfa89..5e79299f6c3f 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -63,7 +63,7 @@ struct master {
 	bool bound;
 
 	const struct component_master_ops *ops;
-	struct device *dev;
+	struct device *parent;
 	struct component_match *match;
 };
 
@@ -95,7 +95,7 @@ static int component_devices_show(struct seq_file *s, void *data)
 	seq_printf(s, "%-40s %20s\n", "master name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
-		   dev_name(m->dev), m->bound ? "bound" : "not bound");
+		   dev_name(m->parent), m->bound ? "bound" : "not bound");
 
 	seq_printf(s, "%-40s %20s\n", "device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
@@ -124,13 +124,13 @@ core_initcall(component_debug_init);
 
 static void component_master_debugfs_add(struct master *m)
 {
-	debugfs_create_file(dev_name(m->dev), 0444, component_debugfs_dir, m,
+	debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
 			    &component_devices_fops);
 }
 
 static void component_master_debugfs_del(struct master *m)
 {
-	debugfs_remove(debugfs_lookup(dev_name(m->dev), component_debugfs_dir));
+	debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
 }
 
 #else
@@ -143,13 +143,13 @@ static void component_master_debugfs_del(struct master *m)
 
 #endif
 
-static struct master *__master_find(struct device *dev,
+static struct master *__master_find(struct device *parent,
 	const struct component_master_ops *ops)
 {
 	struct master *m;
 
 	list_for_each_entry(m, &masters, node)
-		if (m->dev == dev && (!ops || m->ops == ops))
+		if (m->parent == parent && (!ops || m->ops == ops))
 			return m;
 
 	return NULL;
@@ -189,7 +189,7 @@ static int find_components(struct master *master)
 		struct component_match_array *mc = &match->compare[i];
 		struct component *c;
 
-		dev_dbg(master->dev, "Looking for component %zu\n", i);
+		dev_dbg(master->parent, "Looking for component %zu\n", i);
 
 		if (match->compare[i].component)
 			continue;
@@ -200,7 +200,7 @@ static int find_components(struct master *master)
 			break;
 		}
 
-		dev_dbg(master->dev, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
+		dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
 
 		/* Attach this component to the master */
 		match->compare[i].duplicate = !!c->master;
@@ -233,28 +233,28 @@ static int try_to_bring_up_master(struct master *master,
 {
 	int ret;
 
-	dev_dbg(master->dev, "trying to bring up master\n");
+	dev_dbg(master->parent, "trying to bring up master\n");
 
 	if (find_components(master)) {
-		dev_dbg(master->dev, "master has incomplete components\n");
+		dev_dbg(master->parent, "master has incomplete components\n");
 		return 0;
 	}
 
 	if (component && component->master != master) {
-		dev_dbg(master->dev, "master is not for this component (%s)\n",
+		dev_dbg(master->parent, "master is not for this component (%s)\n",
 			dev_name(component->dev));
 		return 0;
 	}
 
-	if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
+	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/* Found all components */
-	ret = master->ops->bind(master->dev);
+	ret = master->ops->bind(master->parent);
 	if (ret < 0) {
-		devres_release_group(master->dev, NULL);
+		devres_release_group(master->parent, NULL);
 		if (ret != -EPROBE_DEFER)
-			dev_info(master->dev, "master bind failed: %d\n", ret);
+			dev_info(master->parent, "master bind failed: %d\n", ret);
 		return ret;
 	}
 
@@ -281,32 +281,27 @@ static int try_to_bring_up_masters(struct component *component)
 static void take_down_master(struct master *master)
 {
 	if (master->bound) {
-		master->ops->unbind(master->dev);
-		devres_release_group(master->dev, NULL);
+		master->ops->unbind(master->parent);
+		devres_release_group(master->parent, NULL);
 		master->bound = false;
 	}
 }
 
-static void component_match_release(struct device *master,
-	struct component_match *match)
+static void devm_component_match_release(struct device *parent, void *res)
 {
+	struct component_match *match = res;
 	unsigned int i;
 
 	for (i = 0; i < match->num; i++) {
 		struct component_match_array *mc = &match->compare[i];
 
 		if (mc->release)
-			mc->release(master, mc->data);
+			mc->release(parent, mc->data);
 	}
 
 	kfree(match->compare);
 }
 
-static void devm_component_match_release(struct device *dev, void *res)
-{
-	component_match_release(dev, res);
-}
-
 static int component_match_realloc(struct component_match *match, size_t num)
 {
 	struct component_match_array *new;
@@ -450,7 +445,7 @@ static void free_master(struct master *master)
 
 /**
  * component_master_add_with_match - register an aggregate driver
- * @dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @ops: callbacks for the aggregate driver
  * @match: component match list for the aggregate driver
  *
@@ -460,7 +455,7 @@ static void free_master(struct master *master)
  * &component_master_ops.bind from @ops. Must be unregistered by calling
  * component_master_del().
  */
-int component_master_add_with_match(struct device *dev,
+int component_master_add_with_match(struct device *parent,
 	const struct component_master_ops *ops,
 	struct component_match *match)
 {
@@ -476,7 +471,7 @@ int component_master_add_with_match(struct device *dev,
 	if (!master)
 		return -ENOMEM;
 
-	master->dev = dev;
+	master->parent = parent;
 	master->ops = ops;
 	master->match = match;
 
@@ -498,20 +493,20 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
 /**
  * component_master_del - unregister an aggregate driver
- * @dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @ops: callbacks for the aggregate driver
  *
  * Unregisters an aggregate driver registered with
  * component_master_add_with_match(). If necessary the aggregate driver is first
  * disassembled by calling &component_master_ops.unbind from @ops.
  */
-void component_master_del(struct device *dev,
+void component_master_del(struct device *parent,
 	const struct component_master_ops *ops)
 {
 	struct master *master;
 
 	mutex_lock(&component_mutex);
-	master = __master_find(dev, ops);
+	master = __master_find(parent, ops);
 	if (master) {
 		take_down_master(master);
 		free_master(master);
@@ -526,7 +521,7 @@ static void component_unbind(struct component *component,
 	WARN_ON(!component->bound);
 
 	if (component->ops && component->ops->unbind)
-		component->ops->unbind(component->dev, master->dev, data);
+		component->ops->unbind(component->dev, master->parent, data);
 	component->bound = false;
 
 	/* Release all resources claimed in the binding of this component */
@@ -535,14 +530,14 @@ static void component_unbind(struct component *component,
 
 /**
  * component_unbind_all - unbind all components of an aggregate driver
- * @master_dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @data: opaque pointer, passed to all components
  *
- * Unbinds all components of the aggregate @dev by passing @data to their
+ * Unbinds all components of the aggregate device by passing @data to their
  * &component_ops.unbind functions. Should be called from
  * &component_master_ops.unbind.
  */
-void component_unbind_all(struct device *master_dev, void *data)
+void component_unbind_all(struct device *parent, void *data)
 {
 	struct master *master;
 	struct component *c;
@@ -550,7 +545,7 @@ void component_unbind_all(struct device *master_dev, void *data)
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(master_dev, NULL);
+	master = __master_find(parent, NULL);
 	if (!master)
 		return;
 
@@ -573,7 +568,7 @@ static int component_bind(struct component *component, struct master *master,
 	 * This allows us to roll-back a failed component without
 	 * affecting anything else.
 	 */
-	if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
+	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/*
@@ -582,14 +577,14 @@ static int component_bind(struct component *component, struct master *master,
 	 * at the appropriate moment.
 	 */
 	if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
-		devres_release_group(master->dev, NULL);
+		devres_release_group(master->parent, NULL);
 		return -ENOMEM;
 	}
 
-	dev_dbg(master->dev, "binding %s (ops %ps)\n",
+	dev_dbg(master->parent, "binding %s (ops %ps)\n",
 		dev_name(component->dev), component->ops);
 
-	ret = component->ops->bind(component->dev, master->dev, data);
+	ret = component->ops->bind(component->dev, master->parent, data);
 	if (!ret) {
 		component->bound = true;
 
@@ -600,16 +595,16 @@ static int component_bind(struct component *component, struct master *master,
 		 * can clean those resources up independently.
 		 */
 		devres_close_group(component->dev, NULL);
-		devres_remove_group(master->dev, NULL);
+		devres_remove_group(master->parent, NULL);
 
-		dev_info(master->dev, "bound %s (ops %ps)\n",
+		dev_info(master->parent, "bound %s (ops %ps)\n",
 			 dev_name(component->dev), component->ops);
 	} else {
 		devres_release_group(component->dev, NULL);
-		devres_release_group(master->dev, NULL);
+		devres_release_group(master->parent, NULL);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(master->dev, "failed to bind %s (ops %ps): %d\n",
+			dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
 				dev_name(component->dev), component->ops, ret);
 	}
 
@@ -618,14 +613,14 @@ static int component_bind(struct component *component, struct master *master,
 
 /**
  * component_bind_all - bind all components of an aggregate driver
- * @master_dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @data: opaque pointer, passed to all components
  *
  * Binds all components of the aggregate @dev by passing @data to their
  * &component_ops.bind functions. Should be called from
  * &component_master_ops.bind.
  */
-int component_bind_all(struct device *master_dev, void *data)
+int component_bind_all(struct device *parent, void *data)
 {
 	struct master *master;
 	struct component *c;
@@ -634,7 +629,7 @@ int component_bind_all(struct device *master_dev, void *data)
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(master_dev, NULL);
+	master = __master_find(parent, NULL);
 	if (!master)
 		return -EINVAL;
 
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 2/7] component: Rename 'dev' to 'parent'
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

Let's rename this struct member to 'parent' to better reflect the
reality that it's the parent device of this psuedo-device. In the next
patch we'll put a 'struct device' inside of this struct so moving this
away simplifies that patch by reducing the number of places that 'dev'
is modified.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c | 89 +++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 47 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index bbe1757dfa89..5e79299f6c3f 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -63,7 +63,7 @@ struct master {
 	bool bound;
 
 	const struct component_master_ops *ops;
-	struct device *dev;
+	struct device *parent;
 	struct component_match *match;
 };
 
@@ -95,7 +95,7 @@ static int component_devices_show(struct seq_file *s, void *data)
 	seq_printf(s, "%-40s %20s\n", "master name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
-		   dev_name(m->dev), m->bound ? "bound" : "not bound");
+		   dev_name(m->parent), m->bound ? "bound" : "not bound");
 
 	seq_printf(s, "%-40s %20s\n", "device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
@@ -124,13 +124,13 @@ core_initcall(component_debug_init);
 
 static void component_master_debugfs_add(struct master *m)
 {
-	debugfs_create_file(dev_name(m->dev), 0444, component_debugfs_dir, m,
+	debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
 			    &component_devices_fops);
 }
 
 static void component_master_debugfs_del(struct master *m)
 {
-	debugfs_remove(debugfs_lookup(dev_name(m->dev), component_debugfs_dir));
+	debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
 }
 
 #else
@@ -143,13 +143,13 @@ static void component_master_debugfs_del(struct master *m)
 
 #endif
 
-static struct master *__master_find(struct device *dev,
+static struct master *__master_find(struct device *parent,
 	const struct component_master_ops *ops)
 {
 	struct master *m;
 
 	list_for_each_entry(m, &masters, node)
-		if (m->dev == dev && (!ops || m->ops == ops))
+		if (m->parent == parent && (!ops || m->ops == ops))
 			return m;
 
 	return NULL;
@@ -189,7 +189,7 @@ static int find_components(struct master *master)
 		struct component_match_array *mc = &match->compare[i];
 		struct component *c;
 
-		dev_dbg(master->dev, "Looking for component %zu\n", i);
+		dev_dbg(master->parent, "Looking for component %zu\n", i);
 
 		if (match->compare[i].component)
 			continue;
@@ -200,7 +200,7 @@ static int find_components(struct master *master)
 			break;
 		}
 
-		dev_dbg(master->dev, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
+		dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
 
 		/* Attach this component to the master */
 		match->compare[i].duplicate = !!c->master;
@@ -233,28 +233,28 @@ static int try_to_bring_up_master(struct master *master,
 {
 	int ret;
 
-	dev_dbg(master->dev, "trying to bring up master\n");
+	dev_dbg(master->parent, "trying to bring up master\n");
 
 	if (find_components(master)) {
-		dev_dbg(master->dev, "master has incomplete components\n");
+		dev_dbg(master->parent, "master has incomplete components\n");
 		return 0;
 	}
 
 	if (component && component->master != master) {
-		dev_dbg(master->dev, "master is not for this component (%s)\n",
+		dev_dbg(master->parent, "master is not for this component (%s)\n",
 			dev_name(component->dev));
 		return 0;
 	}
 
-	if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
+	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/* Found all components */
-	ret = master->ops->bind(master->dev);
+	ret = master->ops->bind(master->parent);
 	if (ret < 0) {
-		devres_release_group(master->dev, NULL);
+		devres_release_group(master->parent, NULL);
 		if (ret != -EPROBE_DEFER)
-			dev_info(master->dev, "master bind failed: %d\n", ret);
+			dev_info(master->parent, "master bind failed: %d\n", ret);
 		return ret;
 	}
 
@@ -281,32 +281,27 @@ static int try_to_bring_up_masters(struct component *component)
 static void take_down_master(struct master *master)
 {
 	if (master->bound) {
-		master->ops->unbind(master->dev);
-		devres_release_group(master->dev, NULL);
+		master->ops->unbind(master->parent);
+		devres_release_group(master->parent, NULL);
 		master->bound = false;
 	}
 }
 
-static void component_match_release(struct device *master,
-	struct component_match *match)
+static void devm_component_match_release(struct device *parent, void *res)
 {
+	struct component_match *match = res;
 	unsigned int i;
 
 	for (i = 0; i < match->num; i++) {
 		struct component_match_array *mc = &match->compare[i];
 
 		if (mc->release)
-			mc->release(master, mc->data);
+			mc->release(parent, mc->data);
 	}
 
 	kfree(match->compare);
 }
 
-static void devm_component_match_release(struct device *dev, void *res)
-{
-	component_match_release(dev, res);
-}
-
 static int component_match_realloc(struct component_match *match, size_t num)
 {
 	struct component_match_array *new;
@@ -450,7 +445,7 @@ static void free_master(struct master *master)
 
 /**
  * component_master_add_with_match - register an aggregate driver
- * @dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @ops: callbacks for the aggregate driver
  * @match: component match list for the aggregate driver
  *
@@ -460,7 +455,7 @@ static void free_master(struct master *master)
  * &component_master_ops.bind from @ops. Must be unregistered by calling
  * component_master_del().
  */
-int component_master_add_with_match(struct device *dev,
+int component_master_add_with_match(struct device *parent,
 	const struct component_master_ops *ops,
 	struct component_match *match)
 {
@@ -476,7 +471,7 @@ int component_master_add_with_match(struct device *dev,
 	if (!master)
 		return -ENOMEM;
 
-	master->dev = dev;
+	master->parent = parent;
 	master->ops = ops;
 	master->match = match;
 
@@ -498,20 +493,20 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
 /**
  * component_master_del - unregister an aggregate driver
- * @dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @ops: callbacks for the aggregate driver
  *
  * Unregisters an aggregate driver registered with
  * component_master_add_with_match(). If necessary the aggregate driver is first
  * disassembled by calling &component_master_ops.unbind from @ops.
  */
-void component_master_del(struct device *dev,
+void component_master_del(struct device *parent,
 	const struct component_master_ops *ops)
 {
 	struct master *master;
 
 	mutex_lock(&component_mutex);
-	master = __master_find(dev, ops);
+	master = __master_find(parent, ops);
 	if (master) {
 		take_down_master(master);
 		free_master(master);
@@ -526,7 +521,7 @@ static void component_unbind(struct component *component,
 	WARN_ON(!component->bound);
 
 	if (component->ops && component->ops->unbind)
-		component->ops->unbind(component->dev, master->dev, data);
+		component->ops->unbind(component->dev, master->parent, data);
 	component->bound = false;
 
 	/* Release all resources claimed in the binding of this component */
@@ -535,14 +530,14 @@ static void component_unbind(struct component *component,
 
 /**
  * component_unbind_all - unbind all components of an aggregate driver
- * @master_dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @data: opaque pointer, passed to all components
  *
- * Unbinds all components of the aggregate @dev by passing @data to their
+ * Unbinds all components of the aggregate device by passing @data to their
  * &component_ops.unbind functions. Should be called from
  * &component_master_ops.unbind.
  */
-void component_unbind_all(struct device *master_dev, void *data)
+void component_unbind_all(struct device *parent, void *data)
 {
 	struct master *master;
 	struct component *c;
@@ -550,7 +545,7 @@ void component_unbind_all(struct device *master_dev, void *data)
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(master_dev, NULL);
+	master = __master_find(parent, NULL);
 	if (!master)
 		return;
 
@@ -573,7 +568,7 @@ static int component_bind(struct component *component, struct master *master,
 	 * This allows us to roll-back a failed component without
 	 * affecting anything else.
 	 */
-	if (!devres_open_group(master->dev, NULL, GFP_KERNEL))
+	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/*
@@ -582,14 +577,14 @@ static int component_bind(struct component *component, struct master *master,
 	 * at the appropriate moment.
 	 */
 	if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
-		devres_release_group(master->dev, NULL);
+		devres_release_group(master->parent, NULL);
 		return -ENOMEM;
 	}
 
-	dev_dbg(master->dev, "binding %s (ops %ps)\n",
+	dev_dbg(master->parent, "binding %s (ops %ps)\n",
 		dev_name(component->dev), component->ops);
 
-	ret = component->ops->bind(component->dev, master->dev, data);
+	ret = component->ops->bind(component->dev, master->parent, data);
 	if (!ret) {
 		component->bound = true;
 
@@ -600,16 +595,16 @@ static int component_bind(struct component *component, struct master *master,
 		 * can clean those resources up independently.
 		 */
 		devres_close_group(component->dev, NULL);
-		devres_remove_group(master->dev, NULL);
+		devres_remove_group(master->parent, NULL);
 
-		dev_info(master->dev, "bound %s (ops %ps)\n",
+		dev_info(master->parent, "bound %s (ops %ps)\n",
 			 dev_name(component->dev), component->ops);
 	} else {
 		devres_release_group(component->dev, NULL);
-		devres_release_group(master->dev, NULL);
+		devres_release_group(master->parent, NULL);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(master->dev, "failed to bind %s (ops %ps): %d\n",
+			dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
 				dev_name(component->dev), component->ops, ret);
 	}
 
@@ -618,14 +613,14 @@ static int component_bind(struct component *component, struct master *master,
 
 /**
  * component_bind_all - bind all components of an aggregate driver
- * @master_dev: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @data: opaque pointer, passed to all components
  *
  * Binds all components of the aggregate @dev by passing @data to their
  * &component_ops.bind functions. Should be called from
  * &component_master_ops.bind.
  */
-int component_bind_all(struct device *master_dev, void *data)
+int component_bind_all(struct device *parent, void *data)
 {
 	struct master *master;
 	struct component *c;
@@ -634,7 +629,7 @@ int component_bind_all(struct device *master_dev, void *data)
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(master_dev, NULL);
+	master = __master_find(parent, NULL);
 	if (!master)
 		return -EINVAL;
 
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 3/7] component: Introduce struct aggregate_device
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

Replace 'struct master' with 'struct aggregate_device' and then rename
'master' to 'adev' everywhere in the code. While we're here, put a
struct device inside the aggregate device so that we can register it
with a bus_type in the next patch.

The diff is large but that's because this is mostly a rename, where
sometimes 'master' is replaced with 'adev' and other times it is
replaced with 'parent' to indicate that the struct device that was being
used is actually the parent of the aggregate device and driver.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c  | 249 ++++++++++++++++++++------------------
 include/linux/component.h |   2 +-
 2 files changed, 134 insertions(+), 117 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 5e79299f6c3f..55e30e0b0952 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -9,6 +9,7 @@
  */
 #include <linux/component.h>
 #include <linux/device.h>
+#include <linux/idr.h>
 #include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -58,18 +59,21 @@ struct component_match {
 	struct component_match_array *compare;
 };
 
-struct master {
+struct aggregate_device {
 	struct list_head node;
 	bool bound;
 
 	const struct component_master_ops *ops;
 	struct device *parent;
+	struct device dev;
 	struct component_match *match;
+
+	int id;
 };
 
 struct component {
 	struct list_head node;
-	struct master *master;
+	struct aggregate_device *adev;
 	bool bound;
 
 	const struct component_ops *ops;
@@ -79,7 +83,9 @@ struct component {
 
 static DEFINE_MUTEX(component_mutex);
 static LIST_HEAD(component_list);
-static LIST_HEAD(masters);
+static LIST_HEAD(aggregate_devices);
+
+static DEFINE_IDA(aggregate_ida);
 
 #ifdef CONFIG_DEBUG_FS
 
@@ -87,12 +93,12 @@ static struct dentry *component_debugfs_dir;
 
 static int component_devices_show(struct seq_file *s, void *data)
 {
-	struct master *m = s->private;
+	struct aggregate_device *m = s->private;
 	struct component_match *match = m->match;
 	size_t i;
 
 	mutex_lock(&component_mutex);
-	seq_printf(s, "%-40s %20s\n", "master name", "status");
+	seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
 		   dev_name(m->parent), m->bound ? "bound" : "not bound");
@@ -122,46 +128,46 @@ static int __init component_debug_init(void)
 
 core_initcall(component_debug_init);
 
-static void component_master_debugfs_add(struct master *m)
+static void component_master_debugfs_add(struct aggregate_device *m)
 {
 	debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
 			    &component_devices_fops);
 }
 
-static void component_master_debugfs_del(struct master *m)
+static void component_master_debugfs_del(struct aggregate_device *m)
 {
 	debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
 }
 
 #else
 
-static void component_master_debugfs_add(struct master *m)
+static void component_master_debugfs_add(struct aggregate_device *m)
 { }
 
-static void component_master_debugfs_del(struct master *m)
+static void component_master_debugfs_del(struct aggregate_device *m)
 { }
 
 #endif
 
-static struct master *__master_find(struct device *parent,
+static struct aggregate_device *__aggregate_find(struct device *parent,
 	const struct component_master_ops *ops)
 {
-	struct master *m;
+	struct aggregate_device *m;
 
-	list_for_each_entry(m, &masters, node)
+	list_for_each_entry(m, &aggregate_devices, node)
 		if (m->parent == parent && (!ops || m->ops == ops))
 			return m;
 
 	return NULL;
 }
 
-static struct component *find_component(struct master *master,
+static struct component *find_component(struct aggregate_device *adev,
 	struct component_match_array *mc)
 {
 	struct component *c;
 
 	list_for_each_entry(c, &component_list, node) {
-		if (c->master && c->master != master)
+		if (c->adev && c->adev != adev)
 			continue;
 
 		if (mc->compare && mc->compare(c->dev, mc->data))
@@ -175,101 +181,102 @@ static struct component *find_component(struct master *master,
 	return NULL;
 }
 
-static int find_components(struct master *master)
+static int find_components(struct aggregate_device *adev)
 {
-	struct component_match *match = master->match;
+	struct component_match *match = adev->match;
 	size_t i;
 	int ret = 0;
 
 	/*
 	 * Scan the array of match functions and attach
-	 * any components which are found to this master.
+	 * any components which are found to this adev.
 	 */
 	for (i = 0; i < match->num; i++) {
 		struct component_match_array *mc = &match->compare[i];
 		struct component *c;
 
-		dev_dbg(master->parent, "Looking for component %zu\n", i);
+		dev_dbg(adev->parent, "Looking for component %zu\n", i);
 
 		if (match->compare[i].component)
 			continue;
 
-		c = find_component(master, mc);
+		c = find_component(adev, mc);
 		if (!c) {
 			ret = -ENXIO;
 			break;
 		}
 
-		dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
+		dev_dbg(adev->parent, "found component %s, duplicate %u\n",
+			dev_name(c->dev), !!c->adev);
 
-		/* Attach this component to the master */
-		match->compare[i].duplicate = !!c->master;
+		/* Attach this component to the adev */
+		match->compare[i].duplicate = !!c->adev;
 		match->compare[i].component = c;
-		c->master = master;
+		c->adev = adev;
 	}
 	return ret;
 }
 
 /* Detach component from associated master */
-static void remove_component(struct master *master, struct component *c)
+static void remove_component(struct aggregate_device *adev, struct component *c)
 {
 	size_t i;
 
-	/* 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;
+	/* Detach the component from this adev. */
+	for (i = 0; i < adev->match->num; i++)
+		if (adev->match->compare[i].component == c)
+			adev->match->compare[i].component = NULL;
 }
 
 /*
- * Try to bring up a master.  If component is NULL, we're interested in
- * this master, otherwise it's a component which must be present to try
- * and bring up the master.
+ * Try to bring up an aggregate device.  If component is NULL, we're interested
+ * in this aggregate device, otherwise it's a component which must be present
+ * to try and bring up the aggregate device.
  *
  * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
  */
-static int try_to_bring_up_master(struct master *master,
+static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
 	struct component *component)
 {
 	int ret;
 
-	dev_dbg(master->parent, "trying to bring up master\n");
+	dev_dbg(adev->parent, "trying to bring up adev\n");
 
-	if (find_components(master)) {
-		dev_dbg(master->parent, "master has incomplete components\n");
+	if (find_components(adev)) {
+		dev_dbg(adev->parent, "master has incomplete components\n");
 		return 0;
 	}
 
-	if (component && component->master != master) {
-		dev_dbg(master->parent, "master is not for this component (%s)\n",
+	if (component && component->adev != adev) {
+		dev_dbg(adev->parent, "master is not for this component (%s)\n",
 			dev_name(component->dev));
 		return 0;
 	}
 
-	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
+	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/* Found all components */
-	ret = master->ops->bind(master->parent);
+	ret = adev->ops->bind(adev->parent);
 	if (ret < 0) {
-		devres_release_group(master->parent, NULL);
+		devres_release_group(adev->parent, NULL);
 		if (ret != -EPROBE_DEFER)
-			dev_info(master->parent, "master bind failed: %d\n", ret);
+			dev_info(adev->parent, "adev bind failed: %d\n", ret);
 		return ret;
 	}
 
-	master->bound = true;
+	adev->bound = true;
 	return 1;
 }
 
 static int try_to_bring_up_masters(struct component *component)
 {
-	struct master *m;
+	struct aggregate_device *adev;
 	int ret = 0;
 
-	list_for_each_entry(m, &masters, node) {
-		if (!m->bound) {
-			ret = try_to_bring_up_master(m, component);
+	list_for_each_entry(adev, &aggregate_devices, node) {
+		if (!adev->bound) {
+			ret = try_to_bring_up_aggregate_device(adev, component);
 			if (ret != 0)
 				break;
 		}
@@ -278,12 +285,12 @@ static int try_to_bring_up_masters(struct component *component)
 	return ret;
 }
 
-static void take_down_master(struct master *master)
+static void take_down_aggregate_device(struct aggregate_device *adev)
 {
-	if (master->bound) {
-		master->ops->unbind(master->parent);
-		devres_release_group(master->parent, NULL);
-		master->bound = false;
+	if (adev->bound) {
+		adev->ops->unbind(adev->parent);
+		devres_release_group(adev->parent, NULL);
+		adev->bound = false;
 	}
 }
 
@@ -324,7 +331,7 @@ static int component_match_realloc(struct component_match *match, size_t num)
 	return 0;
 }
 
-static void __component_match_add(struct device *master,
+static void __component_match_add(struct device *parent,
 	struct component_match **matchptr,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *),
@@ -344,7 +351,7 @@ static void __component_match_add(struct device *master,
 			return;
 		}
 
-		devres_add(master, match);
+		devres_add(parent, match);
 
 		*matchptr = match;
 	}
@@ -370,13 +377,13 @@ static void __component_match_add(struct device *master,
 
 /**
  * component_match_add_release - add a component match entry with release callback
- * @master: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @matchptr: pointer to the list of component matches
  * @release: release function for @compare_data
  * @compare: compare function to match against all components
  * @compare_data: opaque pointer passed to the @compare function
  *
- * Adds a new component match to the list stored in @matchptr, which the @master
+ * Adds a new component match to the list stored in @matchptr, which the
  * aggregate driver needs to function. The list of component matches pointed to
  * by @matchptr must be initialized to NULL before adding the first match. This
  * only matches against components added with component_add().
@@ -388,19 +395,19 @@ static void __component_match_add(struct device *master,
  *
  * See also component_match_add() and component_match_add_typed().
  */
-void component_match_add_release(struct device *master,
+void component_match_add_release(struct device *parent,
 	struct component_match **matchptr,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *), void *compare_data)
 {
-	__component_match_add(master, matchptr, release, compare, NULL,
+	__component_match_add(parent, matchptr, release, compare, NULL,
 			      compare_data);
 }
 EXPORT_SYMBOL(component_match_add_release);
 
 /**
  * component_match_add_typed - add a component match entry for a typed component
- * @master: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @matchptr: pointer to the list of component matches
  * @compare_typed: compare function to match against all typed components
  * @compare_data: opaque pointer passed to the @compare function
@@ -415,32 +422,33 @@ EXPORT_SYMBOL(component_match_add_release);
  *
  * See also component_match_add_release() and component_match_add_typed().
  */
-void component_match_add_typed(struct device *master,
+void component_match_add_typed(struct device *parent,
 	struct component_match **matchptr,
 	int (*compare_typed)(struct device *, int, void *), void *compare_data)
 {
-	__component_match_add(master, matchptr, NULL, NULL, compare_typed,
+	__component_match_add(parent, matchptr, NULL, NULL, compare_typed,
 			      compare_data);
 }
 EXPORT_SYMBOL(component_match_add_typed);
 
-static void free_master(struct master *master)
+static void free_aggregate_device(struct aggregate_device *adev)
 {
-	struct component_match *match = master->match;
+	struct component_match *match = adev->match;
 	int i;
 
-	component_master_debugfs_del(master);
-	list_del(&master->node);
+	component_master_debugfs_del(adev);
+	list_del(&adev->node);
 
 	if (match) {
 		for (i = 0; i < match->num; i++) {
 			struct component *c = match->compare[i].component;
 			if (c)
-				c->master = NULL;
+				c->adev = NULL;
 		}
 	}
 
-	kfree(master);
+	ida_free(&aggregate_ida, adev->id);
+	kfree(adev);
 }
 
 /**
@@ -459,31 +467,40 @@ int component_master_add_with_match(struct device *parent,
 	const struct component_master_ops *ops,
 	struct component_match *match)
 {
-	struct master *master;
-	int ret;
+	struct aggregate_device *adev;
+	int ret, id;
 
 	/* Reallocate the match array for its true size */
 	ret = component_match_realloc(match, match->num);
 	if (ret)
 		return ret;
 
-	master = kzalloc(sizeof(*master), GFP_KERNEL);
-	if (!master)
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
 		return -ENOMEM;
 
-	master->parent = parent;
-	master->ops = ops;
-	master->match = match;
+	id = ida_alloc(&aggregate_ida, GFP_KERNEL);
+	if (id < 0) {
+		kfree(adev);
+		return id;
+	}
+
+	adev->id = id;
+	adev->parent = parent;
+	adev->dev.parent = parent;
+	adev->ops = ops;
+	adev->match = match;
+	dev_set_name(&adev->dev, "aggregate%d", id);
 
-	component_master_debugfs_add(master);
-	/* Add to the list of available masters. */
+	component_master_debugfs_add(adev);
+	/* Add to the list of available aggregate devices. */
 	mutex_lock(&component_mutex);
-	list_add(&master->node, &masters);
+	list_add(&adev->node, &aggregate_devices);
 
-	ret = try_to_bring_up_master(master, NULL);
+	ret = try_to_bring_up_aggregate_device(adev, NULL);
 
 	if (ret < 0)
-		free_master(master);
+		free_aggregate_device(adev);
 
 	mutex_unlock(&component_mutex);
 
@@ -503,25 +520,25 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
 void component_master_del(struct device *parent,
 	const struct component_master_ops *ops)
 {
-	struct master *master;
+	struct aggregate_device *adev;
 
 	mutex_lock(&component_mutex);
-	master = __master_find(parent, ops);
-	if (master) {
-		take_down_master(master);
-		free_master(master);
+	adev = __aggregate_find(parent, ops);
+	if (adev) {
+		take_down_aggregate_device(adev);
+		free_aggregate_device(adev);
 	}
 	mutex_unlock(&component_mutex);
 }
 EXPORT_SYMBOL_GPL(component_master_del);
 
 static void component_unbind(struct component *component,
-	struct master *master, void *data)
+	struct aggregate_device *adev, void *data)
 {
 	WARN_ON(!component->bound);
 
 	if (component->ops && component->ops->unbind)
-		component->ops->unbind(component->dev, master->parent, data);
+		component->ops->unbind(component->dev, adev->parent, data);
 	component->bound = false;
 
 	/* Release all resources claimed in the binding of this component */
@@ -539,26 +556,26 @@ static void component_unbind(struct component *component,
  */
 void component_unbind_all(struct device *parent, void *data)
 {
-	struct master *master;
+	struct aggregate_device *adev;
 	struct component *c;
 	size_t i;
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(parent, NULL);
-	if (!master)
+	adev = __aggregate_find(parent, NULL);
+	if (!adev)
 		return;
 
 	/* 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);
+	for (i = adev->match->num; i--; )
+		if (!adev->match->compare[i].duplicate) {
+			c = adev->match->compare[i].component;
+			component_unbind(c, adev, data);
 		}
 }
 EXPORT_SYMBOL_GPL(component_unbind_all);
 
-static int component_bind(struct component *component, struct master *master,
+static int component_bind(struct component *component, struct aggregate_device *adev,
 	void *data)
 {
 	int ret;
@@ -568,7 +585,7 @@ static int component_bind(struct component *component, struct master *master,
 	 * This allows us to roll-back a failed component without
 	 * affecting anything else.
 	 */
-	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
+	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/*
@@ -577,14 +594,14 @@ static int component_bind(struct component *component, struct master *master,
 	 * at the appropriate moment.
 	 */
 	if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
-		devres_release_group(master->parent, NULL);
+		devres_release_group(adev->parent, NULL);
 		return -ENOMEM;
 	}
 
-	dev_dbg(master->parent, "binding %s (ops %ps)\n",
+	dev_dbg(adev->parent, "binding %s (ops %ps)\n",
 		dev_name(component->dev), component->ops);
 
-	ret = component->ops->bind(component->dev, master->parent, data);
+	ret = component->ops->bind(component->dev, adev->parent, data);
 	if (!ret) {
 		component->bound = true;
 
@@ -595,16 +612,16 @@ static int component_bind(struct component *component, struct master *master,
 		 * can clean those resources up independently.
 		 */
 		devres_close_group(component->dev, NULL);
-		devres_remove_group(master->parent, NULL);
+		devres_remove_group(adev->parent, NULL);
 
-		dev_info(master->parent, "bound %s (ops %ps)\n",
+		dev_info(adev->parent, "bound %s (ops %ps)\n",
 			 dev_name(component->dev), component->ops);
 	} else {
 		devres_release_group(component->dev, NULL);
-		devres_release_group(master->parent, NULL);
+		devres_release_group(adev->parent, NULL);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
+			dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
 				dev_name(component->dev), component->ops, ret);
 	}
 
@@ -622,31 +639,31 @@ static int component_bind(struct component *component, struct master *master,
  */
 int component_bind_all(struct device *parent, void *data)
 {
-	struct master *master;
+	struct aggregate_device *adev;
 	struct component *c;
 	size_t i;
 	int ret = 0;
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(parent, NULL);
-	if (!master)
+	adev = __aggregate_find(parent, NULL);
+	if (!adev)
 		return -EINVAL;
 
 	/* 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);
+	for (i = 0; i < adev->match->num; i++)
+		if (!adev->match->compare[i].duplicate) {
+			c = adev->match->compare[i].component;
+			ret = component_bind(c, adev, data);
 			if (ret)
 				break;
 		}
 
 	if (ret != 0) {
 		for (; i > 0; i--)
-			if (!master->match->compare[i - 1].duplicate) {
-				c = master->match->compare[i - 1].component;
-				component_unbind(c, master, data);
+			if (!adev->match->compare[i - 1].duplicate) {
+				c = adev->match->compare[i - 1].component;
+				component_unbind(c, adev, data);
 			}
 	}
 
@@ -675,8 +692,8 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
 
 	ret = try_to_bring_up_masters(component);
 	if (ret < 0) {
-		if (component->master)
-			remove_component(component->master, component);
+		if (component->adev)
+			remove_component(component->adev, component);
 		list_del(&component->node);
 
 		kfree(component);
@@ -757,9 +774,9 @@ void component_del(struct device *dev, const struct component_ops *ops)
 			break;
 		}
 
-	if (component && component->master) {
-		take_down_master(component->master);
-		remove_component(component->master, component);
+	if (component && component->adev) {
+		take_down_aggregate_device(component->adev);
+		remove_component(component->adev, component);
 	}
 
 	mutex_unlock(&component_mutex);
diff --git a/include/linux/component.h b/include/linux/component.h
index 16de18f473d7..71bfc3862633 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -41,7 +41,7 @@ void component_del(struct device *, const struct component_ops *);
 int component_bind_all(struct device *master, void *master_data);
 void component_unbind_all(struct device *master, void *master_data);
 
-struct master;
+struct aggregate_device;
 
 /**
  * struct component_master_ops - callback for the aggregate driver
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 3/7] component: Introduce struct aggregate_device
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

Replace 'struct master' with 'struct aggregate_device' and then rename
'master' to 'adev' everywhere in the code. While we're here, put a
struct device inside the aggregate device so that we can register it
with a bus_type in the next patch.

The diff is large but that's because this is mostly a rename, where
sometimes 'master' is replaced with 'adev' and other times it is
replaced with 'parent' to indicate that the struct device that was being
used is actually the parent of the aggregate device and driver.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c  | 249 ++++++++++++++++++++------------------
 include/linux/component.h |   2 +-
 2 files changed, 134 insertions(+), 117 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 5e79299f6c3f..55e30e0b0952 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -9,6 +9,7 @@
  */
 #include <linux/component.h>
 #include <linux/device.h>
+#include <linux/idr.h>
 #include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
@@ -58,18 +59,21 @@ struct component_match {
 	struct component_match_array *compare;
 };
 
-struct master {
+struct aggregate_device {
 	struct list_head node;
 	bool bound;
 
 	const struct component_master_ops *ops;
 	struct device *parent;
+	struct device dev;
 	struct component_match *match;
+
+	int id;
 };
 
 struct component {
 	struct list_head node;
-	struct master *master;
+	struct aggregate_device *adev;
 	bool bound;
 
 	const struct component_ops *ops;
@@ -79,7 +83,9 @@ struct component {
 
 static DEFINE_MUTEX(component_mutex);
 static LIST_HEAD(component_list);
-static LIST_HEAD(masters);
+static LIST_HEAD(aggregate_devices);
+
+static DEFINE_IDA(aggregate_ida);
 
 #ifdef CONFIG_DEBUG_FS
 
@@ -87,12 +93,12 @@ static struct dentry *component_debugfs_dir;
 
 static int component_devices_show(struct seq_file *s, void *data)
 {
-	struct master *m = s->private;
+	struct aggregate_device *m = s->private;
 	struct component_match *match = m->match;
 	size_t i;
 
 	mutex_lock(&component_mutex);
-	seq_printf(s, "%-40s %20s\n", "master name", "status");
+	seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
 		   dev_name(m->parent), m->bound ? "bound" : "not bound");
@@ -122,46 +128,46 @@ static int __init component_debug_init(void)
 
 core_initcall(component_debug_init);
 
-static void component_master_debugfs_add(struct master *m)
+static void component_master_debugfs_add(struct aggregate_device *m)
 {
 	debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
 			    &component_devices_fops);
 }
 
-static void component_master_debugfs_del(struct master *m)
+static void component_master_debugfs_del(struct aggregate_device *m)
 {
 	debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
 }
 
 #else
 
-static void component_master_debugfs_add(struct master *m)
+static void component_master_debugfs_add(struct aggregate_device *m)
 { }
 
-static void component_master_debugfs_del(struct master *m)
+static void component_master_debugfs_del(struct aggregate_device *m)
 { }
 
 #endif
 
-static struct master *__master_find(struct device *parent,
+static struct aggregate_device *__aggregate_find(struct device *parent,
 	const struct component_master_ops *ops)
 {
-	struct master *m;
+	struct aggregate_device *m;
 
-	list_for_each_entry(m, &masters, node)
+	list_for_each_entry(m, &aggregate_devices, node)
 		if (m->parent == parent && (!ops || m->ops == ops))
 			return m;
 
 	return NULL;
 }
 
-static struct component *find_component(struct master *master,
+static struct component *find_component(struct aggregate_device *adev,
 	struct component_match_array *mc)
 {
 	struct component *c;
 
 	list_for_each_entry(c, &component_list, node) {
-		if (c->master && c->master != master)
+		if (c->adev && c->adev != adev)
 			continue;
 
 		if (mc->compare && mc->compare(c->dev, mc->data))
@@ -175,101 +181,102 @@ static struct component *find_component(struct master *master,
 	return NULL;
 }
 
-static int find_components(struct master *master)
+static int find_components(struct aggregate_device *adev)
 {
-	struct component_match *match = master->match;
+	struct component_match *match = adev->match;
 	size_t i;
 	int ret = 0;
 
 	/*
 	 * Scan the array of match functions and attach
-	 * any components which are found to this master.
+	 * any components which are found to this adev.
 	 */
 	for (i = 0; i < match->num; i++) {
 		struct component_match_array *mc = &match->compare[i];
 		struct component *c;
 
-		dev_dbg(master->parent, "Looking for component %zu\n", i);
+		dev_dbg(adev->parent, "Looking for component %zu\n", i);
 
 		if (match->compare[i].component)
 			continue;
 
-		c = find_component(master, mc);
+		c = find_component(adev, mc);
 		if (!c) {
 			ret = -ENXIO;
 			break;
 		}
 
-		dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
+		dev_dbg(adev->parent, "found component %s, duplicate %u\n",
+			dev_name(c->dev), !!c->adev);
 
-		/* Attach this component to the master */
-		match->compare[i].duplicate = !!c->master;
+		/* Attach this component to the adev */
+		match->compare[i].duplicate = !!c->adev;
 		match->compare[i].component = c;
-		c->master = master;
+		c->adev = adev;
 	}
 	return ret;
 }
 
 /* Detach component from associated master */
-static void remove_component(struct master *master, struct component *c)
+static void remove_component(struct aggregate_device *adev, struct component *c)
 {
 	size_t i;
 
-	/* 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;
+	/* Detach the component from this adev. */
+	for (i = 0; i < adev->match->num; i++)
+		if (adev->match->compare[i].component == c)
+			adev->match->compare[i].component = NULL;
 }
 
 /*
- * Try to bring up a master.  If component is NULL, we're interested in
- * this master, otherwise it's a component which must be present to try
- * and bring up the master.
+ * Try to bring up an aggregate device.  If component is NULL, we're interested
+ * in this aggregate device, otherwise it's a component which must be present
+ * to try and bring up the aggregate device.
  *
  * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
  */
-static int try_to_bring_up_master(struct master *master,
+static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
 	struct component *component)
 {
 	int ret;
 
-	dev_dbg(master->parent, "trying to bring up master\n");
+	dev_dbg(adev->parent, "trying to bring up adev\n");
 
-	if (find_components(master)) {
-		dev_dbg(master->parent, "master has incomplete components\n");
+	if (find_components(adev)) {
+		dev_dbg(adev->parent, "master has incomplete components\n");
 		return 0;
 	}
 
-	if (component && component->master != master) {
-		dev_dbg(master->parent, "master is not for this component (%s)\n",
+	if (component && component->adev != adev) {
+		dev_dbg(adev->parent, "master is not for this component (%s)\n",
 			dev_name(component->dev));
 		return 0;
 	}
 
-	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
+	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/* Found all components */
-	ret = master->ops->bind(master->parent);
+	ret = adev->ops->bind(adev->parent);
 	if (ret < 0) {
-		devres_release_group(master->parent, NULL);
+		devres_release_group(adev->parent, NULL);
 		if (ret != -EPROBE_DEFER)
-			dev_info(master->parent, "master bind failed: %d\n", ret);
+			dev_info(adev->parent, "adev bind failed: %d\n", ret);
 		return ret;
 	}
 
-	master->bound = true;
+	adev->bound = true;
 	return 1;
 }
 
 static int try_to_bring_up_masters(struct component *component)
 {
-	struct master *m;
+	struct aggregate_device *adev;
 	int ret = 0;
 
-	list_for_each_entry(m, &masters, node) {
-		if (!m->bound) {
-			ret = try_to_bring_up_master(m, component);
+	list_for_each_entry(adev, &aggregate_devices, node) {
+		if (!adev->bound) {
+			ret = try_to_bring_up_aggregate_device(adev, component);
 			if (ret != 0)
 				break;
 		}
@@ -278,12 +285,12 @@ static int try_to_bring_up_masters(struct component *component)
 	return ret;
 }
 
-static void take_down_master(struct master *master)
+static void take_down_aggregate_device(struct aggregate_device *adev)
 {
-	if (master->bound) {
-		master->ops->unbind(master->parent);
-		devres_release_group(master->parent, NULL);
-		master->bound = false;
+	if (adev->bound) {
+		adev->ops->unbind(adev->parent);
+		devres_release_group(adev->parent, NULL);
+		adev->bound = false;
 	}
 }
 
@@ -324,7 +331,7 @@ static int component_match_realloc(struct component_match *match, size_t num)
 	return 0;
 }
 
-static void __component_match_add(struct device *master,
+static void __component_match_add(struct device *parent,
 	struct component_match **matchptr,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *),
@@ -344,7 +351,7 @@ static void __component_match_add(struct device *master,
 			return;
 		}
 
-		devres_add(master, match);
+		devres_add(parent, match);
 
 		*matchptr = match;
 	}
@@ -370,13 +377,13 @@ static void __component_match_add(struct device *master,
 
 /**
  * component_match_add_release - add a component match entry with release callback
- * @master: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @matchptr: pointer to the list of component matches
  * @release: release function for @compare_data
  * @compare: compare function to match against all components
  * @compare_data: opaque pointer passed to the @compare function
  *
- * Adds a new component match to the list stored in @matchptr, which the @master
+ * Adds a new component match to the list stored in @matchptr, which the
  * aggregate driver needs to function. The list of component matches pointed to
  * by @matchptr must be initialized to NULL before adding the first match. This
  * only matches against components added with component_add().
@@ -388,19 +395,19 @@ static void __component_match_add(struct device *master,
  *
  * See also component_match_add() and component_match_add_typed().
  */
-void component_match_add_release(struct device *master,
+void component_match_add_release(struct device *parent,
 	struct component_match **matchptr,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *), void *compare_data)
 {
-	__component_match_add(master, matchptr, release, compare, NULL,
+	__component_match_add(parent, matchptr, release, compare, NULL,
 			      compare_data);
 }
 EXPORT_SYMBOL(component_match_add_release);
 
 /**
  * component_match_add_typed - add a component match entry for a typed component
- * @master: device with the aggregate driver
+ * @parent: parent device of the aggregate driver
  * @matchptr: pointer to the list of component matches
  * @compare_typed: compare function to match against all typed components
  * @compare_data: opaque pointer passed to the @compare function
@@ -415,32 +422,33 @@ EXPORT_SYMBOL(component_match_add_release);
  *
  * See also component_match_add_release() and component_match_add_typed().
  */
-void component_match_add_typed(struct device *master,
+void component_match_add_typed(struct device *parent,
 	struct component_match **matchptr,
 	int (*compare_typed)(struct device *, int, void *), void *compare_data)
 {
-	__component_match_add(master, matchptr, NULL, NULL, compare_typed,
+	__component_match_add(parent, matchptr, NULL, NULL, compare_typed,
 			      compare_data);
 }
 EXPORT_SYMBOL(component_match_add_typed);
 
-static void free_master(struct master *master)
+static void free_aggregate_device(struct aggregate_device *adev)
 {
-	struct component_match *match = master->match;
+	struct component_match *match = adev->match;
 	int i;
 
-	component_master_debugfs_del(master);
-	list_del(&master->node);
+	component_master_debugfs_del(adev);
+	list_del(&adev->node);
 
 	if (match) {
 		for (i = 0; i < match->num; i++) {
 			struct component *c = match->compare[i].component;
 			if (c)
-				c->master = NULL;
+				c->adev = NULL;
 		}
 	}
 
-	kfree(master);
+	ida_free(&aggregate_ida, adev->id);
+	kfree(adev);
 }
 
 /**
@@ -459,31 +467,40 @@ int component_master_add_with_match(struct device *parent,
 	const struct component_master_ops *ops,
 	struct component_match *match)
 {
-	struct master *master;
-	int ret;
+	struct aggregate_device *adev;
+	int ret, id;
 
 	/* Reallocate the match array for its true size */
 	ret = component_match_realloc(match, match->num);
 	if (ret)
 		return ret;
 
-	master = kzalloc(sizeof(*master), GFP_KERNEL);
-	if (!master)
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
 		return -ENOMEM;
 
-	master->parent = parent;
-	master->ops = ops;
-	master->match = match;
+	id = ida_alloc(&aggregate_ida, GFP_KERNEL);
+	if (id < 0) {
+		kfree(adev);
+		return id;
+	}
+
+	adev->id = id;
+	adev->parent = parent;
+	adev->dev.parent = parent;
+	adev->ops = ops;
+	adev->match = match;
+	dev_set_name(&adev->dev, "aggregate%d", id);
 
-	component_master_debugfs_add(master);
-	/* Add to the list of available masters. */
+	component_master_debugfs_add(adev);
+	/* Add to the list of available aggregate devices. */
 	mutex_lock(&component_mutex);
-	list_add(&master->node, &masters);
+	list_add(&adev->node, &aggregate_devices);
 
-	ret = try_to_bring_up_master(master, NULL);
+	ret = try_to_bring_up_aggregate_device(adev, NULL);
 
 	if (ret < 0)
-		free_master(master);
+		free_aggregate_device(adev);
 
 	mutex_unlock(&component_mutex);
 
@@ -503,25 +520,25 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
 void component_master_del(struct device *parent,
 	const struct component_master_ops *ops)
 {
-	struct master *master;
+	struct aggregate_device *adev;
 
 	mutex_lock(&component_mutex);
-	master = __master_find(parent, ops);
-	if (master) {
-		take_down_master(master);
-		free_master(master);
+	adev = __aggregate_find(parent, ops);
+	if (adev) {
+		take_down_aggregate_device(adev);
+		free_aggregate_device(adev);
 	}
 	mutex_unlock(&component_mutex);
 }
 EXPORT_SYMBOL_GPL(component_master_del);
 
 static void component_unbind(struct component *component,
-	struct master *master, void *data)
+	struct aggregate_device *adev, void *data)
 {
 	WARN_ON(!component->bound);
 
 	if (component->ops && component->ops->unbind)
-		component->ops->unbind(component->dev, master->parent, data);
+		component->ops->unbind(component->dev, adev->parent, data);
 	component->bound = false;
 
 	/* Release all resources claimed in the binding of this component */
@@ -539,26 +556,26 @@ static void component_unbind(struct component *component,
  */
 void component_unbind_all(struct device *parent, void *data)
 {
-	struct master *master;
+	struct aggregate_device *adev;
 	struct component *c;
 	size_t i;
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(parent, NULL);
-	if (!master)
+	adev = __aggregate_find(parent, NULL);
+	if (!adev)
 		return;
 
 	/* 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);
+	for (i = adev->match->num; i--; )
+		if (!adev->match->compare[i].duplicate) {
+			c = adev->match->compare[i].component;
+			component_unbind(c, adev, data);
 		}
 }
 EXPORT_SYMBOL_GPL(component_unbind_all);
 
-static int component_bind(struct component *component, struct master *master,
+static int component_bind(struct component *component, struct aggregate_device *adev,
 	void *data)
 {
 	int ret;
@@ -568,7 +585,7 @@ static int component_bind(struct component *component, struct master *master,
 	 * This allows us to roll-back a failed component without
 	 * affecting anything else.
 	 */
-	if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
+	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/*
@@ -577,14 +594,14 @@ static int component_bind(struct component *component, struct master *master,
 	 * at the appropriate moment.
 	 */
 	if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
-		devres_release_group(master->parent, NULL);
+		devres_release_group(adev->parent, NULL);
 		return -ENOMEM;
 	}
 
-	dev_dbg(master->parent, "binding %s (ops %ps)\n",
+	dev_dbg(adev->parent, "binding %s (ops %ps)\n",
 		dev_name(component->dev), component->ops);
 
-	ret = component->ops->bind(component->dev, master->parent, data);
+	ret = component->ops->bind(component->dev, adev->parent, data);
 	if (!ret) {
 		component->bound = true;
 
@@ -595,16 +612,16 @@ static int component_bind(struct component *component, struct master *master,
 		 * can clean those resources up independently.
 		 */
 		devres_close_group(component->dev, NULL);
-		devres_remove_group(master->parent, NULL);
+		devres_remove_group(adev->parent, NULL);
 
-		dev_info(master->parent, "bound %s (ops %ps)\n",
+		dev_info(adev->parent, "bound %s (ops %ps)\n",
 			 dev_name(component->dev), component->ops);
 	} else {
 		devres_release_group(component->dev, NULL);
-		devres_release_group(master->parent, NULL);
+		devres_release_group(adev->parent, NULL);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
+			dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
 				dev_name(component->dev), component->ops, ret);
 	}
 
@@ -622,31 +639,31 @@ static int component_bind(struct component *component, struct master *master,
  */
 int component_bind_all(struct device *parent, void *data)
 {
-	struct master *master;
+	struct aggregate_device *adev;
 	struct component *c;
 	size_t i;
 	int ret = 0;
 
 	WARN_ON(!mutex_is_locked(&component_mutex));
 
-	master = __master_find(parent, NULL);
-	if (!master)
+	adev = __aggregate_find(parent, NULL);
+	if (!adev)
 		return -EINVAL;
 
 	/* 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);
+	for (i = 0; i < adev->match->num; i++)
+		if (!adev->match->compare[i].duplicate) {
+			c = adev->match->compare[i].component;
+			ret = component_bind(c, adev, data);
 			if (ret)
 				break;
 		}
 
 	if (ret != 0) {
 		for (; i > 0; i--)
-			if (!master->match->compare[i - 1].duplicate) {
-				c = master->match->compare[i - 1].component;
-				component_unbind(c, master, data);
+			if (!adev->match->compare[i - 1].duplicate) {
+				c = adev->match->compare[i - 1].component;
+				component_unbind(c, adev, data);
 			}
 	}
 
@@ -675,8 +692,8 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
 
 	ret = try_to_bring_up_masters(component);
 	if (ret < 0) {
-		if (component->master)
-			remove_component(component->master, component);
+		if (component->adev)
+			remove_component(component->adev, component);
 		list_del(&component->node);
 
 		kfree(component);
@@ -757,9 +774,9 @@ void component_del(struct device *dev, const struct component_ops *ops)
 			break;
 		}
 
-	if (component && component->master) {
-		take_down_master(component->master);
-		remove_component(component->master, component);
+	if (component && component->adev) {
+		take_down_aggregate_device(component->adev);
+		remove_component(component->adev, component);
 	}
 
 	mutex_unlock(&component_mutex);
diff --git a/include/linux/component.h b/include/linux/component.h
index 16de18f473d7..71bfc3862633 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -41,7 +41,7 @@ void component_del(struct device *, const struct component_ops *);
 int component_bind_all(struct device *master, void *master_data);
 void component_unbind_all(struct device *master, void *master_data);
 
-struct master;
+struct aggregate_device;
 
 /**
  * struct component_master_ops - callback for the aggregate driver
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 4/7] component: Introduce the aggregate bus_type
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

The component driver only provides 'bind' and 'unbind' callbacks to tell
the host driver that it is time to assemble the aggregate driver now
that all the components have probed. The component driver model doesn't
attempt to resolve runtime PM or suspend/resume ordering, and explicitly
mentions this in the code. This lack of support leads to some pretty
gnarly usages of the 'prepare' and 'complete' power management hooks in
drivers that host the aggregate device, and it fully breaks down when
faced with ordering shutdown between the various components, the
aggregate driver, and the host driver that registers the whole thing.

In a concrete example, the MSM display driver at drivers/gpu/drm/msm is
using 'prepare' and 'complete' to call the drm helpers
drm_mode_config_helper_suspend() and drm_mode_config_helper_resume()
respectively, so that it can move the aggregate driver suspend/resume
callbacks to be before and after the components that make up the drm
device call any suspend/resume hooks they have. This only works as long
as the component devices don't do anything in their own 'prepare' and
'complete' callbacks. If they did, then the ordering would be incorrect
and we would be doing something in the component drivers before the
aggregate driver could do anything. Yuck!

Similarly, when trying to add shutdown support to the MSM driver we run
across a problem where we're trying to shutdown the drm device via
drm_atomic_helper_shutdown(), but some of the devices in the encoder
chain have already been shutdown. This time, the component devices
aren't the problem (although they could be if they did anything in their
shutdown callbacks), but there's a DSI to eDP bridge in the encoder
chain that has already been shutdown before the driver hosting the
aggregate device runs shutdown. The ordering of driver probe is like
this:

 1. msm_pdev_probe() (host driver)
 2. DSI bridge
 3. aggregate bind

When it comes to shutdown we have this order:

 1. DSI bridge
 2. msm_pdev_shutdown() (host driver)

and so the bridge is already off, but we want to communicate to it to
turn things off on the display during msm_pdev_shutdown(). Double yuck!
Unfortunately, this time we can't split shutdown into multiple phases
and swap msm_pdev_shutdown() with the DSI bridge.

Let's make the component driver into an actual device driver that has
probe/remove/shutdown functions. The driver will only be bound to the
aggregate device once all component drivers have called component_add()
to indicate they're ready to assemble the aggregate driver. This allows
us to attach shutdown logic (and in the future runtime PM logic) to the
aggregate driver so that it runs the hooks in the correct order.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c  | 429 +++++++++++++++++++++++++++-----------
 include/linux/component.h |  60 +++++-
 2 files changed, 359 insertions(+), 130 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 55e30e0b0952..a6c0bb7ccdbc 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -15,6 +15,9 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/debugfs.h>
+#include <linux/pm_runtime.h>
+
+#include "base.h"
 
 /**
  * DOC: overview
@@ -60,9 +63,6 @@ struct component_match {
 };
 
 struct aggregate_device {
-	struct list_head node;
-	bool bound;
-
 	const struct component_master_ops *ops;
 	struct device *parent;
 	struct device dev;
@@ -71,6 +71,11 @@ struct aggregate_device {
 	int id;
 };
 
+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+	return container_of(d, struct aggregate_device, dev);
+}
+
 struct component {
 	struct list_head node;
 	struct aggregate_device *adev;
@@ -83,7 +88,6 @@ struct component {
 
 static DEFINE_MUTEX(component_mutex);
 static LIST_HEAD(component_list);
-static LIST_HEAD(aggregate_devices);
 
 static DEFINE_IDA(aggregate_ida);
 
@@ -101,7 +105,7 @@ static int component_devices_show(struct seq_file *s, void *data)
 	seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
-		   dev_name(m->parent), m->bound ? "bound" : "not bound");
+		   dev_name(m->parent), m->dev.driver ? "bound" : "not bound");
 
 	seq_printf(s, "%-40s %20s\n", "device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
@@ -149,16 +153,21 @@ static void component_master_debugfs_del(struct aggregate_device *m)
 
 #endif
 
-static struct aggregate_device *__aggregate_find(struct device *parent,
-	const struct component_master_ops *ops)
+struct aggregate_bus_find_data {
+	const struct component_master_ops *ops;
+	struct device *parent;
+};
+
+static int aggregate_bus_find_match(struct device *dev, const void *_data)
 {
-	struct aggregate_device *m;
+	struct aggregate_device *adev = to_aggregate_device(dev);
+	const struct aggregate_bus_find_data *data = _data;
 
-	list_for_each_entry(m, &aggregate_devices, node)
-		if (m->parent == parent && (!ops || m->ops == ops))
-			return m;
+	if (dev->parent == data->parent &&
+	    (!data->ops || adev->ops == data->ops))
+		return 1;
 
-	return NULL;
+	return 0;
 }
 
 static struct component *find_component(struct aggregate_device *adev,
@@ -185,7 +194,6 @@ static int find_components(struct aggregate_device *adev)
 {
 	struct component_match *match = adev->match;
 	size_t i;
-	int ret = 0;
 
 	/*
 	 * Scan the array of match functions and attach
@@ -201,10 +209,8 @@ static int find_components(struct aggregate_device *adev)
 			continue;
 
 		c = find_component(adev, mc);
-		if (!c) {
-			ret = -ENXIO;
-			break;
-		}
+		if (!c)
+			return 0;
 
 		dev_dbg(adev->parent, "found component %s, duplicate %u\n",
 			dev_name(c->dev), !!c->adev);
@@ -212,9 +218,12 @@ static int find_components(struct aggregate_device *adev)
 		/* Attach this component to the adev */
 		match->compare[i].duplicate = !!c->adev;
 		match->compare[i].component = c;
+		/* Matches put in component_del() */
+		get_device(&adev->dev);
 		c->adev = adev;
 	}
-	return ret;
+
+	return 1;
 }
 
 /* Detach component from associated master */
@@ -228,72 +237,6 @@ static void remove_component(struct aggregate_device *adev, struct component *c)
 			adev->match->compare[i].component = NULL;
 }
 
-/*
- * Try to bring up an aggregate device.  If component is NULL, we're interested
- * in this aggregate device, otherwise it's a component which must be present
- * to try and bring up the aggregate device.
- *
- * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
- */
-static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
-	struct component *component)
-{
-	int ret;
-
-	dev_dbg(adev->parent, "trying to bring up adev\n");
-
-	if (find_components(adev)) {
-		dev_dbg(adev->parent, "master has incomplete components\n");
-		return 0;
-	}
-
-	if (component && component->adev != adev) {
-		dev_dbg(adev->parent, "master is not for this component (%s)\n",
-			dev_name(component->dev));
-		return 0;
-	}
-
-	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
-		return -ENOMEM;
-
-	/* Found all components */
-	ret = adev->ops->bind(adev->parent);
-	if (ret < 0) {
-		devres_release_group(adev->parent, NULL);
-		if (ret != -EPROBE_DEFER)
-			dev_info(adev->parent, "adev bind failed: %d\n", ret);
-		return ret;
-	}
-
-	adev->bound = true;
-	return 1;
-}
-
-static int try_to_bring_up_masters(struct component *component)
-{
-	struct aggregate_device *adev;
-	int ret = 0;
-
-	list_for_each_entry(adev, &aggregate_devices, node) {
-		if (!adev->bound) {
-			ret = try_to_bring_up_aggregate_device(adev, component);
-			if (ret != 0)
-				break;
-		}
-	}
-
-	return ret;
-}
-
-static void take_down_aggregate_device(struct aggregate_device *adev)
-{
-	if (adev->bound) {
-		adev->ops->unbind(adev->parent);
-		devres_release_group(adev->parent, NULL);
-		adev->bound = false;
-	}
-}
-
 static void devm_component_match_release(struct device *parent, void *res)
 {
 	struct component_match *match = res;
@@ -437,7 +380,6 @@ static void free_aggregate_device(struct aggregate_device *adev)
 	int i;
 
 	component_master_debugfs_del(adev);
-	list_del(&adev->node);
 
 	if (match) {
 		for (i = 0; i < match->num; i++) {
@@ -451,19 +393,140 @@ static void free_aggregate_device(struct aggregate_device *adev)
 	kfree(adev);
 }
 
-/**
- * component_master_add_with_match - register an aggregate driver
- * @parent: parent device of the aggregate driver
- * @ops: callbacks for the aggregate driver
- * @match: component match list for the aggregate driver
- *
- * Registers a new aggregate driver consisting of the components added to @match
- * by calling one of the component_match_add() functions. Once all components in
- * @match are available, it will be assembled by calling
- * &component_master_ops.bind from @ops. Must be unregistered by calling
- * component_master_del().
- */
-int component_master_add_with_match(struct device *parent,
+static void aggregate_device_release(struct device *dev)
+{
+	struct aggregate_device *adev = to_aggregate_device(dev);
+
+	free_aggregate_device(adev);
+}
+
+static int aggregate_device_match(struct device *dev, struct device_driver *drv)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(drv);
+	struct aggregate_device *adev = to_aggregate_device(dev);
+	int ret;
+
+	if (adrv->match != adev->match)
+		return 0;
+
+	/* Should we start to assemble? */
+	mutex_lock(&component_mutex);
+	ret = find_components(adev);
+	mutex_unlock(&component_mutex);
+
+	return ret;
+}
+
+/* TODO: Remove once all aggregate drivers use component_aggregate_register() */
+static int component_probe_bind(struct aggregate_device *adev)
+{
+	return adev->ops->bind(adev->parent);
+}
+
+static void component_remove_unbind(struct aggregate_device *adev)
+{
+	adev->ops->unbind(adev->parent);
+}
+
+static int aggregate_driver_probe(struct device *dev)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+	struct aggregate_device *adev = to_aggregate_device(dev);
+	bool modern = adrv->probe != component_probe_bind;
+	int ret;
+
+	/* Only do runtime PM when drivers migrate */
+	if (modern) {
+		pm_runtime_get_noresume(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	mutex_lock(&component_mutex);
+	if (devres_open_group(adev->parent, NULL, GFP_KERNEL)) {
+		ret = adrv->probe(adev);
+		if (ret)
+			devres_release_group(dev->parent, NULL);
+	} else {
+		ret = -ENOMEM;
+	}
+	mutex_unlock(&component_mutex);
+
+	if (ret && modern) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_put_noidle(dev);
+	}
+
+	return ret;
+}
+
+static int aggregate_driver_remove(struct device *dev)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+	bool modern = adrv->remove != component_remove_unbind;
+
+	/* Only do runtime PM when drivers migrate */
+	if (modern)
+		pm_runtime_get_sync(dev);
+	adrv->remove(to_aggregate_device(dev));
+	devres_release_group(dev->parent, NULL);
+	if (!modern)
+		return 0;
+
+	pm_runtime_put_noidle(dev);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
+
+	return 0;
+}
+
+static void aggregate_driver_shutdown(struct device *dev)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+
+	if (adrv && adrv->shutdown)
+		adrv->shutdown(to_aggregate_device(dev));
+}
+
+static struct bus_type aggregate_bus_type = {
+	.name		= "aggregate",
+	.match		= aggregate_device_match,
+	.probe		= aggregate_driver_probe,
+	.remove		= aggregate_driver_remove,
+	.shutdown	= aggregate_driver_shutdown,
+};
+
+/* Callers take ownership of return value, should call put_device() */
+static struct aggregate_device *__aggregate_find(struct device *parent,
+	const struct component_master_ops *ops)
+{
+	struct device *dev;
+	struct aggregate_bus_find_data data = {
+		.ops = ops,
+		.parent = parent,
+	};
+
+	dev = bus_find_device(&aggregate_bus_type, NULL, &data,
+			      aggregate_bus_find_match);
+
+	return dev ? to_aggregate_device(dev) : NULL;
+}
+
+static int aggregate_driver_register(struct aggregate_driver *adrv)
+{
+	adrv->driver.bus = &aggregate_bus_type;
+	return driver_register(&adrv->driver);
+}
+
+static void aggregate_driver_unregister(struct aggregate_driver *adrv)
+{
+	driver_unregister(&adrv->driver);
+}
+
+static struct aggregate_device *aggregate_device_add(struct device *parent,
 	const struct component_master_ops *ops,
 	struct component_match *match)
 {
@@ -473,41 +536,113 @@ int component_master_add_with_match(struct device *parent,
 	/* Reallocate the match array for its true size */
 	ret = component_match_realloc(match, match->num);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
 	if (!adev)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	id = ida_alloc(&aggregate_ida, GFP_KERNEL);
 	if (id < 0) {
 		kfree(adev);
-		return id;
+		return ERR_PTR(id);
 	}
 
 	adev->id = id;
 	adev->parent = parent;
+
 	adev->dev.parent = parent;
+	adev->dev.bus = &aggregate_bus_type;
+	adev->dev.release = aggregate_device_release;
 	adev->ops = ops;
 	adev->match = match;
 	dev_set_name(&adev->dev, "aggregate%d", id);
 
+	ret = device_register(&adev->dev);
+	if (ret) {
+		put_device(&adev->dev);
+		return ERR_PTR(ret);
+	}
+
 	component_master_debugfs_add(adev);
-	/* Add to the list of available aggregate devices. */
-	mutex_lock(&component_mutex);
-	list_add(&adev->node, &aggregate_devices);
 
-	ret = try_to_bring_up_aggregate_device(adev, NULL);
+	return adev;
+}
+
+/**
+ * component_master_add_with_match - register an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @ops: callbacks for the aggregate driver
+ * @match: component match list for the aggregate driver
+ *
+ * Registers a new aggregate driver consisting of the components added to @match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available, it will be assembled by calling
+ * &component_master_ops.bind from @ops. Must be unregistered by calling
+ * component_master_del().
+ */
+int component_master_add_with_match(struct device *parent,
+	const struct component_master_ops *ops,
+	struct component_match *match)
+{
+	struct aggregate_driver *adrv;
+	struct aggregate_device *adev;
+	int ret = 0;
+
+	adev = aggregate_device_add(parent, ops, match);
+	if (IS_ERR(adev))
+		return PTR_ERR(adev);
 
-	if (ret < 0)
-		free_aggregate_device(adev);
+	adrv = kzalloc(sizeof(*adrv), GFP_KERNEL);
+	if (!adrv) {
+		ret = -ENOMEM;
+		goto err;
+	}
 
-	mutex_unlock(&component_mutex);
+	adrv->probe = component_probe_bind;
+	adrv->remove = component_remove_unbind;
+	adrv->driver.owner = THIS_MODULE;
+	adrv->driver.name = dev_name(&adev->dev);
+	adrv->match = match;
 
-	return ret < 0 ? ret : 0;
+	ret = aggregate_driver_register(adrv);
+	if (!ret)
+		return 0;
+err:
+	put_device(&adev->dev);
+	kfree(adrv);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
+/**
+ * component_aggregate_register - register an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @adrv: aggregate driver to register
+ *
+ * Registers a new aggregate driver consisting of the components added to @adrv.match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available, the aggregate driver will be assembled by calling
+ * &adrv.bind. Must be unregistered by calling component_aggregate_unregister().
+ */
+int component_aggregate_register(struct device *parent,
+	struct aggregate_driver *adrv)
+{
+	struct aggregate_device *adev;
+	int ret;
+
+	adev = aggregate_device_add(parent, NULL, adrv->match);
+	if (IS_ERR(adev))
+		return PTR_ERR(adev);
+
+	ret = aggregate_driver_register(adrv);
+	if (ret)
+		put_device(&adev->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(component_aggregate_register);
+
 /**
  * component_master_del - unregister an aggregate driver
  * @parent: parent device of the aggregate driver
@@ -521,17 +656,53 @@ void component_master_del(struct device *parent,
 	const struct component_master_ops *ops)
 {
 	struct aggregate_device *adev;
+	struct aggregate_driver *adrv;
+	struct device_driver *drv;
 
 	mutex_lock(&component_mutex);
 	adev = __aggregate_find(parent, ops);
+	mutex_unlock(&component_mutex);
+
 	if (adev) {
-		take_down_aggregate_device(adev);
-		free_aggregate_device(adev);
+		drv = adev->dev.driver;
+		if (drv) {
+			adrv = to_aggregate_driver(drv);
+			aggregate_driver_unregister(adrv);
+			kfree(adrv);
+		}
+
+		device_unregister(&adev->dev);
 	}
-	mutex_unlock(&component_mutex);
+	put_device(&adev->dev);
 }
 EXPORT_SYMBOL_GPL(component_master_del);
 
+/**
+ * component_aggregate_unregister - unregister an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @adrv: registered aggregate driver
+ *
+ * Unregisters an aggregate driver registered with
+ * component_aggregate_register(). If necessary the aggregate driver is first
+ * disassembled.
+ */
+void component_aggregate_unregister(struct device *parent,
+	struct aggregate_driver *adrv)
+{
+	struct aggregate_device *adev;
+
+	mutex_lock(&component_mutex);
+	adev = __aggregate_find(parent, NULL);
+	mutex_unlock(&component_mutex);
+
+	if (adev)
+		device_unregister(&adev->dev);
+	put_device(&adev->dev);
+
+	aggregate_driver_unregister(adrv);
+}
+EXPORT_SYMBOL_GPL(component_aggregate_unregister);
+
 static void component_unbind(struct component *component,
 	struct aggregate_device *adev, void *data)
 {
@@ -572,6 +743,8 @@ void component_unbind_all(struct device *parent, void *data)
 			c = adev->match->compare[i].component;
 			component_unbind(c, adev, data);
 		}
+
+	put_device(&adev->dev);
 }
 EXPORT_SYMBOL_GPL(component_unbind_all);
 
@@ -666,6 +839,7 @@ int component_bind_all(struct device *parent, void *data)
 				component_unbind(c, adev, data);
 			}
 	}
+	put_device(&adev->dev);
 
 	return ret;
 }
@@ -675,7 +849,6 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
 	int subcomponent)
 {
 	struct component *component;
-	int ret;
 
 	component = kzalloc(sizeof(*component), GFP_KERNEL);
 	if (!component)
@@ -689,18 +862,10 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
 
 	mutex_lock(&component_mutex);
 	list_add_tail(&component->node, &component_list);
-
-	ret = try_to_bring_up_masters(component);
-	if (ret < 0) {
-		if (component->adev)
-			remove_component(component->adev, component);
-		list_del(&component->node);
-
-		kfree(component);
-	}
 	mutex_unlock(&component_mutex);
 
-	return ret < 0 ? ret : 0;
+	/* Try to bind */
+	return bus_rescan_devices(&aggregate_bus_type);
 }
 
 /**
@@ -764,6 +929,7 @@ EXPORT_SYMBOL_GPL(component_add);
  */
 void component_del(struct device *dev, const struct component_ops *ops)
 {
+	struct aggregate_device *adev = NULL;
 	struct component *c, *component = NULL;
 
 	mutex_lock(&component_mutex);
@@ -775,13 +941,26 @@ void component_del(struct device *dev, const struct component_ops *ops)
 		}
 
 	if (component && component->adev) {
-		take_down_aggregate_device(component->adev);
-		remove_component(component->adev, component);
+		adev = component->adev;
+		remove_component(adev, component);
 	}
 
 	mutex_unlock(&component_mutex);
 
+	if (adev) {
+		/* Force unbind */
+		if (adev->dev.driver)
+			device_driver_detach(&adev->dev);
+		put_device(&adev->dev);
+	}
+
 	WARN_ON(!component);
 	kfree(component);
 }
 EXPORT_SYMBOL_GPL(component_del);
+
+static int __init aggregate_bus_init(void)
+{
+	return bus_register(&aggregate_bus_type);
+}
+postcore_initcall(aggregate_bus_init);
diff --git a/include/linux/component.h b/include/linux/component.h
index 71bfc3862633..bc71d34a3416 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -3,9 +3,7 @@
 #define COMPONENT_H
 
 #include <linux/stddef.h>
-
-
-struct device;
+#include <linux/device.h>
 
 /**
  * struct component_ops - callbacks for component drivers
@@ -82,11 +80,63 @@ struct component_master_ops {
 	void (*unbind)(struct device *master);
 };
 
+struct component_match;
+
+/**
+ * struct aggregate_driver - Aggregate driver (made up of other drivers)
+ * @driver: device driver
+ * @match: component match list
+ */
+struct aggregate_driver {
+	/**
+	 * @probe:
+	 *
+	 * Called when all components or the aggregate driver, as specified in
+	 * the @match list are
+	 * ready. Usually there are 3 steps to bind an aggregate driver:
+	 *
+	 * 1. Allocate a struct aggregate_driver.
+	 *
+	 * 2. Bind all components to the aggregate driver by calling
+	 *    component_bind_all() with the aggregate driver structure as opaque
+	 *    pointer data.
+	 *
+	 * 3. Register the aggregate driver with the subsystem to publish its
+	 *    interfaces.
+	 */
+	int (*probe)(struct aggregate_device *adev);
+	/**
+	 * @remove:
+	 *
+	 * Called when either the aggregate driver, using
+	 * component_aggregate_unregister(), or one of its components, using
+	 * component_del(), is unregistered.
+	 */
+	void (*remove)(struct aggregate_device *adev);
+	/**
+	 * @shutdown:
+	 *
+	 * Called when the system is shutting down.
+	 */
+	void (*shutdown)(struct aggregate_device *adev);
+
+	struct component_match *match;
+	struct device_driver	driver;
+};
+
+static inline struct aggregate_driver *to_aggregate_driver(struct device_driver *d)
+{
+	return container_of(d, struct aggregate_driver, driver);
+}
+
+int component_aggregate_register(struct device *parent,
+	struct aggregate_driver *adrv);
+void component_aggregate_unregister(struct device *parent,
+	struct aggregate_driver *adrv);
+
 void component_master_del(struct device *,
 	const struct component_master_ops *);
 
-struct component_match;
-
 int component_master_add_with_match(struct device *,
 	const struct component_master_ops *, struct component_match *);
 void component_match_add_release(struct device *master,
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 4/7] component: Introduce the aggregate bus_type
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

The component driver only provides 'bind' and 'unbind' callbacks to tell
the host driver that it is time to assemble the aggregate driver now
that all the components have probed. The component driver model doesn't
attempt to resolve runtime PM or suspend/resume ordering, and explicitly
mentions this in the code. This lack of support leads to some pretty
gnarly usages of the 'prepare' and 'complete' power management hooks in
drivers that host the aggregate device, and it fully breaks down when
faced with ordering shutdown between the various components, the
aggregate driver, and the host driver that registers the whole thing.

In a concrete example, the MSM display driver at drivers/gpu/drm/msm is
using 'prepare' and 'complete' to call the drm helpers
drm_mode_config_helper_suspend() and drm_mode_config_helper_resume()
respectively, so that it can move the aggregate driver suspend/resume
callbacks to be before and after the components that make up the drm
device call any suspend/resume hooks they have. This only works as long
as the component devices don't do anything in their own 'prepare' and
'complete' callbacks. If they did, then the ordering would be incorrect
and we would be doing something in the component drivers before the
aggregate driver could do anything. Yuck!

Similarly, when trying to add shutdown support to the MSM driver we run
across a problem where we're trying to shutdown the drm device via
drm_atomic_helper_shutdown(), but some of the devices in the encoder
chain have already been shutdown. This time, the component devices
aren't the problem (although they could be if they did anything in their
shutdown callbacks), but there's a DSI to eDP bridge in the encoder
chain that has already been shutdown before the driver hosting the
aggregate device runs shutdown. The ordering of driver probe is like
this:

 1. msm_pdev_probe() (host driver)
 2. DSI bridge
 3. aggregate bind

When it comes to shutdown we have this order:

 1. DSI bridge
 2. msm_pdev_shutdown() (host driver)

and so the bridge is already off, but we want to communicate to it to
turn things off on the display during msm_pdev_shutdown(). Double yuck!
Unfortunately, this time we can't split shutdown into multiple phases
and swap msm_pdev_shutdown() with the DSI bridge.

Let's make the component driver into an actual device driver that has
probe/remove/shutdown functions. The driver will only be bound to the
aggregate device once all component drivers have called component_add()
to indicate they're ready to assemble the aggregate driver. This allows
us to attach shutdown logic (and in the future runtime PM logic) to the
aggregate driver so that it runs the hooks in the correct order.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c  | 429 +++++++++++++++++++++++++++-----------
 include/linux/component.h |  60 +++++-
 2 files changed, 359 insertions(+), 130 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 55e30e0b0952..a6c0bb7ccdbc 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -15,6 +15,9 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/debugfs.h>
+#include <linux/pm_runtime.h>
+
+#include "base.h"
 
 /**
  * DOC: overview
@@ -60,9 +63,6 @@ struct component_match {
 };
 
 struct aggregate_device {
-	struct list_head node;
-	bool bound;
-
 	const struct component_master_ops *ops;
 	struct device *parent;
 	struct device dev;
@@ -71,6 +71,11 @@ struct aggregate_device {
 	int id;
 };
 
+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+	return container_of(d, struct aggregate_device, dev);
+}
+
 struct component {
 	struct list_head node;
 	struct aggregate_device *adev;
@@ -83,7 +88,6 @@ struct component {
 
 static DEFINE_MUTEX(component_mutex);
 static LIST_HEAD(component_list);
-static LIST_HEAD(aggregate_devices);
 
 static DEFINE_IDA(aggregate_ida);
 
@@ -101,7 +105,7 @@ static int component_devices_show(struct seq_file *s, void *data)
 	seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
-		   dev_name(m->parent), m->bound ? "bound" : "not bound");
+		   dev_name(m->parent), m->dev.driver ? "bound" : "not bound");
 
 	seq_printf(s, "%-40s %20s\n", "device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
@@ -149,16 +153,21 @@ static void component_master_debugfs_del(struct aggregate_device *m)
 
 #endif
 
-static struct aggregate_device *__aggregate_find(struct device *parent,
-	const struct component_master_ops *ops)
+struct aggregate_bus_find_data {
+	const struct component_master_ops *ops;
+	struct device *parent;
+};
+
+static int aggregate_bus_find_match(struct device *dev, const void *_data)
 {
-	struct aggregate_device *m;
+	struct aggregate_device *adev = to_aggregate_device(dev);
+	const struct aggregate_bus_find_data *data = _data;
 
-	list_for_each_entry(m, &aggregate_devices, node)
-		if (m->parent == parent && (!ops || m->ops == ops))
-			return m;
+	if (dev->parent == data->parent &&
+	    (!data->ops || adev->ops == data->ops))
+		return 1;
 
-	return NULL;
+	return 0;
 }
 
 static struct component *find_component(struct aggregate_device *adev,
@@ -185,7 +194,6 @@ static int find_components(struct aggregate_device *adev)
 {
 	struct component_match *match = adev->match;
 	size_t i;
-	int ret = 0;
 
 	/*
 	 * Scan the array of match functions and attach
@@ -201,10 +209,8 @@ static int find_components(struct aggregate_device *adev)
 			continue;
 
 		c = find_component(adev, mc);
-		if (!c) {
-			ret = -ENXIO;
-			break;
-		}
+		if (!c)
+			return 0;
 
 		dev_dbg(adev->parent, "found component %s, duplicate %u\n",
 			dev_name(c->dev), !!c->adev);
@@ -212,9 +218,12 @@ static int find_components(struct aggregate_device *adev)
 		/* Attach this component to the adev */
 		match->compare[i].duplicate = !!c->adev;
 		match->compare[i].component = c;
+		/* Matches put in component_del() */
+		get_device(&adev->dev);
 		c->adev = adev;
 	}
-	return ret;
+
+	return 1;
 }
 
 /* Detach component from associated master */
@@ -228,72 +237,6 @@ static void remove_component(struct aggregate_device *adev, struct component *c)
 			adev->match->compare[i].component = NULL;
 }
 
-/*
- * Try to bring up an aggregate device.  If component is NULL, we're interested
- * in this aggregate device, otherwise it's a component which must be present
- * to try and bring up the aggregate device.
- *
- * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
- */
-static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
-	struct component *component)
-{
-	int ret;
-
-	dev_dbg(adev->parent, "trying to bring up adev\n");
-
-	if (find_components(adev)) {
-		dev_dbg(adev->parent, "master has incomplete components\n");
-		return 0;
-	}
-
-	if (component && component->adev != adev) {
-		dev_dbg(adev->parent, "master is not for this component (%s)\n",
-			dev_name(component->dev));
-		return 0;
-	}
-
-	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
-		return -ENOMEM;
-
-	/* Found all components */
-	ret = adev->ops->bind(adev->parent);
-	if (ret < 0) {
-		devres_release_group(adev->parent, NULL);
-		if (ret != -EPROBE_DEFER)
-			dev_info(adev->parent, "adev bind failed: %d\n", ret);
-		return ret;
-	}
-
-	adev->bound = true;
-	return 1;
-}
-
-static int try_to_bring_up_masters(struct component *component)
-{
-	struct aggregate_device *adev;
-	int ret = 0;
-
-	list_for_each_entry(adev, &aggregate_devices, node) {
-		if (!adev->bound) {
-			ret = try_to_bring_up_aggregate_device(adev, component);
-			if (ret != 0)
-				break;
-		}
-	}
-
-	return ret;
-}
-
-static void take_down_aggregate_device(struct aggregate_device *adev)
-{
-	if (adev->bound) {
-		adev->ops->unbind(adev->parent);
-		devres_release_group(adev->parent, NULL);
-		adev->bound = false;
-	}
-}
-
 static void devm_component_match_release(struct device *parent, void *res)
 {
 	struct component_match *match = res;
@@ -437,7 +380,6 @@ static void free_aggregate_device(struct aggregate_device *adev)
 	int i;
 
 	component_master_debugfs_del(adev);
-	list_del(&adev->node);
 
 	if (match) {
 		for (i = 0; i < match->num; i++) {
@@ -451,19 +393,140 @@ static void free_aggregate_device(struct aggregate_device *adev)
 	kfree(adev);
 }
 
-/**
- * component_master_add_with_match - register an aggregate driver
- * @parent: parent device of the aggregate driver
- * @ops: callbacks for the aggregate driver
- * @match: component match list for the aggregate driver
- *
- * Registers a new aggregate driver consisting of the components added to @match
- * by calling one of the component_match_add() functions. Once all components in
- * @match are available, it will be assembled by calling
- * &component_master_ops.bind from @ops. Must be unregistered by calling
- * component_master_del().
- */
-int component_master_add_with_match(struct device *parent,
+static void aggregate_device_release(struct device *dev)
+{
+	struct aggregate_device *adev = to_aggregate_device(dev);
+
+	free_aggregate_device(adev);
+}
+
+static int aggregate_device_match(struct device *dev, struct device_driver *drv)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(drv);
+	struct aggregate_device *adev = to_aggregate_device(dev);
+	int ret;
+
+	if (adrv->match != adev->match)
+		return 0;
+
+	/* Should we start to assemble? */
+	mutex_lock(&component_mutex);
+	ret = find_components(adev);
+	mutex_unlock(&component_mutex);
+
+	return ret;
+}
+
+/* TODO: Remove once all aggregate drivers use component_aggregate_register() */
+static int component_probe_bind(struct aggregate_device *adev)
+{
+	return adev->ops->bind(adev->parent);
+}
+
+static void component_remove_unbind(struct aggregate_device *adev)
+{
+	adev->ops->unbind(adev->parent);
+}
+
+static int aggregate_driver_probe(struct device *dev)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+	struct aggregate_device *adev = to_aggregate_device(dev);
+	bool modern = adrv->probe != component_probe_bind;
+	int ret;
+
+	/* Only do runtime PM when drivers migrate */
+	if (modern) {
+		pm_runtime_get_noresume(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	mutex_lock(&component_mutex);
+	if (devres_open_group(adev->parent, NULL, GFP_KERNEL)) {
+		ret = adrv->probe(adev);
+		if (ret)
+			devres_release_group(dev->parent, NULL);
+	} else {
+		ret = -ENOMEM;
+	}
+	mutex_unlock(&component_mutex);
+
+	if (ret && modern) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_put_noidle(dev);
+	}
+
+	return ret;
+}
+
+static int aggregate_driver_remove(struct device *dev)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+	bool modern = adrv->remove != component_remove_unbind;
+
+	/* Only do runtime PM when drivers migrate */
+	if (modern)
+		pm_runtime_get_sync(dev);
+	adrv->remove(to_aggregate_device(dev));
+	devres_release_group(dev->parent, NULL);
+	if (!modern)
+		return 0;
+
+	pm_runtime_put_noidle(dev);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
+
+	return 0;
+}
+
+static void aggregate_driver_shutdown(struct device *dev)
+{
+	const struct aggregate_driver *adrv = to_aggregate_driver(dev->driver);
+
+	if (adrv && adrv->shutdown)
+		adrv->shutdown(to_aggregate_device(dev));
+}
+
+static struct bus_type aggregate_bus_type = {
+	.name		= "aggregate",
+	.match		= aggregate_device_match,
+	.probe		= aggregate_driver_probe,
+	.remove		= aggregate_driver_remove,
+	.shutdown	= aggregate_driver_shutdown,
+};
+
+/* Callers take ownership of return value, should call put_device() */
+static struct aggregate_device *__aggregate_find(struct device *parent,
+	const struct component_master_ops *ops)
+{
+	struct device *dev;
+	struct aggregate_bus_find_data data = {
+		.ops = ops,
+		.parent = parent,
+	};
+
+	dev = bus_find_device(&aggregate_bus_type, NULL, &data,
+			      aggregate_bus_find_match);
+
+	return dev ? to_aggregate_device(dev) : NULL;
+}
+
+static int aggregate_driver_register(struct aggregate_driver *adrv)
+{
+	adrv->driver.bus = &aggregate_bus_type;
+	return driver_register(&adrv->driver);
+}
+
+static void aggregate_driver_unregister(struct aggregate_driver *adrv)
+{
+	driver_unregister(&adrv->driver);
+}
+
+static struct aggregate_device *aggregate_device_add(struct device *parent,
 	const struct component_master_ops *ops,
 	struct component_match *match)
 {
@@ -473,41 +536,113 @@ int component_master_add_with_match(struct device *parent,
 	/* Reallocate the match array for its true size */
 	ret = component_match_realloc(match, match->num);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
 	if (!adev)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	id = ida_alloc(&aggregate_ida, GFP_KERNEL);
 	if (id < 0) {
 		kfree(adev);
-		return id;
+		return ERR_PTR(id);
 	}
 
 	adev->id = id;
 	adev->parent = parent;
+
 	adev->dev.parent = parent;
+	adev->dev.bus = &aggregate_bus_type;
+	adev->dev.release = aggregate_device_release;
 	adev->ops = ops;
 	adev->match = match;
 	dev_set_name(&adev->dev, "aggregate%d", id);
 
+	ret = device_register(&adev->dev);
+	if (ret) {
+		put_device(&adev->dev);
+		return ERR_PTR(ret);
+	}
+
 	component_master_debugfs_add(adev);
-	/* Add to the list of available aggregate devices. */
-	mutex_lock(&component_mutex);
-	list_add(&adev->node, &aggregate_devices);
 
-	ret = try_to_bring_up_aggregate_device(adev, NULL);
+	return adev;
+}
+
+/**
+ * component_master_add_with_match - register an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @ops: callbacks for the aggregate driver
+ * @match: component match list for the aggregate driver
+ *
+ * Registers a new aggregate driver consisting of the components added to @match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available, it will be assembled by calling
+ * &component_master_ops.bind from @ops. Must be unregistered by calling
+ * component_master_del().
+ */
+int component_master_add_with_match(struct device *parent,
+	const struct component_master_ops *ops,
+	struct component_match *match)
+{
+	struct aggregate_driver *adrv;
+	struct aggregate_device *adev;
+	int ret = 0;
+
+	adev = aggregate_device_add(parent, ops, match);
+	if (IS_ERR(adev))
+		return PTR_ERR(adev);
 
-	if (ret < 0)
-		free_aggregate_device(adev);
+	adrv = kzalloc(sizeof(*adrv), GFP_KERNEL);
+	if (!adrv) {
+		ret = -ENOMEM;
+		goto err;
+	}
 
-	mutex_unlock(&component_mutex);
+	adrv->probe = component_probe_bind;
+	adrv->remove = component_remove_unbind;
+	adrv->driver.owner = THIS_MODULE;
+	adrv->driver.name = dev_name(&adev->dev);
+	adrv->match = match;
 
-	return ret < 0 ? ret : 0;
+	ret = aggregate_driver_register(adrv);
+	if (!ret)
+		return 0;
+err:
+	put_device(&adev->dev);
+	kfree(adrv);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
+/**
+ * component_aggregate_register - register an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @adrv: aggregate driver to register
+ *
+ * Registers a new aggregate driver consisting of the components added to @adrv.match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available, the aggregate driver will be assembled by calling
+ * &adrv.bind. Must be unregistered by calling component_aggregate_unregister().
+ */
+int component_aggregate_register(struct device *parent,
+	struct aggregate_driver *adrv)
+{
+	struct aggregate_device *adev;
+	int ret;
+
+	adev = aggregate_device_add(parent, NULL, adrv->match);
+	if (IS_ERR(adev))
+		return PTR_ERR(adev);
+
+	ret = aggregate_driver_register(adrv);
+	if (ret)
+		put_device(&adev->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(component_aggregate_register);
+
 /**
  * component_master_del - unregister an aggregate driver
  * @parent: parent device of the aggregate driver
@@ -521,17 +656,53 @@ void component_master_del(struct device *parent,
 	const struct component_master_ops *ops)
 {
 	struct aggregate_device *adev;
+	struct aggregate_driver *adrv;
+	struct device_driver *drv;
 
 	mutex_lock(&component_mutex);
 	adev = __aggregate_find(parent, ops);
+	mutex_unlock(&component_mutex);
+
 	if (adev) {
-		take_down_aggregate_device(adev);
-		free_aggregate_device(adev);
+		drv = adev->dev.driver;
+		if (drv) {
+			adrv = to_aggregate_driver(drv);
+			aggregate_driver_unregister(adrv);
+			kfree(adrv);
+		}
+
+		device_unregister(&adev->dev);
 	}
-	mutex_unlock(&component_mutex);
+	put_device(&adev->dev);
 }
 EXPORT_SYMBOL_GPL(component_master_del);
 
+/**
+ * component_aggregate_unregister - unregister an aggregate driver
+ * @parent: parent device of the aggregate driver
+ * @adrv: registered aggregate driver
+ *
+ * Unregisters an aggregate driver registered with
+ * component_aggregate_register(). If necessary the aggregate driver is first
+ * disassembled.
+ */
+void component_aggregate_unregister(struct device *parent,
+	struct aggregate_driver *adrv)
+{
+	struct aggregate_device *adev;
+
+	mutex_lock(&component_mutex);
+	adev = __aggregate_find(parent, NULL);
+	mutex_unlock(&component_mutex);
+
+	if (adev)
+		device_unregister(&adev->dev);
+	put_device(&adev->dev);
+
+	aggregate_driver_unregister(adrv);
+}
+EXPORT_SYMBOL_GPL(component_aggregate_unregister);
+
 static void component_unbind(struct component *component,
 	struct aggregate_device *adev, void *data)
 {
@@ -572,6 +743,8 @@ void component_unbind_all(struct device *parent, void *data)
 			c = adev->match->compare[i].component;
 			component_unbind(c, adev, data);
 		}
+
+	put_device(&adev->dev);
 }
 EXPORT_SYMBOL_GPL(component_unbind_all);
 
@@ -666,6 +839,7 @@ int component_bind_all(struct device *parent, void *data)
 				component_unbind(c, adev, data);
 			}
 	}
+	put_device(&adev->dev);
 
 	return ret;
 }
@@ -675,7 +849,6 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
 	int subcomponent)
 {
 	struct component *component;
-	int ret;
 
 	component = kzalloc(sizeof(*component), GFP_KERNEL);
 	if (!component)
@@ -689,18 +862,10 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
 
 	mutex_lock(&component_mutex);
 	list_add_tail(&component->node, &component_list);
-
-	ret = try_to_bring_up_masters(component);
-	if (ret < 0) {
-		if (component->adev)
-			remove_component(component->adev, component);
-		list_del(&component->node);
-
-		kfree(component);
-	}
 	mutex_unlock(&component_mutex);
 
-	return ret < 0 ? ret : 0;
+	/* Try to bind */
+	return bus_rescan_devices(&aggregate_bus_type);
 }
 
 /**
@@ -764,6 +929,7 @@ EXPORT_SYMBOL_GPL(component_add);
  */
 void component_del(struct device *dev, const struct component_ops *ops)
 {
+	struct aggregate_device *adev = NULL;
 	struct component *c, *component = NULL;
 
 	mutex_lock(&component_mutex);
@@ -775,13 +941,26 @@ void component_del(struct device *dev, const struct component_ops *ops)
 		}
 
 	if (component && component->adev) {
-		take_down_aggregate_device(component->adev);
-		remove_component(component->adev, component);
+		adev = component->adev;
+		remove_component(adev, component);
 	}
 
 	mutex_unlock(&component_mutex);
 
+	if (adev) {
+		/* Force unbind */
+		if (adev->dev.driver)
+			device_driver_detach(&adev->dev);
+		put_device(&adev->dev);
+	}
+
 	WARN_ON(!component);
 	kfree(component);
 }
 EXPORT_SYMBOL_GPL(component_del);
+
+static int __init aggregate_bus_init(void)
+{
+	return bus_register(&aggregate_bus_type);
+}
+postcore_initcall(aggregate_bus_init);
diff --git a/include/linux/component.h b/include/linux/component.h
index 71bfc3862633..bc71d34a3416 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -3,9 +3,7 @@
 #define COMPONENT_H
 
 #include <linux/stddef.h>
-
-
-struct device;
+#include <linux/device.h>
 
 /**
  * struct component_ops - callbacks for component drivers
@@ -82,11 +80,63 @@ struct component_master_ops {
 	void (*unbind)(struct device *master);
 };
 
+struct component_match;
+
+/**
+ * struct aggregate_driver - Aggregate driver (made up of other drivers)
+ * @driver: device driver
+ * @match: component match list
+ */
+struct aggregate_driver {
+	/**
+	 * @probe:
+	 *
+	 * Called when all components or the aggregate driver, as specified in
+	 * the @match list are
+	 * ready. Usually there are 3 steps to bind an aggregate driver:
+	 *
+	 * 1. Allocate a struct aggregate_driver.
+	 *
+	 * 2. Bind all components to the aggregate driver by calling
+	 *    component_bind_all() with the aggregate driver structure as opaque
+	 *    pointer data.
+	 *
+	 * 3. Register the aggregate driver with the subsystem to publish its
+	 *    interfaces.
+	 */
+	int (*probe)(struct aggregate_device *adev);
+	/**
+	 * @remove:
+	 *
+	 * Called when either the aggregate driver, using
+	 * component_aggregate_unregister(), or one of its components, using
+	 * component_del(), is unregistered.
+	 */
+	void (*remove)(struct aggregate_device *adev);
+	/**
+	 * @shutdown:
+	 *
+	 * Called when the system is shutting down.
+	 */
+	void (*shutdown)(struct aggregate_device *adev);
+
+	struct component_match *match;
+	struct device_driver	driver;
+};
+
+static inline struct aggregate_driver *to_aggregate_driver(struct device_driver *d)
+{
+	return container_of(d, struct aggregate_driver, driver);
+}
+
+int component_aggregate_register(struct device *parent,
+	struct aggregate_driver *adrv);
+void component_aggregate_unregister(struct device *parent,
+	struct aggregate_driver *adrv);
+
 void component_master_del(struct device *,
 	const struct component_master_ops *);
 
-struct component_match;
-
 int component_master_add_with_match(struct device *,
 	const struct component_master_ops *, struct component_match *);
 void component_match_add_release(struct device *master,
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 5/7] component: Use dev.parent instead of adev->parent
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

We left this in place to ease the code diff, but now we can remove it
because the aggregate device parent pointer is the same.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index a6c0bb7ccdbc..155aab7eefa6 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -64,7 +64,6 @@ struct component_match {
 
 struct aggregate_device {
 	const struct component_master_ops *ops;
-	struct device *parent;
 	struct device dev;
 	struct component_match *match;
 
@@ -105,7 +104,7 @@ static int component_devices_show(struct seq_file *s, void *data)
 	seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
-		   dev_name(m->parent), m->dev.driver ? "bound" : "not bound");
+		   dev_name(m->dev.parent), m->dev.driver ? "bound" : "not bound");
 
 	seq_printf(s, "%-40s %20s\n", "device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
@@ -134,13 +133,13 @@ core_initcall(component_debug_init);
 
 static void component_master_debugfs_add(struct aggregate_device *m)
 {
-	debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
+	debugfs_create_file(dev_name(m->dev.parent), 0444, component_debugfs_dir, m,
 			    &component_devices_fops);
 }
 
 static void component_master_debugfs_del(struct aggregate_device *m)
 {
-	debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
+	debugfs_remove(debugfs_lookup(dev_name(m->dev.parent), component_debugfs_dir));
 }
 
 #else
@@ -203,7 +202,7 @@ static int find_components(struct aggregate_device *adev)
 		struct component_match_array *mc = &match->compare[i];
 		struct component *c;
 
-		dev_dbg(adev->parent, "Looking for component %zu\n", i);
+		dev_dbg(adev->dev.parent, "Looking for component %zu\n", i);
 
 		if (match->compare[i].component)
 			continue;
@@ -212,7 +211,7 @@ static int find_components(struct aggregate_device *adev)
 		if (!c)
 			return 0;
 
-		dev_dbg(adev->parent, "found component %s, duplicate %u\n",
+		dev_dbg(adev->dev.parent, "found component %s, duplicate %u\n",
 			dev_name(c->dev), !!c->adev);
 
 		/* Attach this component to the adev */
@@ -420,12 +419,12 @@ static int aggregate_device_match(struct device *dev, struct device_driver *drv)
 /* TODO: Remove once all aggregate drivers use component_aggregate_register() */
 static int component_probe_bind(struct aggregate_device *adev)
 {
-	return adev->ops->bind(adev->parent);
+	return adev->ops->bind(adev->dev.parent);
 }
 
 static void component_remove_unbind(struct aggregate_device *adev)
 {
-	adev->ops->unbind(adev->parent);
+	adev->ops->unbind(adev->dev.parent);
 }
 
 static int aggregate_driver_probe(struct device *dev)
@@ -443,7 +442,7 @@ static int aggregate_driver_probe(struct device *dev)
 	}
 
 	mutex_lock(&component_mutex);
-	if (devres_open_group(adev->parent, NULL, GFP_KERNEL)) {
+	if (devres_open_group(adev->dev.parent, NULL, GFP_KERNEL)) {
 		ret = adrv->probe(adev);
 		if (ret)
 			devres_release_group(dev->parent, NULL);
@@ -549,8 +548,6 @@ static struct aggregate_device *aggregate_device_add(struct device *parent,
 	}
 
 	adev->id = id;
-	adev->parent = parent;
-
 	adev->dev.parent = parent;
 	adev->dev.bus = &aggregate_bus_type;
 	adev->dev.release = aggregate_device_release;
@@ -709,7 +706,7 @@ static void component_unbind(struct component *component,
 	WARN_ON(!component->bound);
 
 	if (component->ops && component->ops->unbind)
-		component->ops->unbind(component->dev, adev->parent, data);
+		component->ops->unbind(component->dev, adev->dev.parent, data);
 	component->bound = false;
 
 	/* Release all resources claimed in the binding of this component */
@@ -758,7 +755,7 @@ static int component_bind(struct component *component, struct aggregate_device *
 	 * This allows us to roll-back a failed component without
 	 * affecting anything else.
 	 */
-	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
+	if (!devres_open_group(adev->dev.parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/*
@@ -767,14 +764,14 @@ static int component_bind(struct component *component, struct aggregate_device *
 	 * at the appropriate moment.
 	 */
 	if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
-		devres_release_group(adev->parent, NULL);
+		devres_release_group(adev->dev.parent, NULL);
 		return -ENOMEM;
 	}
 
-	dev_dbg(adev->parent, "binding %s (ops %ps)\n",
+	dev_dbg(adev->dev.parent, "binding %s (ops %ps)\n",
 		dev_name(component->dev), component->ops);
 
-	ret = component->ops->bind(component->dev, adev->parent, data);
+	ret = component->ops->bind(component->dev, adev->dev.parent, data);
 	if (!ret) {
 		component->bound = true;
 
@@ -785,16 +782,16 @@ static int component_bind(struct component *component, struct aggregate_device *
 		 * can clean those resources up independently.
 		 */
 		devres_close_group(component->dev, NULL);
-		devres_remove_group(adev->parent, NULL);
+		devres_remove_group(adev->dev.parent, NULL);
 
-		dev_info(adev->parent, "bound %s (ops %ps)\n",
+		dev_info(adev->dev.parent, "bound %s (ops %ps)\n",
 			 dev_name(component->dev), component->ops);
 	} else {
 		devres_release_group(component->dev, NULL);
-		devres_release_group(adev->parent, NULL);
+		devres_release_group(adev->dev.parent, NULL);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
+			dev_err(adev->dev.parent, "failed to bind %s (ops %ps): %d\n",
 				dev_name(component->dev), component->ops, ret);
 	}
 
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 5/7] component: Use dev.parent instead of adev->parent
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

We left this in place to ease the code diff, but now we can remove it
because the aggregate device parent pointer is the same.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index a6c0bb7ccdbc..155aab7eefa6 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -64,7 +64,6 @@ struct component_match {
 
 struct aggregate_device {
 	const struct component_master_ops *ops;
-	struct device *parent;
 	struct device dev;
 	struct component_match *match;
 
@@ -105,7 +104,7 @@ static int component_devices_show(struct seq_file *s, void *data)
 	seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
 	seq_printf(s, "%-40s %20s\n\n",
-		   dev_name(m->parent), m->dev.driver ? "bound" : "not bound");
+		   dev_name(m->dev.parent), m->dev.driver ? "bound" : "not bound");
 
 	seq_printf(s, "%-40s %20s\n", "device name", "status");
 	seq_puts(s, "-------------------------------------------------------------\n");
@@ -134,13 +133,13 @@ core_initcall(component_debug_init);
 
 static void component_master_debugfs_add(struct aggregate_device *m)
 {
-	debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
+	debugfs_create_file(dev_name(m->dev.parent), 0444, component_debugfs_dir, m,
 			    &component_devices_fops);
 }
 
 static void component_master_debugfs_del(struct aggregate_device *m)
 {
-	debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
+	debugfs_remove(debugfs_lookup(dev_name(m->dev.parent), component_debugfs_dir));
 }
 
 #else
@@ -203,7 +202,7 @@ static int find_components(struct aggregate_device *adev)
 		struct component_match_array *mc = &match->compare[i];
 		struct component *c;
 
-		dev_dbg(adev->parent, "Looking for component %zu\n", i);
+		dev_dbg(adev->dev.parent, "Looking for component %zu\n", i);
 
 		if (match->compare[i].component)
 			continue;
@@ -212,7 +211,7 @@ static int find_components(struct aggregate_device *adev)
 		if (!c)
 			return 0;
 
-		dev_dbg(adev->parent, "found component %s, duplicate %u\n",
+		dev_dbg(adev->dev.parent, "found component %s, duplicate %u\n",
 			dev_name(c->dev), !!c->adev);
 
 		/* Attach this component to the adev */
@@ -420,12 +419,12 @@ static int aggregate_device_match(struct device *dev, struct device_driver *drv)
 /* TODO: Remove once all aggregate drivers use component_aggregate_register() */
 static int component_probe_bind(struct aggregate_device *adev)
 {
-	return adev->ops->bind(adev->parent);
+	return adev->ops->bind(adev->dev.parent);
 }
 
 static void component_remove_unbind(struct aggregate_device *adev)
 {
-	adev->ops->unbind(adev->parent);
+	adev->ops->unbind(adev->dev.parent);
 }
 
 static int aggregate_driver_probe(struct device *dev)
@@ -443,7 +442,7 @@ static int aggregate_driver_probe(struct device *dev)
 	}
 
 	mutex_lock(&component_mutex);
-	if (devres_open_group(adev->parent, NULL, GFP_KERNEL)) {
+	if (devres_open_group(adev->dev.parent, NULL, GFP_KERNEL)) {
 		ret = adrv->probe(adev);
 		if (ret)
 			devres_release_group(dev->parent, NULL);
@@ -549,8 +548,6 @@ static struct aggregate_device *aggregate_device_add(struct device *parent,
 	}
 
 	adev->id = id;
-	adev->parent = parent;
-
 	adev->dev.parent = parent;
 	adev->dev.bus = &aggregate_bus_type;
 	adev->dev.release = aggregate_device_release;
@@ -709,7 +706,7 @@ static void component_unbind(struct component *component,
 	WARN_ON(!component->bound);
 
 	if (component->ops && component->ops->unbind)
-		component->ops->unbind(component->dev, adev->parent, data);
+		component->ops->unbind(component->dev, adev->dev.parent, data);
 	component->bound = false;
 
 	/* Release all resources claimed in the binding of this component */
@@ -758,7 +755,7 @@ static int component_bind(struct component *component, struct aggregate_device *
 	 * This allows us to roll-back a failed component without
 	 * affecting anything else.
 	 */
-	if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
+	if (!devres_open_group(adev->dev.parent, NULL, GFP_KERNEL))
 		return -ENOMEM;
 
 	/*
@@ -767,14 +764,14 @@ static int component_bind(struct component *component, struct aggregate_device *
 	 * at the appropriate moment.
 	 */
 	if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
-		devres_release_group(adev->parent, NULL);
+		devres_release_group(adev->dev.parent, NULL);
 		return -ENOMEM;
 	}
 
-	dev_dbg(adev->parent, "binding %s (ops %ps)\n",
+	dev_dbg(adev->dev.parent, "binding %s (ops %ps)\n",
 		dev_name(component->dev), component->ops);
 
-	ret = component->ops->bind(component->dev, adev->parent, data);
+	ret = component->ops->bind(component->dev, adev->dev.parent, data);
 	if (!ret) {
 		component->bound = true;
 
@@ -785,16 +782,16 @@ static int component_bind(struct component *component, struct aggregate_device *
 		 * can clean those resources up independently.
 		 */
 		devres_close_group(component->dev, NULL);
-		devres_remove_group(adev->parent, NULL);
+		devres_remove_group(adev->dev.parent, NULL);
 
-		dev_info(adev->parent, "bound %s (ops %ps)\n",
+		dev_info(adev->dev.parent, "bound %s (ops %ps)\n",
 			 dev_name(component->dev), component->ops);
 	} else {
 		devres_release_group(component->dev, NULL);
-		devres_release_group(adev->parent, NULL);
+		devres_release_group(adev->dev.parent, NULL);
 
 		if (ret != -EPROBE_DEFER)
-			dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
+			dev_err(adev->dev.parent, "failed to bind %s (ops %ps): %d\n",
 				dev_name(component->dev), component->ops, ret);
 	}
 
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 6/7] component: Move struct aggregate_device out to header file
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

This allows aggregate driver writers to use the device passed to their
probe/remove/shutdown functions properly instead of treating it as an
opaque pointer.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c  | 13 -------------
 include/linux/component.h | 15 +++++++++++++--
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 155aab7eefa6..1b4c84453319 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -62,19 +62,6 @@ struct component_match {
 	struct component_match_array *compare;
 };
 
-struct aggregate_device {
-	const struct component_master_ops *ops;
-	struct device dev;
-	struct component_match *match;
-
-	int id;
-};
-
-static inline struct aggregate_device *to_aggregate_device(struct device *d)
-{
-	return container_of(d, struct aggregate_device, dev);
-}
-
 struct component {
 	struct list_head node;
 	struct aggregate_device *adev;
diff --git a/include/linux/component.h b/include/linux/component.h
index bc71d34a3416..d19cc3418d12 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -39,7 +39,7 @@ void component_del(struct device *, const struct component_ops *);
 int component_bind_all(struct device *master, void *master_data);
 void component_unbind_all(struct device *master, void *master_data);
 
-struct aggregate_device;
+struct component_match;
 
 /**
  * struct component_master_ops - callback for the aggregate driver
@@ -80,7 +80,18 @@ struct component_master_ops {
 	void (*unbind)(struct device *master);
 };
 
-struct component_match;
+struct aggregate_device {
+	const struct component_master_ops *ops;
+	struct device dev;
+	struct component_match *match;
+
+	int id;
+};
+
+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+	return container_of(d, struct aggregate_device, dev);
+}
 
 /**
  * struct aggregate_driver - Aggregate driver (made up of other drivers)
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 6/7] component: Move struct aggregate_device out to header file
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

This allows aggregate driver writers to use the device passed to their
probe/remove/shutdown functions properly instead of treating it as an
opaque pointer.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/base/component.c  | 13 -------------
 include/linux/component.h | 15 +++++++++++++--
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 155aab7eefa6..1b4c84453319 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -62,19 +62,6 @@ struct component_match {
 	struct component_match_array *compare;
 };
 
-struct aggregate_device {
-	const struct component_master_ops *ops;
-	struct device dev;
-	struct component_match *match;
-
-	int id;
-};
-
-static inline struct aggregate_device *to_aggregate_device(struct device *d)
-{
-	return container_of(d, struct aggregate_device, dev);
-}
-
 struct component {
 	struct list_head node;
 	struct aggregate_device *adev;
diff --git a/include/linux/component.h b/include/linux/component.h
index bc71d34a3416..d19cc3418d12 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -39,7 +39,7 @@ void component_del(struct device *, const struct component_ops *);
 int component_bind_all(struct device *master, void *master_data);
 void component_unbind_all(struct device *master, void *master_data);
 
-struct aggregate_device;
+struct component_match;
 
 /**
  * struct component_master_ops - callback for the aggregate driver
@@ -80,7 +80,18 @@ struct component_master_ops {
 	void (*unbind)(struct device *master);
 };
 
-struct component_match;
+struct aggregate_device {
+	const struct component_master_ops *ops;
+	struct device dev;
+	struct component_match *match;
+
+	int id;
+};
+
+static inline struct aggregate_device *to_aggregate_device(struct device *d)
+{
+	return container_of(d, struct aggregate_device, dev);
+}
 
 /**
  * struct aggregate_driver - Aggregate driver (made up of other drivers)
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 7/7] drm/msm: Migrate to aggregate driver
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  0:25   ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, linux-arm-msm, dri-devel, freedreno, Daniel Vetter,
	Rafael J. Wysocki, Rob Clark, Russell King, Saravana Kannan

The device lists are poorly ordered when the component device code is
used. This is because component_master_add_with_match() returns 0
regardless of component devices calling component_add() first. It can
really only fail if an allocation fails, in which case everything is
going bad and we're out of memory. The driver that registers the
aggregate driver, can succeed at probe and put the attached device on
the DPM lists before any of the component devices are probed and put on
the lists.

Within the component device framework this usually isn't that bad
because the real driver work is done at bind time via
component{,master}_ops::bind(). It becomes a problem when the driver
core, or host driver, wants to operate on the component device outside
of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
driver core doesn't understand the relationship between the host device
and the component devices and could possibly try to operate on component
devices when they're already removed from the system or shut down.

Normally, device links or probe defer would reorder the lists and put
devices that depend on other devices in the lists at the correct
location, but with component devices this doesn't happen because this
information isn't expressed anywhere. Drivers simply succeed at
registering their component or the aggregate driver with the component
framework and wait for their bind() callback to be called once the other
components are ready. In summary, the drivers that make up the aggregate
driver can probe in any order.

This ordering problem becomes fairly obvious when shutting down the
device with a DSI controller connected to a DSI bridge that is
controlled via i2c. In this case, the msm display driver wants to tear
down the display pipeline on shutdown via msm_pdev_shutdown() by calling
drm_atomic_helper_shutdown(), and it can't do that unless the whole
display chain is still probed and active in the system. When a display
bridge is on i2c, the i2c device for the bridge will be created whenever
the i2c controller probes, which could be before or after the msm
display driver probes. If the i2c controller probes after the display
driver, then the i2c controller will be shutdown before the display
controller during system wide shutdown and thus i2c transactions will
stop working before the display pipeline is shut down. This means we'll
have the display bridge trying to access an i2c bus that's shut down
because drm_atomic_helper_shutdown() is trying to disable the bridge
after the bridge is off.

The solution is to make the aggregate driver into a real struct driver
that is bound to a device when the other component devices have all
probed. Now that the component driver code is a proper bus, we can
simply register an aggregate driver with that bus via
component_aggregate_register() and then attach the shutdown hook to that
driver to be sure that the shutdown for the display pipeline is called
before any of the component device driver shutdown hooks are called.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

As stated in the cover letter, this isn't perfect but it still works. I
get a warning from runtime PM that the parent device (e00000.mdss) is
not runtime PM enabled but the child device (the aggregate device) is
being enabled by the bus logic. I need to move around the place that the
parent device is runtime PM enabled and probably keep it powered up
during the entire time that the driver is probed until the aggregate
driver probes.

 drivers/gpu/drm/msm/msm_drv.c | 47 +++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e1104d2454e2..0c64e6a2ce25 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1265,19 +1265,35 @@ static int add_gpu_components(struct device *dev,
 	return 0;
 }
 
-static int msm_drm_bind(struct device *dev)
+static int msm_drm_bind(struct aggregate_device *adev)
 {
-	return msm_drm_init(dev, &msm_driver);
+	return msm_drm_init(adev->dev.parent, &msm_driver);
 }
 
-static void msm_drm_unbind(struct device *dev)
+static void msm_drm_unbind(struct aggregate_device *adev)
 {
-	msm_drm_uninit(dev);
+	msm_drm_uninit(adev->dev.parent);
+}
+
+static void msm_drm_shutdown(struct aggregate_device *adev)
+{
+	struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->dev.parent));
+	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
+
+	if (!priv || !priv->kms)
+		return;
+
+	drm_atomic_helper_shutdown(drm);
 }
 
-static const struct component_master_ops msm_drm_ops = {
-	.bind = msm_drm_bind,
-	.unbind = msm_drm_unbind,
+static struct aggregate_driver msm_drm_aggregate_driver = {
+	.probe = msm_drm_bind,
+	.remove = msm_drm_unbind,
+	.shutdown = msm_drm_shutdown,
+	.driver = {
+		.name	= "msm_drm",
+		.owner	= THIS_MODULE,
+	},
 };
 
 /*
@@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
-	ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
+	msm_drm_aggregate_driver.match = match;
+	ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver);
 	if (ret)
 		goto fail;
 
@@ -1319,23 +1336,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
 
 static int msm_pdev_remove(struct platform_device *pdev)
 {
-	component_master_del(&pdev->dev, &msm_drm_ops);
+	component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
 	of_platform_depopulate(&pdev->dev);
 
 	return 0;
 }
 
-static void msm_pdev_shutdown(struct platform_device *pdev)
-{
-	struct drm_device *drm = platform_get_drvdata(pdev);
-	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
-
-	if (!priv || !priv->kms)
-		return;
-
-	drm_atomic_helper_shutdown(drm);
-}
-
 static const struct of_device_id dt_match[] = {
 	{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
 	{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
@@ -1351,7 +1357,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
 static struct platform_driver msm_platform_driver = {
 	.probe      = msm_pdev_probe,
 	.remove     = msm_pdev_remove,
-	.shutdown   = msm_pdev_shutdown,
 	.driver     = {
 		.name   = "msm",
 		.of_match_table = dt_match,
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 7/7] drm/msm: Migrate to aggregate driver
@ 2021-05-20  0:25   ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  0:25 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Saravana Kannan, Rafael J. Wysocki, linux-arm-msm, linux-kernel,
	dri-devel, Daniel Vetter, Russell King, freedreno

The device lists are poorly ordered when the component device code is
used. This is because component_master_add_with_match() returns 0
regardless of component devices calling component_add() first. It can
really only fail if an allocation fails, in which case everything is
going bad and we're out of memory. The driver that registers the
aggregate driver, can succeed at probe and put the attached device on
the DPM lists before any of the component devices are probed and put on
the lists.

Within the component device framework this usually isn't that bad
because the real driver work is done at bind time via
component{,master}_ops::bind(). It becomes a problem when the driver
core, or host driver, wants to operate on the component device outside
of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
driver core doesn't understand the relationship between the host device
and the component devices and could possibly try to operate on component
devices when they're already removed from the system or shut down.

Normally, device links or probe defer would reorder the lists and put
devices that depend on other devices in the lists at the correct
location, but with component devices this doesn't happen because this
information isn't expressed anywhere. Drivers simply succeed at
registering their component or the aggregate driver with the component
framework and wait for their bind() callback to be called once the other
components are ready. In summary, the drivers that make up the aggregate
driver can probe in any order.

This ordering problem becomes fairly obvious when shutting down the
device with a DSI controller connected to a DSI bridge that is
controlled via i2c. In this case, the msm display driver wants to tear
down the display pipeline on shutdown via msm_pdev_shutdown() by calling
drm_atomic_helper_shutdown(), and it can't do that unless the whole
display chain is still probed and active in the system. When a display
bridge is on i2c, the i2c device for the bridge will be created whenever
the i2c controller probes, which could be before or after the msm
display driver probes. If the i2c controller probes after the display
driver, then the i2c controller will be shutdown before the display
controller during system wide shutdown and thus i2c transactions will
stop working before the display pipeline is shut down. This means we'll
have the display bridge trying to access an i2c bus that's shut down
because drm_atomic_helper_shutdown() is trying to disable the bridge
after the bridge is off.

The solution is to make the aggregate driver into a real struct driver
that is bound to a device when the other component devices have all
probed. Now that the component driver code is a proper bus, we can
simply register an aggregate driver with that bus via
component_aggregate_register() and then attach the shutdown hook to that
driver to be sure that the shutdown for the display pipeline is called
before any of the component device driver shutdown hooks are called.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

As stated in the cover letter, this isn't perfect but it still works. I
get a warning from runtime PM that the parent device (e00000.mdss) is
not runtime PM enabled but the child device (the aggregate device) is
being enabled by the bus logic. I need to move around the place that the
parent device is runtime PM enabled and probably keep it powered up
during the entire time that the driver is probed until the aggregate
driver probes.

 drivers/gpu/drm/msm/msm_drv.c | 47 +++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e1104d2454e2..0c64e6a2ce25 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1265,19 +1265,35 @@ static int add_gpu_components(struct device *dev,
 	return 0;
 }
 
-static int msm_drm_bind(struct device *dev)
+static int msm_drm_bind(struct aggregate_device *adev)
 {
-	return msm_drm_init(dev, &msm_driver);
+	return msm_drm_init(adev->dev.parent, &msm_driver);
 }
 
-static void msm_drm_unbind(struct device *dev)
+static void msm_drm_unbind(struct aggregate_device *adev)
 {
-	msm_drm_uninit(dev);
+	msm_drm_uninit(adev->dev.parent);
+}
+
+static void msm_drm_shutdown(struct aggregate_device *adev)
+{
+	struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->dev.parent));
+	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
+
+	if (!priv || !priv->kms)
+		return;
+
+	drm_atomic_helper_shutdown(drm);
 }
 
-static const struct component_master_ops msm_drm_ops = {
-	.bind = msm_drm_bind,
-	.unbind = msm_drm_unbind,
+static struct aggregate_driver msm_drm_aggregate_driver = {
+	.probe = msm_drm_bind,
+	.remove = msm_drm_unbind,
+	.shutdown = msm_drm_shutdown,
+	.driver = {
+		.name	= "msm_drm",
+		.owner	= THIS_MODULE,
+	},
 };
 
 /*
@@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
-	ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
+	msm_drm_aggregate_driver.match = match;
+	ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver);
 	if (ret)
 		goto fail;
 
@@ -1319,23 +1336,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
 
 static int msm_pdev_remove(struct platform_device *pdev)
 {
-	component_master_del(&pdev->dev, &msm_drm_ops);
+	component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
 	of_platform_depopulate(&pdev->dev);
 
 	return 0;
 }
 
-static void msm_pdev_shutdown(struct platform_device *pdev)
-{
-	struct drm_device *drm = platform_get_drvdata(pdev);
-	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
-
-	if (!priv || !priv->kms)
-		return;
-
-	drm_atomic_helper_shutdown(drm);
-}
-
 static const struct of_device_id dt_match[] = {
 	{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
 	{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
@@ -1351,7 +1357,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
 static struct platform_driver msm_platform_driver = {
 	.probe      = msm_pdev_probe,
 	.remove     = msm_pdev_remove,
-	.shutdown   = msm_pdev_shutdown,
 	.driver     = {
 		.name   = "msm",
 		.of_match_table = dt_match,
-- 
https://chromeos.dev


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
  2021-05-20  0:25 ` Stephen Boyd
@ 2021-05-20  1:27   ` Saravana Kannan
  -1 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20  1:27 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel, freedreno,
	Daniel Vetter, Rafael J. Wysocki, Rob Clark, Russell King

On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> This series is from discussion we had on reordering the device lists for
> drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> the aggregate device onto and then we probe the device once all the
> components are probed and call component_add(). The probe/remove hooks
> are where the bind/unbind calls go, and then a shutdown hook is added
> that can be used to shutdown the drm display pipeline at the right time.
>
> This works for me on my sc7180 board, but I'm currently struggling with
> the last patch where we migrate the msm driver. It runs into a runtime
> PM problem where the parent device isn't runtime PM enabled yet. I'm
> still trying to figure out a clean solution there. Moving runtime PM
> around breaks boot and I think that's because the power domain is off.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
>
> [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
>

I skimmed through the series and in general the idea is good, but I'm
not sure why each component user needs to be converted/"modern" before
it can make use of the benefits of this series. Why not just have
wrapper functions around the component ops that the new aggregate bus
driver can just call? That'll give all the existing component users
the new ability to use the new ops without having to have two
versions. That'll also allow us to do other improvements (I have some
in mind) that'll apply to all the component users instead of only the
converted ones.

-Saravana

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
@ 2021-05-20  1:27   ` Saravana Kannan
  0 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20  1:27 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Daniel Vetter, linux-arm-msm, LKML, dri-devel,
	Greg Kroah-Hartman, Russell King, freedreno

On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> This series is from discussion we had on reordering the device lists for
> drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> the aggregate device onto and then we probe the device once all the
> components are probed and call component_add(). The probe/remove hooks
> are where the bind/unbind calls go, and then a shutdown hook is added
> that can be used to shutdown the drm display pipeline at the right time.
>
> This works for me on my sc7180 board, but I'm currently struggling with
> the last patch where we migrate the msm driver. It runs into a runtime
> PM problem where the parent device isn't runtime PM enabled yet. I'm
> still trying to figure out a clean solution there. Moving runtime PM
> around breaks boot and I think that's because the power domain is off.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
>
> [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
>

I skimmed through the series and in general the idea is good, but I'm
not sure why each component user needs to be converted/"modern" before
it can make use of the benefits of this series. Why not just have
wrapper functions around the component ops that the new aggregate bus
driver can just call? That'll give all the existing component users
the new ability to use the new ops without having to have two
versions. That'll also allow us to do other improvements (I have some
in mind) that'll apply to all the component users instead of only the
converted ones.

-Saravana

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
  2021-05-20  1:27   ` Saravana Kannan
@ 2021-05-20  1:41     ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  1:41 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel, freedreno,
	Daniel Vetter, Rafael J. Wysocki, Rob Clark, Russell King

Quoting Saravana Kannan (2021-05-19 18:27:50)
> On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > This series is from discussion we had on reordering the device lists for
> > drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> > the aggregate device onto and then we probe the device once all the
> > components are probed and call component_add(). The probe/remove hooks
> > are where the bind/unbind calls go, and then a shutdown hook is added
> > that can be used to shutdown the drm display pipeline at the right time.
> >
> > This works for me on my sc7180 board, but I'm currently struggling with
> > the last patch where we migrate the msm driver. It runs into a runtime
> > PM problem where the parent device isn't runtime PM enabled yet. I'm
> > still trying to figure out a clean solution there. Moving runtime PM
> > around breaks boot and I think that's because the power domain is off.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > Cc: Saravana Kannan <saravanak@google.com>
> >
> > [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
> >
>
> I skimmed through the series and in general the idea is good, but I'm
> not sure why each component user needs to be converted/"modern" before
> it can make use of the benefits of this series. Why not just have
> wrapper functions around the component ops that the new aggregate bus
> driver can just call? That'll give all the existing component users
> the new ability to use the new ops without having to have two
> versions.

The existing users can only have one or the other. Either use the ops
structure or use the struct aggregate_driver. What benefits of this
series are they not gaining?

> That'll also allow us to do other improvements (I have some
> in mind) that'll apply to all the component users instead of only the
> converted ones.

What do you have in mind? I didn't want to convert drivers over to the
new way of doing things without making them consciously change their
code. Otherwise I worry it will break things in random, subtle ways. The
last patch, as I mentioned above in the cover, causes warnings because
the display driver is enabling runtime PM in an odd spot as part of the
bind callback of the aggregate/master. That should move out of there and
into the msm_pdev driver that registers the aggregate from what I can
tell.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
@ 2021-05-20  1:41     ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-20  1:41 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, Daniel Vetter, linux-arm-msm, LKML, dri-devel,
	Greg Kroah-Hartman, Russell King, freedreno

Quoting Saravana Kannan (2021-05-19 18:27:50)
> On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > This series is from discussion we had on reordering the device lists for
> > drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> > the aggregate device onto and then we probe the device once all the
> > components are probed and call component_add(). The probe/remove hooks
> > are where the bind/unbind calls go, and then a shutdown hook is added
> > that can be used to shutdown the drm display pipeline at the right time.
> >
> > This works for me on my sc7180 board, but I'm currently struggling with
> > the last patch where we migrate the msm driver. It runs into a runtime
> > PM problem where the parent device isn't runtime PM enabled yet. I'm
> > still trying to figure out a clean solution there. Moving runtime PM
> > around breaks boot and I think that's because the power domain is off.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > Cc: Saravana Kannan <saravanak@google.com>
> >
> > [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
> >
>
> I skimmed through the series and in general the idea is good, but I'm
> not sure why each component user needs to be converted/"modern" before
> it can make use of the benefits of this series. Why not just have
> wrapper functions around the component ops that the new aggregate bus
> driver can just call? That'll give all the existing component users
> the new ability to use the new ops without having to have two
> versions.

The existing users can only have one or the other. Either use the ops
structure or use the struct aggregate_driver. What benefits of this
series are they not gaining?

> That'll also allow us to do other improvements (I have some
> in mind) that'll apply to all the component users instead of only the
> converted ones.

What do you have in mind? I didn't want to convert drivers over to the
new way of doing things without making them consciously change their
code. Otherwise I worry it will break things in random, subtle ways. The
last patch, as I mentioned above in the cover, causes warnings because
the display driver is enabling runtime PM in an odd spot as part of the
bind callback of the aggregate/master. That should move out of there and
into the msm_pdev driver that registers the aggregate from what I can
tell.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
  2021-05-20  1:41     ` Stephen Boyd
@ 2021-05-20 19:30       ` Saravana Kannan
  -1 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20 19:30 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel, freedreno,
	Daniel Vetter, Rafael J. Wysocki, Rob Clark, Russell King

On Wed, May 19, 2021 at 6:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Saravana Kannan (2021-05-19 18:27:50)
> > On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > This series is from discussion we had on reordering the device lists for
> > > drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> > > the aggregate device onto and then we probe the device once all the
> > > components are probed and call component_add(). The probe/remove hooks
> > > are where the bind/unbind calls go, and then a shutdown hook is added
> > > that can be used to shutdown the drm display pipeline at the right time.
> > >
> > > This works for me on my sc7180 board, but I'm currently struggling with
> > > the last patch where we migrate the msm driver. It runs into a runtime
> > > PM problem where the parent device isn't runtime PM enabled yet. I'm
> > > still trying to figure out a clean solution there. Moving runtime PM
> > > around breaks boot and I think that's because the power domain is off.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > >
> > > [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
> > >
> >
> > I skimmed through the series and in general the idea is good, but I'm
> > not sure why each component user needs to be converted/"modern" before
> > it can make use of the benefits of this series. Why not just have
> > wrapper functions around the component ops that the new aggregate bus
> > driver can just call? That'll give all the existing component users
> > the new ability to use the new ops without having to have two
> > versions.
>
> The existing users can only have one or the other. Either use the ops
> structure or use the struct aggregate_driver. What benefits of this
> series are they not gaining?

As I mentioned earlier, if we add device links between the aggregate
device (consumer) and all the component devices (suppliers), it'll
take care of a lot of the ordering issues (probe, suspend, runtime PM)
and dependency issues (unbind the master device if a component driver
unbinds). It'll allow us to delete a lot of the code in the component
framework too. I can send the patch for the device links once your
series settles. So having two implementations comes in the way of a
clean up and code improvement because we'll have to keep a lot of the
component code for the purpose of the "legacy" ops.

> > That'll also allow us to do other improvements (I have some
> > in mind) that'll apply to all the component users instead of only the
> > converted ones.
>
> What do you have in mind? I didn't want to convert drivers over to the
> new way of doing things without making them consciously change their
> code.

What ordering/behavior would you be changing with the new ops? If the
new shutdown ops isn't used, it really shouldn't change anything. Put
another way, if we ignore your msm driver changes, we should be able
to switch to having a real device for the "master" without making any
functional change. If you are causing any functional change with the
new ops, maybe you can key it off a flag that needs to be set? That
way, we'll have one API/ops but still be backward compatible if you
are worried about breaking existing users?

> Otherwise I worry it will break things in random, subtle ways. The
> last patch, as I mentioned above in the cover, causes warnings because
> the display driver is enabling runtime PM in an odd spot as part of the
> bind callback of the aggregate/master. That should move out of there and
> into the msm_pdev driver that registers the aggregate from what I can
> tell.

Can you give more context? I think if you create device links with
RPM_ACTIVE and PM_RUNTIME flags, it should ensure runtime PM
correctness.

-Saravana

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
@ 2021-05-20 19:30       ` Saravana Kannan
  0 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20 19:30 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Daniel Vetter, linux-arm-msm, LKML, dri-devel,
	Greg Kroah-Hartman, Russell King, freedreno

On Wed, May 19, 2021 at 6:41 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Saravana Kannan (2021-05-19 18:27:50)
> > On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > This series is from discussion we had on reordering the device lists for
> > > drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> > > the aggregate device onto and then we probe the device once all the
> > > components are probed and call component_add(). The probe/remove hooks
> > > are where the bind/unbind calls go, and then a shutdown hook is added
> > > that can be used to shutdown the drm display pipeline at the right time.
> > >
> > > This works for me on my sc7180 board, but I'm currently struggling with
> > > the last patch where we migrate the msm driver. It runs into a runtime
> > > PM problem where the parent device isn't runtime PM enabled yet. I'm
> > > still trying to figure out a clean solution there. Moving runtime PM
> > > around breaks boot and I think that's because the power domain is off.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > >
> > > [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
> > >
> >
> > I skimmed through the series and in general the idea is good, but I'm
> > not sure why each component user needs to be converted/"modern" before
> > it can make use of the benefits of this series. Why not just have
> > wrapper functions around the component ops that the new aggregate bus
> > driver can just call? That'll give all the existing component users
> > the new ability to use the new ops without having to have two
> > versions.
>
> The existing users can only have one or the other. Either use the ops
> structure or use the struct aggregate_driver. What benefits of this
> series are they not gaining?

As I mentioned earlier, if we add device links between the aggregate
device (consumer) and all the component devices (suppliers), it'll
take care of a lot of the ordering issues (probe, suspend, runtime PM)
and dependency issues (unbind the master device if a component driver
unbinds). It'll allow us to delete a lot of the code in the component
framework too. I can send the patch for the device links once your
series settles. So having two implementations comes in the way of a
clean up and code improvement because we'll have to keep a lot of the
component code for the purpose of the "legacy" ops.

> > That'll also allow us to do other improvements (I have some
> > in mind) that'll apply to all the component users instead of only the
> > converted ones.
>
> What do you have in mind? I didn't want to convert drivers over to the
> new way of doing things without making them consciously change their
> code.

What ordering/behavior would you be changing with the new ops? If the
new shutdown ops isn't used, it really shouldn't change anything. Put
another way, if we ignore your msm driver changes, we should be able
to switch to having a real device for the "master" without making any
functional change. If you are causing any functional change with the
new ops, maybe you can key it off a flag that needs to be set? That
way, we'll have one API/ops but still be backward compatible if you
are worried about breaking existing users?

> Otherwise I worry it will break things in random, subtle ways. The
> last patch, as I mentioned above in the cover, causes warnings because
> the display driver is enabling runtime PM in an odd spot as part of the
> bind callback of the aggregate/master. That should move out of there and
> into the msm_pdev driver that registers the aggregate from what I can
> tell.

Can you give more context? I think if you create device links with
RPM_ACTIVE and PM_RUNTIME flags, it should ensure runtime PM
correctness.

-Saravana

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/7] drm/msm: Migrate to aggregate driver
  2021-05-20  0:25   ` Stephen Boyd
@ 2021-05-20 19:58     ` Daniel Vetter
  -1 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-05-20 19:58 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, linux-arm-msm, dri-devel,
	freedreno, Daniel Vetter, Rafael J. Wysocki, Rob Clark,
	Russell King, Saravana Kannan

On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> The device lists are poorly ordered when the component device code is
> used. This is because component_master_add_with_match() returns 0
> regardless of component devices calling component_add() first. It can
> really only fail if an allocation fails, in which case everything is
> going bad and we're out of memory. The driver that registers the
> aggregate driver, can succeed at probe and put the attached device on
> the DPM lists before any of the component devices are probed and put on
> the lists.
> 
> Within the component device framework this usually isn't that bad
> because the real driver work is done at bind time via
> component{,master}_ops::bind(). It becomes a problem when the driver
> core, or host driver, wants to operate on the component device outside
> of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
> driver core doesn't understand the relationship between the host device
> and the component devices and could possibly try to operate on component
> devices when they're already removed from the system or shut down.
> 
> Normally, device links or probe defer would reorder the lists and put
> devices that depend on other devices in the lists at the correct
> location, but with component devices this doesn't happen because this
> information isn't expressed anywhere. Drivers simply succeed at
> registering their component or the aggregate driver with the component
> framework and wait for their bind() callback to be called once the other
> components are ready. In summary, the drivers that make up the aggregate
> driver can probe in any order.
> 
> This ordering problem becomes fairly obvious when shutting down the
> device with a DSI controller connected to a DSI bridge that is
> controlled via i2c. In this case, the msm display driver wants to tear
> down the display pipeline on shutdown via msm_pdev_shutdown() by calling
> drm_atomic_helper_shutdown(), and it can't do that unless the whole
> display chain is still probed and active in the system. When a display
> bridge is on i2c, the i2c device for the bridge will be created whenever
> the i2c controller probes, which could be before or after the msm
> display driver probes. If the i2c controller probes after the display
> driver, then the i2c controller will be shutdown before the display
> controller during system wide shutdown and thus i2c transactions will
> stop working before the display pipeline is shut down. This means we'll
> have the display bridge trying to access an i2c bus that's shut down
> because drm_atomic_helper_shutdown() is trying to disable the bridge
> after the bridge is off.
> 
> The solution is to make the aggregate driver into a real struct driver
> that is bound to a device when the other component devices have all
> probed. Now that the component driver code is a proper bus, we can
> simply register an aggregate driver with that bus via
> component_aggregate_register() and then attach the shutdown hook to that
> driver to be sure that the shutdown for the display pipeline is called
> before any of the component device driver shutdown hooks are called.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> As stated in the cover letter, this isn't perfect but it still works. I
> get a warning from runtime PM that the parent device (e00000.mdss) is
> not runtime PM enabled but the child device (the aggregate device) is
> being enabled by the bus logic. I need to move around the place that the
> parent device is runtime PM enabled and probably keep it powered up
> during the entire time that the driver is probed until the aggregate
> driver probes.
> 
>  drivers/gpu/drm/msm/msm_drv.c | 47 +++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index e1104d2454e2..0c64e6a2ce25 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1265,19 +1265,35 @@ static int add_gpu_components(struct device *dev,
>  	return 0;
>  }
>  
> -static int msm_drm_bind(struct device *dev)
> +static int msm_drm_bind(struct aggregate_device *adev)
>  {
> -	return msm_drm_init(dev, &msm_driver);
> +	return msm_drm_init(adev->dev.parent, &msm_driver);
>  }
>  
> -static void msm_drm_unbind(struct device *dev)
> +static void msm_drm_unbind(struct aggregate_device *adev)
>  {
> -	msm_drm_uninit(dev);
> +	msm_drm_uninit(adev->dev.parent);
> +}
> +
> +static void msm_drm_shutdown(struct aggregate_device *adev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->dev.parent));
> +	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> +
> +	if (!priv || !priv->kms)
> +		return;
> +
> +	drm_atomic_helper_shutdown(drm);
>  }
>  
> -static const struct component_master_ops msm_drm_ops = {
> -	.bind = msm_drm_bind,
> -	.unbind = msm_drm_unbind,
> +static struct aggregate_driver msm_drm_aggregate_driver = {
> +	.probe = msm_drm_bind,
> +	.remove = msm_drm_unbind,
> +	.shutdown = msm_drm_shutdown,
> +	.driver = {
> +		.name	= "msm_drm",
> +		.owner	= THIS_MODULE,
> +	},
>  };
>  
>  /*
> @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto fail;
>  
> -	ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
> +	msm_drm_aggregate_driver.match = match;

This is a bit awkward design, because it means the driver struct can't be
made const, and it will blow up when you have multiple instance of the
same driver. I think the match should stay as part of the register
function call, and be stored in the aggregate_device struct somewhere.

Otherwise I think this looks really solid and fixes your issue properly.
Obviously needs careful review from Greg KH for the device model side of
things, and from Rafael Wysocki for pm side.

Bunch of thoughts from a very cursory reading:

- I think it'd be good if we pass the aggregate_device to all components
  when we bind them, plus the void * parameter just to make this less
  disruptive. Even more device model goodies.

- Maybe splatter a pile of sysfs links around so that this all becomes
  visible? Could be interesting for debugging ordering issues. Just an
  idea, feel free to entirely ignore.

- Needs solid kerneldoc for everything exposed to drivers and good
  overview DOC:

- Needs deprecation warnings in the kerneldoc for all the
  component_master_* and if feasible with a mechanical conversion,
  converting existing users. I'd like to not be stuck with the old model
  forever, plus this will give a pile more people to review this code
  here.

Anyway the name changes in probe and remove hooks below are already worth
this on their own imo. That's why I'd like to see them in all drivers.

Cheers, Daniel

> +	ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver);
>  	if (ret)
>  		goto fail;
>  
> @@ -1319,23 +1336,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
>  
>  static int msm_pdev_remove(struct platform_device *pdev)
>  {
> -	component_master_del(&pdev->dev, &msm_drm_ops);
> +	component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
>  	of_platform_depopulate(&pdev->dev);
>  
>  	return 0;
>  }
>  
> -static void msm_pdev_shutdown(struct platform_device *pdev)
> -{
> -	struct drm_device *drm = platform_get_drvdata(pdev);
> -	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> -
> -	if (!priv || !priv->kms)
> -		return;
> -
> -	drm_atomic_helper_shutdown(drm);
> -}
> -
>  static const struct of_device_id dt_match[] = {
>  	{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
>  	{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> @@ -1351,7 +1357,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
>  static struct platform_driver msm_platform_driver = {
>  	.probe      = msm_pdev_probe,
>  	.remove     = msm_pdev_remove,
> -	.shutdown   = msm_pdev_shutdown,
>  	.driver     = {
>  		.name   = "msm",
>  		.of_match_table = dt_match,
> -- 
> https://chromeos.dev
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/7] drm/msm: Migrate to aggregate driver
@ 2021-05-20 19:58     ` Daniel Vetter
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-05-20 19:58 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Saravana Kannan, Rafael J. Wysocki, Daniel Vetter, linux-arm-msm,
	linux-kernel, dri-devel, Greg Kroah-Hartman, Russell King,
	freedreno

On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> The device lists are poorly ordered when the component device code is
> used. This is because component_master_add_with_match() returns 0
> regardless of component devices calling component_add() first. It can
> really only fail if an allocation fails, in which case everything is
> going bad and we're out of memory. The driver that registers the
> aggregate driver, can succeed at probe and put the attached device on
> the DPM lists before any of the component devices are probed and put on
> the lists.
> 
> Within the component device framework this usually isn't that bad
> because the real driver work is done at bind time via
> component{,master}_ops::bind(). It becomes a problem when the driver
> core, or host driver, wants to operate on the component device outside
> of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
> driver core doesn't understand the relationship between the host device
> and the component devices and could possibly try to operate on component
> devices when they're already removed from the system or shut down.
> 
> Normally, device links or probe defer would reorder the lists and put
> devices that depend on other devices in the lists at the correct
> location, but with component devices this doesn't happen because this
> information isn't expressed anywhere. Drivers simply succeed at
> registering their component or the aggregate driver with the component
> framework and wait for their bind() callback to be called once the other
> components are ready. In summary, the drivers that make up the aggregate
> driver can probe in any order.
> 
> This ordering problem becomes fairly obvious when shutting down the
> device with a DSI controller connected to a DSI bridge that is
> controlled via i2c. In this case, the msm display driver wants to tear
> down the display pipeline on shutdown via msm_pdev_shutdown() by calling
> drm_atomic_helper_shutdown(), and it can't do that unless the whole
> display chain is still probed and active in the system. When a display
> bridge is on i2c, the i2c device for the bridge will be created whenever
> the i2c controller probes, which could be before or after the msm
> display driver probes. If the i2c controller probes after the display
> driver, then the i2c controller will be shutdown before the display
> controller during system wide shutdown and thus i2c transactions will
> stop working before the display pipeline is shut down. This means we'll
> have the display bridge trying to access an i2c bus that's shut down
> because drm_atomic_helper_shutdown() is trying to disable the bridge
> after the bridge is off.
> 
> The solution is to make the aggregate driver into a real struct driver
> that is bound to a device when the other component devices have all
> probed. Now that the component driver code is a proper bus, we can
> simply register an aggregate driver with that bus via
> component_aggregate_register() and then attach the shutdown hook to that
> driver to be sure that the shutdown for the display pipeline is called
> before any of the component device driver shutdown hooks are called.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> As stated in the cover letter, this isn't perfect but it still works. I
> get a warning from runtime PM that the parent device (e00000.mdss) is
> not runtime PM enabled but the child device (the aggregate device) is
> being enabled by the bus logic. I need to move around the place that the
> parent device is runtime PM enabled and probably keep it powered up
> during the entire time that the driver is probed until the aggregate
> driver probes.
> 
>  drivers/gpu/drm/msm/msm_drv.c | 47 +++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index e1104d2454e2..0c64e6a2ce25 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1265,19 +1265,35 @@ static int add_gpu_components(struct device *dev,
>  	return 0;
>  }
>  
> -static int msm_drm_bind(struct device *dev)
> +static int msm_drm_bind(struct aggregate_device *adev)
>  {
> -	return msm_drm_init(dev, &msm_driver);
> +	return msm_drm_init(adev->dev.parent, &msm_driver);
>  }
>  
> -static void msm_drm_unbind(struct device *dev)
> +static void msm_drm_unbind(struct aggregate_device *adev)
>  {
> -	msm_drm_uninit(dev);
> +	msm_drm_uninit(adev->dev.parent);
> +}
> +
> +static void msm_drm_shutdown(struct aggregate_device *adev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->dev.parent));
> +	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> +
> +	if (!priv || !priv->kms)
> +		return;
> +
> +	drm_atomic_helper_shutdown(drm);
>  }
>  
> -static const struct component_master_ops msm_drm_ops = {
> -	.bind = msm_drm_bind,
> -	.unbind = msm_drm_unbind,
> +static struct aggregate_driver msm_drm_aggregate_driver = {
> +	.probe = msm_drm_bind,
> +	.remove = msm_drm_unbind,
> +	.shutdown = msm_drm_shutdown,
> +	.driver = {
> +		.name	= "msm_drm",
> +		.owner	= THIS_MODULE,
> +	},
>  };
>  
>  /*
> @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto fail;
>  
> -	ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
> +	msm_drm_aggregate_driver.match = match;

This is a bit awkward design, because it means the driver struct can't be
made const, and it will blow up when you have multiple instance of the
same driver. I think the match should stay as part of the register
function call, and be stored in the aggregate_device struct somewhere.

Otherwise I think this looks really solid and fixes your issue properly.
Obviously needs careful review from Greg KH for the device model side of
things, and from Rafael Wysocki for pm side.

Bunch of thoughts from a very cursory reading:

- I think it'd be good if we pass the aggregate_device to all components
  when we bind them, plus the void * parameter just to make this less
  disruptive. Even more device model goodies.

- Maybe splatter a pile of sysfs links around so that this all becomes
  visible? Could be interesting for debugging ordering issues. Just an
  idea, feel free to entirely ignore.

- Needs solid kerneldoc for everything exposed to drivers and good
  overview DOC:

- Needs deprecation warnings in the kerneldoc for all the
  component_master_* and if feasible with a mechanical conversion,
  converting existing users. I'd like to not be stuck with the old model
  forever, plus this will give a pile more people to review this code
  here.

Anyway the name changes in probe and remove hooks below are already worth
this on their own imo. That's why I'd like to see them in all drivers.

Cheers, Daniel

> +	ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver);
>  	if (ret)
>  		goto fail;
>  
> @@ -1319,23 +1336,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
>  
>  static int msm_pdev_remove(struct platform_device *pdev)
>  {
> -	component_master_del(&pdev->dev, &msm_drm_ops);
> +	component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
>  	of_platform_depopulate(&pdev->dev);
>  
>  	return 0;
>  }
>  
> -static void msm_pdev_shutdown(struct platform_device *pdev)
> -{
> -	struct drm_device *drm = platform_get_drvdata(pdev);
> -	struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> -
> -	if (!priv || !priv->kms)
> -		return;
> -
> -	drm_atomic_helper_shutdown(drm);
> -}
> -
>  static const struct of_device_id dt_match[] = {
>  	{ .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
>  	{ .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> @@ -1351,7 +1357,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
>  static struct platform_driver msm_platform_driver = {
>  	.probe      = msm_pdev_probe,
>  	.remove     = msm_pdev_remove,
> -	.shutdown   = msm_pdev_shutdown,
>  	.driver     = {
>  		.name   = "msm",
>  		.of_match_table = dt_match,
> -- 
> https://chromeos.dev
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
  2021-05-20  1:41     ` Stephen Boyd
@ 2021-05-20 20:03       ` Daniel Vetter
  -1 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-05-20 20:03 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Saravana Kannan, Greg Kroah-Hartman, LKML, linux-arm-msm,
	dri-devel, freedreno, Daniel Vetter, Rafael J. Wysocki,
	Rob Clark, Russell King

On Wed, May 19, 2021 at 09:41:27PM -0400, Stephen Boyd wrote:
> Quoting Saravana Kannan (2021-05-19 18:27:50)
> > On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > This series is from discussion we had on reordering the device lists for
> > > drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> > > the aggregate device onto and then we probe the device once all the
> > > components are probed and call component_add(). The probe/remove hooks
> > > are where the bind/unbind calls go, and then a shutdown hook is added
> > > that can be used to shutdown the drm display pipeline at the right time.
> > >
> > > This works for me on my sc7180 board, but I'm currently struggling with
> > > the last patch where we migrate the msm driver. It runs into a runtime
> > > PM problem where the parent device isn't runtime PM enabled yet. I'm
> > > still trying to figure out a clean solution there. Moving runtime PM
> > > around breaks boot and I think that's because the power domain is off.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > >
> > > [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
> > >
> >
> > I skimmed through the series and in general the idea is good, but I'm
> > not sure why each component user needs to be converted/"modern" before
> > it can make use of the benefits of this series. Why not just have
> > wrapper functions around the component ops that the new aggregate bus
> > driver can just call? That'll give all the existing component users
> > the new ability to use the new ops without having to have two
> > versions.
> 
> The existing users can only have one or the other. Either use the ops
> structure or use the struct aggregate_driver. What benefits of this
> series are they not gaining?
> 
> > That'll also allow us to do other improvements (I have some
> > in mind) that'll apply to all the component users instead of only the
> > converted ones.
> 
> What do you have in mind? I didn't want to convert drivers over to the
> new way of doing things without making them consciously change their
> code. Otherwise I worry it will break things in random, subtle ways. The
> last patch, as I mentioned above in the cover, causes warnings because
> the display driver is enabling runtime PM in an odd spot as part of the
> bind callback of the aggregate/master. That should move out of there and
> into the msm_pdev driver that registers the aggregate from what I can
> tell.

Hm yeah that's annoying. Another thing to check is that there's no locking
issues with lockdep enabled. But there's plenty of other places that
register/bind drivers within other drivers, so it should all work.

I think this is a good reason why more drivers should be converted (in
separate patches) so that we get a lot more testing and can find bugs in
the design.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/7] component: Make into an aggregate bus
@ 2021-05-20 20:03       ` Daniel Vetter
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-05-20 20:03 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Saravana Kannan, Rafael J. Wysocki, Greg Kroah-Hartman, LKML,
	dri-devel, Daniel Vetter, linux-arm-msm, Russell King, freedreno

On Wed, May 19, 2021 at 09:41:27PM -0400, Stephen Boyd wrote:
> Quoting Saravana Kannan (2021-05-19 18:27:50)
> > On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > This series is from discussion we had on reordering the device lists for
> > > drm shutdown paths[1]. I've introduced an 'aggregate' bus that we put
> > > the aggregate device onto and then we probe the device once all the
> > > components are probed and call component_add(). The probe/remove hooks
> > > are where the bind/unbind calls go, and then a shutdown hook is added
> > > that can be used to shutdown the drm display pipeline at the right time.
> > >
> > > This works for me on my sc7180 board, but I'm currently struggling with
> > > the last patch where we migrate the msm driver. It runs into a runtime
> > > PM problem where the parent device isn't runtime PM enabled yet. I'm
> > > still trying to figure out a clean solution there. Moving runtime PM
> > > around breaks boot and I think that's because the power domain is off.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > > Cc: Saravana Kannan <saravanak@google.com>
> > >
> > > [1] https://lore.kernel.org/r/20210508074118.1621729-1-swboyd@chromium.org
> > >
> >
> > I skimmed through the series and in general the idea is good, but I'm
> > not sure why each component user needs to be converted/"modern" before
> > it can make use of the benefits of this series. Why not just have
> > wrapper functions around the component ops that the new aggregate bus
> > driver can just call? That'll give all the existing component users
> > the new ability to use the new ops without having to have two
> > versions.
> 
> The existing users can only have one or the other. Either use the ops
> structure or use the struct aggregate_driver. What benefits of this
> series are they not gaining?
> 
> > That'll also allow us to do other improvements (I have some
> > in mind) that'll apply to all the component users instead of only the
> > converted ones.
> 
> What do you have in mind? I didn't want to convert drivers over to the
> new way of doing things without making them consciously change their
> code. Otherwise I worry it will break things in random, subtle ways. The
> last patch, as I mentioned above in the cover, causes warnings because
> the display driver is enabling runtime PM in an odd spot as part of the
> bind callback of the aggregate/master. That should move out of there and
> into the msm_pdev driver that registers the aggregate from what I can
> tell.

Hm yeah that's annoying. Another thing to check is that there's no locking
issues with lockdep enabled. But there's plenty of other places that
register/bind drivers within other drivers, so it should all work.

I think this is a good reason why more drivers should be converted (in
separate patches) so that we get a lot more testing and can find bugs in
the design.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/7] component: Introduce struct aggregate_device
  2021-05-20  0:25   ` Stephen Boyd
@ 2021-05-20 20:20     ` Saravana Kannan
  -1 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20 20:20 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel, freedreno,
	Daniel Vetter, Rafael J. Wysocki, Rob Clark, Russell King

On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Replace 'struct master' with 'struct aggregate_device' and then rename
> 'master' to 'adev' everywhere in the code. While we're here, put a
> struct device inside the aggregate device so that we can register it
> with a bus_type in the next patch.
>
> The diff is large but that's because this is mostly a rename, where
> sometimes 'master' is replaced with 'adev' and other times it is
> replaced with 'parent' to indicate that the struct device that was being
> used is actually the parent of the aggregate device and driver.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/base/component.c  | 249 ++++++++++++++++++++------------------
>  include/linux/component.h |   2 +-
>  2 files changed, 134 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 5e79299f6c3f..55e30e0b0952 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -9,6 +9,7 @@
>   */
>  #include <linux/component.h>
>  #include <linux/device.h>
> +#include <linux/idr.h>
>  #include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> @@ -58,18 +59,21 @@ struct component_match {
>         struct component_match_array *compare;
>  };
>
> -struct master {
> +struct aggregate_device {
>         struct list_head node;
>         bool bound;
>
>         const struct component_master_ops *ops;
>         struct device *parent;
> +       struct device dev;
>         struct component_match *match;
> +
> +       int id;
>  };
>
>  struct component {
>         struct list_head node;
> -       struct master *master;
> +       struct aggregate_device *adev;
>         bool bound;
>
>         const struct component_ops *ops;
> @@ -79,7 +83,9 @@ struct component {
>
>  static DEFINE_MUTEX(component_mutex);
>  static LIST_HEAD(component_list);
> -static LIST_HEAD(masters);
> +static LIST_HEAD(aggregate_devices);
> +
> +static DEFINE_IDA(aggregate_ida);
>
>  #ifdef CONFIG_DEBUG_FS
>
> @@ -87,12 +93,12 @@ static struct dentry *component_debugfs_dir;
>
>  static int component_devices_show(struct seq_file *s, void *data)
>  {
> -       struct master *m = s->private;
> +       struct aggregate_device *m = s->private;
>         struct component_match *match = m->match;
>         size_t i;
>
>         mutex_lock(&component_mutex);
> -       seq_printf(s, "%-40s %20s\n", "master name", "status");
> +       seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
>         seq_puts(s, "-------------------------------------------------------------\n");
>         seq_printf(s, "%-40s %20s\n\n",
>                    dev_name(m->parent), m->bound ? "bound" : "not bound");
> @@ -122,46 +128,46 @@ static int __init component_debug_init(void)
>
>  core_initcall(component_debug_init);
>
> -static void component_master_debugfs_add(struct master *m)
> +static void component_master_debugfs_add(struct aggregate_device *m)
>  {
>         debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
>                             &component_devices_fops);
>  }
>
> -static void component_master_debugfs_del(struct master *m)
> +static void component_master_debugfs_del(struct aggregate_device *m)
>  {
>         debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
>  }
>
>  #else
>
> -static void component_master_debugfs_add(struct master *m)
> +static void component_master_debugfs_add(struct aggregate_device *m)
>  { }
>
> -static void component_master_debugfs_del(struct master *m)
> +static void component_master_debugfs_del(struct aggregate_device *m)
>  { }
>
>  #endif
>
> -static struct master *__master_find(struct device *parent,
> +static struct aggregate_device *__aggregate_find(struct device *parent,
>         const struct component_master_ops *ops)
>  {
> -       struct master *m;
> +       struct aggregate_device *m;
>
> -       list_for_each_entry(m, &masters, node)
> +       list_for_each_entry(m, &aggregate_devices, node)
>                 if (m->parent == parent && (!ops || m->ops == ops))
>                         return m;
>
>         return NULL;
>  }
>
> -static struct component *find_component(struct master *master,
> +static struct component *find_component(struct aggregate_device *adev,
>         struct component_match_array *mc)
>  {
>         struct component *c;
>
>         list_for_each_entry(c, &component_list, node) {
> -               if (c->master && c->master != master)
> +               if (c->adev && c->adev != adev)
>                         continue;
>
>                 if (mc->compare && mc->compare(c->dev, mc->data))
> @@ -175,101 +181,102 @@ static struct component *find_component(struct master *master,
>         return NULL;
>  }
>
> -static int find_components(struct master *master)
> +static int find_components(struct aggregate_device *adev)
>  {
> -       struct component_match *match = master->match;
> +       struct component_match *match = adev->match;
>         size_t i;
>         int ret = 0;
>
>         /*
>          * Scan the array of match functions and attach
> -        * any components which are found to this master.
> +        * any components which are found to this adev.
>          */
>         for (i = 0; i < match->num; i++) {
>                 struct component_match_array *mc = &match->compare[i];
>                 struct component *c;
>
> -               dev_dbg(master->parent, "Looking for component %zu\n", i);
> +               dev_dbg(adev->parent, "Looking for component %zu\n", i);
>
>                 if (match->compare[i].component)
>                         continue;
>
> -               c = find_component(master, mc);
> +               c = find_component(adev, mc);
>                 if (!c) {
>                         ret = -ENXIO;
>                         break;
>                 }
>
> -               dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
> +               dev_dbg(adev->parent, "found component %s, duplicate %u\n",
> +                       dev_name(c->dev), !!c->adev);
>
> -               /* Attach this component to the master */
> -               match->compare[i].duplicate = !!c->master;
> +               /* Attach this component to the adev */
> +               match->compare[i].duplicate = !!c->adev;
>                 match->compare[i].component = c;
> -               c->master = master;
> +               c->adev = adev;
>         }
>         return ret;
>  }
>
>  /* Detach component from associated master */
> -static void remove_component(struct master *master, struct component *c)
> +static void remove_component(struct aggregate_device *adev, struct component *c)
>  {
>         size_t i;
>
> -       /* 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;
> +       /* Detach the component from this adev. */
> +       for (i = 0; i < adev->match->num; i++)
> +               if (adev->match->compare[i].component == c)
> +                       adev->match->compare[i].component = NULL;
>  }
>
>  /*
> - * Try to bring up a master.  If component is NULL, we're interested in
> - * this master, otherwise it's a component which must be present to try
> - * and bring up the master.
> + * Try to bring up an aggregate device.  If component is NULL, we're interested
> + * in this aggregate device, otherwise it's a component which must be present
> + * to try and bring up the aggregate device.
>   *
>   * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
>   */
> -static int try_to_bring_up_master(struct master *master,
> +static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
>         struct component *component)
>  {
>         int ret;
>
> -       dev_dbg(master->parent, "trying to bring up master\n");
> +       dev_dbg(adev->parent, "trying to bring up adev\n");
>
> -       if (find_components(master)) {
> -               dev_dbg(master->parent, "master has incomplete components\n");
> +       if (find_components(adev)) {
> +               dev_dbg(adev->parent, "master has incomplete components\n");
>                 return 0;
>         }
>
> -       if (component && component->master != master) {
> -               dev_dbg(master->parent, "master is not for this component (%s)\n",
> +       if (component && component->adev != adev) {
> +               dev_dbg(adev->parent, "master is not for this component (%s)\n",
>                         dev_name(component->dev));
>                 return 0;
>         }
>
> -       if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> +       if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
>                 return -ENOMEM;
>
>         /* Found all components */
> -       ret = master->ops->bind(master->parent);
> +       ret = adev->ops->bind(adev->parent);
>         if (ret < 0) {
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>                 if (ret != -EPROBE_DEFER)
> -                       dev_info(master->parent, "master bind failed: %d\n", ret);
> +                       dev_info(adev->parent, "adev bind failed: %d\n", ret);
>                 return ret;
>         }
>
> -       master->bound = true;
> +       adev->bound = true;
>         return 1;
>  }
>
>  static int try_to_bring_up_masters(struct component *component)
>  {
> -       struct master *m;
> +       struct aggregate_device *adev;
>         int ret = 0;
>
> -       list_for_each_entry(m, &masters, node) {
> -               if (!m->bound) {
> -                       ret = try_to_bring_up_master(m, component);
> +       list_for_each_entry(adev, &aggregate_devices, node) {
> +               if (!adev->bound) {
> +                       ret = try_to_bring_up_aggregate_device(adev, component);
>                         if (ret != 0)
>                                 break;
>                 }
> @@ -278,12 +285,12 @@ static int try_to_bring_up_masters(struct component *component)
>         return ret;
>  }
>
> -static void take_down_master(struct master *master)
> +static void take_down_aggregate_device(struct aggregate_device *adev)
>  {
> -       if (master->bound) {
> -               master->ops->unbind(master->parent);
> -               devres_release_group(master->parent, NULL);
> -               master->bound = false;
> +       if (adev->bound) {
> +               adev->ops->unbind(adev->parent);
> +               devres_release_group(adev->parent, NULL);
> +               adev->bound = false;
>         }
>  }
>
> @@ -324,7 +331,7 @@ static int component_match_realloc(struct component_match *match, size_t num)
>         return 0;
>  }
>
> -static void __component_match_add(struct device *master,
> +static void __component_match_add(struct device *parent,
>         struct component_match **matchptr,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *),
> @@ -344,7 +351,7 @@ static void __component_match_add(struct device *master,
>                         return;
>                 }
>
> -               devres_add(master, match);
> +               devres_add(parent, match);
>
>                 *matchptr = match;
>         }
> @@ -370,13 +377,13 @@ static void __component_match_add(struct device *master,
>
>  /**
>   * component_match_add_release - add a component match entry with release callback
> - * @master: device with the aggregate driver
> + * @parent: parent device of the aggregate driver
>   * @matchptr: pointer to the list of component matches
>   * @release: release function for @compare_data
>   * @compare: compare function to match against all components
>   * @compare_data: opaque pointer passed to the @compare function
>   *
> - * Adds a new component match to the list stored in @matchptr, which the @master
> + * Adds a new component match to the list stored in @matchptr, which the
>   * aggregate driver needs to function. The list of component matches pointed to
>   * by @matchptr must be initialized to NULL before adding the first match. This
>   * only matches against components added with component_add().
> @@ -388,19 +395,19 @@ static void __component_match_add(struct device *master,
>   *
>   * See also component_match_add() and component_match_add_typed().
>   */
> -void component_match_add_release(struct device *master,
> +void component_match_add_release(struct device *parent,
>         struct component_match **matchptr,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *), void *compare_data)
>  {
> -       __component_match_add(master, matchptr, release, compare, NULL,
> +       __component_match_add(parent, matchptr, release, compare, NULL,
>                               compare_data);
>  }
>  EXPORT_SYMBOL(component_match_add_release);
>
>  /**
>   * component_match_add_typed - add a component match entry for a typed component
> - * @master: device with the aggregate driver
> + * @parent: parent device of the aggregate driver
>   * @matchptr: pointer to the list of component matches
>   * @compare_typed: compare function to match against all typed components
>   * @compare_data: opaque pointer passed to the @compare function
> @@ -415,32 +422,33 @@ EXPORT_SYMBOL(component_match_add_release);
>   *
>   * See also component_match_add_release() and component_match_add_typed().
>   */
> -void component_match_add_typed(struct device *master,
> +void component_match_add_typed(struct device *parent,
>         struct component_match **matchptr,
>         int (*compare_typed)(struct device *, int, void *), void *compare_data)
>  {
> -       __component_match_add(master, matchptr, NULL, NULL, compare_typed,
> +       __component_match_add(parent, matchptr, NULL, NULL, compare_typed,
>                               compare_data);
>  }
>  EXPORT_SYMBOL(component_match_add_typed);
>
> -static void free_master(struct master *master)
> +static void free_aggregate_device(struct aggregate_device *adev)
>  {
> -       struct component_match *match = master->match;
> +       struct component_match *match = adev->match;
>         int i;
>
> -       component_master_debugfs_del(master);
> -       list_del(&master->node);
> +       component_master_debugfs_del(adev);
> +       list_del(&adev->node);
>
>         if (match) {
>                 for (i = 0; i < match->num; i++) {
>                         struct component *c = match->compare[i].component;
>                         if (c)
> -                               c->master = NULL;
> +                               c->adev = NULL;
>                 }
>         }
>
> -       kfree(master);
> +       ida_free(&aggregate_ida, adev->id);
> +       kfree(adev);
>  }
>
>  /**
> @@ -459,31 +467,40 @@ int component_master_add_with_match(struct device *parent,
>         const struct component_master_ops *ops,
>         struct component_match *match)
>  {
> -       struct master *master;
> -       int ret;
> +       struct aggregate_device *adev;
> +       int ret, id;
>
>         /* Reallocate the match array for its true size */
>         ret = component_match_realloc(match, match->num);
>         if (ret)
>                 return ret;
>
> -       master = kzalloc(sizeof(*master), GFP_KERNEL);
> -       if (!master)
> +       adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +       if (!adev)
>                 return -ENOMEM;
>
> -       master->parent = parent;
> -       master->ops = ops;
> -       master->match = match;
> +       id = ida_alloc(&aggregate_ida, GFP_KERNEL);
> +       if (id < 0) {
> +               kfree(adev);
> +               return id;
> +       }
> +
> +       adev->id = id;
> +       adev->parent = parent;
> +       adev->dev.parent = parent;

Don't set adev->dev.parent. You are creating a functional 1-1
dependency where none exists. The real dependencies are the 1-many
dependencies between the aggregate and the components. Use device
links to capture that and enforce proper suspend/resume and runtime PM
ordering.

-Saravana

> +       adev->ops = ops;
> +       adev->match = match;
> +       dev_set_name(&adev->dev, "aggregate%d", id);
>
> -       component_master_debugfs_add(master);
> -       /* Add to the list of available masters. */
> +       component_master_debugfs_add(adev);
> +       /* Add to the list of available aggregate devices. */
>         mutex_lock(&component_mutex);
> -       list_add(&master->node, &masters);
> +       list_add(&adev->node, &aggregate_devices);
>
> -       ret = try_to_bring_up_master(master, NULL);
> +       ret = try_to_bring_up_aggregate_device(adev, NULL);
>
>         if (ret < 0)
> -               free_master(master);
> +               free_aggregate_device(adev);
>
>         mutex_unlock(&component_mutex);
>
> @@ -503,25 +520,25 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
>  void component_master_del(struct device *parent,
>         const struct component_master_ops *ops)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>
>         mutex_lock(&component_mutex);
> -       master = __master_find(parent, ops);
> -       if (master) {
> -               take_down_master(master);
> -               free_master(master);
> +       adev = __aggregate_find(parent, ops);
> +       if (adev) {
> +               take_down_aggregate_device(adev);
> +               free_aggregate_device(adev);
>         }
>         mutex_unlock(&component_mutex);
>  }
>  EXPORT_SYMBOL_GPL(component_master_del);
>
>  static void component_unbind(struct component *component,
> -       struct master *master, void *data)
> +       struct aggregate_device *adev, void *data)
>  {
>         WARN_ON(!component->bound);
>
>         if (component->ops && component->ops->unbind)
> -               component->ops->unbind(component->dev, master->parent, data);
> +               component->ops->unbind(component->dev, adev->parent, data);
>         component->bound = false;
>
>         /* Release all resources claimed in the binding of this component */
> @@ -539,26 +556,26 @@ static void component_unbind(struct component *component,
>   */
>  void component_unbind_all(struct device *parent, void *data)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>         struct component *c;
>         size_t i;
>
>         WARN_ON(!mutex_is_locked(&component_mutex));
>
> -       master = __master_find(parent, NULL);
> -       if (!master)
> +       adev = __aggregate_find(parent, NULL);
> +       if (!adev)
>                 return;
>
>         /* 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);
> +       for (i = adev->match->num; i--; )
> +               if (!adev->match->compare[i].duplicate) {
> +                       c = adev->match->compare[i].component;
> +                       component_unbind(c, adev, data);
>                 }
>  }
>  EXPORT_SYMBOL_GPL(component_unbind_all);
>
> -static int component_bind(struct component *component, struct master *master,
> +static int component_bind(struct component *component, struct aggregate_device *adev,
>         void *data)
>  {
>         int ret;
> @@ -568,7 +585,7 @@ static int component_bind(struct component *component, struct master *master,
>          * This allows us to roll-back a failed component without
>          * affecting anything else.
>          */
> -       if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> +       if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
>                 return -ENOMEM;
>
>         /*
> @@ -577,14 +594,14 @@ static int component_bind(struct component *component, struct master *master,
>          * at the appropriate moment.
>          */
>         if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>                 return -ENOMEM;
>         }
>
> -       dev_dbg(master->parent, "binding %s (ops %ps)\n",
> +       dev_dbg(adev->parent, "binding %s (ops %ps)\n",
>                 dev_name(component->dev), component->ops);
>
> -       ret = component->ops->bind(component->dev, master->parent, data);
> +       ret = component->ops->bind(component->dev, adev->parent, data);
>         if (!ret) {
>                 component->bound = true;
>
> @@ -595,16 +612,16 @@ static int component_bind(struct component *component, struct master *master,
>                  * can clean those resources up independently.
>                  */
>                 devres_close_group(component->dev, NULL);
> -               devres_remove_group(master->parent, NULL);
> +               devres_remove_group(adev->parent, NULL);
>
> -               dev_info(master->parent, "bound %s (ops %ps)\n",
> +               dev_info(adev->parent, "bound %s (ops %ps)\n",
>                          dev_name(component->dev), component->ops);
>         } else {
>                 devres_release_group(component->dev, NULL);
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>
>                 if (ret != -EPROBE_DEFER)
> -                       dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
> +                       dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
>                                 dev_name(component->dev), component->ops, ret);
>         }
>
> @@ -622,31 +639,31 @@ static int component_bind(struct component *component, struct master *master,
>   */
>  int component_bind_all(struct device *parent, void *data)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>         struct component *c;
>         size_t i;
>         int ret = 0;
>
>         WARN_ON(!mutex_is_locked(&component_mutex));
>
> -       master = __master_find(parent, NULL);
> -       if (!master)
> +       adev = __aggregate_find(parent, NULL);
> +       if (!adev)
>                 return -EINVAL;
>
>         /* 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);
> +       for (i = 0; i < adev->match->num; i++)
> +               if (!adev->match->compare[i].duplicate) {
> +                       c = adev->match->compare[i].component;
> +                       ret = component_bind(c, adev, data);
>                         if (ret)
>                                 break;
>                 }
>
>         if (ret != 0) {
>                 for (; i > 0; i--)
> -                       if (!master->match->compare[i - 1].duplicate) {
> -                               c = master->match->compare[i - 1].component;
> -                               component_unbind(c, master, data);
> +                       if (!adev->match->compare[i - 1].duplicate) {
> +                               c = adev->match->compare[i - 1].component;
> +                               component_unbind(c, adev, data);
>                         }
>         }
>
> @@ -675,8 +692,8 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
>
>         ret = try_to_bring_up_masters(component);
>         if (ret < 0) {
> -               if (component->master)
> -                       remove_component(component->master, component);
> +               if (component->adev)
> +                       remove_component(component->adev, component);
>                 list_del(&component->node);
>
>                 kfree(component);
> @@ -757,9 +774,9 @@ void component_del(struct device *dev, const struct component_ops *ops)
>                         break;
>                 }
>
> -       if (component && component->master) {
> -               take_down_master(component->master);
> -               remove_component(component->master, component);
> +       if (component && component->adev) {
> +               take_down_aggregate_device(component->adev);
> +               remove_component(component->adev, component);
>         }
>
>         mutex_unlock(&component_mutex);
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 16de18f473d7..71bfc3862633 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -41,7 +41,7 @@ void component_del(struct device *, const struct component_ops *);
>  int component_bind_all(struct device *master, void *master_data);
>  void component_unbind_all(struct device *master, void *master_data);
>
> -struct master;
> +struct aggregate_device;
>
>  /**
>   * struct component_master_ops - callback for the aggregate driver
> --
> https://chromeos.dev
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/7] component: Introduce struct aggregate_device
@ 2021-05-20 20:20     ` Saravana Kannan
  0 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20 20:20 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Daniel Vetter, linux-arm-msm, LKML, dri-devel,
	Greg Kroah-Hartman, Russell King, freedreno

On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Replace 'struct master' with 'struct aggregate_device' and then rename
> 'master' to 'adev' everywhere in the code. While we're here, put a
> struct device inside the aggregate device so that we can register it
> with a bus_type in the next patch.
>
> The diff is large but that's because this is mostly a rename, where
> sometimes 'master' is replaced with 'adev' and other times it is
> replaced with 'parent' to indicate that the struct device that was being
> used is actually the parent of the aggregate device and driver.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/base/component.c  | 249 ++++++++++++++++++++------------------
>  include/linux/component.h |   2 +-
>  2 files changed, 134 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 5e79299f6c3f..55e30e0b0952 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -9,6 +9,7 @@
>   */
>  #include <linux/component.h>
>  #include <linux/device.h>
> +#include <linux/idr.h>
>  #include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> @@ -58,18 +59,21 @@ struct component_match {
>         struct component_match_array *compare;
>  };
>
> -struct master {
> +struct aggregate_device {
>         struct list_head node;
>         bool bound;
>
>         const struct component_master_ops *ops;
>         struct device *parent;
> +       struct device dev;
>         struct component_match *match;
> +
> +       int id;
>  };
>
>  struct component {
>         struct list_head node;
> -       struct master *master;
> +       struct aggregate_device *adev;
>         bool bound;
>
>         const struct component_ops *ops;
> @@ -79,7 +83,9 @@ struct component {
>
>  static DEFINE_MUTEX(component_mutex);
>  static LIST_HEAD(component_list);
> -static LIST_HEAD(masters);
> +static LIST_HEAD(aggregate_devices);
> +
> +static DEFINE_IDA(aggregate_ida);
>
>  #ifdef CONFIG_DEBUG_FS
>
> @@ -87,12 +93,12 @@ static struct dentry *component_debugfs_dir;
>
>  static int component_devices_show(struct seq_file *s, void *data)
>  {
> -       struct master *m = s->private;
> +       struct aggregate_device *m = s->private;
>         struct component_match *match = m->match;
>         size_t i;
>
>         mutex_lock(&component_mutex);
> -       seq_printf(s, "%-40s %20s\n", "master name", "status");
> +       seq_printf(s, "%-40s %20s\n", "aggregate_device name", "status");
>         seq_puts(s, "-------------------------------------------------------------\n");
>         seq_printf(s, "%-40s %20s\n\n",
>                    dev_name(m->parent), m->bound ? "bound" : "not bound");
> @@ -122,46 +128,46 @@ static int __init component_debug_init(void)
>
>  core_initcall(component_debug_init);
>
> -static void component_master_debugfs_add(struct master *m)
> +static void component_master_debugfs_add(struct aggregate_device *m)
>  {
>         debugfs_create_file(dev_name(m->parent), 0444, component_debugfs_dir, m,
>                             &component_devices_fops);
>  }
>
> -static void component_master_debugfs_del(struct master *m)
> +static void component_master_debugfs_del(struct aggregate_device *m)
>  {
>         debugfs_remove(debugfs_lookup(dev_name(m->parent), component_debugfs_dir));
>  }
>
>  #else
>
> -static void component_master_debugfs_add(struct master *m)
> +static void component_master_debugfs_add(struct aggregate_device *m)
>  { }
>
> -static void component_master_debugfs_del(struct master *m)
> +static void component_master_debugfs_del(struct aggregate_device *m)
>  { }
>
>  #endif
>
> -static struct master *__master_find(struct device *parent,
> +static struct aggregate_device *__aggregate_find(struct device *parent,
>         const struct component_master_ops *ops)
>  {
> -       struct master *m;
> +       struct aggregate_device *m;
>
> -       list_for_each_entry(m, &masters, node)
> +       list_for_each_entry(m, &aggregate_devices, node)
>                 if (m->parent == parent && (!ops || m->ops == ops))
>                         return m;
>
>         return NULL;
>  }
>
> -static struct component *find_component(struct master *master,
> +static struct component *find_component(struct aggregate_device *adev,
>         struct component_match_array *mc)
>  {
>         struct component *c;
>
>         list_for_each_entry(c, &component_list, node) {
> -               if (c->master && c->master != master)
> +               if (c->adev && c->adev != adev)
>                         continue;
>
>                 if (mc->compare && mc->compare(c->dev, mc->data))
> @@ -175,101 +181,102 @@ static struct component *find_component(struct master *master,
>         return NULL;
>  }
>
> -static int find_components(struct master *master)
> +static int find_components(struct aggregate_device *adev)
>  {
> -       struct component_match *match = master->match;
> +       struct component_match *match = adev->match;
>         size_t i;
>         int ret = 0;
>
>         /*
>          * Scan the array of match functions and attach
> -        * any components which are found to this master.
> +        * any components which are found to this adev.
>          */
>         for (i = 0; i < match->num; i++) {
>                 struct component_match_array *mc = &match->compare[i];
>                 struct component *c;
>
> -               dev_dbg(master->parent, "Looking for component %zu\n", i);
> +               dev_dbg(adev->parent, "Looking for component %zu\n", i);
>
>                 if (match->compare[i].component)
>                         continue;
>
> -               c = find_component(master, mc);
> +               c = find_component(adev, mc);
>                 if (!c) {
>                         ret = -ENXIO;
>                         break;
>                 }
>
> -               dev_dbg(master->parent, "found component %s, duplicate %u\n", dev_name(c->dev), !!c->master);
> +               dev_dbg(adev->parent, "found component %s, duplicate %u\n",
> +                       dev_name(c->dev), !!c->adev);
>
> -               /* Attach this component to the master */
> -               match->compare[i].duplicate = !!c->master;
> +               /* Attach this component to the adev */
> +               match->compare[i].duplicate = !!c->adev;
>                 match->compare[i].component = c;
> -               c->master = master;
> +               c->adev = adev;
>         }
>         return ret;
>  }
>
>  /* Detach component from associated master */
> -static void remove_component(struct master *master, struct component *c)
> +static void remove_component(struct aggregate_device *adev, struct component *c)
>  {
>         size_t i;
>
> -       /* 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;
> +       /* Detach the component from this adev. */
> +       for (i = 0; i < adev->match->num; i++)
> +               if (adev->match->compare[i].component == c)
> +                       adev->match->compare[i].component = NULL;
>  }
>
>  /*
> - * Try to bring up a master.  If component is NULL, we're interested in
> - * this master, otherwise it's a component which must be present to try
> - * and bring up the master.
> + * Try to bring up an aggregate device.  If component is NULL, we're interested
> + * in this aggregate device, otherwise it's a component which must be present
> + * to try and bring up the aggregate device.
>   *
>   * Returns 1 for successful bringup, 0 if not ready, or -ve errno.
>   */
> -static int try_to_bring_up_master(struct master *master,
> +static int try_to_bring_up_aggregate_device(struct aggregate_device *adev,
>         struct component *component)
>  {
>         int ret;
>
> -       dev_dbg(master->parent, "trying to bring up master\n");
> +       dev_dbg(adev->parent, "trying to bring up adev\n");
>
> -       if (find_components(master)) {
> -               dev_dbg(master->parent, "master has incomplete components\n");
> +       if (find_components(adev)) {
> +               dev_dbg(adev->parent, "master has incomplete components\n");
>                 return 0;
>         }
>
> -       if (component && component->master != master) {
> -               dev_dbg(master->parent, "master is not for this component (%s)\n",
> +       if (component && component->adev != adev) {
> +               dev_dbg(adev->parent, "master is not for this component (%s)\n",
>                         dev_name(component->dev));
>                 return 0;
>         }
>
> -       if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> +       if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
>                 return -ENOMEM;
>
>         /* Found all components */
> -       ret = master->ops->bind(master->parent);
> +       ret = adev->ops->bind(adev->parent);
>         if (ret < 0) {
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>                 if (ret != -EPROBE_DEFER)
> -                       dev_info(master->parent, "master bind failed: %d\n", ret);
> +                       dev_info(adev->parent, "adev bind failed: %d\n", ret);
>                 return ret;
>         }
>
> -       master->bound = true;
> +       adev->bound = true;
>         return 1;
>  }
>
>  static int try_to_bring_up_masters(struct component *component)
>  {
> -       struct master *m;
> +       struct aggregate_device *adev;
>         int ret = 0;
>
> -       list_for_each_entry(m, &masters, node) {
> -               if (!m->bound) {
> -                       ret = try_to_bring_up_master(m, component);
> +       list_for_each_entry(adev, &aggregate_devices, node) {
> +               if (!adev->bound) {
> +                       ret = try_to_bring_up_aggregate_device(adev, component);
>                         if (ret != 0)
>                                 break;
>                 }
> @@ -278,12 +285,12 @@ static int try_to_bring_up_masters(struct component *component)
>         return ret;
>  }
>
> -static void take_down_master(struct master *master)
> +static void take_down_aggregate_device(struct aggregate_device *adev)
>  {
> -       if (master->bound) {
> -               master->ops->unbind(master->parent);
> -               devres_release_group(master->parent, NULL);
> -               master->bound = false;
> +       if (adev->bound) {
> +               adev->ops->unbind(adev->parent);
> +               devres_release_group(adev->parent, NULL);
> +               adev->bound = false;
>         }
>  }
>
> @@ -324,7 +331,7 @@ static int component_match_realloc(struct component_match *match, size_t num)
>         return 0;
>  }
>
> -static void __component_match_add(struct device *master,
> +static void __component_match_add(struct device *parent,
>         struct component_match **matchptr,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *),
> @@ -344,7 +351,7 @@ static void __component_match_add(struct device *master,
>                         return;
>                 }
>
> -               devres_add(master, match);
> +               devres_add(parent, match);
>
>                 *matchptr = match;
>         }
> @@ -370,13 +377,13 @@ static void __component_match_add(struct device *master,
>
>  /**
>   * component_match_add_release - add a component match entry with release callback
> - * @master: device with the aggregate driver
> + * @parent: parent device of the aggregate driver
>   * @matchptr: pointer to the list of component matches
>   * @release: release function for @compare_data
>   * @compare: compare function to match against all components
>   * @compare_data: opaque pointer passed to the @compare function
>   *
> - * Adds a new component match to the list stored in @matchptr, which the @master
> + * Adds a new component match to the list stored in @matchptr, which the
>   * aggregate driver needs to function. The list of component matches pointed to
>   * by @matchptr must be initialized to NULL before adding the first match. This
>   * only matches against components added with component_add().
> @@ -388,19 +395,19 @@ static void __component_match_add(struct device *master,
>   *
>   * See also component_match_add() and component_match_add_typed().
>   */
> -void component_match_add_release(struct device *master,
> +void component_match_add_release(struct device *parent,
>         struct component_match **matchptr,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *), void *compare_data)
>  {
> -       __component_match_add(master, matchptr, release, compare, NULL,
> +       __component_match_add(parent, matchptr, release, compare, NULL,
>                               compare_data);
>  }
>  EXPORT_SYMBOL(component_match_add_release);
>
>  /**
>   * component_match_add_typed - add a component match entry for a typed component
> - * @master: device with the aggregate driver
> + * @parent: parent device of the aggregate driver
>   * @matchptr: pointer to the list of component matches
>   * @compare_typed: compare function to match against all typed components
>   * @compare_data: opaque pointer passed to the @compare function
> @@ -415,32 +422,33 @@ EXPORT_SYMBOL(component_match_add_release);
>   *
>   * See also component_match_add_release() and component_match_add_typed().
>   */
> -void component_match_add_typed(struct device *master,
> +void component_match_add_typed(struct device *parent,
>         struct component_match **matchptr,
>         int (*compare_typed)(struct device *, int, void *), void *compare_data)
>  {
> -       __component_match_add(master, matchptr, NULL, NULL, compare_typed,
> +       __component_match_add(parent, matchptr, NULL, NULL, compare_typed,
>                               compare_data);
>  }
>  EXPORT_SYMBOL(component_match_add_typed);
>
> -static void free_master(struct master *master)
> +static void free_aggregate_device(struct aggregate_device *adev)
>  {
> -       struct component_match *match = master->match;
> +       struct component_match *match = adev->match;
>         int i;
>
> -       component_master_debugfs_del(master);
> -       list_del(&master->node);
> +       component_master_debugfs_del(adev);
> +       list_del(&adev->node);
>
>         if (match) {
>                 for (i = 0; i < match->num; i++) {
>                         struct component *c = match->compare[i].component;
>                         if (c)
> -                               c->master = NULL;
> +                               c->adev = NULL;
>                 }
>         }
>
> -       kfree(master);
> +       ida_free(&aggregate_ida, adev->id);
> +       kfree(adev);
>  }
>
>  /**
> @@ -459,31 +467,40 @@ int component_master_add_with_match(struct device *parent,
>         const struct component_master_ops *ops,
>         struct component_match *match)
>  {
> -       struct master *master;
> -       int ret;
> +       struct aggregate_device *adev;
> +       int ret, id;
>
>         /* Reallocate the match array for its true size */
>         ret = component_match_realloc(match, match->num);
>         if (ret)
>                 return ret;
>
> -       master = kzalloc(sizeof(*master), GFP_KERNEL);
> -       if (!master)
> +       adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +       if (!adev)
>                 return -ENOMEM;
>
> -       master->parent = parent;
> -       master->ops = ops;
> -       master->match = match;
> +       id = ida_alloc(&aggregate_ida, GFP_KERNEL);
> +       if (id < 0) {
> +               kfree(adev);
> +               return id;
> +       }
> +
> +       adev->id = id;
> +       adev->parent = parent;
> +       adev->dev.parent = parent;

Don't set adev->dev.parent. You are creating a functional 1-1
dependency where none exists. The real dependencies are the 1-many
dependencies between the aggregate and the components. Use device
links to capture that and enforce proper suspend/resume and runtime PM
ordering.

-Saravana

> +       adev->ops = ops;
> +       adev->match = match;
> +       dev_set_name(&adev->dev, "aggregate%d", id);
>
> -       component_master_debugfs_add(master);
> -       /* Add to the list of available masters. */
> +       component_master_debugfs_add(adev);
> +       /* Add to the list of available aggregate devices. */
>         mutex_lock(&component_mutex);
> -       list_add(&master->node, &masters);
> +       list_add(&adev->node, &aggregate_devices);
>
> -       ret = try_to_bring_up_master(master, NULL);
> +       ret = try_to_bring_up_aggregate_device(adev, NULL);
>
>         if (ret < 0)
> -               free_master(master);
> +               free_aggregate_device(adev);
>
>         mutex_unlock(&component_mutex);
>
> @@ -503,25 +520,25 @@ EXPORT_SYMBOL_GPL(component_master_add_with_match);
>  void component_master_del(struct device *parent,
>         const struct component_master_ops *ops)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>
>         mutex_lock(&component_mutex);
> -       master = __master_find(parent, ops);
> -       if (master) {
> -               take_down_master(master);
> -               free_master(master);
> +       adev = __aggregate_find(parent, ops);
> +       if (adev) {
> +               take_down_aggregate_device(adev);
> +               free_aggregate_device(adev);
>         }
>         mutex_unlock(&component_mutex);
>  }
>  EXPORT_SYMBOL_GPL(component_master_del);
>
>  static void component_unbind(struct component *component,
> -       struct master *master, void *data)
> +       struct aggregate_device *adev, void *data)
>  {
>         WARN_ON(!component->bound);
>
>         if (component->ops && component->ops->unbind)
> -               component->ops->unbind(component->dev, master->parent, data);
> +               component->ops->unbind(component->dev, adev->parent, data);
>         component->bound = false;
>
>         /* Release all resources claimed in the binding of this component */
> @@ -539,26 +556,26 @@ static void component_unbind(struct component *component,
>   */
>  void component_unbind_all(struct device *parent, void *data)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>         struct component *c;
>         size_t i;
>
>         WARN_ON(!mutex_is_locked(&component_mutex));
>
> -       master = __master_find(parent, NULL);
> -       if (!master)
> +       adev = __aggregate_find(parent, NULL);
> +       if (!adev)
>                 return;
>
>         /* 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);
> +       for (i = adev->match->num; i--; )
> +               if (!adev->match->compare[i].duplicate) {
> +                       c = adev->match->compare[i].component;
> +                       component_unbind(c, adev, data);
>                 }
>  }
>  EXPORT_SYMBOL_GPL(component_unbind_all);
>
> -static int component_bind(struct component *component, struct master *master,
> +static int component_bind(struct component *component, struct aggregate_device *adev,
>         void *data)
>  {
>         int ret;
> @@ -568,7 +585,7 @@ static int component_bind(struct component *component, struct master *master,
>          * This allows us to roll-back a failed component without
>          * affecting anything else.
>          */
> -       if (!devres_open_group(master->parent, NULL, GFP_KERNEL))
> +       if (!devres_open_group(adev->parent, NULL, GFP_KERNEL))
>                 return -ENOMEM;
>
>         /*
> @@ -577,14 +594,14 @@ static int component_bind(struct component *component, struct master *master,
>          * at the appropriate moment.
>          */
>         if (!devres_open_group(component->dev, component, GFP_KERNEL)) {
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>                 return -ENOMEM;
>         }
>
> -       dev_dbg(master->parent, "binding %s (ops %ps)\n",
> +       dev_dbg(adev->parent, "binding %s (ops %ps)\n",
>                 dev_name(component->dev), component->ops);
>
> -       ret = component->ops->bind(component->dev, master->parent, data);
> +       ret = component->ops->bind(component->dev, adev->parent, data);
>         if (!ret) {
>                 component->bound = true;
>
> @@ -595,16 +612,16 @@ static int component_bind(struct component *component, struct master *master,
>                  * can clean those resources up independently.
>                  */
>                 devres_close_group(component->dev, NULL);
> -               devres_remove_group(master->parent, NULL);
> +               devres_remove_group(adev->parent, NULL);
>
> -               dev_info(master->parent, "bound %s (ops %ps)\n",
> +               dev_info(adev->parent, "bound %s (ops %ps)\n",
>                          dev_name(component->dev), component->ops);
>         } else {
>                 devres_release_group(component->dev, NULL);
> -               devres_release_group(master->parent, NULL);
> +               devres_release_group(adev->parent, NULL);
>
>                 if (ret != -EPROBE_DEFER)
> -                       dev_err(master->parent, "failed to bind %s (ops %ps): %d\n",
> +                       dev_err(adev->parent, "failed to bind %s (ops %ps): %d\n",
>                                 dev_name(component->dev), component->ops, ret);
>         }
>
> @@ -622,31 +639,31 @@ static int component_bind(struct component *component, struct master *master,
>   */
>  int component_bind_all(struct device *parent, void *data)
>  {
> -       struct master *master;
> +       struct aggregate_device *adev;
>         struct component *c;
>         size_t i;
>         int ret = 0;
>
>         WARN_ON(!mutex_is_locked(&component_mutex));
>
> -       master = __master_find(parent, NULL);
> -       if (!master)
> +       adev = __aggregate_find(parent, NULL);
> +       if (!adev)
>                 return -EINVAL;
>
>         /* 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);
> +       for (i = 0; i < adev->match->num; i++)
> +               if (!adev->match->compare[i].duplicate) {
> +                       c = adev->match->compare[i].component;
> +                       ret = component_bind(c, adev, data);
>                         if (ret)
>                                 break;
>                 }
>
>         if (ret != 0) {
>                 for (; i > 0; i--)
> -                       if (!master->match->compare[i - 1].duplicate) {
> -                               c = master->match->compare[i - 1].component;
> -                               component_unbind(c, master, data);
> +                       if (!adev->match->compare[i - 1].duplicate) {
> +                               c = adev->match->compare[i - 1].component;
> +                               component_unbind(c, adev, data);
>                         }
>         }
>
> @@ -675,8 +692,8 @@ static int __component_add(struct device *dev, const struct component_ops *ops,
>
>         ret = try_to_bring_up_masters(component);
>         if (ret < 0) {
> -               if (component->master)
> -                       remove_component(component->master, component);
> +               if (component->adev)
> +                       remove_component(component->adev, component);
>                 list_del(&component->node);
>
>                 kfree(component);
> @@ -757,9 +774,9 @@ void component_del(struct device *dev, const struct component_ops *ops)
>                         break;
>                 }
>
> -       if (component && component->master) {
> -               take_down_master(component->master);
> -               remove_component(component->master, component);
> +       if (component && component->adev) {
> +               take_down_aggregate_device(component->adev);
> +               remove_component(component->adev, component);
>         }
>
>         mutex_unlock(&component_mutex);
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 16de18f473d7..71bfc3862633 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -41,7 +41,7 @@ void component_del(struct device *, const struct component_ops *);
>  int component_bind_all(struct device *master, void *master_data);
>  void component_unbind_all(struct device *master, void *master_data);
>
> -struct master;
> +struct aggregate_device;
>
>  /**
>   * struct component_master_ops - callback for the aggregate driver
> --
> https://chromeos.dev
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/7] drm/msm: Migrate to aggregate driver
  2021-05-20 19:58     ` Daniel Vetter
@ 2021-05-20 20:22       ` Saravana Kannan
  -1 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20 20:22 UTC (permalink / raw)
  To: Stephen Boyd, Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel,
	freedreno, Rafael J. Wysocki, Rob Clark, Russell King,
	Saravana Kannan
  Cc: Daniel Vetter

On Thu, May 20, 2021 at 12:58 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> > The device lists are poorly ordered when the component device code is
> > used. This is because component_master_add_with_match() returns 0
> > regardless of component devices calling component_add() first. It can
> > really only fail if an allocation fails, in which case everything is
> > going bad and we're out of memory. The driver that registers the
> > aggregate driver, can succeed at probe and put the attached device on
> > the DPM lists before any of the component devices are probed and put on
> > the lists.
> >
> > Within the component device framework this usually isn't that bad
> > because the real driver work is done at bind time via
> > component{,master}_ops::bind(). It becomes a problem when the driver
> > core, or host driver, wants to operate on the component device outside
> > of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
> > driver core doesn't understand the relationship between the host device
> > and the component devices and could possibly try to operate on component
> > devices when they're already removed from the system or shut down.
> >
> > Normally, device links or probe defer would reorder the lists and put
> > devices that depend on other devices in the lists at the correct
> > location, but with component devices this doesn't happen because this
> > information isn't expressed anywhere. Drivers simply succeed at
> > registering their component or the aggregate driver with the component
> > framework and wait for their bind() callback to be called once the other
> > components are ready. In summary, the drivers that make up the aggregate
> > driver can probe in any order.
> >
> > This ordering problem becomes fairly obvious when shutting down the
> > device with a DSI controller connected to a DSI bridge that is
> > controlled via i2c. In this case, the msm display driver wants to tear
> > down the display pipeline on shutdown via msm_pdev_shutdown() by calling
> > drm_atomic_helper_shutdown(), and it can't do that unless the whole
> > display chain is still probed and active in the system. When a display
> > bridge is on i2c, the i2c device for the bridge will be created whenever
> > the i2c controller probes, which could be before or after the msm
> > display driver probes. If the i2c controller probes after the display
> > driver, then the i2c controller will be shutdown before the display
> > controller during system wide shutdown and thus i2c transactions will
> > stop working before the display pipeline is shut down. This means we'll
> > have the display bridge trying to access an i2c bus that's shut down
> > because drm_atomic_helper_shutdown() is trying to disable the bridge
> > after the bridge is off.
> >
> > The solution is to make the aggregate driver into a real struct driver
> > that is bound to a device when the other component devices have all
> > probed. Now that the component driver code is a proper bus, we can
> > simply register an aggregate driver with that bus via
> > component_aggregate_register() and then attach the shutdown hook to that
> > driver to be sure that the shutdown for the display pipeline is called
> > before any of the component device driver shutdown hooks are called.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >
> > As stated in the cover letter, this isn't perfect but it still works. I
> > get a warning from runtime PM that the parent device (e00000.mdss) is
> > not runtime PM enabled but the child device (the aggregate device) is
> > being enabled by the bus logic. I need to move around the place that the
> > parent device is runtime PM enabled and probably keep it powered up
> > during the entire time that the driver is probed until the aggregate
> > driver probes.
> >
> >  drivers/gpu/drm/msm/msm_drv.c | 47 +++++++++++++++++++----------------
> >  1 file changed, 26 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index e1104d2454e2..0c64e6a2ce25 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1265,19 +1265,35 @@ static int add_gpu_components(struct device *dev,
> >       return 0;
> >  }
> >
> > -static int msm_drm_bind(struct device *dev)
> > +static int msm_drm_bind(struct aggregate_device *adev)
> >  {
> > -     return msm_drm_init(dev, &msm_driver);
> > +     return msm_drm_init(adev->dev.parent, &msm_driver);
> >  }
> >
> > -static void msm_drm_unbind(struct device *dev)
> > +static void msm_drm_unbind(struct aggregate_device *adev)
> >  {
> > -     msm_drm_uninit(dev);
> > +     msm_drm_uninit(adev->dev.parent);
> > +}
> > +
> > +static void msm_drm_shutdown(struct aggregate_device *adev)
> > +{
> > +     struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->dev.parent));
> > +     struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> > +
> > +     if (!priv || !priv->kms)
> > +             return;
> > +
> > +     drm_atomic_helper_shutdown(drm);
> >  }
> >
> > -static const struct component_master_ops msm_drm_ops = {
> > -     .bind = msm_drm_bind,
> > -     .unbind = msm_drm_unbind,
> > +static struct aggregate_driver msm_drm_aggregate_driver = {
> > +     .probe = msm_drm_bind,
> > +     .remove = msm_drm_unbind,
> > +     .shutdown = msm_drm_shutdown,
> > +     .driver = {
> > +             .name   = "msm_drm",
> > +             .owner  = THIS_MODULE,
> > +     },
> >  };
> >
> >  /*
> > @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > -     ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
> > +     msm_drm_aggregate_driver.match = match;
>
> This is a bit awkward design, because it means the driver struct can't be
> made const, and it will blow up when you have multiple instance of the
> same driver. I think the match should stay as part of the register
> function call, and be stored in the aggregate_device struct somewhere.
>
> Otherwise I think this looks really solid and fixes your issue properly.
> Obviously needs careful review from Greg KH for the device model side of
> things, and from Rafael Wysocki for pm side.
>
> Bunch of thoughts from a very cursory reading:
>
> - I think it'd be good if we pass the aggregate_device to all components
>   when we bind them, plus the void * parameter just to make this less
>   disruptive. Even more device model goodies.
>
> - Maybe splatter a pile of sysfs links around so that this all becomes
>   visible? Could be interesting for debugging ordering issues. Just an
>   idea, feel free to entirely ignore.

Device links give this for free too! You can check what components
make up a device and their state too.

-Saravana

>
> - Needs solid kerneldoc for everything exposed to drivers and good
>   overview DOC:
>
> - Needs deprecation warnings in the kerneldoc for all the
>   component_master_* and if feasible with a mechanical conversion,
>   converting existing users. I'd like to not be stuck with the old model
>   forever, plus this will give a pile more people to review this code
>   here.
>
> Anyway the name changes in probe and remove hooks below are already worth
> this on their own imo. That's why I'd like to see them in all drivers.
>
> Cheers, Daniel
>
> > +     ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver);
> >       if (ret)
> >               goto fail;
> >
> > @@ -1319,23 +1336,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
> >
> >  static int msm_pdev_remove(struct platform_device *pdev)
> >  {
> > -     component_master_del(&pdev->dev, &msm_drm_ops);
> > +     component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
> >       of_platform_depopulate(&pdev->dev);
> >
> >       return 0;
> >  }
> >
> > -static void msm_pdev_shutdown(struct platform_device *pdev)
> > -{
> > -     struct drm_device *drm = platform_get_drvdata(pdev);
> > -     struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> > -
> > -     if (!priv || !priv->kms)
> > -             return;
> > -
> > -     drm_atomic_helper_shutdown(drm);
> > -}
> > -
> >  static const struct of_device_id dt_match[] = {
> >       { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
> >       { .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> > @@ -1351,7 +1357,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
> >  static struct platform_driver msm_platform_driver = {
> >       .probe      = msm_pdev_probe,
> >       .remove     = msm_pdev_remove,
> > -     .shutdown   = msm_pdev_shutdown,
> >       .driver     = {
> >               .name   = "msm",
> >               .of_match_table = dt_match,
> > --
> > https://chromeos.dev
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/7] drm/msm: Migrate to aggregate driver
@ 2021-05-20 20:22       ` Saravana Kannan
  0 siblings, 0 replies; 34+ messages in thread
From: Saravana Kannan @ 2021-05-20 20:22 UTC (permalink / raw)
  To: Stephen Boyd, Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel,
	freedreno, Rafael J. Wysocki, Rob Clark, Russell King,
	Saravana Kannan
  Cc: Daniel Vetter

On Thu, May 20, 2021 at 12:58 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> > The device lists are poorly ordered when the component device code is
> > used. This is because component_master_add_with_match() returns 0
> > regardless of component devices calling component_add() first. It can
> > really only fail if an allocation fails, in which case everything is
> > going bad and we're out of memory. The driver that registers the
> > aggregate driver, can succeed at probe and put the attached device on
> > the DPM lists before any of the component devices are probed and put on
> > the lists.
> >
> > Within the component device framework this usually isn't that bad
> > because the real driver work is done at bind time via
> > component{,master}_ops::bind(). It becomes a problem when the driver
> > core, or host driver, wants to operate on the component device outside
> > of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
> > driver core doesn't understand the relationship between the host device
> > and the component devices and could possibly try to operate on component
> > devices when they're already removed from the system or shut down.
> >
> > Normally, device links or probe defer would reorder the lists and put
> > devices that depend on other devices in the lists at the correct
> > location, but with component devices this doesn't happen because this
> > information isn't expressed anywhere. Drivers simply succeed at
> > registering their component or the aggregate driver with the component
> > framework and wait for their bind() callback to be called once the other
> > components are ready. In summary, the drivers that make up the aggregate
> > driver can probe in any order.
> >
> > This ordering problem becomes fairly obvious when shutting down the
> > device with a DSI controller connected to a DSI bridge that is
> > controlled via i2c. In this case, the msm display driver wants to tear
> > down the display pipeline on shutdown via msm_pdev_shutdown() by calling
> > drm_atomic_helper_shutdown(), and it can't do that unless the whole
> > display chain is still probed and active in the system. When a display
> > bridge is on i2c, the i2c device for the bridge will be created whenever
> > the i2c controller probes, which could be before or after the msm
> > display driver probes. If the i2c controller probes after the display
> > driver, then the i2c controller will be shutdown before the display
> > controller during system wide shutdown and thus i2c transactions will
> > stop working before the display pipeline is shut down. This means we'll
> > have the display bridge trying to access an i2c bus that's shut down
> > because drm_atomic_helper_shutdown() is trying to disable the bridge
> > after the bridge is off.
> >
> > The solution is to make the aggregate driver into a real struct driver
> > that is bound to a device when the other component devices have all
> > probed. Now that the component driver code is a proper bus, we can
> > simply register an aggregate driver with that bus via
> > component_aggregate_register() and then attach the shutdown hook to that
> > driver to be sure that the shutdown for the display pipeline is called
> > before any of the component device driver shutdown hooks are called.
> >
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > Cc: Saravana Kannan <saravanak@google.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >
> > As stated in the cover letter, this isn't perfect but it still works. I
> > get a warning from runtime PM that the parent device (e00000.mdss) is
> > not runtime PM enabled but the child device (the aggregate device) is
> > being enabled by the bus logic. I need to move around the place that the
> > parent device is runtime PM enabled and probably keep it powered up
> > during the entire time that the driver is probed until the aggregate
> > driver probes.
> >
> >  drivers/gpu/drm/msm/msm_drv.c | 47 +++++++++++++++++++----------------
> >  1 file changed, 26 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index e1104d2454e2..0c64e6a2ce25 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1265,19 +1265,35 @@ static int add_gpu_components(struct device *dev,
> >       return 0;
> >  }
> >
> > -static int msm_drm_bind(struct device *dev)
> > +static int msm_drm_bind(struct aggregate_device *adev)
> >  {
> > -     return msm_drm_init(dev, &msm_driver);
> > +     return msm_drm_init(adev->dev.parent, &msm_driver);
> >  }
> >
> > -static void msm_drm_unbind(struct device *dev)
> > +static void msm_drm_unbind(struct aggregate_device *adev)
> >  {
> > -     msm_drm_uninit(dev);
> > +     msm_drm_uninit(adev->dev.parent);
> > +}
> > +
> > +static void msm_drm_shutdown(struct aggregate_device *adev)
> > +{
> > +     struct drm_device *drm = platform_get_drvdata(to_platform_device(adev->dev.parent));
> > +     struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> > +
> > +     if (!priv || !priv->kms)
> > +             return;
> > +
> > +     drm_atomic_helper_shutdown(drm);
> >  }
> >
> > -static const struct component_master_ops msm_drm_ops = {
> > -     .bind = msm_drm_bind,
> > -     .unbind = msm_drm_unbind,
> > +static struct aggregate_driver msm_drm_aggregate_driver = {
> > +     .probe = msm_drm_bind,
> > +     .remove = msm_drm_unbind,
> > +     .shutdown = msm_drm_shutdown,
> > +     .driver = {
> > +             .name   = "msm_drm",
> > +             .owner  = THIS_MODULE,
> > +     },
> >  };
> >
> >  /*
> > @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > -     ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
> > +     msm_drm_aggregate_driver.match = match;
>
> This is a bit awkward design, because it means the driver struct can't be
> made const, and it will blow up when you have multiple instance of the
> same driver. I think the match should stay as part of the register
> function call, and be stored in the aggregate_device struct somewhere.
>
> Otherwise I think this looks really solid and fixes your issue properly.
> Obviously needs careful review from Greg KH for the device model side of
> things, and from Rafael Wysocki for pm side.
>
> Bunch of thoughts from a very cursory reading:
>
> - I think it'd be good if we pass the aggregate_device to all components
>   when we bind them, plus the void * parameter just to make this less
>   disruptive. Even more device model goodies.
>
> - Maybe splatter a pile of sysfs links around so that this all becomes
>   visible? Could be interesting for debugging ordering issues. Just an
>   idea, feel free to entirely ignore.

Device links give this for free too! You can check what components
make up a device and their state too.

-Saravana

>
> - Needs solid kerneldoc for everything exposed to drivers and good
>   overview DOC:
>
> - Needs deprecation warnings in the kerneldoc for all the
>   component_master_* and if feasible with a mechanical conversion,
>   converting existing users. I'd like to not be stuck with the old model
>   forever, plus this will give a pile more people to review this code
>   here.
>
> Anyway the name changes in probe and remove hooks below are already worth
> this on their own imo. That's why I'd like to see them in all drivers.
>
> Cheers, Daniel
>
> > +     ret = component_aggregate_register(&pdev->dev, &msm_drm_aggregate_driver);
> >       if (ret)
> >               goto fail;
> >
> > @@ -1319,23 +1336,12 @@ static int msm_pdev_probe(struct platform_device *pdev)
> >
> >  static int msm_pdev_remove(struct platform_device *pdev)
> >  {
> > -     component_master_del(&pdev->dev, &msm_drm_ops);
> > +     component_aggregate_unregister(&pdev->dev, &msm_drm_aggregate_driver);
> >       of_platform_depopulate(&pdev->dev);
> >
> >       return 0;
> >  }
> >
> > -static void msm_pdev_shutdown(struct platform_device *pdev)
> > -{
> > -     struct drm_device *drm = platform_get_drvdata(pdev);
> > -     struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> > -
> > -     if (!priv || !priv->kms)
> > -             return;
> > -
> > -     drm_atomic_helper_shutdown(drm);
> > -}
> > -
> >  static const struct of_device_id dt_match[] = {
> >       { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 },
> >       { .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 },
> > @@ -1351,7 +1357,6 @@ MODULE_DEVICE_TABLE(of, dt_match);
> >  static struct platform_driver msm_platform_driver = {
> >       .probe      = msm_pdev_probe,
> >       .remove     = msm_pdev_remove,
> > -     .shutdown   = msm_pdev_shutdown,
> >       .driver     = {
> >               .name   = "msm",
> >               .of_match_table = dt_match,
> > --
> > https://chromeos.dev
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/7] component: Introduce struct aggregate_device
  2021-05-20 20:20     ` Saravana Kannan
@ 2021-05-24  6:01       ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-24  6:01 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Greg Kroah-Hartman, LKML, linux-arm-msm, dri-devel, freedreno,
	Daniel Vetter, Rafael J. Wysocki, Rob Clark, Russell King

Quoting Saravana Kannan (2021-05-20 13:20:45)
> On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > -       master->parent = parent;
> > -       master->ops = ops;
> > -       master->match = match;
> > +       id = ida_alloc(&aggregate_ida, GFP_KERNEL);
> > +       if (id < 0) {
> > +               kfree(adev);
> > +               return id;
> > +       }
> > +
> > +       adev->id = id;
> > +       adev->parent = parent;
> > +       adev->dev.parent = parent;
>
> Don't set adev->dev.parent. You are creating a functional 1-1
> dependency where none exists. The real dependencies are the 1-many
> dependencies between the aggregate and the components. Use device
> links to capture that and enforce proper suspend/resume and runtime PM
> ordering.
>

Ah ok. Yeah it seems like that was the thing causing me runtime PM
problems. I've removed the parent patch from this series now and I'll
look at working in the device links now.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 3/7] component: Introduce struct aggregate_device
@ 2021-05-24  6:01       ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-24  6:01 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Rafael J. Wysocki, Daniel Vetter, linux-arm-msm, LKML, dri-devel,
	Greg Kroah-Hartman, Russell King, freedreno

Quoting Saravana Kannan (2021-05-20 13:20:45)
> On Wed, May 19, 2021 at 5:25 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > -       master->parent = parent;
> > -       master->ops = ops;
> > -       master->match = match;
> > +       id = ida_alloc(&aggregate_ida, GFP_KERNEL);
> > +       if (id < 0) {
> > +               kfree(adev);
> > +               return id;
> > +       }
> > +
> > +       adev->id = id;
> > +       adev->parent = parent;
> > +       adev->dev.parent = parent;
>
> Don't set adev->dev.parent. You are creating a functional 1-1
> dependency where none exists. The real dependencies are the 1-many
> dependencies between the aggregate and the components. Use device
> links to capture that and enforce proper suspend/resume and runtime PM
> ordering.
>

Ah ok. Yeah it seems like that was the thing causing me runtime PM
problems. I've removed the parent patch from this series now and I'll
look at working in the device links now.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/7] drm/msm: Migrate to aggregate driver
  2021-05-20 19:58     ` Daniel Vetter
@ 2021-05-24  6:45       ` Stephen Boyd
  -1 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-24  6:45 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Greg Kroah-Hartman, linux-kernel, linux-arm-msm, dri-devel,
	freedreno, Daniel Vetter, Rafael J. Wysocki, Rob Clark,
	Russell King, Saravana Kannan

Quoting Daniel Vetter (2021-05-20 12:58:52)
> On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> > @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > -     ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
> > +     msm_drm_aggregate_driver.match = match;
>
> This is a bit awkward design, because it means the driver struct can't be
> made const, and it will blow up when you have multiple instance of the
> same driver. I think the match should stay as part of the register
> function call, and be stored in the aggregate_device struct somewhere.

Got it. The driver struct can't be const for other reasons but I agree
it is awkward. I'm currently using the match pointer to figure out if
this aggregate driver is related to the aggregate device. The match
pointer is already stored in the aggregate_device. The problem is we
need to know if some driver is associated with some aggregate_device,
and so I took the easy way out and registered the aggregate_device at
the same time that the aggregate_driver is registered and stashed the
match pointer in both structures to match them up later during driver
binding.

If we want to support multiple aggregate_devices for an aggregate_driver
then I'll have to come up with some other way of associating the
aggregate_devices created in the component code with the aggregate
driver that registered it. I suppose a list of aggregate_devices will
work. Is any sort of driver doing this right now and registering the
bind/unbind ops with multiple devices? I just wonder if there's any
point in doing it if it will always be a 1:1 relationship between
aggregate device and driver.

>
> Otherwise I think this looks really solid and fixes your issue properly.
> Obviously needs careful review from Greg KH for the device model side of
> things, and from Rafael Wysocki for pm side.

Yeah apparently it fixes my issue because the aggregate_device is added
after all the other devices described in DT (and the i2c bridge) have
been added to the dpm list. Otherwise I would still have problems, but
using device links should help me guarantee the aggregate_device is in
the right location on the list. I still have to check that the i2c
bridge is linked to the DSI encoder though.

>
> Bunch of thoughts from a very cursory reading:
>
> - I think it'd be good if we pass the aggregate_device to all components
>   when we bind them, plus the void * parameter just to make this less
>   disruptive. Even more device model goodies.

So the idea is to pass aggregate_device into the struct
component_ops::{bind,unbind}() functions? Right now it takes the parent
device, so we'll need to introduce another set of function pointers for
the "modern" way of doing things in the component and then pass the
aggregate_device pointer instead of the parent. I can roll that into
another patch and then deprecate the bind/unbind function pointers.

I'll pass the aggregate_device instead of a device pointer so that
compilation will break if the code isn't migrated properly. I also see
that in the msm case the component driver probe is mostly just punted
into the component bind ops. I'd like to change that so the component
drivers get all their resources in their real probe, i.e. platform
driver probe, and then only do things related to making the graphics
card "whole" in their bind. This mostly means that power management
stuff will move out of the bind callback and into the probe callback and
then only once the power management stuff is ready will we actually
register the component device.

>
> - Maybe splatter a pile of sysfs links around so that this all becomes
>   visible? Could be interesting for debugging ordering issues. Just an
>   idea, feel free to entirely ignore.

Sure. I'll do the device link stuff from the components to the aggregate
driver and that should help, as Saravana mentioned earlier.

>
> - Needs solid kerneldoc for everything exposed to drivers and good
>   overview DOC:

Ok I'll layer that on at the end.

>
> - Needs deprecation warnings in the kerneldoc for all the
>   component_master_* and if feasible with a mechanical conversion,
>   converting existing users. I'd like to not be stuck with the old model
>   forever, plus this will give a pile more people to review this code
>   here.

Ok. I'll dust off coccinelle or just do it by hand. There aren't that
many. I hope.

>
> Anyway the name changes in probe and remove hooks below are already worth
> this on their own imo. That's why I'd like to see them in all drivers.
>

Cool, thanks for taking a look. It may take me a couple more days to get
v2 out the door and I'll have to spend a bunch of time converting more
drivers to shake out more problems.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/7] drm/msm: Migrate to aggregate driver
@ 2021-05-24  6:45       ` Stephen Boyd
  0 siblings, 0 replies; 34+ messages in thread
From: Stephen Boyd @ 2021-05-24  6:45 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Saravana Kannan, Rafael J. Wysocki, Daniel Vetter, linux-arm-msm,
	linux-kernel, dri-devel, Greg Kroah-Hartman, Russell King,
	freedreno

Quoting Daniel Vetter (2021-05-20 12:58:52)
> On Wed, May 19, 2021 at 05:25:19PM -0700, Stephen Boyd wrote:
> > @@ -1306,7 +1322,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > -     ret = component_master_add_with_match(&pdev->dev, &msm_drm_ops, match);
> > +     msm_drm_aggregate_driver.match = match;
>
> This is a bit awkward design, because it means the driver struct can't be
> made const, and it will blow up when you have multiple instance of the
> same driver. I think the match should stay as part of the register
> function call, and be stored in the aggregate_device struct somewhere.

Got it. The driver struct can't be const for other reasons but I agree
it is awkward. I'm currently using the match pointer to figure out if
this aggregate driver is related to the aggregate device. The match
pointer is already stored in the aggregate_device. The problem is we
need to know if some driver is associated with some aggregate_device,
and so I took the easy way out and registered the aggregate_device at
the same time that the aggregate_driver is registered and stashed the
match pointer in both structures to match them up later during driver
binding.

If we want to support multiple aggregate_devices for an aggregate_driver
then I'll have to come up with some other way of associating the
aggregate_devices created in the component code with the aggregate
driver that registered it. I suppose a list of aggregate_devices will
work. Is any sort of driver doing this right now and registering the
bind/unbind ops with multiple devices? I just wonder if there's any
point in doing it if it will always be a 1:1 relationship between
aggregate device and driver.

>
> Otherwise I think this looks really solid and fixes your issue properly.
> Obviously needs careful review from Greg KH for the device model side of
> things, and from Rafael Wysocki for pm side.

Yeah apparently it fixes my issue because the aggregate_device is added
after all the other devices described in DT (and the i2c bridge) have
been added to the dpm list. Otherwise I would still have problems, but
using device links should help me guarantee the aggregate_device is in
the right location on the list. I still have to check that the i2c
bridge is linked to the DSI encoder though.

>
> Bunch of thoughts from a very cursory reading:
>
> - I think it'd be good if we pass the aggregate_device to all components
>   when we bind them, plus the void * parameter just to make this less
>   disruptive. Even more device model goodies.

So the idea is to pass aggregate_device into the struct
component_ops::{bind,unbind}() functions? Right now it takes the parent
device, so we'll need to introduce another set of function pointers for
the "modern" way of doing things in the component and then pass the
aggregate_device pointer instead of the parent. I can roll that into
another patch and then deprecate the bind/unbind function pointers.

I'll pass the aggregate_device instead of a device pointer so that
compilation will break if the code isn't migrated properly. I also see
that in the msm case the component driver probe is mostly just punted
into the component bind ops. I'd like to change that so the component
drivers get all their resources in their real probe, i.e. platform
driver probe, and then only do things related to making the graphics
card "whole" in their bind. This mostly means that power management
stuff will move out of the bind callback and into the probe callback and
then only once the power management stuff is ready will we actually
register the component device.

>
> - Maybe splatter a pile of sysfs links around so that this all becomes
>   visible? Could be interesting for debugging ordering issues. Just an
>   idea, feel free to entirely ignore.

Sure. I'll do the device link stuff from the components to the aggregate
driver and that should help, as Saravana mentioned earlier.

>
> - Needs solid kerneldoc for everything exposed to drivers and good
>   overview DOC:

Ok I'll layer that on at the end.

>
> - Needs deprecation warnings in the kerneldoc for all the
>   component_master_* and if feasible with a mechanical conversion,
>   converting existing users. I'd like to not be stuck with the old model
>   forever, plus this will give a pile more people to review this code
>   here.

Ok. I'll dust off coccinelle or just do it by hand. There aren't that
many. I hope.

>
> Anyway the name changes in probe and remove hooks below are already worth
> this on their own imo. That's why I'd like to see them in all drivers.
>

Cool, thanks for taking a look. It may take me a couple more days to get
v2 out the door and I'll have to spend a bunch of time converting more
drivers to shake out more problems.

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2021-05-24  6:45 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20  0:25 [PATCH 0/7] component: Make into an aggregate bus Stephen Boyd
2021-05-20  0:25 ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 1/7] component: Drop 'dev' argument to component_match_realloc() Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 2/7] component: Rename 'dev' to 'parent' Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 3/7] component: Introduce struct aggregate_device Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20 20:20   ` Saravana Kannan
2021-05-20 20:20     ` Saravana Kannan
2021-05-24  6:01     ` Stephen Boyd
2021-05-24  6:01       ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 4/7] component: Introduce the aggregate bus_type Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 5/7] component: Use dev.parent instead of adev->parent Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 6/7] component: Move struct aggregate_device out to header file Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20  0:25 ` [PATCH 7/7] drm/msm: Migrate to aggregate driver Stephen Boyd
2021-05-20  0:25   ` Stephen Boyd
2021-05-20 19:58   ` Daniel Vetter
2021-05-20 19:58     ` Daniel Vetter
2021-05-20 20:22     ` Saravana Kannan
2021-05-20 20:22       ` Saravana Kannan
2021-05-24  6:45     ` Stephen Boyd
2021-05-24  6:45       ` Stephen Boyd
2021-05-20  1:27 ` [PATCH 0/7] component: Make into an aggregate bus Saravana Kannan
2021-05-20  1:27   ` Saravana Kannan
2021-05-20  1:41   ` Stephen Boyd
2021-05-20  1:41     ` Stephen Boyd
2021-05-20 19:30     ` Saravana Kannan
2021-05-20 19:30       ` Saravana Kannan
2021-05-20 20:03     ` Daniel Vetter
2021-05-20 20:03       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.