[PATCH v2 00/10] devm_led_classdev_register() usage problem

[PATCH v2 00/10] devm_led_classdev_register() usage problem

[George Stark]

This patch series fixes the problem of devm_led_classdev_register misusing.

The basic problem is described in [1]. Shortly when devm_led_classdev_register()
is used then led_classdev_unregister() called after driver's remove() callback.
led_classdev_unregister() calls driver's brightness_set callback and that callback
may use resources which were destroyed already in driver's remove().

After discussion with maintainers [2] [3] we decided:
1) don't touch led subsytem core code and don't remove led_set_brightness() from it
but fix drivers
2) don't use devm_led_classdev_unregister

So the solution is to use devm wrappers for all resources
driver's brightness_set() depends on. And introduce dedicated devm wrapper
for mutex as it's often used resource.

[1] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/
[2] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mc132b9b350fa51931b4fcfe14705d9f06e91421f
[3] https://lore.kernel.org/lkml/8704539b-ed3b-44e6-aa82-586e2f895e2b@salutedevices.com/T/#mdbf572a85c33f869a553caf986b6228bb65c8383

George Stark (10):
  devm-helpers: introduce devm_mutex_init
  leds: aw2013: unlock mutex before destroying it
  leds: aw2013: use devm API to cleanup module's resources
  leds: aw200xx: use devm API to cleanup module's resources
  leds: lp3952: use devm API to cleanup module's resources
  leds: lm3532: use devm API to cleanup module's resources
  leds: nic78bx: use devm API to cleanup module's resources
  leds: mlxreg: use devm_mutex_init for mutex initializtion
  leds: an30259a: use devm_mutext_init for mutext initialization
  leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds

 drivers/leds/leds-an30259a.c | 14 ++++----------
 drivers/leds/leds-aw200xx.c  | 36 +++++++++++++++++++++++++-----------
 drivers/leds/leds-aw2013.c   | 28 ++++++++++++++++------------
 drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
 drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
 drivers/leds/leds-mlxreg.c   | 15 ++++-----------
 drivers/leds/leds-nic78bx.c  | 25 +++++++++++++------------
 drivers/leds/leds-powernv.c  | 23 ++++++++---------------
 include/linux/devm-helpers.h | 18 ++++++++++++++++++
 9 files changed, 116 insertions(+), 93 deletions(-)

--
2.38.4

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

[George Stark]

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() is
extended so introduce devm_mutex_init().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 include/linux/devm-helpers.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index 74891802200d..2f56e476776f 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
 	return devm_add_action(dev, devm_work_drop, w);
 }
 
+static inline void devm_mutex_release(void *res)
+{
+	mutex_destroy(res);
+}
+
+/**
+ * devm_mutex_init - Resource-managed mutex initialization
+ * @dev:	Device which lifetime work is bound to
+ * @lock:	Pointer to a mutex
+ *
+ * Initialize mutex which is automatically destroyed when driver is detached.
+ */
+static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+	mutex_init(lock);
+	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
+}
+
 #endif
-- 
2.38.4

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

[Andy Shevchenko]

On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> 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() is
> extended so introduce devm_mutex_init().

...

Do you need to include mutex.h?

...

> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev:       Device which lifetime work is bound to
> + * @lock:      Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.

the driver

Have you run scripts/kernel-doc -v -Wall -none ... against this file?
I'm pretty sure it will complain.

> + */


-- 
With Best Regards,
Andy Shevchenko

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

[George Stark]

Hello Andy

Thanks for the review.

On 12/4/23 21:11, Andy Shevchenko wrote:
> On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> 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() is
>> extended so introduce devm_mutex_init().
> 
> ...
> 
> Do you need to include mutex.h?
It's already included in linux/device.h which is included in 
devm-helpers. Should I include mutex.h explicitly?

> 
> ...
> 
>> +/**
>> + * devm_mutex_init - Resource-managed mutex initialization
>> + * @dev:       Device which lifetime work is bound to
>> + * @lock:      Pointer to a mutex
>> + *
>> + * Initialize mutex which is automatically destroyed when driver is detached.
> 
> the driver
> 
> Have you run scripts/kernel-doc -v -Wall -none ... against this file?
> I'm pretty sure it will complain.
It does with warning "No description found for return value". Fixed

> 
>> + */
> 
> 

-- 
Best regards
George

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

[Hans de Goede]

Hi,

On 12/6/23 08:56, George Stark wrote:
> Hello Andy
> 
> Thanks for the review.
> 
> On 12/4/23 21:11, Andy Shevchenko wrote:
>> On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> 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() is
>>> extended so introduce devm_mutex_init().
>>
>> ...
>>
>> Do you need to include mutex.h?
> It's already included in linux/device.h which is included in devm-helpers. Should I include mutex.h explicitly?

Yes you must explicitly include all headers you use definitions
from. Relying on other headers to do this for you is error prone.

Regards,

Hans

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

[Christophe Leroy]

Le 04/12/2023 à 19:05, George Stark a écrit :
> 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() is
> extended so introduce devm_mutex_init().

This is not needed for patch 2. Should patch 2 go first ?

> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   include/linux/devm-helpers.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..2f56e476776f 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
>   	return devm_add_action(dev, devm_work_drop, w);
>   }
>   
> +static inline void devm_mutex_release(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev:	Device which lifetime work is bound to
> + * @lock:	Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>   #endif

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

[Matti Vaittinen]

On 12/4/23 20:05, 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() is
> extended so introduce devm_mutex_init().
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   include/linux/devm-helpers.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..2f56e476776f 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
>   	return devm_add_action(dev, devm_work_drop, w);
>   }
>   
> +static inline void devm_mutex_release(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev:	Device which lifetime work is bound to

Work? Copy-paste error?

> + * @lock:	Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>   #endif

Doesn't the mutex stuff need a header inclusion?

Yours,
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

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

[Hans de Goede]

Hi George,

On 12/4/23 19:05, 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() is
> extended so introduce devm_mutex_init().
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>  include/linux/devm-helpers.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
> index 74891802200d..2f56e476776f 100644
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
>  	return devm_add_action(dev, devm_work_drop, w);
>  }
>  
> +static inline void devm_mutex_release(void *res)
> +{
> +	mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource-managed mutex initialization
> + * @dev:	Device which lifetime work is bound to
> + * @lock:	Pointer to a mutex
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> +	mutex_init(lock);
> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
> +}
> +
>  #endif

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.

Regards,

Hans

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

[George Stark]

Hello Hans

Thanks for the review.

On 12/6/23 18:01, Hans de Goede wrote:
> Hi George,
> 
> On 12/4/23 19:05, 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() is
>> extended so introduce devm_mutex_init().
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> ---
>>   include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>> index 74891802200d..2f56e476776f 100644
>> --- a/include/linux/devm-helpers.h
>> +++ b/include/linux/devm-helpers.h
>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
>>   	return devm_add_action(dev, devm_work_drop, w);
>>   }
>>   
>> +static inline void devm_mutex_release(void *res)
>> +{
>> +	mutex_destroy(res);
>> +}
>> +
>> +/**
>> + * devm_mutex_init - Resource-managed mutex initialization
>> + * @dev:	Device which lifetime work is bound to
>> + * @lock:	Pointer to a mutex
>> + *
>> + * Initialize mutex which is automatically destroyed when driver is detached.
>> + */
>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>> +{
>> +	mutex_init(lock);
>> +	return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>> +}
>> +
>>   #endif
> 
> 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.

I tried to put devm_mutex_init itself in mutex.h and it could've helped 
too but it's not the place for devm API.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 

-- 
Best regards
George

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

[Hans de Goede]

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,
>>
>> On 12/4/23 19:05, 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() is
>>> extended so introduce devm_mutex_init().
>>>
>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>> ---
>>>   include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>>> index 74891802200d..2f56e476776f 100644
>>> --- a/include/linux/devm-helpers.h
>>> +++ b/include/linux/devm-helpers.h
>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
>>>       return devm_add_action(dev, devm_work_drop, w);
>>>   }
>>>   +static inline void devm_mutex_release(void *res)
>>> +{
>>> +    mutex_destroy(res);
>>> +}
>>> +
>>> +/**
>>> + * devm_mutex_init - Resource-managed mutex initialization
>>> + * @dev:    Device which lifetime work is bound to
>>> + * @lock:    Pointer to a mutex
>>> + *
>>> + * Initialize mutex which is automatically destroyed when driver is detached.
>>> + */
>>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>>> +{
>>> +    mutex_init(lock);
>>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>> +}
>>> +
>>>   #endif
>>
>> 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 see 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.

Regards,

Hans

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

[Waiman Long]

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,
>>>
>>> On 12/4/23 19:05, 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() is
>>>> extended so introduce devm_mutex_init().
>>>>
>>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>>> ---
>>>>    include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>>>    1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>>>> index 74891802200d..2f56e476776f 100644
>>>> --- a/include/linux/devm-helpers.h
>>>> +++ b/include/linux/devm-helpers.h
>>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct device *dev,
>>>>        return devm_add_action(dev, devm_work_drop, w);
>>>>    }
>>>>    +static inline void devm_mutex_release(void *res)
>>>> +{
>>>> +    mutex_destroy(res);
>>>> +}
>>>> +
>>>> +/**
>>>> + * devm_mutex_init - Resource-managed mutex initialization
>>>> + * @dev:    Device which lifetime work is bound to
>>>> + * @lock:    Pointer to a mutex
>>>> + *
>>>> + * Initialize mutex which is automatically destroyed when driver is detached.
>>>> + */
>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
>>>> +{
>>>> +    mutex_init(lock);
>>>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>>> +}
>>>> +
>>>>    #endif
>>> 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 see 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.

Cheers,
Longman

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

[George Stark]

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.


> Cheers,
> Longman
> 

-- 
Best regards
George

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

[Waiman Long]

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
>>
>

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

[Waiman Long]

