Re: [PATCH v10 37/69] drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [High] Hardware is suspended before canceling asynchronous HPD work, crea=
ting a race condition that leads to I2C timeouts during system suspend.
- [Medium] The disable_hpd callbacks do not cancel the pending delayed HPD =
work, allowing spurious HPD events.
--

--- Patch [37]: [PATCH v10 37/69] drm/rockchip: dw_hdmi_qp: Control the HPD=
 IRQ line via the bridge HPD ops ---
commit ad6fbe00662ae75dcead6bf784de9de47ed9dd6c
Author: Cristian Ciocaltea <[email protected]>

drm/rockchip: dw_hdmi_qp: Control the HPD IRQ line via the bridge HPD ops

The HPD interrupt must not be delivered before the DRM connector is fully
set up, and it should be quiesced while the display pipeline is suspended.
On top of that, the threaded handler re-arms HPD unconditionally after the
hotplug debounce, which must not override an explicit disable requested by
the DRM core.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu=
/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 46a61112c14f4..253da2162ce6f 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
> @@ -308,12 +309,14 @@ static void dw_hdmi_qp_rk3588_enable_hpd(struct dw_=
hdmi_qp *dw_hdmi, void *data)
>  	struct rockchip_hdmi_qp *hdmi =3D (struct rockchip_hdmi_qp *)data;
> =20
>  	dw_hdmi_qp_rk3588_unmask_hpd(hdmi);
> +	enable_irq(hdmi->hpd_irq);
>  }
> =20
>  static void dw_hdmi_qp_rk3588_disable_hpd(struct dw_hdmi_qp *dw_hdmi, vo=
id *data)
>  {
>  	struct rockchip_hdmi_qp *hdmi =3D (struct rockchip_hdmi_qp *)data;
> =20
> +	disable_irq(hdmi->hpd_irq);
>  	dw_hdmi_qp_rk3588_mask_hpd(hdmi);
>  }

[Severity: Medium]
The DRM core disables HPD (e.g., during driver unbind or DRM pipeline disab=
le),
which invokes dw_hdmi_qp_rk3588_disable_hpd() and dw_hdmi_qp_rk3576_disable=
_hpd().
This disables the HPD IRQ but does not cancel the already queued hdmi->hpd_=
work.

Could this delayed work execute up to 150ms later, probing the connector and
running EDID reads when the driver expects HPD to be fully quiesced, result=
ing
in spurious HPD events?

[ ... ]
> @@ -688,14 +693,14 @@ static int dw_hdmi_qp_rockchip_bind(struct device *=
dev, struct device *master,
>  	if (plat_data.cec_irq < 0)
>  		return plat_data.cec_irq;
> =20
> -	irq =3D platform_get_irq_byname(pdev, "hpd");
> -	if (irq < 0)
> -		return irq;
> +	hdmi->hpd_irq =3D platform_get_irq_byname(pdev, "hpd");
> +	if (hdmi->hpd_irq < 0)
> +		return hdmi->hpd_irq;
> =20
> -	ret =3D devm_request_threaded_irq(dev, irq,
> +	ret =3D devm_request_threaded_irq(dev, hdmi->hpd_irq,
>  					cfg->ctrl_ops->hardirq_callback,
>  					cfg->ctrl_ops->irq_callback,
> -					IRQF_SHARED, "dw-hdmi-qp-hpd",
> +					IRQF_NO_AUTOEN, "dw-hdmi-qp-hpd",
>  					hdmi);
>  	if (ret)
>  		return ret;

[Severity: High]
When the system suspends, dw_hdmi_qp_rockchip_suspend() is called. It first
calls dw_hdmi_qp_suspend(), which disables the main_irq used for I2C
completions. It then calls cancel_delayed_work_sync(&hdmi->hpd_work):

dw_hdmi_qp_rockchip_suspend() {
    ...
    dw_hdmi_qp_suspend(dev, hdmi->hdmi);
    cancel_delayed_work_sync(&hdmi->hpd_work);
    ...
}

If the HPD work is executing concurrently, it will attempt to read the EDID=
 via
I2C, but since main_irq is disabled, the I2C read will time out.

Can this race condition lead to I2C timeouts and bus hangs during system
suspend?

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