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 <[email protected]> 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/[email protected]?part=26
