Boris Brezillon boris.brezillon@free-electrons.com writes:
On Wed, 22 Nov 2017 13:16:08 -0800 Eric Anholt eric@anholt.net wrote:
Boris Brezillon boris.brezillon@free-electrons.com writes:
With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's passed a refcount object that has its counter set to 0. In this driver, this is a valid use case since we want to increment ->usecnt only when the BO object starts to be used by real HW components and this is definitely not the case when the BO is created.
Fix the problem by using refcount_inc_not_zero() instead of refcount_inc() and fallback to refcount_set(1) when refcount_inc_not_zero() returns false. Note that this 2-steps operation is not racy here because the whole section is protected by a mutex which guarantees that the counter does not change between the refcount_inc_not_zero() and refcount_set() calls.
If we're not following the model, and protecting the refcount by a mutex, shouldn't we just be using addition and subtraction instead of refcount's atomics?
Actually, this mutex is protecting the bo->madv value which has to be checked when the counter reaches 0 (when decrementing) or 1 (when incrementing). We just benefit from this protection here, but ideally it would be better to have an refcount_inc_allow_zero() as suggested by Daniel.
Let me restate this to see if it makes sense: The refcount is always >= 0, this is is the only path that increases the refcount from 0 to 1, and it's (incidentally) protected by a mutex, so there's no race between the attempted increase from nonzero and the set from nonzero to 1.
This seems fine to me as a bugfix.