On 12/6/23 16: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,
>>>>
>>>> On 12/4/23 19:05, 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() is
>>>>> extended so introduce devm_mutex_init().
>>>>>
>>>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>>>> ---
>>>>>    include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>>>>    1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/devm-helpers.h 
>>>>> b/include/linux/devm-helpers.h
>>>>> index 74891802200d..2f56e476776f 100644
>>>>> --- a/include/linux/devm-helpers.h
>>>>> +++ b/include/linux/devm-helpers.h
>>>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct 
>>>>> device *dev,
>>>>>        return devm_add_action(dev, devm_work_drop, w);
>>>>>    }
>>>>>    +static inline void devm_mutex_release(void *res)
>>>>> +{
>>>>> +    mutex_destroy(res);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * devm_mutex_init - Resource-managed mutex initialization
>>>>> + * @dev:    Device which lifetime work is bound to
>>>>> + * @lock:    Pointer to a mutex
>>>>> + *
>>>>> + * Initialize mutex which is automatically destroyed when driver 
>>>>> is detached.
>>>>> + */
>>>>> +static inline int devm_mutex_init(struct device *dev, struct 
>>>>> mutex *lock)
>>>>> +{
>>>>> +    mutex_init(lock);
>>>>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>>>> +}
>>>>> +
>>>>>    #endif
>>>> 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 see 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.

OTOH, one thing that we can probably do in mutex.h is something like

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a33aa9eb9fc3..7db7862de3f1 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -83,6 +83,9 @@ struct mutex {

  extern void mutex_destroy(struct mutex *lock);

+/* mutex_destroy() is a real function, not a NOP */
+#define mutex_destroy  mutex_destroy
+
  #else

----------------------------------------

Now in some devm files, you can use the absense/presence of 
mutex_destroy macro to decide on what to do.

Cheers,
Longman

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

[Christophe Leroy]

Le 06/12/2023 à 19:58, George Stark a écrit :
> [Vous ne recevez pas souvent de courriers de gnstark@salutedevices.com. 
> Découvrez pourquoi ceci est important à 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Hans
> 
> Thanks for the review.
> 
> On 12/6/23 18:01, Hans de Goede wrote:
>> Hi George,
>>
>> On 12/4/23 19:05, 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() is
>>> extended so introduce devm_mutex_init().
>>>
>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>> ---
>>>   include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
>>> index 74891802200d..2f56e476776f 100644
>>> --- a/include/linux/devm-helpers.h
>>> +++ b/include/linux/devm-helpers.h
>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct 
>>> device *dev,
>>>      return devm_add_action(dev, devm_work_drop, w);
>>>   }
>>>
>>> +static inline void devm_mutex_release(void *res)
>>> +{
>>> +    mutex_destroy(res);
>>> +}
>>> +
>>> +/**
>>> + * devm_mutex_init - Resource-managed mutex initialization
>>> + * @dev:    Device which lifetime work is bound to
>>> + * @lock:   Pointer to a mutex
>>> + *
>>> + * Initialize mutex which is automatically destroyed when driver is 
>>> detached.
>>> + */
>>> +static inline int devm_mutex_init(struct device *dev, struct mutex 
>>> *lock)
>>> +{
>>> +    mutex_init(lock);
>>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>> +}
>>> +
>>>   #endif
>>
>> 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.
> 
> I tried to put devm_mutex_init itself in mutex.h and it could've helped
> too but it's not the place for devm API.
> 

What do you mean by "it's not the place for devm API" ?

If you do a 'grep devm_ include/linux/' you'll find devm_ functions in 
almost 100 .h files. Why wouldn't mutex.h be the place for 
devm_mutex_init() ?

Christophe

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

[Christophe Leroy]

Le 06/12/2023 à 23:14, Christophe Leroy a écrit :
> 
> 
> Le 06/12/2023 à 19:58, George Stark a écrit :
>> [Vous ne recevez pas souvent de courriers de 
>> gnstark@salutedevices.com. Découvrez pourquoi ceci est important à 
>> https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Hello Hans
>>
>> Thanks for the review.
>>
>> On 12/6/23 18:01, Hans de Goede wrote:
>>> Hi George,
>>>
>>> On 12/4/23 19:05, 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() is
>>>> extended so introduce devm_mutex_init().
>>>>
>>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>>> ---
>>>>   include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>>>   1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/include/linux/devm-helpers.h 
>>>> b/include/linux/devm-helpers.h
>>>> index 74891802200d..2f56e476776f 100644
>>>> --- a/include/linux/devm-helpers.h
>>>> +++ b/include/linux/devm-helpers.h
>>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct 
>>>> device *dev,
>>>>      return devm_add_action(dev, devm_work_drop, w);
>>>>   }
>>>>
>>>> +static inline void devm_mutex_release(void *res)
>>>> +{
>>>> +    mutex_destroy(res);
>>>> +}
>>>> +
>>>> +/**
>>>> + * devm_mutex_init - Resource-managed mutex initialization
>>>> + * @dev:    Device which lifetime work is bound to
>>>> + * @lock:   Pointer to a mutex
>>>> + *
>>>> + * Initialize mutex which is automatically destroyed when driver is 
>>>> detached.
>>>> + */
>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex 
>>>> *lock)
>>>> +{
>>>> +    mutex_init(lock);
>>>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>>> +}
>>>> +
>>>>   #endif
>>>
>>> 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.
>>
>> I tried to put devm_mutex_init itself in mutex.h and it could've helped
>> too but it's not the place for devm API.
>>
> 
> What do you mean by "it's not the place for devm API" ?
> 
> If you do a 'grep devm_ include/linux/' you'll find devm_ functions in 
> almost 100 .h files. Why wouldn't mutex.h be the place for 
> devm_mutex_init() ?

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

Wouldn't that work ?

Christophe

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

[George Stark]

Hello Christophe

On 12/7/23 01:37, Christophe Leroy wrote:
> 
> 
> Le 06/12/2023 à 23:14, Christophe Leroy a écrit :
>>
>>
>> Le 06/12/2023 à 19:58, George Stark a écrit :
>>> [Vous ne recevez pas souvent de courriers de
>>> gnstark@salutedevices.com. Découvrez pourquoi ceci est important à
>>> https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> Hello Hans
>>>
>>> Thanks for the review.
>>>
>>> On 12/6/23 18:01, Hans de Goede wrote:
>>>> Hi George,
>>>>
>>>> On 12/4/23 19:05, 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() is
>>>>> extended so introduce devm_mutex_init().
>>>>>
>>>>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>>>>> ---
>>>>>    include/linux/devm-helpers.h | 18 ++++++++++++++++++
>>>>>    1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/devm-helpers.h
>>>>> b/include/linux/devm-helpers.h
>>>>> index 74891802200d..2f56e476776f 100644
>>>>> --- a/include/linux/devm-helpers.h
>>>>> +++ b/include/linux/devm-helpers.h
>>>>> @@ -76,4 +76,22 @@ static inline int devm_work_autocancel(struct
>>>>> device *dev,
>>>>>       return devm_add_action(dev, devm_work_drop, w);
>>>>>    }
>>>>>
>>>>> +static inline void devm_mutex_release(void *res)
>>>>> +{
>>>>> +    mutex_destroy(res);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * devm_mutex_init - Resource-managed mutex initialization
>>>>> + * @dev:    Device which lifetime work is bound to
>>>>> + * @lock:   Pointer to a mutex
>>>>> + *
>>>>> + * Initialize mutex which is automatically destroyed when driver is
>>>>> detached.
>>>>> + */
>>>>> +static inline int devm_mutex_init(struct device *dev, struct mutex
>>>>> *lock)
>>>>> +{
>>>>> +    mutex_init(lock);
>>>>> +    return devm_add_action_or_reset(dev, devm_mutex_release, lock);
>>>>> +}
>>>>> +
>>>>>    #endif
>>>>
>>>> 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.
>>>
>>> I tried to put devm_mutex_init itself in mutex.h and it could've helped
>>> too but it's not the place for devm API.
>>>
>>
>> What do you mean by "it's not the place for devm API" ?
>>
>> If you do a 'grep devm_ include/linux/' you'll find devm_ functions in
>> almost 100 .h files. Why wouldn't mutex.h be the place for
>> devm_mutex_init() ?
mutex.h's maintainers believe so.

https://lore.kernel.org/lkml/070c174c-057a-46de-ae8e-836e9e20eceb@salutedevices.com/T/#mb42e1d7760816b0cedd3130e08f29690496b5ac2
> 
> 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;

> 
> Wouldn't that work ?
> 
> Christophe

-- 
Best regards
George

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

[Andy Shevchenko]

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 :
> >> Le 06/12/2023 à 19:58, George Stark a écrit :
> >>> On 12/6/23 18:01, Hans de Goede wrote:
> >>>> On 12/4/23 19:05, George Stark wrote:

...

> >>>> 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.
> >>>
> >>> I tried to put devm_mutex_init itself in mutex.h and it could've helped
> >>> too but it's not the place for devm API.
> >>>
> >>
> >> What do you mean by "it's not the place for devm API" ?
> >>
> >> If you do a 'grep devm_ include/linux/' you'll find devm_ functions in
> >> almost 100 .h files. Why wouldn't mutex.h be the place for
> >> devm_mutex_init() ?
> mutex.h's maintainers believe so.
>
> https://lore.kernel.org/lkml/070c174c-057a-46de-ae8e-836e9e20eceb@salutedevices.com/T/#mb42e1d7760816b0cedd3130e08f29690496b5ac2
> >
> > 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;
>
> > Wouldn't that work ?

No. It will require inclusion of device.h (which is a twisted hell
from the header perspective) into mutex.h. Completely unappreciated
move.

-- 
With Best Regards,
Andy Shevchenko

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

[Christophe Leroy]

Le 07/12/2023 à 12:59, 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 :
>>>> Le 06/12/2023 à 19:58, George Stark a écrit :
>>>>> On 12/6/23 18:01, Hans de Goede wrote:
>>>>>> On 12/4/23 19:05, George Stark wrote:
> 
> ...
> 
>>>>>> 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.
>>>>>
>>>>> I tried to put devm_mutex_init itself in mutex.h and it could've helped
>>>>> too but it's not the place for devm API.
>>>>>
>>>>
>>>> What do you mean by "it's not the place for devm API" ?
>>>>
>>>> If you do a 'grep devm_ include/linux/' you'll find devm_ functions in
>>>> almost 100 .h files. Why wouldn't mutex.h be the place for
>>>> devm_mutex_init() ?
>> mutex.h's maintainers believe so.
>>
>> https://lore.kernel.org/lkml/070c174c-057a-46de-ae8e-836e9e20eceb@salutedevices.com/T/#mb42e1d7760816b0cedd3130e08f29690496b5ac2
>>>
>>> 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;
>>
>>> Wouldn't that work ?
> 
> No. It will require inclusion of device.h (which is a twisted hell
> from the header perspective) into mutex.h. Completely unappreciated
> move.
> 

I see no reason for including device.h, I think a forward declaration of 
struct device would be enough, as done in linux/gpio.h

Am I missing something ?

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

[Andy Shevchenko]

On Thu, Dec 7, 2023 at 2:31 PM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> Le 07/12/2023 à 12:59, 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 :
> >>>> Le 06/12/2023 à 19:58, George Stark a écrit :
> >>>>> On 12/6/23 18:01, Hans de Goede wrote:
> >>>>>> On 12/4/23 19:05, George Stark wrote:

...

> >>>>>> 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);

^^^^ (1)

> >>>>>> #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.
> >>>>>
> >>>>> I tried to put devm_mutex_init itself in mutex.h and it could've helped
> >>>>> too but it's not the place for devm API.
> >>>>>
> >>>>
> >>>> What do you mean by "it's not the place for devm API" ?
> >>>>
> >>>> If you do a 'grep devm_ include/linux/' you'll find devm_ functions in
> >>>> almost 100 .h files. Why wouldn't mutex.h be the place for
> >>>> devm_mutex_init() ?
> >> mutex.h's maintainers believe so.
> >>
> >> https://lore.kernel.org/lkml/070c174c-057a-46de-ae8e-836e9e20eceb@salutedevices.com/T/#mb42e1d7760816b0cedd3130e08f29690496b5ac2
> >>>
> >>> 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;
> >>
> >>> Wouldn't that work ?
> >
> > No. It will require inclusion of device.h (which is a twisted hell
> > from the header perspective) into mutex.h. Completely unappreciated
> > move.
>
> I see no reason for including device.h, I think a forward declaration of
> struct device would be enough, as done in linux/gpio.h
>
> Am I missing something ?

Yes, see (1) above. If you want to have it in the header, you must
provide an API, which is located in device.h. The idea about
mutex-debug.c is interesting, but the file naming and the devm_*() API
for _initing_ the mutex seems confusing.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Christophe Leroy]

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.

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

[George Stark]

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
+
+extern int devm_mutex_init(struct device *dev, struct mutex *lock);
+
+#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

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

[Christophe Leroy]

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 ?

> +
> +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.

> +
> +#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
> 
> 

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

[George Stark]

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

[PATCH v2 02/10] leds: aw2013: unlock mutex before destroying it

[George Stark]

In the probe() callback in case of error mutex is destroyed being locked
which is not allowed so unlock the mute before destroying.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-aw2013.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index 59765640b70f..c2bc0782c0cd 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -397,6 +397,7 @@ static int aw2013_probe(struct i2c_client *client)
 	regulator_disable(chip->vcc_regulator);
 
 error:
+	mutex_unlock(&chip->mutex);
 	mutex_destroy(&chip->mutex);
 	return ret;
 }
