Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dma-fence: broken KUnit test_signaling() after keeping fence->ops on signal - [Low] dma-fence: sparse warnings from remaining rcu_access_pointer() on fence->ops --
commit 4607587506fc5e0428c9d4ca148fc5a0c94b0003 Author: Philipp Stanner <[email protected]> dma-fence: Remove RCU protection for fence->ops This commit removes the RCU protection and __rcu annotation for struct dma_fence::ops, and stops clearing fence->ops to NULL when signaling fences without release or wait callbacks. > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index bd58688b81a72..5dab647e3f3f9 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -353,7 +353,6 @@ void __dma_fence_might_wait(void) > void dma_fence_signal_timestamp_locked(struct dma_fence *fence, > ktime_t timestamp) > { > - const struct dma_fence_ops *ops; > struct dma_fence_cb *cur, *tmp; > struct list_head cb_list; > > @@ -365,15 +364,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence > *fence, > > trace_dma_fence_signaled(fence); > > - /* > - * When neither a release nor a wait operation is specified set the ops > - * pointer to NULL to allow the fence structure to become independent > - * from who originally issued it. > - */ > - ops = rcu_dereference_protected(fence->ops, true); > - if (!ops->release && !ops->wait) > - RCU_INIT_POINTER(fence->ops, NULL); > - [Severity: Medium] Does keeping fence->ops set here break test_signaling() in drivers/dma-buf/st-dma-fence.c? In drivers/dma-buf/st-dma-fence.c, test_signaling() (added in commit 3695d754ffbb ("dma-buf/selftests: test RCU ops and inline lock v2")) creates a mock fence using mock_ops (which defines neither .release nor .wait), signals it via dma_fence_check_and_signal(), and then checks: if (rcu_dereference_protected(f->ops, true)) { KUNIT_FAIL(test, "Fence ops not cleared on signal"); goto err_free; } Since dma_fence_signal_timestamp_locked() no longer clears fence->ops to NULL, won't this KUnit check unconditionally fail? Additionally, passing the non-__rcu f->ops pointer to rcu_dereference_protected() there will trigger a Sparse address-space warning. > /* Stash the cb_list before replacing it with the timestamp */ > list_replace(&fence->cb_list, &cb_list); > [ ... ] > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index dd07d128adc3b..874848a4aec46 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -72,7 +72,7 @@ struct dma_fence { > spinlock_t *extern_lock; > spinlock_t inline_lock; > }; > - const struct dma_fence_ops __rcu *ops; > + const struct dma_fence_ops *ops; [Severity: Low] Should the remaining rcu_access_pointer(fence->ops) callers in the tree also be updated now that the __rcu annotation is removed from struct dma_fence::ops? While dma_fence_is_array() and dma_fence_is_chain() are updated below, several other functions converted in commits f4cc3ab824d6, 764f241ad227, and 7519ba791b00 still wrap fence->ops in rcu_access_pointer(), which causes Sparse to emit address-space mismatch warnings when building with C=1: drivers/gpu/drm/drm_crtc.c:fence_to_crtc(): BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops); drivers/gpu/drm/scheduler/sched_fence.c:to_drm_sched_fence(): if (rcu_access_pointer(f->ops) == &drm_sched_fence_ops_scheduled) return container_of(f, struct drm_sched_fence, scheduled); if (rcu_access_pointer(f->ops) == &drm_sched_fence_ops_finished) return container_of(f, struct drm_sched_fence, finished); drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c:to_amdgpu_amdkfd_fence(): if (rcu_access_pointer(f->ops) == &amdkfd_fence_ops) return fence; drivers/gpu/host1x/fence.c:host1x_fence_extract(): if (rcu_access_pointer(fence->ops) != &host1x_syncpt_fence_ops) return -EINVAL; > /* > * We clear the callback list on kref_put so that by the time we > * release the fence it is unused. No one should be adding to the [ ... ] > @@ -769,7 +761,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops; > */ > static inline bool dma_fence_is_array(struct dma_fence *fence) > { > - return rcu_access_pointer(fence->ops) == &dma_fence_array_ops; > + return fence->ops == &dma_fence_array_ops; > } > > /** > @@ -780,7 +772,7 @@ static inline bool dma_fence_is_array(struct dma_fence > *fence) > */ > static inline bool dma_fence_is_chain(struct dma_fence *fence) > { > - return rcu_access_pointer(fence->ops) == &dma_fence_chain_ops; > + return fence->ops == &dma_fence_chain_ops; > } > > /** -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
