Re: [PATCH v9 27/61] drm/rockchip: dw_hdmi_qp: Avoid spurious HPD IRQ thread wakeups

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

Pre-existing issues:
- [High] Use-After-Free and resource leaks in component bind due to mismatched devm_ and drmm_ lifecycles.
--

--- Patch [27]: [PATCH v9 27/61] drm/rockchip: dw_hdmi_qp: Avoid spurious HPD IRQ thread wakeups ---
commit fe0521a96f0ae3fca18406e54bcdc6ddbecf6e85
Author: Cristian Ciocaltea <[email protected]>

drm/rockchip: dw_hdmi_qp: Avoid spurious HPD IRQ thread wakeups

The RK3576 and RK3588 HPD hardirq handlers read shared status registers
and currently wake the threaded handler whenever any bit in those
registers is set. This commit masks the status value with the SoC-specific
HPD interrupt bit to avoid spurious IRQ thread wakeups.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 2aceff11d14b5..7e3e86d056116 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c

[ ... ]

> @@ -381,6 +384,8 @@ static irqreturn_t dw_hdmi_qp_rk3588_hardirq(int irq, void *dev_id)
>  
>  	regmap_read(hdmi->regmap, RK3588_GRF_SOC_STATUS1, &intr_stat);
>  
> +	intr_stat &= hdmi->port_id ? RK3588_HDMI1_OHPD_INT : RK3588_HDMI0_OHPD_INT;
> +

[Severity: High]
This is a pre-existing issue, but could this handler trigger a use-after-free
after the component is unbound?

It looks like dw_hdmi_qp_rockchip_bind() allocates the hdmi context using
drmm_kzalloc() (tied to the DRM aggregate device), but registers this IRQ
handler using devm_request_threaded_irq() (tied to the platform device):

drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c:dw_hdmi_qp_rockchip_bind() {
    ...
    ret = devm_request_threaded_irq(dev, irq,
                                    cfg->ctrl_ops->hardirq_callback,
                                    cfg->ctrl_ops->irq_callback,
                                    IRQF_SHARED, "dw-hdmi-qp-hpd",
                                    hdmi);
    ...
}

When the DRM component unbinds, drmm_ frees the hdmi struct. However, since
the platform device itself isn't removed, the devm_ allocated IRQ handler
remains active.

If an interrupt fires after unbind, wouldn't dw_hdmi_qp_rk3588_hardirq()
dereference the freed hdmi pointer when calling regmap_read()?

Should dw_hdmi_qp_rockchip_unbind() manually free the IRQ and other resources,
or should the IRQ be registered with a drmm_ wrapper instead?

>  	if (intr_stat) {
>  		if (hdmi->port_id)
>  			val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=27