-- 
2.38.4

Re: [PATCH v2 02/10] leds: aw2013: unlock mutex before destroying it

[Andy Shevchenko]

On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> wrote:
>
> In the probe() callback in case of error mutex is destroyed being locked
> which is not allowed so unlock the mute before destroying.

Sounds to me like this is a real fix, hence the Fixes tag and being
first in the series.

You free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 02/10] leds: aw2013: unlock mutex before destroying it

[Christophe Leroy]

Le 04/12/2023 à 19:05, George Stark a écrit :
> In the probe() callback in case of error mutex is destroyed being locked
> which is not allowed so unlock the mute before destroying.

Should there be a fixes: tag ? For instance on 59ea3c9faf32 ("leds: add 
aw2013 driver") ?

> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   drivers/leds/leds-aw2013.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 59765640b70f..c2bc0782c0cd 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -397,6 +397,7 @@ static int aw2013_probe(struct i2c_client *client)
>   	regulator_disable(chip->vcc_regulator);
>   
>   error:
> +	mutex_unlock(&chip->mutex);
>   	mutex_destroy(&chip->mutex);
>   	return ret;
>   }

Re: [PATCH v2 02/10] leds: aw2013: unlock mutex before destroying it

[George Stark]

Hello Christophe

Thanks for the review

