Re: [PATCH v10 35/69] drm/rockchip: dw_hdmi_qp: Implement .{enable,disable}_hpd() PHY ops

[sashiko-bot@kernel.org]

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

New issues:
- [High] The threaded IRQ handlers unconditionally call `.enable_hpd()` to unmask the HPD interrupt, which overrides the DRM core's explicit request to disable HPD via `.disable_hpd()`.

Pre-existing issues:
- [High] Vulnerability to Use-After-Free during unbind/teardown because the delayed work can be re-queued by an active IRQ after it has been canceled.
--

--- Patch [35]: [PATCH v10 35/69] drm/rockchip: dw_hdmi_qp: Implement .{enable,disable}_hpd() PHY ops ---
commit f5a2c115ab2bc65d86865686a80a9dc03bbb514c
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: dw_hdmi_qp: Implement .{enable,disable}_hpd() PHY ops

This commit implements .enable_hpd() and .disable_hpd() phy ops for RK3576
and RK3588 SoCs. It consolidates the interrupt clear-and-unmask sequence
into enable_hpd() and the mask-only operation into disable_hpd(), replacing
open-coded register writes.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 64e8e4c12cbd2..1965641fbadf1 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
[ ... ]
> @@ -364,15 +385,11 @@ static irqreturn_t dw_hdmi_qp_rk3576_hardirq(int irq, void *dev_id)
>  static irqreturn_t dw_hdmi_qp_rk3576_irq(int irq, void *dev_id)
>  {
>  	struct rockchip_hdmi_qp *hdmi = dev_id;
> -	u32 val;
>  
> -	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_CLR, 1);
> -	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
>  	mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
>  			 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
>  
> -	val = FIELD_PREP_WM16(RK3576_HDMI_HPD_INT_MSK, 0);
> -	regmap_write(hdmi->regmap, RK3576_IOC_MISC_CON0, val);
> +	dw_hdmi_qp_rk3576_enable_hpd(NULL, hdmi);

[Severity: High]
Does this unconditionally unmask the HPD interrupt, potentially overriding
the DRM core's desired HPD state?

If an HPD interrupt is triggered right before the DRM core calls
dw_hdmi_qp_rk3576_disable_hpd() (e.g., during display teardown or suspend),
the hardirq handler will mask the interrupt and schedule this threaded
handler. If the DRM core then disables HPD, keeping it masked, will this
unconditional call to dw_hdmi_qp_rk3576_enable_hpd() in the threaded handler
override the DRM core's request and unmask the interrupt again?

Also, this isn't a bug introduced by this patch, but does calling
mod_delayed_work() here leave a use-after-free window during teardown?

During dw_hdmi_qp_rockchip_unbind(), cancel_delayed_work_sync(&hdmi->hpd_work)
is called. However, since the HPD IRQ is managed by devm_request_threaded_irq()
on the platform device, the IRQ remains active during unbind. If a hardware
HPD interrupt fires immediately after cancel_delayed_work_sync() completes,
could this threaded handler run and re-queue the work right before the hdmi
structure is freed?

>  
>  	return IRQ_HANDLED;
>  }
[ ... ]
> @@ -401,22 +414,11 @@ static irqreturn_t dw_hdmi_qp_rk3588_hardirq(int irq, void *dev_id)
>  static irqreturn_t dw_hdmi_qp_rk3588_irq(int irq, void *dev_id)
>  {
>  	struct rockchip_hdmi_qp *hdmi = dev_id;
> -	u32 val;
> -
> -	if (hdmi->port_id)
> -		val = FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_CLR, 1);
> -	else
> -		val = FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_CLR, 1);
> -	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
>  
>  	mod_delayed_work(system_percpu_wq, &hdmi->hpd_work,
>  			 msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
>  
> -	if (hdmi->port_id)
> -		val |= FIELD_PREP_WM16(RK3588_HDMI1_HPD_INT_MSK, 0);
> -	else
> -		val |= FIELD_PREP_WM16(RK3588_HDMI0_HPD_INT_MSK, 0);
> -	regmap_write(hdmi->regmap, RK3588_GRF_SOC_CON2, val);
> +	dw_hdmi_qp_rk3588_enable_hpd(NULL, hdmi);

[Severity: High]
Could the unconditional call to dw_hdmi_qp_rk3588_enable_hpd() here
unintentionally re-enable the HPD interrupt when the DRM core expected it
to remain disabled?

As with the RK3576 path, if a hardware event triggers just before teardown,
this threaded handler might run after the DRM core has already requested
the interrupt be disabled, breaking the intended HPD state machine.

Also, this is a pre-existing issue, but is there a use-after-free risk
during driver unbind?

If a hardware interrupt fires after cancel_delayed_work_sync() has run in
dw_hdmi_qp_rockchip_unbind(), the still-active IRQ handler will call
mod_delayed_work() and re-queue the work right before the hdmi structure
gets freed.

>  
>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-dw-hdmi-qp-scramb-v10-0-294364b2cf15@collabora.com?part=35