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@xxxxxxx>
Reported-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
---
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
