diff mbox series

[v10,30/69] drm/rockchip: dw_hdmi_qp: Cancel pending HPD work on suspend

Message ID 20260731-dw-hdmi-qp-scramb-v10-30-294364b2cf15@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea July 31, 2026, 4:19 p.m. UTC 
The HPD threaded handler debounces hotplug events by delaying
hdmi->hpd_work for 150 ms.  Nothing discards that work when the system
goes to sleep: drm_kms_helper_poll_disable() only disables the HPD IRQ
line via the bridge .hpd_disable() op, and disable_irq() merely waits
for the threaded handler, which has already returned after arming the
timer.

If the debounce expires during suspend, its non-freezable work may probe
the connector and read the EDID over DDC.  Since genpd powers off the
HDMI controller's power domain in ->suspend_noirq(), this access may
hit unpowered hardware and hang the system.

Cancel the work in the suspend callback.  The DRM device is registered
as a consumer of the HDMI device, so it always suspends first and the
HPD IRQ is already disabled by that point, meaning nothing can re-arm
the work behind our back.  No counterpart is needed on resume, as the
work gets scheduled again on the next hotplug interrupt.

Fixes: 128a9bf8ace2 ("drm/rockchip: Add basic RK3588 HDMI output support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260723015909.7F79A1F000E9@smtp.kernel.org/
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org July 31, 2026, 8:30 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Deadlock during suspend: disabling the main IRQ before cancelling the HPD work causes I2C timeouts.

Pre-existing issues:
- [High] Incomplete fix: HPD IRQ is not actually disabled, allowing delayed work to be re-armed during suspend.
- [High] Use-After-Free of `hdmi` due to devres/drmm mismatch on component unbind.
--

--- Patch [30]: [PATCH v10 30/69] drm/rockchip: dw_hdmi_qp: Cancel pending HPD work on suspend ---
commit 9b084d07cd142a4d5581877b274f1d064cd6ebd9
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

drm/rockchip: dw_hdmi_qp: Cancel pending HPD work on suspend

This commit cancels the pending HPD work during the suspend callback.
The intent is to prevent the work from probing the connector and reading
the EDID after the HDMI controller's power domain has been powered off.

Closes: https://lore.kernel.org/all/20260723015909.7F79A1F000E9@smtp.kernel.org/

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> index 646a4cb968309..e9fda67b8ca94 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> @@ -728,8 +728,12 @@ static int __maybe_unused dw_hdmi_qp_rockchip_suspend(struct device *dev)
>  {
>  	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
>  
> -	if (hdmi)
> -		dw_hdmi_qp_suspend(dev, hdmi->hdmi);
> +	if (!hdmi)
> +		return 0;
> +
> +	dw_hdmi_qp_suspend(dev, hdmi->hdmi);
> +
> +	cancel_delayed_work_sync(&hdmi->hpd_work);

[Severity: High]
Does this code cause a deadlock during suspend if the HPD work is actively
running?

Because dw_hdmi_qp_suspend() is called first, it explicitly disables the
main IRQ.

If the HPD work is running and reads the EDID over DDC, the read relies on
dw_hdmi_qp_i2c_read(), which waits for a completion signaled by the main IRQ
handler.

Since the main IRQ is already disabled, will the I2C transfers time out
repeatedly, stalling cancel_delayed_work_sync() and completely hanging the
suspend process?

[Severity: High]
This isn't a bug introduced by this patch, but does this actually prevent the
work from running on unpowered hardware?

The commit message assumes drm_kms_helper_poll_disable() disables the HPD IRQ
via the bridge .hpd_disable() op.

However, rk3588_hdmi_phy_ops does not implement .disable_hpd:

static const struct dw_hdmi_qp_phy_ops rk3588_hdmi_phy_ops = {
	.init		= dw_hdmi_qp_rk3588_phy_init,
	.disable	= dw_hdmi_qp_rk3588_phy_disable,
	.read_hpd	= dw_hdmi_qp_rk3588_read_hpd,
	.setup_hpd	= dw_hdmi_qp_rk3588_setup_hpd,
};

This makes the DRM core's attempt to mask the IRQ a no-op, and the separate
HPD IRQ requested in dw_hdmi_qp_rockchip_bind() remains unmasked.

If a hotplug event occurs after cancel_delayed_work_sync() returns, but
before suspend_noirq disables all IRQs, will the handler fire and re-arm the
work on system_percpu_wq?

Since this workqueue is not freezable, could the work still execute after the
power domain is off?

[Severity: High]
This is a pre-existing issue, but does the component unbind path have a
use-after-free issue?

In dw_hdmi_qp_rockchip_bind(), the hdmi context is tied to the DRM device's
lifetime:

	hdmi = drmm_kzalloc(drm, sizeof(*hdmi), GFP_KERNEL);

However, the HPD IRQ is tied to the platform device's lifetime:

	ret = devm_request_threaded_irq(hdmi->dev, irq,
					cfg->ctrl_ops->hardirq_callback,
					cfg->ctrl_ops->irq_callback,
					IRQF_SHARED, "dw-hdmi-qp-hpd",
					hdmi);

When the component is unbound (e.g., driver removal or DRM teardown), the
DRM device frees all drmm memory, including the hdmi structure.

Because the platform device itself isn't removed, its devm resources are not
released, leaving the IRQ active.

If an HPD interrupt fires subsequently, will the handler dereference the
freed hdmi pointer?

>  
>  	return 0;
>  }
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
index 646a4cb96830..e9fda67b8ca9 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
@@ -728,8 +728,12 @@  static int __maybe_unused dw_hdmi_qp_rockchip_suspend(struct device *dev)
 {
 	struct rockchip_hdmi_qp *hdmi = dev_get_drvdata(dev);
 
-	if (hdmi)
-		dw_hdmi_qp_suspend(dev, hdmi->hdmi);
+	if (!hdmi)
+		return 0;
+
+	dw_hdmi_qp_suspend(dev, hdmi->hdmi);
+
+	cancel_delayed_work_sync(&hdmi->hpd_work);
 
 	return 0;
 }