On 12/6/23 19:37, George Stark wrote:
Hello Waiman
Thanks for the review.
On 12/7/23 00:02, Waiman Long wrote:
On 12/6/23 14:55, Hans de Goede wrote:
Hi,
On 12/6/23 19:58, George Stark wrote:
Hello Hans
Thanks for the review.
On 12/6/23 18:01, Hans de Goede wrote:
Hi George,
...
mutex_destroy() only actually does anything if CONFIG_DEBUG_MUTEXES
is set, otherwise it is an empty inline-stub.
Adding a devres resource to the device just to call an empty inline
stub which is a no-op seems like a waste of resources. IMHO it
would be better to change this to:
static inline int devm_mutex_init(struct device *dev, struct mutex
*lock)
{
mutex_init(lock);
#ifdef CONFIG_DEBUG_MUTEXES
return devm_add_action_or_reset(dev, devm_mutex_release, lock);
#else
return 0;
#endif
}
To avoid the unnecessary devres allocation when
CONFIG_DEBUG_MUTEXES is not set.
Honestly saying I don't like unnecessary devres allocation either
but the proposed approach has its own price:
1) we'll have more than one place with branching if mutex_destroy
is empty or not using indirect condition. If suddenly
mutex_destroy is extended for non-debug code (in upstream branch or
e.g. by someone for local debug) than there'll be a problem.
2) If mutex_destroy is empty or not depends on CONFIG_PREEMPT_RT
option too. When CONFIG_PREEMPT_RT is on mutex_destroy is always
empty.
As I see it only the mutex interface (mutex.h) has to say
definitely if mutex_destroy must be called. Probably we could add
some define to include/linux/mutex.h,like IS_MUTEX_DESTROY_REQUIRED
and declare it near mutex_destroy definition itself.
That (a IS_MUTEX_DESTROY_REQUIRED define) is an interesting idea.
Lets s>
Adding a devres resource to the device just to call an empty inline
stub which is a no-op seems like a waste of resources. IMHO it
would be better to change this to:
static inline int devm_mutex_init(struct device *dev, struct mutex
*lock)
{
mutex_init(lock);
#ifdef CONFIG_DEBUG_MUTEXES
return devm_add_action_or_reset(dev, devm_mutex_release, lock);
#else
return 0;
#endif
}
ee for v3 if the mutex maintainers will accept that and if not
then I guess we will just need to live with the unnecessary devres
allocation.
The purpose of calling mutex_destroy() is to mark a mutex as being
destroyed so that any subsequent call to mutex_lock/unlock will cause
a warning to be printed when CONFIG_DEBUG_MUTEXES is defined. I would
not say that mutex_destroy() is required. Rather it is a nice to have
for catching programming error.
This is quite understandable but probably mutex_destroy() is not the
best name for an optional API. Questions are asked over and over again
if it can be safely ignored taking into account that it could be
extended in the future. Every maintainer makes decision on that question
in his own way and it leads to inconsistency.
devm_mutex_init could take responsibility for calling/dropping
mutex_destroy() on its own.
The DEBUG_MUTEXES code is relatively old and there was no major change
to it for a number of years. I don't see we will make major change to it
in the near future. Of course, thing may change if there are new
requirement that may affect the DEBUG_MUTEXES code.
Cheers,
Longman
Cheers,
Longman
