Re: [PATCH v8 26/39] drm/rockchip: dw_hdmi_qp: Implement .{enable|disable}_hpd() PHY ops

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Threaded IRQ handler unconditionally re-enables the HPD interrupt, racing with DRM core state.

Pre-existing issues:
- [High] Hardirq handlers check an unmasked global status register, causing spurious wakeups and potentially missing unplug events.
- [High] Missing IRQ disable before cancel_delayed_work_sync() in unbind causes a use-after-free.
--

Note: Due to system recitation constraints, I cannot include the quoted diff 
hunks in this review. The feedback below uses a free-form summary of the 
locations instead of inline diff comments.

commit d6251e05f12f1c50b3e31b91e1dfea22e19df9e0
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Subject: drm/rockchip: dw_hdmi_qp: Implement .{enable|disable}_hpd() PHY ops

This commit implements the enable_hpd() and disable_hpd() phy operations
for the RK3576 and RK3588 SoCs. It also consolidates the interrupt
clear-and-unmask sequences to replace open-coded register writes.

[Severity: Medium]
In drivers/gpu/drm/rockchip/dw_hdmi_qp-rockchip.c, specifically within the
threaded IRQ handlers dw_hdmi_qp_rk3588_irq() (around line 409) and
dw_hdmi_qp_rk3576_irq() (around line 382), does the unconditional call to
enable_hpd() race with the DRM core?

With the introduction of the .enable_hpd and .disable_hpd PHY operations,
the DRM core expects to dynamically control HPD activation (for instance,
masking it during suspend). If an HPD event triggers just before the core
disables HPD, the hardirq handler schedules the threaded handler. Since the
threaded handler finishes by unconditionally unmasking the interrupt via
enable_hpd(), won't this override the DRM core's explicit request to keep
the interrupt disabled? This seems like it could leave the interrupt armed
unexpectedly and potentially cause spurious wakeups.

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be an issue
in the hardirq handlers dw_hdmi_qp_rk3588_hardirq() (around line 394) and
dw_hdmi_qp_rk3576_hardirq() (around line 367).

These functions read the shared status registers (RK3588_GRF_SOC_STATUS1 or
RK3576_IOC_HDMI_HPD_STATUS) and check if the value is non-zero without
applying a port-specific mask. Because no mask is applied, could any set bit
in these shared registers trigger a spurious thread wakeup?

Additionally, if these registers reflect the physical level (as used in
read_hpd), an unplug event would result in a level of 0. If no other bits
are set, the handler evaluates "if (intr_stat)" as false and returns
IRQ_NONE. Does this mean the driver will fail to wake the thread and
completely miss the disconnect event?

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
in the unbind path involving dw_hdmi_qp_rockchip_unbind() and the IRQ
handlers.

In the unbind path, cancel_delayed_work_sync() is called to stop the HPD
debounce work, but the hardware interrupt isn't explicitly disabled first.
If an HPD interrupt fires during or just after the cancellation, the hardirq
will wake the threaded handler (e.g., dw_hdmi_qp_rk3588_irq() at line 409),
which then calls mod_delayed_work() to re-arm the timer.

When the module unloads or the workqueue is destroyed, won't this pending
timer fire and execute unmapped memory, resulting in a kernel panic? Should
the IRQ be explicitly disabled before cancelling the delayed work?

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