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@amd.com Reported-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Christian König christian.koenig@amd.com Cc: stable@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); } dma_resv_iter_end(&cursor);
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@amd.com Reported-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Christian König christian.koenig@amd.com Cc: stable@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;
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.
Regards, Lucas
} dma_resv_iter_end(&cursor);
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@amd.com Reported-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Christian König christian.koenig@amd.com Cc: stable@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.
Regards, Christian.
Regards, Lucas
} dma_resv_iter_end(&cursor);
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@amd.com Reported-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Christian König christian.koenig@amd.com Cc: stable@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.
Regards, Lucas
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@amd.com Reported-by: Lucas Stach l.stach@pengutronix.de Signed-off-by: Christian König christian.koenig@amd.com Cc: stable@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
linaro-mm-sig@lists.linaro.org
-
Christian König -
Lucas Stach