[PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[Liam Zuiderhoek]

Inline the function drm_simple_encoder_init for meson,
as per the todo list on docs.kernel.org/gpu/todo.html

Signed-off-by: Liam Zuiderhoek <zuiderhoekl@gmail.com>
---
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 55c0601df3c6..f4100e23dd27 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -44,9 +44,17 @@ struct meson_encoder_hdmi {
 	struct cec_notifier *cec_notifier;
 };
 
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
+	.destroy = drm_encoder_cleanup,
+};
+
 #define bridge_to_meson_encoder_hdmi(x) \
 	container_of(x, struct meson_encoder_hdmi, bridge)
 
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static int meson_encoder_hdmi_attach(struct drm_bridge *bridge,
 				     struct drm_encoder *encoder,
 				     enum drm_bridge_attach_flags flags)
@@ -407,8 +415,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
 	meson_encoder_hdmi->priv = priv;
 
 	/* Encoder */
-	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
-				      DRM_MODE_ENCODER_TMDS);
+	ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
+			       &drm_simple_encoder_funcs_cleanup,
+			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret) {
 		dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n");
 		goto err_put_node;
-- 
2.43.0

Re: [PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces a duplicate definition of the `drm_simple_encoder_funcs_cleanup` structure, causing a build failure.
--

commit 2bbec9d596493f3ca9a28c3ddb628d5753d83aa2
Author: Liam Zuiderhoek <zuiderhoekl@gmail.com>

GPU: drm: meson: inline call to drm_simple_encoder_init()

This commit inlines the function drm_simple_encoder_init for the meson
driver. It replaces the simple encoder init call with drm_encoder_init
and a custom cleanup struct.

> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 55c0601df3c62..f4100e23dd270 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> @@ -44,9 +44,17 @@ struct meson_encoder_hdmi {
>  	struct cec_notifier *cec_notifier;
>  };
>  
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
>  #define bridge_to_meson_encoder_hdmi(x) \
>  	container_of(x, struct meson_encoder_hdmi, bridge)
>  
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> +	.destroy = drm_encoder_cleanup,
> +};

[Severity: Low]
Does this code cause a compiler error? It appears that the static structure
drm_simple_encoder_funcs_cleanup is defined twice in the same file.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622193250.211429-1-zuiderhoekl@gmail.com?part=1

[PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[Liam Zuiderhoek]

This commit inlines the function drm_simple_encoder_init for the meson
driver. It replaces the simple encoder init call with drm_encoder_init
and a custom cleanup struct.

Signed-off-by: Liam Zuiderhoek <zuiderhoekl@gmail.com>
---
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 55c0601df3c6..0dd3a76ad959 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -44,6 +44,10 @@ struct meson_encoder_hdmi {
 	struct cec_notifier *cec_notifier;
 };
 
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
+	.destroy = drm_encoder_cleanup,
+};
+
 #define bridge_to_meson_encoder_hdmi(x) \
 	container_of(x, struct meson_encoder_hdmi, bridge)
 
@@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
 	meson_encoder_hdmi->priv = priv;
 
 	/* Encoder */
-	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
-				      DRM_MODE_ENCODER_TMDS);
+	ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
+			       &drm_simple_encoder_funcs_cleanup,
+			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret) {
 		dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n");
 		goto err_put_node;
-- 
2.43.0

Re: [PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The DRM bridge is added to the global bridge list but is not removed on subsequent error paths, leading to a Use-After-Free.
- [High] The devm-managed encoder is registered to the DRM device, causing a Use-After-Free on unbind if the DRM device outlives the platform device.
--

commit 38b3bb11e881972fb31cc13c9266c01adfa5805b
Author: Liam Zuiderhoek <zuiderhoekl@gmail.com>

GPU: drm: meson: inline call to drm_simple_encoder_init()

This commit inlines the function drm_simple_encoder_init for the meson
driver. It replaces the simple encoder init call with drm_encoder_init
and a custom cleanup struct.

> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 55c0601df3c62..0dd3a76ad959e 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c

[ ... ]

> @@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
>  	meson_encoder_hdmi->priv = priv;
>  
>  	/* Encoder */
> -	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
> -				      DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
> +			       &drm_simple_encoder_funcs_cleanup,
> +			       DRM_MODE_ENCODER_TMDS, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but does registering a devres
managed encoder with the DRM device cause a use-after-free during unbind?

The encoder structure is allocated earlier using devres:

	meson_encoder_hdmi = devm_drm_bridge_alloc(priv->dev, ...);

It is then initialized and registered into the DRM device's mode config list
here. If the driver is unbound and the platform device is destroyed, devres
will free the encoder memory. However, if userspace holds an open DRM file
descriptor, the DRM device outlives the platform device.

When the file descriptor is later closed and the DRM device is finally
destroyed, drm_mode_config_cleanup() will attempt to call the .destroy
callback on the freed encoder memory.

>  	if (ret) {
>  		dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n");
>  		goto err_put_node;

[Severity: High]
This is a pre-existing issue, but does this error path lead to a
use-after-free?

Earlier in meson_encoder_hdmi_probe(), the bridge is added to the global
bridge list:

	drm_bridge_add(&meson_encoder_hdmi->bridge);

If drm_encoder_init() fails, we jump to err_put_node, which returns without
calling drm_bridge_remove(). The devres cleanup will then free
meson_encoder_hdmi, leaving a dangling pointer in the DRM core's global
bridge list.

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622194649.218419-1-zuiderhoekl@gmail.com?part=1

[PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[Liam Zuiderhoek]

This commit inlines the function drm_simple_encoder_init for the meson
driver. It replaces the simple encoder init call with drm_encoder_init
and a custom cleanup struct.

Signed-off-by: Liam Zuiderhoek <zuiderhoekl@gmail.com>
---
 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
index 55c0601df3c6..0dd3a76ad959 100644
--- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
@@ -44,6 +44,10 @@ struct meson_encoder_hdmi {
 	struct cec_notifier *cec_notifier;
 };
 
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
+	.destroy = drm_encoder_cleanup,
+};
+
 #define bridge_to_meson_encoder_hdmi(x) \
 	container_of(x, struct meson_encoder_hdmi, bridge)
 
@@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
 	meson_encoder_hdmi->priv = priv;
 
 	/* Encoder */
-	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
-				      DRM_MODE_ENCODER_TMDS);
+	ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
+			       &drm_simple_encoder_funcs_cleanup,
+			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret) {
 		dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n");
 		goto err_put_node;
-- 
2.43.0

Re: [PATCH] GPU: drm: meson: inline call to drm_simple_encoder_init()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The commit claims to inline `drm_simple_encoder_init` for the entire meson driver, but it only modifies `meson_encoder_hdmi.c`, leaving other encoders unconverted.

Pre-existing issues:
- [High] Missing `drm_bridge_remove()` on error paths in `meson_encoder_hdmi_probe()` leaves the bridge registered in the global `bridge_list`, leading to a dangling pointer and potential Use-After-Free.
--

commit 640b38eb942af61527b421bb5d62cb275bca9574
Author: Liam Zuiderhoek <zuiderhoekl@gmail.com>

GPU: drm: meson: inline call to drm_simple_encoder_init()

This commit inlines the function drm_simple_encoder_init for the meson
driver. It replaces the simple encoder init call with drm_encoder_init
and a custom cleanup struct.

> This commit inlines the function drm_simple_encoder_init for the meson
> driver.

[Severity: Medium]
Is the commit message description accurate for the scope of this patch?

The commit states it inlines the function for the meson driver, but this patch 
only modifies the HDMI encoder (meson_encoder_hdmi.c). 

The CVBS (meson_encoder_cvbs_probe) and DSI (meson_encoder_dsi_probe) encoders 
in the same driver still use drm_simple_encoder_init().

Should this patch update those files as well, or should the commit message be 
clarified to specify it only targets the HDMI encoder?

> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 55c0601df3c62..0dd3a76ad959e 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c

[ ... ]

> @@ -407,8 +411,9 @@ int meson_encoder_hdmi_probe(struct meson_drm *priv)
>  	meson_encoder_hdmi->priv = priv;
>  
>  	/* Encoder */
> -	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
> -				      DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(priv->drm, &meson_encoder_hdmi->encoder,
> +			       &drm_simple_encoder_funcs_cleanup,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
>  	if (ret) {
>  		dev_err_probe(priv->dev, ret, "Failed to init HDMI encoder\n");
>  		goto err_put_node;

[Severity: High]
This is a pre-existing issue, but does this error path leave the bridge 
registered in the global bridge list, leading to a potential use-after-free?

In meson_encoder_hdmi_probe(), drm_bridge_add() is called before the encoder 
initialization. 

If we fail here at drm_encoder_init() (or later during 
drm_bridge_connector_init()), the code jumps to err_put_node which does not 
call drm_bridge_remove():

err_put_node:
	of_node_put(remote);
	return ret;

Similarly, if cec_notifier_conn_register() fails later in the probe, it returns 
directly without cleaning up the bridge.

The cleanup function meson_encoder_hdmi_remove() will not catch this because 
priv->encoders[MESON_ENC_HDMI] is only assigned at the very end of a successful 
probe. 

If the still-registered bridge is later found via of_drm_find_and_get_bridge() 
and its callbacks are invoked, could it dereference the freed priv pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629173902.29415-1-zuiderhoekl@gmail.com?part=1