[PATCH] drm/bridge: analogix_dp: Fix runtime PM refcount leak in set_bridge()

[PATCH] drm/bridge: analogix_dp: Fix runtime PM refcount leak in set_bridge()

[laixiangwu]

analogix_dp_set_bridge() calls pm_runtime_get_sync() to power up the
DP core, but if the subsequent analogix_dp_init_analog_func() fails it
returns the error directly, without the matching pm_runtime_put_sync().
The two other error paths in the function already jump to the
out_dp_init label, which drops the reference.

Every failed bridge enable therefore increments the runtime PM usage
counter, and repeated failures (e.g. during hotplug storms) prevent
the device from ever runtime suspending again.

Jump to out_dp_init like the other error paths so the runtime PM
reference is released.

Fixes: 3efe2ace7f5b ("drm/bridge: analogix_dp: move basic controller init into runtime PM")
Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6df696374..c9c1375a9 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -998,7 +998,7 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 
 	ret = analogix_dp_init_analog_func(dp);
 	if (ret)
-		return ret;
+		goto out_dp_init;
 
 	/*
 	 * According to DP spec v1.3 chap 3.5.1.2 Link Training,

Re: [PATCH] drm/bridge: analogix_dp: Fix runtime PM refcount leak in set_bridge()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/bridge: analogix_dp: dead error check on analogix_dp_init_analog_func()
--

commit 84ec40424622aaaaa038f462b132aaae0d50ac8c
Author: laixiangwu <laixiangwu@cmss.chinamobile.com>

drm/bridge: analogix_dp: Fix runtime PM refcount leak in set_bridge()

This commit changes the error path after analogix_dp_init_analog_func() in
analogix_dp_set_bridge() to jump to out_dp_init so pm_runtime_put_sync() is
called on failure.

>     drm/bridge: analogix_dp: Fix runtime PM refcount leak in set_bridge()
>     
>     analogix_dp_set_bridge() calls pm_runtime_get_sync() to power up the
>     DP core, but if the subsequent analogix_dp_init_analog_func() fails it
>     returns the error directly, without the matching pm_runtime_put_sync().
>     The two other error paths in the function already jump to the
>     out_dp_init label, which drops the reference.
>     
>     Every failed bridge enable therefore increments the runtime PM usage
>     counter, and repeated failures (e.g. during hotplug storms) prevent
>     the device from ever runtime suspending again.
>     
>     Jump to out_dp_init like the other error paths so the runtime PM
>     reference is released.
>     
>     Fixes: 3efe2ace7f5b ("drm/bridge: analogix_dp: move basic controller init into runtime PM")
>     Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6df6963740d2c..c9c1375a93182 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -998,7 +998,7 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>  
>  	ret = analogix_dp_init_analog_func(dp);
>  	if (ret)
> -		return ret;
> +		goto out_dp_init;

[Severity: Medium]
Can analogix_dp_init_analog_func() actually fail here?

In commit 917c8d192ba6ad ("drm/bridge: analogix_dp: don't wait for PLL lock
too early"), which landed in the same series as commit 3efe2ace7f5b, the PLL
lock polling loop and the only error return (-ETIMEDOUT) were removed from
analogix_dp_init_analog_func() in
drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c:

int analogix_dp_init_analog_func(struct analogix_dp_device *dp)
{
	u32 reg;

	analogix_dp_set_analog_power_down(dp, POWER_ALL, 0);

	reg = PLL_LOCK_CHG;
	writel(reg, dp->reg_base + ANALOGIX_DP_COMMON_INT_STA_1);

	reg = readl(dp->reg_base + ANALOGIX_DP_DEBUG_CTL);
	reg &= ~(F_PLL_LOCK | PLL_LOCK_CTRL);
	writel(reg, dp->reg_base + ANALOGIX_DP_DEBUG_CTL);

	/* Power up PLL */
	analogix_dp_set_pll_power_down(dp, 0);

	/* Enable Serdes FIFO function and Link symbol clock domain module */
	reg = readl(dp->reg_base + ANALOGIX_DP_FUNC_EN_2);
	reg &= ~(SERDES_FIFO_FUNC_EN_N | LS_CLK_DOMAIN_FUNC_EN_N
		| AUX_FUNC_EN_N);
	writel(reg, dp->reg_base + ANALOGIX_DP_FUNC_EN_2);
	return 0;
}

Because analogix_dp_init_analog_func() only performs register reads/writes
and void helper calls, it unconditionally returns 0, while the two functions
in analogix_dp_set_bridge() that can fail at runtime
(analogix_dp_detect_hpd() and analogix_dp_commit()) already jump to
out_dp_init.

Since this error branch is unreachable and no runtime PM refcount leak can
occur, would it be better to change analogix_dp_init_analog_func() in
drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c and
drivers/gpu/drm/bridge/analogix/analogix_dp_core.h to return void (matching
analogix_dp_init_dp() and the other analogix_dp_init_*() helpers) and drop
the dead error check in analogix_dp_set_bridge()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030082820.1457-1-laixiangwu@cmss.chinamobile.com?part=1