All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Russell King <rmk+kernel@arm.linux.org.uk>,
	Rob Clark <robdclark@gmail.com>,
	dri-devel@lists.freedesktop.org
Subject: [PATCH] component: Move host device to end of device lists on binding
Date: Sat,  8 May 2021 00:41:18 -0700	[thread overview]
Message-ID: <20210508074118.1621729-1-swboyd@chromium.org> (raw)

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 host device (called master_dev in
the code), can succeed at probe and be put on the device 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 host with the component framework and
wait for their bind() callback to be called once the other components
are ready. We could make various device links between 'master_dev' and
'component->dev' but it's not necessary. Let's simply move the hosting
device to the end of the device lists when the component device fully
binds. This way we know that all components are present and have probed
properly and now the host device has really probed so it's safe to
assume the host driver ops can operate on any component device.

This fixes the msm display driver shutdown path when the DSI controller
is 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 at 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.

Moving the host device to the end of the lists at bind time moves the
drm_atomic_helper_shutdown() call before the i2c bus is shutdown.
This fixes the immediate problem, but we could improve it a bit by
modeling device links from the component devices to the host device
indicating that they supply something, although it is slightly different
because the consumer doesn't need the suppliers to probe to succeed.

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

diff --git a/drivers/base/component.c b/drivers/base/component.c
index dcfbe7251dc4..de645420bae2 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -15,6 +15,8 @@
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 
+#include "base.h"
+
 /**
  * DOC: overview
  *
@@ -657,6 +659,14 @@ int component_bind_all(struct device *master_dev, void *data)
 				c = master->match->compare[i - 1].component;
 				component_unbind(c, master, data);
 			}
+	} else {
+		/*
+		 * Move to the tail of the list so that master_dev driver ops
+		 * like 'shutdown' or 'remove' are called before any of the
+		 * dependencies that the components have are shutdown or
+		 * removed.
+		 */
+		device_pm_move_to_tail(master_dev);
 	}
 
 	return ret;

base-commit: 9f4ad9e425a1d3b6a34617b8ea226d56a119a717
-- 
https://chromeos.dev


WARNING: multiple messages have this Message-ID
From: Stephen Boyd <swboyd@chromium.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Russell King <rmk+kernel@arm.linux.org.uk>
Subject: [PATCH] component: Move host device to end of device lists on binding
Date: Sat,  8 May 2021 00:41:18 -0700	[thread overview]
Message-ID: <20210508074118.1621729-1-swboyd@chromium.org> (raw)

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 host device (called master_dev in
the code), can succeed at probe and be put on the device 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 host with the component framework and
wait for their bind() callback to be called once the other components
are ready. We could make various device links between 'master_dev' and
'component->dev' but it's not necessary. Let's simply move the hosting
device to the end of the device lists when the component device fully
binds. This way we know that all components are present and have probed
properly and now the host device has really probed so it's safe to
assume the host driver ops can operate on any component device.

This fixes the msm display driver shutdown path when the DSI controller
is 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 at 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.

Moving the host device to the end of the lists at bind time moves the
drm_atomic_helper_shutdown() call before the i2c bus is shutdown.
This fixes the immediate problem, but we could improve it a bit by
modeling device links from the component devices to the host device
indicating that they supply something, although it is slightly different
because the consumer doesn't need the suppliers to probe to succeed.

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

diff --git a/drivers/base/component.c b/drivers/base/component.c
index dcfbe7251dc4..de645420bae2 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -15,6 +15,8 @@
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 
+#include "base.h"
+
 /**
  * DOC: overview
  *
@@ -657,6 +659,14 @@ int component_bind_all(struct device *master_dev, void *data)
 				c = master->match->compare[i - 1].component;
 				component_unbind(c, master, data);
 			}
+	} else {
+		/*
+		 * Move to the tail of the list so that master_dev driver ops
+		 * like 'shutdown' or 'remove' are called before any of the
+		 * dependencies that the components have are shutdown or
+		 * removed.
+		 */
+		device_pm_move_to_tail(master_dev);
 	}
 
 	return ret;

base-commit: 9f4ad9e425a1d3b6a34617b8ea226d56a119a717
-- 
https://chromeos.dev


             reply	other threads:[~2021-05-08  7:41 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-08  7:41 Stephen Boyd [this message]
2021-05-08  7:41 ` [PATCH] component: Move host device to end of device lists on binding Stephen Boyd
2021-05-10 11:59 ` Rafael J. Wysocki
2021-05-10 11:59   ` Rafael J. Wysocki
2021-05-11 17:59   ` Saravana Kannan
2021-05-11 17:59     ` Saravana Kannan
2021-05-10 16:05 ` Daniel Vetter
2021-05-10 16:05   ` Daniel Vetter
2021-05-10 17:52   ` Stephen Boyd
2021-05-10 17:52     ` Stephen Boyd
2021-05-10 18:26     ` Daniel Vetter
2021-05-10 18:26       ` Daniel Vetter
2021-05-10 19:08       ` Stephen Boyd
2021-05-10 19:08         ` Stephen Boyd
2021-05-11 10:52         ` Rafael J. Wysocki
2021-05-11 10:52           ` Rafael J. Wysocki
2021-05-11 13:39           ` Daniel Vetter
2021-05-11 13:39             ` Daniel Vetter
2021-05-11 17:19             ` Stephen Boyd
2021-05-11 17:19               ` Stephen Boyd
2021-05-11 17:25               ` Daniel Vetter
2021-05-11 17:25                 ` Daniel Vetter
2021-05-11 19:12               ` Saravana Kannan
2021-05-11 19:12                 ` Saravana Kannan
2021-05-11 17:00           ` Stephen Boyd
2021-05-11 17:00             ` Stephen Boyd
2021-05-11 17:20             ` Rafael J. Wysocki
2021-05-11 17:20               ` Rafael J. Wysocki
2021-05-13 13:40   ` Russell King - ARM Linux admin
2021-05-13 13:40     ` Russell King - ARM Linux admin
2021-05-11 14:42 ` Russell King - ARM Linux admin
2021-05-11 14:42   ` Russell King - ARM Linux admin
2021-05-11 17:22   ` Stephen Boyd
2021-05-11 17:22     ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210508074118.1621729-1-swboyd@chromium.org \
    --to=swboyd@chromium.org \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=rmk+kernel@arm.linux.org.uk \
    --cc=robdclark@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.