Re: [PATCH v2] drm/rockchip: dsi: Open-code drm_simple_encoder_init()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of `drm_encoder_init()` is ignored, which can lead to a NULL pointer dereference on subsequent error cleanup paths.
--

commit 386dea7155c8b56dde923c52be1155ed455ebd09
Author: Diogo Silva <diogompaissilva@gmail.com>

drm/rockchip: dsi: Open-code drm_simple_encoder_init()

This patch removes the dependency on the deprecated simple KMS helper from
rockchip DRM drivers by inlining drm_simple_encoder_init into the drivers.

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c

[ ... ]

> @@ -604,7 +607,8 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	}
>  
>  	drm_encoder_helper_add(encoder, &dw_hdmi_rockchip_encoder_helper_funcs);
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> +	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
> +			 DRM_MODE_ENCODER_TMDS, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but does the return value of
drm_encoder_init() need to be checked here?

If drm_encoder_init() fails, encoder->dev remains NULL. If the subsequent
dw_hdmi_bind() call fails, the error path jumps to err_bind and calls
drm_encoder_cleanup(encoder). This explicitly dereferences encoder->dev
inside drm_mode_object_unregister(), which would cause a NULL pointer
dereference.

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c

[ ... ]

> @@ -586,7 +589,8 @@ static int dw_hdmi_qp_rockchip_bind(struct device *dev, struct device *master,
>  		return ret;
>  
>  	drm_encoder_helper_add(encoder, &dw_hdmi_qp_rockchip_encoder_helper_funcs);
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> +	drm_encoder_init(drm, encoder, &dw_hdmi_qp_rockchip_encoder_funcs,
> +			 DRM_MODE_ENCODER_TMDS, NULL);

[Severity: High]
This is a pre-existing issue, but should this code check the return value of
drm_encoder_init() for errors?

Similar to the previous file, if this initialization fails and leaves
encoder->dev as NULL, a subsequent failure in dw_hdmi_qp_bind() triggers
an error path that explicitly calls drm_encoder_cleanup(encoder), leading
to a NULL pointer dereference.

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c

[ ... ]

> @@ -696,7 +699,8 @@ rk3066_hdmi_register(struct drm_device *drm, struct rk3066_hdmi *hdmi)
>  		return -EPROBE_DEFER;
>  
>  	drm_encoder_helper_add(encoder, &rk3066_hdmi_encoder_helper_funcs);
> -	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
> +	drm_encoder_init(drm, encoder, &rk3066_hdmi_encoder_funcs,
> +			 DRM_MODE_ENCODER_TMDS, NULL);

[Severity: High]
This is a pre-existing issue, but what happens if drm_encoder_init() fails
here?

If it fails and a subsequent initialization step fails, the error cleanup will
call the encoder's destroy function, causing a NULL pointer dereference since
encoder->dev would be NULL.

[ ... ]

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