[PATCH 0/2] Fix the shutdown callback of a drm component device

[PATCH 0/2] Fix the shutdown callback of a drm component device

[Heiko Stuebner]

Probe deferral can cause the platform-data we access to be freed already
causing ugly splats on system shutdown.

Fix this via a small component-helper and adapting the rockchip-drm
shutdown handler.

Heiko Stuebner (2):
  drivers: base: component: add function to query the bound status
  drm/rockchip: Fix shutdown when no drm-device is set up

 drivers/base/component.c                    | 14 ++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  6 ++++--
 include/linux/component.h                   |  4 +++-
 3 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.47.2

[PATCH 1/2] drivers: base: component: add function to query the bound status

[Heiko Stuebner]

The component helpers already expose the bound status in debugfs, but at
times it might be necessary to also check that state in the kernel and
act differently depending on the result.

For example the shutdown handler of a drm-driver might need to stop
a whole output pipeline if the drm device is up and running, but may
run into problems if that drm-device has never been set up before,
for example because the binding deferred.

So add a little helper that returns the bound status for a componet
device.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/base/component.c  | 14 ++++++++++++++
 include/linux/component.h |  4 +++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 741497324d78..d63e01f4851d 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -569,6 +569,20 @@ void component_master_del(struct device *parent,
 }
 EXPORT_SYMBOL_GPL(component_master_del);
 
+bool component_master_is_bound(struct device *parent,
+	const struct component_master_ops *ops)
+{
+	struct aggregate_device *adev;
+
+	guard(mutex)(&component_mutex);
+	adev = __aggregate_find(parent, ops);
+	if (!adev)
+		return 0;
+
+	return adev->bound;
+}
+EXPORT_SYMBOL_GPL(component_master_is_bound);
+
 static void component_unbind(struct component *component,
 	struct aggregate_device *adev, void *data)
 {
diff --git a/include/linux/component.h b/include/linux/component.h
index df4aa75c9e7c..9d6c66401280 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -3,7 +3,7 @@
 #define COMPONENT_H
 
 #include <linux/stddef.h>
-
+#include <linux/types.h>
 
 struct device;
 
@@ -90,6 +90,8 @@ int component_compare_dev_name(struct device *dev, void *data);
 
 void component_master_del(struct device *,
 	const struct component_master_ops *);
+bool component_master_is_bound(struct device *parent,
+	const struct component_master_ops *ops);
 
 struct component_match;
 
-- 
2.47.2

Re: [PATCH 1/2] drivers: base: component: add function to query the bound status

[Greg KH]

On Fri, Feb 21, 2025 at 12:41:40AM +0100, Heiko Stuebner wrote:
> The component helpers already expose the bound status in debugfs, but at
> times it might be necessary to also check that state in the kernel and
> act differently depending on the result.
> 
> For example the shutdown handler of a drm-driver might need to stop
> a whole output pipeline if the drm device is up and running, but may
> run into problems if that drm-device has never been set up before,
> for example because the binding deferred.
> 
> So add a little helper that returns the bound status for a componet
> device.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/base/component.c  | 14 ++++++++++++++
>  include/linux/component.h |  4 +++-
>  2 files changed, 17 insertions(+), 1 deletion(-)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/2] drivers: base: component: add function to query the bound status

[Heiko Stübner]

Hi Greg,

Am Freitag, 21. Februar 2025, 07:14:07 MEZ schrieb Greg KH:
> On Fri, Feb 21, 2025 at 12:41:40AM +0100, Heiko Stuebner wrote:
> > The component helpers already expose the bound status in debugfs, but at
> > times it might be necessary to also check that state in the kernel and
> > act differently depending on the result.
> > 
> > For example the shutdown handler of a drm-driver might need to stop
> > a whole output pipeline if the drm device is up and running, but may
> > run into problems if that drm-device has never been set up before,
> > for example because the binding deferred.
> > 
> > So add a little helper that returns the bound status for a componet
> > device.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  drivers/base/component.c  | 14 ++++++++++++++
> >  include/linux/component.h |  4 +++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

just for safety and not stepping on people's toes, does the Acked-by mean
that this patch can go together with its user through the drm-misc tree?


Thanks a lot
Heiko

Re: [PATCH 1/2] drivers: base: component: add function to query the bound status

