Re: [PATCH v2 01/10] devm-helpers: introduce devm_mutex_init

[George Stark <gnstark@salutedevices.com>]

On 12/7/23 16:01, Christophe Leroy wrote:
> 
> 
> Le 07/12/2023 à 13:51, George Stark a écrit :
>>
>>
>> On 12/7/23 15:28, Christophe Leroy wrote:
>>>
>>>
>>> Le 07/12/2023 à 13:02, Andy Shevchenko a écrit :
>>>> On Thu, Dec 7, 2023 at 1:23 AM George Stark
>>>> <gnstark@salutedevices.com> wrote:
>>>>> On 12/7/23 01:37, Christophe Leroy wrote:
>>>>>> Le 06/12/2023 à 23:14, Christophe Leroy a écrit :
>>>>
>>>> ...
>>>>
>>>>>> Looking at it closer, I have the feeling that you want to do
>>>>>> similar to
>>>>>> devm_gpio_request() in linux/gpio.h :
>>>>>>
>>>>>> In linux/mutex.h, add a prototype for devm_mutex_init() when
>>>>>> CONFIG_DEBUG_MUTEXES is defined and an empty static inline otherwise.
>>>>>> Then define devm_mutex_init() in kernel/locking/mutex-debug.c
>>>>>
>>>>> Yes, this would be almost perfect decision. BTW just as in linux/gpio.h
>>>>> we wouldn't have to include whole "linux/device.h" into mutex.h, only
>>>>> add forward declaration of struct device;
>>>>
>>>> In case you place it into a C-file. Otherwise you need a header for
>>>> the API and that is not acceptable for mutex.h.
>>>>
>>>
>>> Right, that's the reason why I'm suggesting to define devm_mutex_init()
>>> in kernel/locking/mutex-debug.c.
>>>
>>> In linux/mutex.h, you define a stub for when CONFIG_DEBUG_MUTEXES is not
>>> set, and the prototype of devm_mutex_init() when CONFIG_DEBUG_MUTEXES is
>>> set.
>>
>> Something like this:
>>
>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>> index a33aa9eb9fc3..4a6041a7fd44 100644
>> --- a/include/linux/mutex.h
>> +++ b/include/linux/mutex.h
>> @@ -21,6 +21,8 @@
>>    #include <linux/debug_locks.h>
>>    #include <linux/cleanup.h>
>>
>> +struct device;
>> +
>>    #ifdef CONFIG_DEBUG_LOCK_ALLOC
>>    # define __DEP_MAP_MUTEX_INITIALIZER(lockname)            \
>>            , .dep_map = {                    \
>> @@ -127,6 +129,20 @@ extern void __mutex_init(struct mutex *lock, const
>> char *name,
>>     */
>>    extern bool mutex_is_locked(struct mutex *lock);
>>
>> +#ifdef CONFIG_DEBUG_MUTEXES
> 
> There is already a CONFIG_DEBUG_MUTEXES block, can you re-use it ?

those CONFIG_DEBUG_MUTEXES blockd are declared before mutex_init macro :(

> 
>> +
>> +extern int devm_mutex_init(struct device *dev, struct mutex *lock);
> 
> 'extern' is pointless and deprecated for function prototypes.
> I know the kernel is full of them, but it is not a good reason to add
> new ones.

Ok

Sure I will send this patch in the right way and then we could have 
proper review but firstly I'd like to hear from Andy and mutex.h's 
maintainers is it acceptable at all?

> 
>> +
>> +#else
>> +
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +    mutex_init(lock);
>> +    return 0;
>> +}
>> +
>> +#endif
>> +
>>    #else /* !CONFIG_PREEMPT_RT */
>>    /*
>>     * Preempt-RT variant based on rtmutexes.
>> @@ -169,6 +185,13 @@ do {                            \
>>                                \
>>        __mutex_init((mutex), #mutex, &__key);        \
>>    } while (0)
>> +
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +    mutex_init(lock);
>> +    return 0;
>> +}
>> +
>>    #endif /* CONFIG_PREEMPT_RT */
>>
>>    /*
>> diff --git a/kernel/locking/mutex-debug.c b/kernel/locking/mutex-debug.c
>> index bc8abb8549d2..d50dfa06e82c 100644
>> --- a/kernel/locking/mutex-debug.c
>> +++ b/kernel/locking/mutex-debug.c
>> @@ -19,6 +19,7 @@
>>    #include <linux/kallsyms.h>
>>    #include <linux/interrupt.h>
>>    #include <linux/debug_locks.h>
>> +#include <linux/device.h>
>>
>>    #include "mutex.h"
>>
>> @@ -104,3 +105,25 @@ void mutex_destroy(struct mutex *lock)
>>    }
>>
>>    EXPORT_SYMBOL_GPL(mutex_destroy);
>> +
>> +static void devm_mutex_release(void *res)
>> +{
>> +    mutex_destroy(res);
>> +}
>> +
>> +/**
>> + * devm_mutex_init - Resource-managed mutex initialization
>> + * @dev:    Device which lifetime mutex is bound to
>> + * @lock:    Pointer to a mutex
>> + *
>> + * Initialize mutex which is automatically destroyed when the driver is
>> detached.
>> + *
>> + * Returns: 0 on success or a negative error code on failure.
>> + */
>> +int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +    mutex_init(lock);
>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(devm_mutex_init);
>> \ No newline at end of file
>>
>>

-- 
Best regards
George