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

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> 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.

Yes, that does look like a bug. I will fix it for v2.

>> +	}
>> +
>>  	/* 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).

Yes.

But is it enough to call drm_bridge_exit(idx), or do we want to
enable_irq() also? We are calling disable_irq_nosync() before scheduling
reset_ork, so if don't enable it again in case of error, it will be
stuck disabled. But what is supposed to happen if/when
sn65dsi83_reset_pipe() fails?

If we enable_irq() again, are we just going to endure endless pain?

/Esben