Re: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing SRCU read lock release in error path of sn65dsi83_atomic_enable().

Pre-existing issues:
- [High] Missing SRCU read lock release in error path of sn65dsi83_reset_work().
--

--- Patch [2]: [PATCH 2/2] drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking ---
commit 0c69248e520fff24e51d45bfab961991fbdb4267
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Fix problem with premature PLL locking

This patch moves the PLL locking sequence from atomic_pre_enable to
atomic_enable. This ensures the DSI HS clock is running before attempting to
lock the PLL.

Link: https://lore.kernel.org/all/20260120-mtkdsi-v1-1-b0f4094f3ac3@gmail.com/

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 32e9eadd7452f..e87bed3a6640b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c

[ ... ]

> @@ -712,11 +697,24 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>  {
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>  	unsigned int pval;
> -	int idx;
> +	int idx, ret;
>  
>  	if (!drm_bridge_enter(bridge, &idx))
>  		return;
>  
> +	/* Enable PLL */
> +	regmap_write(ctx->regmap, REG_RC_PLL_EN, REG_RC_PLL_EN_PLL_EN);
> +	usleep_range(3000, 4000);
> +	ret = regmap_read_poll_timeout(ctx->regmap, REG_RC_LVDS_PLL, pval,
> +				       pval & REG_RC_LVDS_PLL_PLL_EN_STAT,
> +				       1000, 100000);
> +	if (ret) {
> +		dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
> +		/* On failure, disable PLL again and exit. */
> +		regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> +		return;

[Severity: High]
Does this error path leak the SRCU read lock acquired by drm_bridge_enter()?

If the PLL lock fails and regmap_read_poll_timeout() returns an error, the
function returns immediately without calling drm_bridge_exit(idx). 

This unreleased lock could cause subsequent calls to synchronize_srcu() to
hang during module unload or bridge unplug.

> +	}
> +
>  	/* Clear all errors that got asserted during initialization. */
>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);

[Severity: High]
This is a pre-existing issue, but I noticed a similar pattern in another
function in this file. Does sn65dsi83_reset_work() also leak the SRCU read
lock on failure?

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
    if (!drm_bridge_enter(&ctx->bridge, &idx))
        return;
    ...
    ret = sn65dsi83_reset_pipe(ctx);
    if (ret) {
        dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
        return;
    }
    ...
    drm_bridge_exit(idx);
}

Like the atomic_enable path, if sn65dsi83_reset_pipe() fails, it appears to
return early without calling drm_bridge_exit(idx).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-ti-sn65dsi83-fixes-v1-0-d85eb5342b98@geanix.com?part=2