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 <[email protected]> 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. > 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 [ ... ] > @@ -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. > + \ > + 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. > + sdt_might_sleep_end(); \ > + __ret; \ > +}) > + [ ... ] > @@ -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? > + 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 evaluate t only once before passing it to sdt_might_sleep_start_timeout() and __dma_fence_wait_any_timeout(), similar to the other macros? > + sdt_might_sleep_end(); \ > + __ret; \ > +}) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=28