On 12/5/23 02:09, Christophe Leroy wrote:
> 
> 
> Le 04/12/2023 à 19:05, George Stark a écrit :
>> In the probe() callback in case of error mutex is destroyed being locked
>> which is not allowed so unlock the mute before destroying.
> 
> Should there be a fixes: tag ? For instance on 59ea3c9faf32 ("leds: add
> aw2013 driver") ?
Ack

> 
>>
>> Signed-off-by: George Stark <gnstark@salutedevices.com>
>> ---
>>    drivers/leds/leds-aw2013.c | 1 +
>>    1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
>> index 59765640b70f..c2bc0782c0cd 100644
>> --- a/drivers/leds/leds-aw2013.c
>> +++ b/drivers/leds/leds-aw2013.c
>> @@ -397,6 +397,7 @@ static int aw2013_probe(struct i2c_client *client)
>>    	regulator_disable(chip->vcc_regulator);
>>    
>>    error:
>> +	mutex_unlock(&chip->mutex);
>>    	mutex_destroy(&chip->mutex);
>>    	return ret;
>>    }

-- 
Best regards
George

[PATCH v2 03/10] leds: aw2013: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-aw2013.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
index c2bc0782c0cd..1a8acf303548 100644
--- a/drivers/leds/leds-aw2013.c
+++ b/drivers/leds/leds-aw2013.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 // Driver for Awinic AW2013 3-channel LED driver
 
