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