On 21/11/2022 14:56, Janusz Krzysztofik wrote:
Users of intel_gt_retire_requests_timeout() expect 0 return value on
success. However, we have no protection from passing back 0 potentially
returned by a call to dma_fence_wait_timeout() when it succedes right
after its timeout has expired.
Is this talking about a potential weakness, or ambiguous kerneldoc, of
dma_fence_wait_timeout, dma_fence_default_wait and
i915_request_wait_timeout? They appear to say 0 return means timeout,
implying unsignaled fence. In other words signaled must return positive
remaining timeout. Implementations seems to allow a race which indeed
appears that return 0 and signaled fence is possible.
If dma_fence_wait can indeed return 0 even when a request is signaled,
then how is timeout ?: -ETIME below correct? It isn't a chance for false
negative in its' callers?
Regards,
Tvrtko
Replace 0 with -ETIME before potentially using the timeout value as return
code, so -ETIME is returned if there are still some requests not retired
after timeout, 0 otherwise.
v3: Use conditional expression, more compact but also better reflecting
intention standing behind the change.
v2: Move the added lines down so flush_submission() is not affected.
Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # v5.5+
---
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index edb881d756309..1dfd01668c79c 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
if (remaining_timeout)
*remaining_timeout = timeout;
- return active_count ? timeout : 0;
+ return active_count ? timeout ?: -ETIME : 0;
}
static void retire_work_handler(struct work_struct *work)
