Re: [PATCH v4 1/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[Esben Haabendal]

"Herve Codina" <herve.codina@xxxxxxxxxxx> writes:

> Hi Esben,
>
> On Mon, 10 Aug 2026 09:19:40 +0200
> Esben Haabendal <esben@xxxxxxxxxx> wrote:
>
>> The error handling of sn65dsi83_reset_pipe() failure in
>> sn65dsi83_reset_work() has been a bit strange all the time, missing both
>> enable_irq() and drm_bridge_exit() in case of failure. But since
>> sn65dsi83_reset_pipe() have never been failing (always returning 0), it has
>> not caused any problems.
>>
>> As we are going to change sn65dsi83_reset_pipe() to return error values in
>> case of failure, we need to fix this.
>>
>> Fixes: ad5c6ecef27e ("drm: bridge: ti-sn65dsi83: Add error recovery mechanism")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: Esben Haabendal <esben@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> index 42b451432bbb..0ccfff401b2a 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
>> @@ -417,10 +417,9 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>>
>>  	/* Reset the pipe */
>>  	ret = sn65dsi83_reset_pipe(ctx);
>> -	if (ret) {
>> +	if (ret)
>>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
>> -		return;
>> -	}
>> +
>>  	if (ctx->irq)
>>  		enable_irq(ctx->irq);
>>
>>
>
> In this v4, enable_irq() is called even if we cannot reset the pipe. I know that
> sn65dsi83_reset_pipe() always returns 0 (and this will be change) but we have to
> take care here.
>
> If the pipe cannot be reset, the sn65dsi83 component could be in a state where
> it can force the interrupt line. This was the reason the irq is disabled
>   https://elixir.bootlin.com/linux/v7.2/source/drivers/gpu/drm/bridge/ti-sn65dsi83.c#L455

Do we agree that this patch series (patch 1 and 2 combined) does not
change this behavior compared to the current state?

Currently, because sn65dsi83_reset_pipe() always returns 0, we call
enable_irq() even if we cannot reset the pipe. So I am not changing
behavior.

I agree, that this might not be the right approach, but it makes more
sense to me to address that specifically on top of the change in this
patch, so we can all agree on what we are changing and why.

Can we keep this commit?

Should I add a new patch that makes the change to this series, or is it
okay to postpone this to later?

Or alternatively, do you want me to drop this patch? And if so, I guess
we should at least add some comment to the next patch about the behavior
change that it introduces in sn65dsi83_reset_work().

/Esben