Re: [PATCH v8 27/39] drm/rockchip: dw_hdmi_qp: Use dw_hdmi_qp_hpd_notify() for HPD reports

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unprotected access to `hdmi->curr_conn` in `dw_hdmi_qp_hpd_notify()` can lead to a TOCTOU NULL pointer dereference.

Pre-existing issues:
- [High] System PM suspend/resume callbacks unconditionally dereference `dev_get_drvdata()`, which can be NULL if component bind hasn't completed.
- [High] Missing `disable_irq()` in `unbind` allows the hotplug interrupt to fire after the DRM master is unbound, leading to a Use-After-Free.
- [High] Using devm_* functions in component bind() ties resources to the platform device's lifetime rather than the component's lifetime, causing resource leaks and permanent bind failures on retries.
--

commit 71c4a3386d116454bc66b990f1d692e5daa00ba8
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Subject: drm/rockchip: dw_hdmi_qp: Use dw_hdmi_qp_hpd_notify() for HPD reports

This commit updates the HPD reporting to notify only the affected connector
instead of polling all of them. It also ensures the detection context runs
on reconnect to properly recover the SCDC state.

> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> --- a/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c

[ ... ]

> @@ -354,14 +353,8 @@ static void dw_hdmi_qp_rk3588_hpd_work(struct work_struct *work)
>  	struct rockchip_hdmi_qp *hdmi = container_of(work,
>  						     struct rockchip_hdmi_qp,
>  						     hpd_work.work);
> -	struct drm_device *drm = hdmi->encoder.encoder.dev;
> -	bool changed;
>  
> -	if (drm) {
> -		changed = drm_helper_hpd_irq_event(drm);
> -		if (changed)
> -			dev_dbg(hdmi->dev, "connector status changed\n");
> -	}
> +	dw_hdmi_qp_hpd_notify(hdmi->hdmi);
>  }

[Severity: High]
Could this call introduce a time-of-check to time-of-use race condition?

In dw_hdmi_qp_hpd_notify(), we evaluate hdmi->curr_conn without locks:

	if (hdmi->curr_conn && status == connector_status_connected)
		drm_connector_helper_hpd_irq_event(hdmi->curr_conn);

If a concurrent atomic modeset (triggered by userspace or hotplug) invokes
atomic_post_disable() between the condition evaluation and the dereference
in drm_connector_helper_hpd_irq_event(), hdmi->curr_conn could be set to
NULL.

A compiler might emit two separate memory reads here, which could lead
to passing a NULL pointer to the helper and causing a kernel crash. 

Would it be better to use a local variable with READ_ONCE() to ensure the
pointer remains valid across the check and the call?

[ ... ]

> @@ -738,8 +731,7 @@ static int __maybe_unused dw_hdmi_qp_rockchip_resume(struct device *dev)
>  
>  	dw_hdmi_qp_resume(dev, hdmi->hdmi);
>  
> -	if (hdmi->encoder.encoder.dev)
> -		drm_helper_hpd_irq_event(hdmi->encoder.encoder.dev);
> +	dw_hdmi_qp_hpd_notify(hdmi->hdmi);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does this function safely handle a NULL
hdmi pointer?

The resume callback fetches the hdmi struct via dev_get_drvdata(dev).

However, if the system suspends before the component framework calls the
bind callback (for instance, during a deferred probe waiting for the VOP),
dev_get_drvdata() will return NULL.

Since dw_hdmi_qp_rockchip_resume() unconditionally accesses hdmi->ctrl_ops
and hdmi->hdmi, wouldn't this cause a kernel panic on system resume?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
HPD interrupt to fire after the DRM master is unbound?

In dw_hdmi_qp_rockchip_unbind(), cancel_delayed_work_sync() is called,
but the IRQ itself is never disabled. 

Because the IRQ was requested using devm_request_threaded_irq() on the
platform device, it remains active until the platform device itself
is removed.

If an HPD event occurs after the component unbinds (which frees the
drmm_kzalloc'd hdmi struct), the IRQ handler will schedule the HPD
work which accesses freed memory, leading to a use-after-free crash.

Should disable_irq() be called during unbind to prevent this?

[Severity: High]
This is a pre-existing issue, but are we at risk of leaking resources
and permanently failing initialization on bind retries?

In dw_hdmi_qp_rockchip_bind(), several devm_ functions like
devm_request_threaded_irq() and devm_clk_get() are used on the platform
device.

Because the component framework allows bind() to fail and be retried,
any failure mid-way through bind() means these resources are not freed,
as the platform device remains active. 

On subsequent retries, devm_request_threaded_irq() will return -EBUSY,
permanently breaking the HDMI port initialization until the platform module
is unloaded.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-dw-hdmi-qp-scramb-v8-0-d79890d00b6a@collabora.com?part=27