Re: [PATCH v19 28/40] dept: assign unique dept_key to each distinct dma fence caller

On Tue, Jul 07, 2026 at 07:33:44AM +0000, sashiko-bot@kernel.org wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly introduced statement expression macros for DMA fence waiting evaluate their timeout argument (`t`) multiple times.
- [Medium] Local variable shadowing in the statement expression macros due to the unhygienic `__ret` variable declaration.
--

commit 4d4caad8d173eededcba1493c060f5ddecbd6b70
Author: Byungchul Park [off-list ref]

dept: assign unique dept_key to each distinct dma fence caller

dma fence can be used at various points in the code and it's very hard
to distinguish dma fences between different usages.  Using a single
dept_key for all the dma fences could trigger false positive reports.

quoted

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index d4c92fd350924..3732849a30b7e 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h

[ ... ]

quoted

@@ -370,8 +370,22 @@ bool dma_fence_check_and_signal_locked(struct dma_fence *fence);
 void dma_fence_signal_locked(struct dma_fence *fence);
 void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
 void dma_fence_signal_timestamp_locked(struct dma_fence *fence, ktime_t timestamp);
-signed long dma_fence_default_wait(struct dma_fence *fence,
+signed long __dma_fence_default_wait(struct dma_fence *fence,
                                 bool intr, signed long timeout);
+
+/*
+ * Associate every caller with its own dept map.
+ */
+#define dma_fence_default_wait(f, intr, t)                           \
+({                                                                   \
+     signed long __ret;                                              \

[Severity: Medium]
Could this local variable declaration lead to shadowing issues? If a caller
passes arguments that reference a variable named __ret in their own scope,
this uninitialized local variable will shadow the caller's variable, causing
undefined behavior.

I will fix it.

quoted

+                                                                     \
+     sdt_might_sleep_start_timeout(NULL, t);                         \
+     __ret = __dma_fence_default_wait(f, intr, t);                   \

[Severity: Medium]
Is it safe to evaluate the timeout argument t multiple times here?

If a caller passes an expression with side effects, such as timeout--, it
would be evaluated twice, which could lead to incorrect timeout logic.

Sure, I will fix it.

quoted

+     sdt_might_sleep_end();                                          \
+     __ret;                                                          \
+})
+

[ ... ]

quoted

@@ -628,12 +642,37 @@ static inline ktime_t dma_fence_timestamp(struct dma_fence *fence)
      return fence->timestamp;
 }

-signed long dma_fence_wait_timeout(struct dma_fence *,
+signed long __dma_fence_wait_timeout(struct dma_fence *,
                                 bool intr, signed long timeout);
-signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
+signed long __dma_fence_wait_any_timeout(struct dma_fence **fences,
                                     uint32_t count,
                                     bool intr, signed long timeout,
                                     uint32_t *idx);
+/*
+ * Associate every caller with its own dept map.
+ */
+#define dma_fence_wait_timeout(f, intr, t)                           \
+({                                                                   \
+     signed long __ret;                                              \
+                                                                     \
+     sdt_might_sleep_start_timeout(NULL, t);                         \
+     __ret = __dma_fence_wait_timeout(f, intr, t);                   \

[Severity: Medium]
Does this code also suffer from the same macro hygiene issues with shadowing
__ret and evaluating the t argument twice?

Okay, I will fix it.

quoted

+     sdt_might_sleep_end();                                          \
+     __ret;                                                          \
+})
+
+/*
+ * Associate every caller with its own dept map.
+ */
+#define dma_fence_wait_any_timeout(fpp, count, intr, t, idx)         \
+({                                                                   \
+     signed long __ret;                                              \
+                                                                     \
+     sdt_might_sleep_start_timeout(NULL, t);                         \
+     __ret = __dma_fence_wait_any_timeout(fpp, count, intr, t, idx); \

[Severity: Medium]
Would it be safer to use a more unique variable name instead of __ret, and

Suggestion?

evaluate t only once before passing it to sdt_might_sleep_start_timeout()
and __dma_fence_wait_any_timeout(), similar to the other macros?

Sure.

	Byungchul

quoted

+     sdt_might_sleep_end();                                          \
+     __ret;                                                          \
+})

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706061928.66713-1-byungchul@sk.com?part=28