Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/bridge: analogix_dp: unclocked IRQ MMIO after moving enable_irq() to bind - [High] drm/bridge: analogix_dp: IRQ left enabled with dangling drm_dev on bind failure --
commit f05b45f646c93eccaff985e6b484d109024db4f2 Author: Damon Ding <[email protected]> drm/bridge: analogix_dp: Move IRQ enable/disable to bind/unbind This commit moves enable_irq() and disable_irq() from the bridge enable and disable handlers into analogix_dp_bind() and analogix_dp_unbind() so that the IRQ remains active across the bound lifetime of the bridge. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index b92d285461926..addd319719de4 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1018,7 +1018,6 @@ static int analogix_dp_set_bridge(struct > analogix_dp_device *dp) > goto out_dp_init; > } > > - enable_irq(dp->irq); > return 0; > > out_dp_init: > @@ -1160,8 +1159,6 @@ static void analogix_dp_bridge_disable(struct > drm_bridge *bridge) > if (dp->dpms_mode != DRM_MODE_DPMS_ON) > return; > > - disable_irq(dp->irq); > - > analogix_dp_set_analog_power_down(dp, POWER_ALL, 1); > > pm_runtime_put_sync(dp->dev); [Severity: High] Can removing disable_irq(dp->irq) before pm_runtime_put_sync(dp->dev) allow analogix_dp_hardirq() or analogix_dp_irq_thread() to perform MMIO accesses on dp->reg_base after analogix_dp_suspend() has gated the clocks? Previously, disable_irq(dp->irq) synchronized any in-flight hardirq or scheduled threaded handler before pm_runtime_put_sync(dp->dev) ran analogix_dp_suspend(): drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_suspend() { phy_power_off(dp->phy); if (dp->plat_data->power_off) dp->plat_data->power_off(dp->plat_data); clk_disable_unprepare(dp->clock); return 0; } In native HPD configurations (!dp->hpd_gpiod), analogix_dp_resume() -> analogix_dp_init_dp() -> analogix_dp_config_interrupt() unmasks HPD interrupts in the controller registers. If an HPD interrupt fires shortly before runtime suspend (for example, right before analogix_dp_bridge_disable(), before the out_dp_init error path in analogix_dp_set_bridge(), or when the autosuspend timer expires after analogix_dpaux_transfer()), analogix_dp_hardirq() returns IRQ_WAKE_THREAD without holding a runtime PM reference. If analogix_dp_suspend() then gates dp->pclk and dp->clock before analogix_dp_irq_thread() runs, analogix_dp_irq_thread() will access ANALOGIX_DP_COMMON_INT_STA_4 and ANALOGIX_DP_INT_STA while the controller is unclocked: drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_irq_thread() { ... irq_type = analogix_dp_get_irq_type(dp); if (!dp->hpd_gpiod && irq_type) analogix_dp_clear_hotplug_interrupts(dp, irq_type); ... } Even with the later patches in this series ("drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection" and "drm/bridge: analogix_dp: Handle HPD notification from downstream bridge"), analogix_dp_require_pm_for_hpd_irq() only keeps runtime PM active during bind for Rockchip when !dp->hpd_gpiod && !dp->force_hpd && !dp->hpd_notify_mode. Exynos native-HPD boards (EXYNOS_DP with !dp->hpd_gpiod) and Rockchip force-hpd boards (!dp->hpd_gpiod && dp->force_hpd, where analogix_dp_config_interrupt() still unmutes DP_IRQ_TYPE_HP_CHANGE and analogix_dp_force_hpd() toggles HPD_STATUS) can still runtime-suspend with dp->irq enabled and unsynchronized. > @@ -1574,6 +1571,8 @@ int analogix_dp_bind(struct analogix_dp_device *dp, > struct drm_device *drm_dev) > goto err_unregister_aux; > } > > + enable_irq(dp->irq); > + > return 0; [Severity: High] What happens if a caller of analogix_dp_bind() fails after analogix_dp_bind() returns 0? Both rockchip_dp_bind() and exynos_dp_bind() call drm_bridge_connector_init() immediately after analogix_dp_bind(), and neither calls analogix_dp_unbind() if drm_bridge_connector_init() fails: drivers/gpu/drm/rockchip/analogix_dp-rockchip.c:rockchip_dp_bind() { ... ret = analogix_dp_bind(dp->adp, drm_dev); if (ret) goto err_cleanup_encoder; connector = drm_bridge_connector_init(dp->drm_dev, dp->plat_data.encoder); if (IS_ERR(connector)) { ret = PTR_ERR(connector); dev_err(dp->dev, "Failed to initialize bridge_connector\n"); goto err_cleanup_encoder; } return 0; err_cleanup_encoder: dp->encoder.encoder.funcs->destroy(&dp->encoder.encoder); return ret; } drivers/gpu/drm/exynos/exynos_dp.c:exynos_dp_bind() { ... ret = analogix_dp_bind(dp->adp, dp->drm_dev); if (ret) { dp->encoder.funcs->destroy(&dp->encoder); return ret; } connector = drm_bridge_connector_init(dp->drm_dev, dp->plat_data.encoder); if (IS_ERR(connector)) { ret = PTR_ERR(connector); dev_err(dp->dev, "Failed to initialize bridge_connector\n"); return ret; } ... } Because component_bind_all() only unbinds previously bound components when a component's bind callback fails, the master bind path frees drm_dev via drm_dev_put() while dp->irq remains enabled and dp->drm_dev still points to the freed drm_device (and in the follow-up commit "drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection", runtime PM also remains active). If an HPD interrupt fires afterwards, wouldn't analogix_dp_irq_thread() pass the freed dp->drm_dev pointer to drm_helper_hpd_irq_event()? drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_irq_thread() { ... if (hpd_detected) { dev_dbg(dp->dev, "Detected cable status changed!\n"); if (dp->drm_dev) drm_helper_hpd_irq_event(dp->drm_dev); } ... } [ ... ] > > > > 完美。 -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
