[PATCH] dma-buf: fix timeout handling in dma_resv_wait_timeout

[Christian König]

Am 28.01.25 um 15:41 schrieb Lucas Stach:
> Am Dienstag, dem 28.01.2025 um 15:28 +0100 schrieb Christian König:
>> Am 28.01.25 um 11:48 schrieb Lucas Stach:
>>> Hi Christian,
>>>
>>> Am Dienstag, dem 28.01.2025 um 11:08 +0100 schrieb Christian König:
>>>> Even the kerneldoc says that with a zero timeout the function should not
>>>> wait for anything, but still return 1 to indicate that the fences are
>>>> signaled now.
>>>>
>>>> Unfortunately that isn't what was implemented, instead of only returning
>>>> 1 we also waited for at least one jiffies.
>>>>
>>>> Fix that by adjusting the handling to what the function is actually
>>>> documented to do.
>>>>
>>>> Reported-by: Marek Olšák <marek.olsak at amd.com>
>>>> Reported-by: Lucas Stach <l.stach at pengutronix.de>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> Cc: <stable at vger.kernel.org>
>>>> ---
>>>>    drivers/dma-buf/dma-resv.c | 11 ++++++-----
>>>>    1 file changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>>>> index 5f8d010516f0..ae92d9d2394d 100644
>>>> --- a/drivers/dma-buf/dma-resv.c
>>>> +++ b/drivers/dma-buf/dma-resv.c
>>>> @@ -684,11 +684,12 @@ long dma_resv_wait_timeout(struct dma_resv *obj, enum dma_resv_usage usage,
>>>>    	dma_resv_iter_begin(&cursor, obj, usage);
>>>>    	dma_resv_for_each_fence_unlocked(&cursor, fence) {
>>>>    
>>>> -		ret = dma_fence_wait_timeout(fence, intr, ret);
>>>> -		if (ret <= 0) {
>>>> -			dma_resv_iter_end(&cursor);
>>>> -			return ret;
>>>> -		}
>>>> +		ret = dma_fence_wait_timeout(fence, intr, timeout);
>>>> +		if (ret <= 0)
>>>> +			break;
>>>> +
>>>> +		/* Even for zero timeout the return value is 1 */
>>>> +		timeout = min(timeout, ret);
>>> This min construct and the comment confused me a bit at first. I think
>>> it would be easier to read as
>>>
>>> /* With a zero timeout dma_fence_wait_timeout makes up a
>>>    * positive return value for already signaled fences.
>>>    */
>>> if (timeout)
>>> 	timeout = ret;
>> Good point, going to change that.
>>
>>> Also please change the initialization of ret on top of the function to
>>> ret = 1 so it has the right value when no fences are present. Now that
>>> you use the timeout variable for the fence wait, there is no point in
>>> initializing ret to the timeout.
>> When no fences are present returning 1 would be incorrect if the timeout
>> value was non zero.
>>
>> Only when the timeout value is zero we should return 1 to indicate that
>> there is nothing to wait for.
>>
> Uh, yea didn't think about this.
>
> Honestly, making up this positive return value requires one to think
> really hard about those code paths. This would all be much cleaner and
> the chaining of multiple waits, like in the function changed here,
> would be much more natural when a 0 return would also be treated as a
> ordinary successful wait and timeouts would be signaled with ETIMEDOUT.
> But that's a large change with lots of callsites to audit, maybe for
> another day.

Yeah, I've suggested that before as well.

But the wait_event_timeout* interfaces follows the same convention: 
https://elixir.bootlin.com/linux/v6.12.11/source/include/linux/wait.h#L393

So we used with that for some reason. My educated guess is that it made 
migration easier because drivers were using wait_event_timeout* before 
dma_resv/dma_fence was invented.

Regards,
Christian.

>
> Regards,
> Lucas