[Greg KH]

On Thu, Feb 27, 2025 at 09:44:23AM +0100, Heiko Stübner wrote:
> Hi Greg,
> 
> Am Freitag, 21. Februar 2025, 07:14:07 MEZ schrieb Greg KH:
> > On Fri, Feb 21, 2025 at 12:41:40AM +0100, Heiko Stuebner wrote:
> > > The component helpers already expose the bound status in debugfs, but at
> > > times it might be necessary to also check that state in the kernel and
> > > act differently depending on the result.
> > > 
> > > For example the shutdown handler of a drm-driver might need to stop
> > > a whole output pipeline if the drm device is up and running, but may
> > > run into problems if that drm-device has never been set up before,
> > > for example because the binding deferred.
> > > 
> > > So add a little helper that returns the bound status for a componet
> > > device.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > >  drivers/base/component.c  | 14 ++++++++++++++
> > >  include/linux/component.h |  4 +++-
> > >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> just for safety and not stepping on people's toes, does the Acked-by mean
> that this patch can go together with its user through the drm-misc tree?

Yup, merge away!

[PATCH 2/2] drm/rockchip: Fix shutdown when no drm-device is set up

[Heiko Stuebner]

When the drm-driver probes, it mainly creates the component device, where
all the sub-drivers (vops, hdmi, etc) hook into.

This will cause the shutdown handler to get called on shutdown, even
though the drm-device might not have been set up, or the component bind
might have failed.

So use the new component helper to check whether the drm-device is up
and only then call the drm-atomic helper to release all the drm magic.

This prevents failures when the drm-device is never set, or has been
freed up already for example by a probe-defer during the component bind.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 439edc165ff6..285b721ff28a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -484,9 +484,11 @@ static void rockchip_drm_platform_remove(struct platform_device *pdev)
 
 static void rockchip_drm_platform_shutdown(struct platform_device *pdev)
 {
-	struct drm_device *drm = platform_get_drvdata(pdev);
+	if (component_master_is_bound(&pdev->dev, &rockchip_drm_ops)) {
+		struct drm_device *drm = platform_get_drvdata(pdev);
 
-	drm_atomic_helper_shutdown(drm);
+		drm_atomic_helper_shutdown(drm);
+	}
 }
 
 static const struct of_device_id rockchip_drm_dt_ids[] = {
-- 
2.47.2

Re: [PATCH 2/2] drm/rockchip: Fix shutdown when no drm-device is set up

[Nicolas Frattaroli]

On Friday, 21 February 2025 00:41:41 Central European Standard Time Heiko 
Stuebner wrote:
> When the drm-driver probes, it mainly creates the component device, where
> all the sub-drivers (vops, hdmi, etc) hook into.
> 
> This will cause the shutdown handler to get called on shutdown, even
> though the drm-device might not have been set up, or the component bind
> might have failed.
> 
> So use the new component helper to check whether the drm-device is up
> and only then call the drm-atomic helper to release all the drm magic.
> 
> This prevents failures when the drm-device is never set, or has been
> freed up already for example by a probe-defer during the component bind.
> 
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Tested on a RK3588 ROCK5B by adding a `return -EPROBE_DEFER` into the middle 
of `rockchip_drm_bind` after `component_bind_all` already ran. Without the 
patch, I get a stacktrace in `drm_atomic_helper_shutdown`. With the patch, I 
don't get one.

Re: [PATCH 0/2] Fix the shutdown callback of a drm component device

[Heiko Stuebner]

On Fri, 21 Feb 2025 00:41:39 +0100, Heiko Stuebner wrote:
> Probe deferral can cause the platform-data we access to be freed already
> causing ugly splats on system shutdown.
> 
> Fix this via a small component-helper and adapting the rockchip-drm
> shutdown handler.
> 
> Heiko Stuebner (2):
>   drivers: base: component: add function to query the bound status
>   drm/rockchip: Fix shutdown when no drm-device is set up
> 
> [...]

Applied, thanks!

[1/2] drivers: base: component: add function to query the bound status
      commit: a6ba2dad0aa4f623ab0def8b6e6888ac00639055
[2/2] drm/rockchip: Fix shutdown when no drm-device is set up
      commit: 4444e4d789d64f053435713e5984f0ef31a7633b

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>