+#include <linux/devm-helpers.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -318,6 +319,13 @@ static int aw2013_probe_dt(struct aw2013 *chip)
 	return 0;
 }
 
+static void aw2013_chip_disable_action(void *data)
+{
+	struct aw2013 *chip = (struct aw2013 *)data;
+
+	aw2013_chip_disable(chip);
+}
+
 static const struct regmap_config aw2013_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -334,7 +342,9 @@ static int aw2013_probe(struct i2c_client *client)
 	if (!chip)
 		return -ENOMEM;
 
-	mutex_init(&chip->mutex);
+	if (devm_mutex_init(&client->dev, &chip->mutex))
+		return -ENOMEM;
+
 	mutex_lock(&chip->mutex);
 
 	chip->client = client;
@@ -378,6 +388,10 @@ static int aw2013_probe(struct i2c_client *client)
 		goto error_reg;
 	}
 
+	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
+	if (ret)
+		goto error_reg;
+
 	ret = aw2013_probe_dt(chip);
 	if (ret < 0)
 		goto error_reg;
@@ -398,19 +412,9 @@ static int aw2013_probe(struct i2c_client *client)
 
 error:
 	mutex_unlock(&chip->mutex);
-	mutex_destroy(&chip->mutex);
 	return ret;
 }
 
-static void aw2013_remove(struct i2c_client *client)
-{
-	struct aw2013 *chip = i2c_get_clientdata(client);
-
-	aw2013_chip_disable(chip);
-
-	mutex_destroy(&chip->mutex);
-}
-
 static const struct of_device_id aw2013_match_table[] = {
 	{ .compatible = "awinic,aw2013", },
 	{ /* sentinel */ },
@@ -424,7 +428,6 @@ static struct i2c_driver aw2013_driver = {
 		.of_match_table = of_match_ptr(aw2013_match_table),
 	},
 	.probe = aw2013_probe,
-	.remove = aw2013_remove,
 };
 
 module_i2c_driver(aw2013_driver);
-- 
2.38.4

Re: [PATCH v2 03/10] leds: aw2013: use devm API to cleanup module's resources

[Andy Shevchenko]

On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> wrote:
>
> In this driver LEDs are registered using devm_led_classdev_register()
> so they are automatically unregistered after module's remove() is done.
> led_classdev_unregister() calls module's led_set_brightness() to turn off
> the LEDs and that callback uses resources which were destroyed already
> in module's remove() so use devm API instead of remove().

...

> +static void aw2013_chip_disable_action(void *data)
> +{
> +       struct aw2013 *chip = (struct aw2013 *)data;
> +
> +       aw2013_chip_disable(chip);
> +}

As with mutex release, this also can be oneliner

static void aw2013_chip_disable_action(void *chip)
{
       aw2013_chip_disable(chip);
}

...

> +       if (devm_mutex_init(&client->dev, &chip->mutex))
> +               return -ENOMEM;

Shouldn be

       ret = devm_mutex_init(&client->dev, &chip->mutex);
       if (ret)
           return ret;

?

> +               return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/10] leds: aw2013: use devm API to cleanup module's resources

[Christophe Leroy]

Le 04/12/2023 à 19:05, George Stark a écrit :
> In this driver LEDs are registered using devm_led_classdev_register()
> so they are automatically unregistered after module's remove() is done.
> led_classdev_unregister() calls module's led_set_brightness() to turn off
> the LEDs and that callback uses resources which were destroyed already
> in module's remove() so use devm API instead of remove().
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   drivers/leds/leds-aw2013.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index c2bc0782c0cd..1a8acf303548 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0+
>   // Driver for Awinic AW2013 3-channel LED driver
>   
> +#include <linux/devm-helpers.h>
>   #include <linux/i2c.h>
>   #include <linux/leds.h>
>   #include <linux/module.h>
> @@ -318,6 +319,13 @@ static int aw2013_probe_dt(struct aw2013 *chip)
>   	return 0;
>   }
>   
> +static void aw2013_chip_disable_action(void *data)
> +{
> +	struct aw2013 *chip = (struct aw2013 *)data;

The cast is not needed because data is void*

And chip is not needed at all, you can pass data to 
aw2013_chip_disable() directly, without a cast either.

> +
> +	aw2013_chip_disable(chip);
> +}
> +
>   static const struct regmap_config aw2013_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
> @@ -334,7 +342,9 @@ static int aw2013_probe(struct i2c_client *client)
>   	if (!chip)
>   		return -ENOMEM;
>   
> -	mutex_init(&chip->mutex);
> +	if (devm_mutex_init(&client->dev, &chip->mutex))
> +		return -ENOMEM;

Why not return the value returned by devm_mutex_init() ?

