Re: [PATCH v1] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Devres allocations in component bind() cause permanent initialization failure (-EBUSY) on probe deferral.
- [High] Active devm-managed IRQ and I2C handlers access hardware with the clock disabled after unbind(), causing a system crash.
--

--- Patch [1]: [PATCH v1] drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API ---
commit 2ee09d3b212b24956cbceb44c6bf494785f5586d
Author: Johan Jonker <jbx6244@gmail.com>

drm/rockchip: rk3066_hdmi: convert to devm_drm_bridge_alloc() API
    
The function devm_drm_bridge_alloc() is the new API for allocating DRM
bridges. This conversion was missed during the initial conversion of all
bridges to the new API.

> diff --git a/drivers/gpu/drm/rockchip/rk3066_hdmi.c b/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> index f28d6a883e099..39c65ee07caac 100644
> --- a/drivers/gpu/drm/rockchip/rk3066_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/rk3066_hdmi.c

[ ... ]

> @@ -747,9 +746,10 @@ static int rk3066_hdmi_bind(struct device *dev, struct device *master,
>  	int irq;
>  	int ret;
>  
> -	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi)
> -		return -ENOMEM;
> +	hdmi = devm_drm_bridge_alloc(dev, struct rk3066_hdmi, bridge,
> +				     &rk3066_hdmi_bridge_funcs);
> +	if (IS_ERR(hdmi))
> +		return PTR_ERR(hdmi);

[Severity: High]
This is a pre-existing issue, but does using devm_ allocations in the
component bind() function cause a permanent failure on probe deferral?

rk3066_hdmi_bind() allocates devres-managed resources using the component
device, including devm_platform_ioremap_resource() right below this hunk.
Devres actions are tied to the device's probe/remove lifecycle, not the
component bind/unbind lifecycle.

If bind() fails, for example when rk3066_hdmi_register() returns -EPROBE_DEFER
due to missing CRTCs, the devres resources are not freed.

When the master DRM device retries probing and calls bind() again,
devm_platform_ioremap_resource() will attempt to request the same physical
memory region via request_mem_region(). Because the first request was never
released by devres, this fails with -EBUSY, preventing the display from ever
initializing.

[Severity: High]
This is a pre-existing issue, but can the active devm-managed IRQ and I2C
handlers access hardware with the clock disabled after unbind(), causing a
system crash?

In rk3066_hdmi_bind(), the HDMI IRQ handler and I2C adapter are registered
using devm_request_threaded_irq() and devm_i2c_add_adapter() on the component
device. 

Because they are devres-managed on the component device's probe/remove
lifecycle, they are not removed when the master DRM device unbinds and calls
rk3066_hdmi_unbind().

In rk3066_hdmi_unbind(), the hardware bus clock is explicitly disabled:

    clk_disable_unprepare(hdmi->hclk);

If an I2C transfer is initiated from userspace (via /dev/i2c-*), or if a
shared IRQ fires after the master is unbound, the still-active handlers
(rk3066_hdmi_i2c_xfer or rk3066_hdmi_hardirq) will execute and access HDMI
hardware registers. Accessing hardware registers while the clock is disabled
causes a synchronous external abort (bus fault), resulting in a kernel panic.

>  
>  	hdmi->dev = dev;
>  	hdmi->drm_dev = drm;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/dea8a038-577b-45da-bff3-703e9099b7e1@gmail.com?part=1