Re: [PATCH v5] drm/rockchip: Remove dependency on DRM simple helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value of drm_encoder_init() leads to NULL pointer dereference on error or unbind paths.
- [High] Incorrect usage of devres (devm_*) functions inside component bind callbacks causes resource leaks and duplicate registrations.
--

commit 58124ed88e765fc2655a16706ba7e0962ef4b3df
Author: Diogo Silva <diogompaissilva@gmail.com>

drm/rockchip: Remove dependency on DRM simple helpers

Simple KMS helper are deprecated since they only add an intermediate
layer between drivers and the atomic modesetting. This patch removes
the drm_simple_encoder_init() helper usage in the rockchip drivers
by open coding it and using the encoder atomic helpers directly.

> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index e159e1627f3c2..68aa66915e8ac 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
[ ... ]
> @@ -988,8 +991,8 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
>  							     dev->of_node);
>  	DRM_DEBUG_KMS("possible_crtcs = 0x%x\n", encoder->possible_crtcs);
>  
> -	ret = drm_simple_encoder_init(drm_dev, encoder,
> -				      DRM_MODE_ENCODER_TMDS);
> +	ret = drm_encoder_init(drm_dev, encoder, &cdn_dp_encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);

[Severity: High]
This is a pre-existing issue, but could using devm_drm_bridge_add() and
other devres functions inside a component bind callback cause resources to
permanently leak on unbind?

The DRM component framework calls bind and unbind callbacks during the master
DRM device lifecycle. However, the component platform devices themselves are
not destroyed during unbind. Because devm allocations are tied to the
platform device's lifetime, they are not freed when unbind runs.

When cdn_dp_bind() is called again, new resources are allocated and registered,
causing duplicate registrations (such as devm_extcon_register_notifier) and
undefined behavior on re-bind.

>  	if (ret) {
>  		DRM_ERROR("failed to initialize encoder with drm\n");
>  		return ret;
[ ... ]
> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> index 9b8981fa19aee..f28d6a883e099 100644
> --- 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 isn't a bug introduced by this patch, but should the return value of
drm_encoder_init() be checked here? 

If drm_encoder_init() fails, the encoder->funcs pointer remains NULL and
initialization continues because the error is ignored. If a subsequent
initialization step fails, or during a later component unbind in
rk3066_hdmi_unbind(), the code unconditionally executes:

    hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder);

Can this lead to a NULL pointer dereference? The patch replaced the function
call without adding the necessary error check that was properly added to
other drivers updated in this same patch.

[Severity: High]
This is also a pre-existing issue, but similar to the devres issues in
cdn_dp_bind() and rockchip_rgb_init(), does using devm_kzalloc() and 
devm_request_threaded_irq() in rk3066_hdmi_bind() cause resources to
permanently leak on unbind?

Because devm allocations are tied to the platform device's lifetime, they
are not freed when unbind runs, leaking resources and causing duplicate IRQ
handler registrations on re-bind.

>  
>  	hdmi->bridge.driver_private = hdmi;
>  	hdmi->bridge.funcs = &rk3066_hdmi_bridge_funcs;
[ ... ]
> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> index add3123e5ce70..2ad24b914989f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
[ ... ]
> @@ -127,7 +130,8 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>  	encoder = &rgb->encoder.encoder;
>  	encoder->possible_crtcs = drm_crtc_mask(crtc);
>  
> -	ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_NONE);
> +	ret = drm_encoder_init(drm_dev, encoder, &rockchip_rgb_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);

[Severity: High]
This is a pre-existing issue, but does the devm allocation in
rockchip_rgb_init() leak memory when the VOP unbinds and re-binds?

rockchip_rgb_init() is called from vop_bind(), and devm_kzalloc() ties the
memory to the VOP device's lifetime, which is not destroyed during an unbind.
When bind is called again, new resources are allocated and registered, leaking
the old ones.

>  	if (ret < 0) {
>  		DRM_DEV_ERROR(drm_dev->dev,
>  			      "failed to initialize encoder: %d\n", ret);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704-rockchip-drm-simple-v5-1-a333f527a4f9@gmail.com?part=1