<[email protected]> 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 <[email protected]>
>
> 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/[email protected]/
>
>> 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
