Re: [PATCH v10 29/69] drm/rockchip: dw_hdmi_qp: Fix invalid drvdata access in PM ops

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

Pre-existing issues:
- [High] Use-After-Free of `hdmi` struct due to `devm_request_threaded_irq`=
 being used in component bind, leaking the IRQ on unbind.
- [High] The HPD hardirq handler checks the raw SOC status register instead=
 of the interrupt pending status, breaking unplug detection and stealing sh=
ared IRQs.
--

--- Patch [29]: [PATCH v10 29/69] drm/rockchip: dw_hdmi_qp: Fix invalid drv=
data access in PM ops ---
commit fc12a8c8d5e7177cf76cf4554fb62df0ebde5bbd
Author: Cristian Ciocaltea <[email protected]>

drm/rockchip: dw_hdmi_qp: Fix invalid drvdata access in PM ops

The system-sleep PM callbacks fetch the driver state via dev_get_drvdata()
and dereference it unconditionally. This patch ensures non-NULL drvdata
always represents live state, allowing each PM callback to rely on a single
NULL check.

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

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu=
/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 9c4cf68d79d50..646a4cb968309 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c

[ ... ]

> @@ -704,6 +704,8 @@ static void dw_hdmi_qp_rockchip_unbind(struct device =
*dev,
>  {
>  	struct rockchip_hdmi_qp *hdmi =3D dev_get_drvdata(dev);
> =20
> +	dev_set_drvdata(dev, NULL);
> +
>  	cancel_delayed_work_sync(&hdmi->hpd_work);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to rely on
devm_request_threaded_irq() for the HPD interrupt when lifecycle is managed
via component bind and unbind?

Looking at dw_hdmi_qp_rockchip_bind(), the IRQ is requested with devm on the
component device:

    ret =3D devm_request_threaded_irq(dev, hdmi->hpd_irq, ...);

Because devm resources are tied to the device's probe and remove lifecycle,
the IRQ is not freed when the component is unbound. When the aggregate DRM
device is torn down, hdmi (allocated via drmm_kzalloc) is freed.

If the HPD interrupt fires afterwards, the still-active IRQ handler will
dereference the freed hdmi pointer.

Also, calling cancel_delayed_work_sync() here doesn't prevent the active IRQ
handler from immediately re-arming the workqueue.

Should this use a non-devm request_threaded_irq() in dw_hdmi_qp_rockchip_bi=
nd()
and call free_irq() here in dw_hdmi_qp_rockchip_unbind()?

[Severity: High]
This is also a pre-existing issue, but does the hardirq handler properly ha=
ndle
interrupts on unplug events?

Looking at dw_hdmi_qp_rk3588_hardirq() and dw_hdmi_qp_rk3576_hardirq(), they
read the raw level register without masking:

    regmap_read(hdmi->regmap, RK3588_GRF_SOC_STATUS1, &intr_stat);
    if (intr_stat) {
        ...

When the HDMI cable is unplugged, the HPD level goes to 0, which triggers t=
he
interrupt. If no other unrelated bits in this 32-bit register are set,
intr_stat evaluates to 0 and the handler returns IRQ_NONE without clearing =
the
interrupt.

Could this cause the kernel to see an unhandled interrupt and disable it
permanently?

Conversely, if any unrelated bit is 1, it will always return IRQ_WAKE_THREA=
D,
falsely claiming interrupts for other devices on this shared IRQ line.

Should this check the interrupt pending status instead of the raw SOC status
register?

>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260731-dw-hdmi-qp=
[email protected]?part=3D29