On 25-12-2022 13:17, Deepak R Varma wrote:
> The refcount_* APIs are designed to address known issues with the
> atomic_t APIs for reference counting. They provide following distinct
> advantages:
> - protect the reference counters from overflow/underflow
> - avoid use-after-free errors
> - provide improved memory ordering guarantee schemes
> - neater and safer.
> Hence, convert the atomic_t count member variable and associated
> atomic_*() API calls to equivalent refcount_t type and refcount_*() API
> calls.
>
> This patch proposal address the following warnings generated by
> the atomic_as_refcounter.cocci coccinelle script
> atomic_add_unless
>
> Signed-off-by: Deepak R Varma <drv@xxxxxxxxx>
> ---
> Please note:
> 1. Proposed changes are compile tested only.
> 2. This patch 1/2 is required to be applied before patch 2/2 due to
> interdependency.
>
> Changes in v2:
> 1. Patch added to the patch series.
> 2. Handle build issues Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Earlier a standalone patch was sent for the i915 base driver only. The
> Kernel Test Robot reported build failure for additional atomic_*() calls
> specific to i915 debugging support when enabled. This version now includes
> those changes as well.
>
>
> drivers/gpu/drm/i915/i915_active.c | 28 +++++++++++++-----------
> drivers/gpu/drm/i915/i915_active.h | 6 ++---
> drivers/gpu/drm/i915/i915_active_types.h | 4 ++--
> 3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 7412abf166a8..5e58d8b1e947 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -92,14 +92,14 @@ static void debug_active_init(struct i915_active *ref)
> static void debug_active_activate(struct i915_active *ref)
> {
> lockdep_assert_held(&ref->tree_lock);
> - if (!atomic_read(&ref->count)) /* before the first inc */
> + if (!refcount_read(&ref->count)) /* before the first inc */
> debug_object_activate(ref, &active_debug_desc);
> }
>
> static void debug_active_deactivate(struct i915_active *ref)
> {
> lockdep_assert_held(&ref->tree_lock);
> - if (!atomic_read(&ref->count)) /* after the last dec */
> + if (!refcount_read(&ref->count)) /* after the last dec */
> debug_object_deactivate(ref, &active_debug_desc);
> }
>
> @@ -133,7 +133,7 @@ __active_retire(struct i915_active *ref)
> GEM_BUG_ON(i915_active_is_idle(ref));
>
> /* return the unused nodes to our slabcache -- flushing the allocator */
> - if (!atomic_dec_and_lock_irqsave(&ref->count, &ref->tree_lock, flags))
> + if (!refcount_dec_and_lock_irqsave(&ref->count, &ref->tree_lock, &flags))
> return;
>
> GEM_BUG_ON(rcu_access_pointer(ref->excl.fence));
> @@ -179,8 +179,8 @@ active_work(struct work_struct *wrk)
> {
> struct i915_active *ref = container_of(wrk, typeof(*ref), work);
>
> - GEM_BUG_ON(!atomic_read(&ref->count));
> - if (atomic_add_unless(&ref->count, -1, 1))
> + GEM_BUG_ON(!refcount_read(&ref->count));
> + if (refcount_dec_not_one(&ref->count))
I'm not sure if this is correct here, I assume we should be adding instead here its decrementing ?
> return;
>
> __active_retire(ref);
> @@ -189,8 +189,8 @@ active_work(struct work_struct *wrk)
> static void
> active_retire(struct i915_active *ref)
> {
> - GEM_BUG_ON(!atomic_read(&ref->count));
> - if (atomic_add_unless(&ref->count, -1, 1))
> + GEM_BUG_ON(!refcount_read(&ref->count));
> + if (refcount_dec_not_one(&ref->count))
> return;
>
> if (ref->flags & I915_ACTIVE_RETIRE_SLEEPS) {
> @@ -354,7 +354,7 @@ void __i915_active_init(struct i915_active *ref,
> ref->cache = NULL;
>
> init_llist_head(&ref->preallocated_barriers);
> - atomic_set(&ref->count, 0);
> + refcount_set(&ref->count, 0);
> __mutex_init(&ref->mutex, "i915_active", mkey);
> __i915_active_fence_init(&ref->excl, NULL, excl_retire);
> INIT_WORK(&ref->work, active_work);
> @@ -445,7 +445,7 @@ int i915_active_add_request(struct i915_active *ref, struct i915_request *rq)
>
> if (replace_barrier(ref, active)) {
> RCU_INIT_POINTER(active->fence, NULL);
> - atomic_dec(&ref->count);
> + refcount_dec(&ref->count);
> }
> if (!__i915_active_fence_set(active, fence))
> __i915_active_acquire(ref);
> @@ -488,14 +488,16 @@ i915_active_set_exclusive(struct i915_active *ref, struct dma_fence *f)
> bool i915_active_acquire_if_busy(struct i915_active *ref)
> {
> debug_active_assert(ref);
> - return atomic_add_unless(&ref->count, 1, 0);
> + return refcount_add_not_zero(1, &ref->count);
> }
>
> static void __i915_active_activate(struct i915_active *ref)
> {
> spin_lock_irq(&ref->tree_lock); /* __active_retire() */
> - if (!atomic_fetch_inc(&ref->count))
> + if (!refcount_inc_not_zero(&ref->count)) {
> + refcount_inc(&ref->count);
> debug_active_activate(ref);
> + }
> spin_unlock_irq(&ref->tree_lock);
> }
>
> @@ -757,7 +759,7 @@ int i915_sw_fence_await_active(struct i915_sw_fence *fence,
> void i915_active_fini(struct i915_active *ref)
> {
> debug_active_fini(ref);
> - GEM_BUG_ON(atomic_read(&ref->count));
> + GEM_BUG_ON(refcount_read(&ref->count));
> GEM_BUG_ON(work_pending(&ref->work));
> mutex_destroy(&ref->mutex);
>
> @@ -927,7 +929,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
>
> first = first->next;
>
> - atomic_dec(&ref->count);
> + refcount_dec(&ref->count);
> intel_engine_pm_put(barrier_to_engine(node));
>
> kmem_cache_free(slab_cache, node);
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index 7eb44132183a..116c7c28466a 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -193,14 +193,14 @@ void i915_active_release(struct i915_active *ref);
>
> static inline void __i915_active_acquire(struct i915_active *ref)
> {
> - GEM_BUG_ON(!atomic_read(&ref->count));
> - atomic_inc(&ref->count);
> + GEM_BUG_ON(!refcount_read(&ref->count));
> + refcount_inc(&ref->count);
> }
>
> static inline bool
> i915_active_is_idle(const struct i915_active *ref)
> {
> - return !atomic_read(&ref->count);
> + return !refcount_read(&ref->count);
> }
>
> void i915_active_fini(struct i915_active *ref);
> diff --git a/drivers/gpu/drm/i915/i915_active_types.h b/drivers/gpu/drm/i915/i915_active_types.h
> index b02a78ac87db..152a3a25d9f7 100644
> --- a/drivers/gpu/drm/i915/i915_active_types.h
> +++ b/drivers/gpu/drm/i915/i915_active_types.h
> @@ -7,7 +7,7 @@
> #ifndef _I915_ACTIVE_TYPES_H_
> #define _I915_ACTIVE_TYPES_H_
>
> -#include <linux/atomic.h>
> +#include <linux/refcount.h>
> #include <linux/dma-fence.h>
> #include <linux/llist.h>
> #include <linux/mutex.h>
> @@ -23,7 +23,7 @@ struct i915_active_fence {
> struct active_node;
>
> struct i915_active {
> - atomic_t count;
> + refcount_t count;
> struct mutex mutex;
>
> spinlock_t tree_lock;
> --
> 2.34.1
>
>
Regards,
~Praveen.
