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 (diff)
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
next 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: linkBe 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.