> +
>   	mutex_lock(&chip->mutex);
>   
>   	chip->client = client;
> @@ -378,6 +388,10 @@ static int aw2013_probe(struct i2c_client *client)
>   		goto error_reg;
>   	}
>   
> +	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
> +	if (ret)
> +		goto error_reg;
> +
>   	ret = aw2013_probe_dt(chip);
>   	if (ret < 0)
>   		goto error_reg;
> @@ -398,19 +412,9 @@ static int aw2013_probe(struct i2c_client *client)
>   
>   error:
>   	mutex_unlock(&chip->mutex);
> -	mutex_destroy(&chip->mutex);
>   	return ret;
>   }
>   
> -static void aw2013_remove(struct i2c_client *client)
> -{
> -	struct aw2013 *chip = i2c_get_clientdata(client);
> -
> -	aw2013_chip_disable(chip);
> -
> -	mutex_destroy(&chip->mutex);
> -}
> -
>   static const struct of_device_id aw2013_match_table[] = {
>   	{ .compatible = "awinic,aw2013", },
>   	{ /* sentinel */ },
> @@ -424,7 +428,6 @@ static struct i2c_driver aw2013_driver = {
>   		.of_match_table = of_match_ptr(aw2013_match_table),
>   	},
>   	.probe = aw2013_probe,
> -	.remove = aw2013_remove,
>   };
>   
>   module_i2c_driver(aw2013_driver);

[PATCH v2 04/10] leds: aw200xx: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-aw200xx.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
index 1d3943f86f7f..b1a097c7c879 100644
--- a/drivers/leds/leds-aw200xx.c
+++ b/drivers/leds/leds-aw200xx.c
@@ -10,6 +10,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/container_of.h>
+#include <linux/devm-helpers.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/leds.h>
@@ -530,6 +531,20 @@ static const struct regmap_config aw200xx_regmap_config = {
 	.disable_locking = true,
 };
 
+static void aw200xx_chip_reset_action(void *data)
+{
+	const struct aw200xx *chip = (struct aw200xx *)data;
+
+	aw200xx_chip_reset(chip);
+}
+
+static void aw200xx_disable_action(void *data)
+{
+	const struct aw200xx *chip = (struct aw200xx *)data;
+
+	aw200xx_disable(chip);
+}
+
 static int aw200xx_probe(struct i2c_client *client)
 {
 	const struct aw200xx_chipdef *cdef;
@@ -568,11 +583,16 @@ static int aw200xx_probe(struct i2c_client *client)
 
 	aw200xx_enable(chip);
 
+	ret = devm_add_action(&client->dev, aw200xx_disable_action, chip);
+	if (ret)
+		return ret;
+
 	ret = aw200xx_chip_check(chip);
 	if (ret)
 		return ret;
 
-	mutex_init(&chip->mutex);
+	if (devm_mutex_init(&client->dev, &chip->mutex))
+		return -ENOMEM;
 
 	/* Need a lock now since after call aw200xx_probe_fw, sysfs nodes created */
 	mutex_lock(&chip->mutex);
@@ -581,6 +601,10 @@ static int aw200xx_probe(struct i2c_client *client)
 	if (ret)
 		goto out_unlock;
 
+	ret = devm_add_action(&client->dev, aw200xx_chip_reset_action, chip);
+	if (ret)
+		goto out_unlock;
+
 	ret = aw200xx_probe_fw(&client->dev, chip);
 	if (ret)
 		goto out_unlock;
@@ -595,15 +619,6 @@ static int aw200xx_probe(struct i2c_client *client)
 	return ret;
 }
 
-static void aw200xx_remove(struct i2c_client *client)
-{
-	struct aw200xx *chip = i2c_get_clientdata(client);
-
-	aw200xx_chip_reset(chip);
-	aw200xx_disable(chip);
-	mutex_destroy(&chip->mutex);
-}
-
 static const struct aw200xx_chipdef aw20036_cdef = {
 	.channels = 36,
 	.display_size_rows_max = 3,
@@ -652,7 +667,6 @@ static struct i2c_driver aw200xx_driver = {
 		.of_match_table = aw200xx_match_table,
 	},
 	.probe_new = aw200xx_probe,
-	.remove = aw200xx_remove,
 	.id_table = aw200xx_id,
 };
 module_i2c_driver(aw200xx_driver);
-- 
2.38.4

Re: [PATCH v2 04/10] leds: aw200xx: use devm API to cleanup module's resources

[Andy Shevchenko]

On Mon, Dec 4, 2023 at 8:07 PM George Stark <gnstark@salutedevices.com> wrote:
>
> In this driver LEDs are registered using devm_led_classdev_register()
> so they are automatically unregistered after module's remove() is done.
> led_classdev_unregister() calls module's led_set_brightness() to turn off
> the LEDs and that callback uses resources which were destroyed already
> in module's remove() so use devm API instead of remove().

...

> +static void aw200xx_chip_reset_action(void *data)
> +{
> +       const struct aw200xx *chip = (struct aw200xx *)data;
> +
> +       aw200xx_chip_reset(chip);
> +}
> +
> +static void aw200xx_disable_action(void *data)
> +{
> +       const struct aw200xx *chip = (struct aw200xx *)data;
> +
> +       aw200xx_disable(chip);
> +}

They can be made oneliners.

...

> +       if (devm_mutex_init(&client->dev, &chip->mutex))
> +               return -ENOMEM;

Do not shadow the real error code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 04/10] leds: aw200xx: use devm API to cleanup module's resources

[Christophe Leroy]

