Received-SPF: pass (google.com: domain of linux-kernel+bounces-163-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <300d2131-87ef-48c1-b162-dcef0d8d5722@redhat.com>
Date: Thu, 14 Dec 2023 16:48:42 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 02/10] locking: introduce devm_mutex_init
Content-Language: en-US
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
 George Stark <gnstark@salutedevices.com>,
 "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
 "pavel@ucw.cz" <pavel@ucw.cz>, "lee@kernel.org" <lee@kernel.org>,
 "vadimp@nvidia.com" <vadimp@nvidia.com>,
 "mpe@ellerman.id.au" <mpe@ellerman.id.au>,
 "npiggin@gmail.com" <npiggin@gmail.com>,
 "hdegoede@redhat.com" <hdegoede@redhat.com>,
 "mazziesaccount@gmail.com" <mazziesaccount@gmail.com>,
 "peterz@infradead.org" <peterz@infradead.org>,
 "mingo@redhat.com" <mingo@redhat.com>, "will@kernel.org" <will@kernel.org>,
 "boqun.feng@gmail.com" <boqun.feng@gmail.com>,
 "nikitos.tr@gmail.com" <nikitos.tr@gmail.com>
Cc: "linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
 "kernel@salutedevices.com" <kernel@salutedevices.com>
References: <20231214173614.2820929-1-gnstark@salutedevices.com>
 <20231214173614.2820929-3-gnstark@salutedevices.com>
 <5c10f66c-3fd8-4861-994b-13e71c24f10a@redhat.com>
 <b158ca2b-7300-4ad0-82b8-e1442d267734@csgroup.eu>
From: Waiman Long <longman@redhat.com>
In-Reply-To: <b158ca2b-7300-4ad0-82b8-e1442d267734@csgroup.eu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 12/14/23 14:53, Christophe Leroy wrote:
>
> Le 14/12/2023 à 19:48, Waiman Long a écrit :
>> On 12/14/23 12:36, George Stark wrote:
>>> Using of devm API leads to a certain order of releasing resources.
>>> So all dependent resources which are not devm-wrapped should be deleted
>>> with respect to devm-release order. Mutex is one of such objects that
>>> often is bound to other resources and has no own devm wrapping.
>>> Since mutex_destroy() actually does nothing in non-debug builds
>>> frequently calling mutex_destroy() is just ignored which is safe for now
>>> but wrong formally and can lead to a problem if mutex_destroy() will be
>>> extended so introduce devm_mutex_init()
>>>
>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>> ---
>>>    include/linux/mutex.h        | 23 +++++++++++++++++++++++
>>>    kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
>>>    2 files changed, 45 insertions(+)
>>>
>>> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
>>> index a33aa9eb9fc3..ebd03ff1ef66 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
>>> +
>>> +int devm_mutex_init(struct device *dev, struct mutex *lock);
>> Please add "extern" to the function declaration to be consistent with
>> other functional declarations in mutex.h.
> 'extern' is pointless and deprecated on function prototypes. Already
> having some is not a good reason to add new ones, errors from the past
> should be avoided nowadays. With time they should all disappear so don't
> add new ones.
Yes, "extern" is optional. It is just a suggestion and I am going to 
argue about that.
>
>>> +
>>> +#else
>>> +
>>> +static inline int devm_mutex_init(struct device *dev, struct mutex
>>> *lock)
>>> +{
>>> +    mutex_init(lock);
>>> +    return 0;
>>> +}
>> I would prefer you to add a devm_mutex_init macro after the function
>> declaration and put this inline function at the end of header if the
>> devm_mutex_init macro isn't defined. In this way, you don't need to
>> repeat this inline function twice as it has no dependency on PREEMPT_RT.
> It is already done that way for other functions in that file. Should be
> kept consistant. I agree with you it is not ideal, maybe we should
> rework that file completely but I don't like the idea of a
> devm_mutex_init macro for that.

devm_mutex_init() is not an API for the core mutex code. That is why I 
want to minimize change to the existing code layout. Putting it at the 
end will reduce confusion when developers look up mutex.h header file to 
find out what mutex functions are available.

Cheers,
Longman