Re: [PATCH v9 29/61] drm/rockchip: dw_hdmi_qp: Implement .{enable,disable}_hpd() PHY ops

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

Pre-existing issues:
- [High] Threaded IRQ handlers unconditionally unmask HPD interrupts and re-queue delayed work, leading to state desync and use-after-free during teardown.
--

--- Patch [29]: [PATCH v9 29/61] drm/rockchip: dw_hdmi_qp: Implement .{enable,disable}_hpd() PHY ops ---
commit 4e3d155ee238497812dbae4f03f0d9c85824af5a
Author: Cristian Ciocaltea <[email protected]>

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

This commit implements the .enable_hpd() and .disable_hpd() phy ops for
RK3576 and RK3588 SoCs. It also consolidates the interrupt clear-and-unmask
and mask-only sequences into these new ops. The .setup_hpd() phy op is removed
as it is no longer needed.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 12cd8db7cc50b..5067e3cf0e881 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]
This is a pre-existing issue, but this threaded IRQ handler unconditionally
unmasks the HPD interrupt and requeues delayed work.

During teardown or driver unbind, the DRM master is unregistered and drmm
resources (including hdmi) are freed. However, because the devm-managed
IRQ isn't destroyed immediately, the IRQ thread could race with or run after
the DRM core's attempt to mask the interrupt.

This unconditionally calls dw_hdmi_qp_rk3576_enable_hpd(), unmasking the
interrupt again. A subsequent hotplug event could then fire the interrupt and
cause the hardirq handler to access the freed hdmi structure, leading to a
use-after-free crash.

Does this need additional teardown synchronization or checking to ensure we
do not re-enable interrupts and requeue work when unbinding?

>  
>  	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]
This is a pre-existing issue, but this RK3588 IRQ handler exhibits the same
behavior as the RK3576 handler above. 

Can this lead to a use-after-free during driver teardown if the IRQ thread
runs after the DRM core tries to mask the interrupt, causing it to be left
armed?

>  
>  	return IRQ_HANDLED;
>  }

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