Le 04/12/2023 à 19:05, George Stark a écrit :
> In this driver LEDs are registered using devm_led_classdev_register()
> so they are automatically unregistered after module's remove() is done.
> led_classdev_unregister() calls module's led_set_brightness() to turn off
> the LEDs and that callback uses resources which were destroyed already
> in module's remove() so use devm API instead of remove().
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>   drivers/leds/leds-aw200xx.c | 36 +++++++++++++++++++++++++-----------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> index 1d3943f86f7f..b1a097c7c879 100644
> --- a/drivers/leds/leds-aw200xx.c
> +++ b/drivers/leds/leds-aw200xx.c
> @@ -10,6 +10,7 @@
>   #include <linux/bitfield.h>
>   #include <linux/bits.h>
>   #include <linux/container_of.h>
> +#include <linux/devm-helpers.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
>   #include <linux/leds.h>
> @@ -530,6 +531,20 @@ static const struct regmap_config aw200xx_regmap_config = {
>   	.disable_locking = true,
>   };
>   
> +static void aw200xx_chip_reset_action(void *data)
> +{
> +	const struct aw200xx *chip = (struct aw200xx *)data;
> +
> +	aw200xx_chip_reset(chip);

Same as previous patch, no need to cast data and no need for chip at 
all, directly do aw200xx_chip_reset(data)

> +}
> +
> +static void aw200xx_disable_action(void *data)
> +{
> +	const struct aw200xx *chip = (struct aw200xx *)data;
> +
> +	aw200xx_disable(chip);

Same

> +}
> +
>   static int aw200xx_probe(struct i2c_client *client)
>   {
>   	const struct aw200xx_chipdef *cdef;
> @@ -568,11 +583,16 @@ static int aw200xx_probe(struct i2c_client *client)
>   
>   	aw200xx_enable(chip);
>   
> +	ret = devm_add_action(&client->dev, aw200xx_disable_action, chip);
> +	if (ret)
> +		return ret;
> +
>   	ret = aw200xx_chip_check(chip);
>   	if (ret)
>   		return ret;
>   
> -	mutex_init(&chip->mutex);
> +	if (devm_mutex_init(&client->dev, &chip->mutex))
> +		return -ENOMEM;

Why not return value returned by dev_mutex_init() directly ?

>   
>   	/* Need a lock now since after call aw200xx_probe_fw, sysfs nodes created */
>   	mutex_lock(&chip->mutex);
> @@ -581,6 +601,10 @@ static int aw200xx_probe(struct i2c_client *client)
>   	if (ret)
>   		goto out_unlock;
>   
> +	ret = devm_add_action(&client->dev, aw200xx_chip_reset_action, chip);
> +	if (ret)
> +		goto out_unlock;
> +
>   	ret = aw200xx_probe_fw(&client->dev, chip);
>   	if (ret)
>   		goto out_unlock;
> @@ -595,15 +619,6 @@ static int aw200xx_probe(struct i2c_client *client)
>   	return ret;
>   }
>   
> -static void aw200xx_remove(struct i2c_client *client)
> -{
> -	struct aw200xx *chip = i2c_get_clientdata(client);
> -
> -	aw200xx_chip_reset(chip);
> -	aw200xx_disable(chip);
> -	mutex_destroy(&chip->mutex);
> -}
> -
>   static const struct aw200xx_chipdef aw20036_cdef = {
>   	.channels = 36,
>   	.display_size_rows_max = 3,
> @@ -652,7 +667,6 @@ static struct i2c_driver aw200xx_driver = {
>   		.of_match_table = aw200xx_match_table,
>   	},
>   	.probe_new = aw200xx_probe,
> -	.remove = aw200xx_remove,
>   	.id_table = aw200xx_id,
>   };
>   module_i2c_driver(aw200xx_driver);

[PATCH v2 05/10] leds: lp3952: use devm API to cleanup module's resources

[George Stark]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses resources which were destroyed already
in module's remove() so use devm API instead of remove().
Also drop explicit turning LEDs off from remove() due to they will be off
anyway by led_classdev_unregister().

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 drivers/leds/leds-lp3952.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
index 3bd55652a706..fc0e02a9768f 100644
--- a/drivers/leds/leds-lp3952.c
+++ b/drivers/leds/leds-lp3952.c
@@ -207,6 +207,13 @@ static const struct regmap_config lp3952_regmap = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static void gpio_set_low_action(void *data)
+{
+	struct lp3952_led_array *priv = (struct lp3952_led_array *)data;
+
+	gpiod_set_value(priv->enable_gpio, 0);
+}
+
 static int lp3952_probe(struct i2c_client *client)
 {
 	int status;
@@ -226,6 +233,10 @@ static int lp3952_probe(struct i2c_client *client)
 		return status;
 	}
 
+	status = devm_add_action(&client->dev, gpio_set_low_action, priv);
+	if (status)
+		return status;
+
 	priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap);
 	if (IS_ERR(priv->regmap)) {
 		int err = PTR_ERR(priv->regmap);
@@ -254,15 +265,6 @@ static int lp3952_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void lp3952_remove(struct i2c_client *client)
-{
-	struct lp3952_led_array *priv;
-
-	priv = i2c_get_clientdata(client);
-	lp3952_on_off(priv, LP3952_LED_ALL, false);
-	gpiod_set_value(priv->enable_gpio, 0);
-}
-
 static const struct i2c_device_id lp3952_id[] = {
 	{LP3952_NAME, 0},
 	{}
@@ -274,7 +276,6 @@ static struct i2c_driver lp3952_i2c_driver = {
 			.name = LP3952_NAME,
 	},
 	.probe = lp3952_probe,
-	.remove = lp3952_remove,
 	.id_table = lp3952_id,
 };
 
-- 
2.38.4