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

[Esben Haabendal <esben@geanix.com>]

"Esben Haabendal" <esben@geanix.com> writes:

> "Herve Codina" <herve.codina@bootlin.com> writes:
>
>> Hi Esben,
>>
>> On Mon, 10 Aug 2026 09:19:40 +0200
>> Esben Haabendal <esben@geanix.com> 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@vger.kernel.org
>>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>>> ---
>>>  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().

Scratch that. I will squash the two patches into one, go back to only
calling enable_irq() when sn65dsi83_reset_pipe() succeeds, but ensure
that drm_bridge_exit() is always called. Basically what I had in v3,
which you both (Luca and Herve) seemed fine with.

I try to explain the situation a bit better in the new squashed commit
message. If nothing else, it helped myself understand what was going on :)

Is it okay to send v5 with this or do I have to wait a bit longer?

/Esben