Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <cbd2705d-9d43-4822-9b5c-ea437a2ccca7@redhat.com>
Date:   Mon, 27 Nov 2023 20:55:18 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 1/1] locking: introduce devm_mutex_init and
 devm_mutex_destroy
Content-Language: en-US
To:     George Stark <gnstark@salutedevices.com>, peterz@infradead.org,
        mingo@redhat.com, will@kernel.org, boqun.feng@gmail.com,
        andriy.shevchenko@linux.intel.com, jic23@kernel.org
Cc:     linux-kernel@vger.kernel.org, kernel@salutedevices.com
References: <20231128000910.73784-1-gnstark@salutedevices.com>
From:   Waiman Long <longman@redhat.com>
In-Reply-To: <20231128000910.73784-1-gnstark@salutedevices.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/27/23 19:09, George Stark wrote:
> Using of devm API leads to 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() and devm_mutex_destroy().
>
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   include/linux/mutex.h  |  3 +++
>   kernel/locking/mutex.c | 37 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a33aa9eb9fc3..7f60cd842322 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -119,6 +119,9 @@ do {									\
>   extern void __mutex_init(struct mutex *lock, const char *name,
>   			 struct lock_class_key *key);
>   
> +int devm_mutex_init(struct device *dev, struct mutex *lock);
> +void devm_mutex_destroy(struct device *dev, struct mutex *lock);
> +
>   /**
>    * mutex_is_locked - is the mutex locked
>    * @lock: the mutex to be queried
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index d973fe6041bf..a73124719dcb 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -56,6 +56,43 @@ __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
>   }
>   EXPORT_SYMBOL(__mutex_init);
>   
> +static void devm_mutex_release(struct device *dev, void *res)
> +{
> +	mutex_destroy(*(struct mutex **)res);
> +}
> +
> +static int devm_mutex_match(struct device *dev, void *res, void *data)
> +{
> +	struct mutex **r = res;
> +
> +	if (WARN_ON(!r || !*r))
> +		return 0;
> +
> +	return *r == data;
> +}
> +
> +int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	struct mutex **ptr;
> +
> +	ptr = devres_alloc(devm_mutex_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	mutex_init(lock);
> +
> +	*ptr = lock;
> +	devres_add(dev, ptr);
> +	return 0;
> +}
> +EXPORT_SYMBOL(devm_mutex_init);
> +
> +void devm_mutex_destroy(struct device *dev, struct mutex *lock)
> +{
> +	devres_release(dev, devm_mutex_release, devm_mutex_match, lock);
> +}
> +EXPORT_SYMBOL(devm_mutex_destroy);
> +
>   /*
>    * @owner: contains: 'struct task_struct *' to the current lock owner,
>    * NULL means not owned. Since task_struct pointers are aligned at

These APIs are specific to devres. I don't believe it is suitable to put 
them into the generic mutex.h header file. All devres_* functions are 
defined in include/linux/device.h which is probabably not included in 
mutex.h. You may consider putting these APIs into device.h instead.

Cheers,
Longman