mbox series

[v7,0/8] devm_led_classdev_register() usage problem

Message ID 20240314201856.1991899-1-gnstark@salutedevices.com
Headers show
Series devm_led_classdev_register() usage problem | expand

Message

George Stark March 14, 2024, 8:18 p.m. UTC
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 subsystem 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

Changelog:
v1->v2:
	revise patch series completely

v2->v3:
locking: add define if mutex_destroy() is not an empty function
	new patch, discussed here [8]

devm-helpers: introduce devm_mutex_init
	previous version [4]
	- revise code based on mutex_destroy define
	- update commit message
	- update devm_mutex_init()'s description

leds: aw2013: unlock mutex before destroying it
	previous version [5]
	- make this patch first in the series
	- add tags Fixes and RvB by Andy

leds: aw2013: use devm API to cleanup module's resources
	previous version [6]
	- make aw2013_chip_disable_action()'s body one line
	- don't shadow devm_mutex_init() return code

leds: aw200xx: use devm API to cleanup module's resources
	previous version [7]
	- make aw200xx_*_action()'s bodies one line
	- don't shadow devm_mutex_init() return code

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 initialization
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
	- those patches were planned but not sent in the series #2 due to mail server
	problem on my side. I revised them according to the comments.

v3->v4:
locking: introduce devm_mutex_init
	new patch
	- move devm_mutex_init implementation completely from devm-helpers.h to mutex.h

locking: add define if mutex_destroy() is not an empty function
	drop the patch [9]

devm-helpers: introduce devm_mutex_init
	drop the patch [10]

leds: aw2013: use devm API to cleanup module's resources
	- add tag Tested-by: Nikita Travkin <nikita@trvn.ru>

v4->v5:
leds: aw2013: unlock mutex before destroying it
	merged separately and removed from the series

locking/mutex: move mutex_destroy() definition lower
	introduce optional refactoring patch

locking/mutex: introduce devm_mutex_init
	leave only one devm_mutex_init definition
	add tag Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

leds* patches
	remove #include <linux/devm-helpers.h> due to devm_mutex_init() in mutex.h now

v5->v6:
locking/mutex: move mutex_destroy() definition lower [11]
	drop the patch due to devm_mutex_init block is big enough to be declared standalone.

locking/mutex: introduce devm_mutex_init
	redesign devm_mutex_init function to macro to keep lockdep feature working
	use typeof to redeclare mutex var in macro body (thanks to checkpatch)
	previous version [12]

v6->v7:
locking/mutex: introduce devm_mutex_init
	fix comment at __devm_mutex_init
	move #include <linux/device.h> upper
	commit message: change devm_mutex_init -> devm_mutex_init(), add point in the end
	fix and move up tag Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
	add tags (in the order received):
	Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
	Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
	Reviewed-by: Marek Behún <kabel@kernel.org>
	Acked-by: Waiman Long <longman@redhat.com>

leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
	remove the patch from this series to send it separately

leds: mlxreg: use devm_mutex_init() for mutex initialization
leds: an30259a: use devm_mutex_init() for mutex initialization
	commit message: change devm_mutex_init -> devm_mutex_init()
	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

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
	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

[4] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mf500af0eda2a9ffc95594607dbe4cb64f2e3c9a8
[5] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mc92df4fb4f7d4187fb01cc1144acfa5fb5230dd2
[6] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m300df89710c43cc2ab598baa16c68dd0a0d7d681
[7] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m8e5c65e0c6b137c91fa00bb9320ad581164d1d0b
[8] https://lore.kernel.org/lkml/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m5f84a4a2f387d49678783e652b9e658e02c27450
[9] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m19ad1fc04c560012c1e27418e3156d0c9306dd84
[10] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m63126025f5d1bdcef69bcad50f2e58274d42e2d
[11] https://lore.kernel.org/lkml/20240307024034.1548605-2-gnstark@salutedevices.com/
[12] https://lore.kernel.org/lkml/20240307024034.1548605-3-gnstark@salutedevices.com/

George Stark (8):
  locking/mutex: introduce devm_mutex_init()
  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 initialization
  leds: an30259a: use devm_mutex_init() for mutex initialization

 drivers/leds/leds-an30259a.c | 14 ++++----------
 drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
 drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
 drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
 drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
 drivers/leds/leds-mlxreg.c   | 14 +++++---------
 drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
 include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
 kernel/locking/mutex-debug.c | 11 +++++++++++
 9 files changed, 122 insertions(+), 74 deletions(-)

--
2.25.1

Comments

Lee Jones March 21, 2024, 6:11 p.m. UTC | #1
On Thu, 14 Mar 2024, George Stark wrote:

> 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 subsystem 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
> 
> Changelog:
> v1->v2:
> 	revise patch series completely
> 
> v2->v3:
> locking: add define if mutex_destroy() is not an empty function
> 	new patch, discussed here [8]
> 
> devm-helpers: introduce devm_mutex_init
> 	previous version [4]
> 	- revise code based on mutex_destroy define
> 	- update commit message
> 	- update devm_mutex_init()'s description
> 
> leds: aw2013: unlock mutex before destroying it
> 	previous version [5]
> 	- make this patch first in the series
> 	- add tags Fixes and RvB by Andy
> 
> leds: aw2013: use devm API to cleanup module's resources
> 	previous version [6]
> 	- make aw2013_chip_disable_action()'s body one line
> 	- don't shadow devm_mutex_init() return code
> 
> leds: aw200xx: use devm API to cleanup module's resources
> 	previous version [7]
> 	- make aw200xx_*_action()'s bodies one line
> 	- don't shadow devm_mutex_init() return code
> 
> 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 initialization
> leds: an30259a: use devm_mutext_init for mutext initialization
> leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
> 	- those patches were planned but not sent in the series #2 due to mail server
> 	problem on my side. I revised them according to the comments.
> 
> v3->v4:
> locking: introduce devm_mutex_init
> 	new patch
> 	- move devm_mutex_init implementation completely from devm-helpers.h to mutex.h
> 
> locking: add define if mutex_destroy() is not an empty function
> 	drop the patch [9]
> 
> devm-helpers: introduce devm_mutex_init
> 	drop the patch [10]
> 
> leds: aw2013: use devm API to cleanup module's resources
> 	- add tag Tested-by: Nikita Travkin <nikita@trvn.ru>
> 
> v4->v5:
> leds: aw2013: unlock mutex before destroying it
> 	merged separately and removed from the series
> 
> locking/mutex: move mutex_destroy() definition lower
> 	introduce optional refactoring patch
> 
> locking/mutex: introduce devm_mutex_init
> 	leave only one devm_mutex_init definition
> 	add tag Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> leds* patches
> 	remove #include <linux/devm-helpers.h> due to devm_mutex_init() in mutex.h now
> 
> v5->v6:
> locking/mutex: move mutex_destroy() definition lower [11]
> 	drop the patch due to devm_mutex_init block is big enough to be declared standalone.
> 
> locking/mutex: introduce devm_mutex_init
> 	redesign devm_mutex_init function to macro to keep lockdep feature working
> 	use typeof to redeclare mutex var in macro body (thanks to checkpatch)
> 	previous version [12]
> 
> v6->v7:
> locking/mutex: introduce devm_mutex_init
> 	fix comment at __devm_mutex_init
> 	move #include <linux/device.h> upper
> 	commit message: change devm_mutex_init -> devm_mutex_init(), add point in the end
> 	fix and move up tag Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 	add tags (in the order received):
> 	Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 	Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 	Reviewed-by: Marek Behún <kabel@kernel.org>
> 	Acked-by: Waiman Long <longman@redhat.com>
> 
> leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds
> 	remove the patch from this series to send it separately
> 
> leds: mlxreg: use devm_mutex_init() for mutex initialization
> leds: an30259a: use devm_mutex_init() for mutex initialization
> 	commit message: change devm_mutex_init -> devm_mutex_init()
> 	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> 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
> 	add tag Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> [4] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mf500af0eda2a9ffc95594607dbe4cb64f2e3c9a8
> [5] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#mc92df4fb4f7d4187fb01cc1144acfa5fb5230dd2
> [6] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m300df89710c43cc2ab598baa16c68dd0a0d7d681
> [7] https://lore.kernel.org/lkml/20231204180603.470421-1-gnstark@salutedevices.com/T/#m8e5c65e0c6b137c91fa00bb9320ad581164d1d0b
> [8] https://lore.kernel.org/lkml/377e4437-7051-4d88-ae68-1460bcd692e1@redhat.com/T/#m5f84a4a2f387d49678783e652b9e658e02c27450
> [9] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m19ad1fc04c560012c1e27418e3156d0c9306dd84
> [10] https://lore.kernel.org/lkml/20231213223020.2713164-1-gnstark@salutedevices.com/T/#m63126025f5d1bdcef69bcad50f2e58274d42e2d
> [11] https://lore.kernel.org/lkml/20240307024034.1548605-2-gnstark@salutedevices.com/
> [12] https://lore.kernel.org/lkml/20240307024034.1548605-3-gnstark@salutedevices.com/
> 
> George Stark (8):
>   locking/mutex: introduce devm_mutex_init()
>   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 initialization
>   leds: an30259a: use devm_mutex_init() for mutex initialization
> 
>  drivers/leds/leds-an30259a.c | 14 ++++----------
>  drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
>  drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
>  drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
>  drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
>  drivers/leds/leds-mlxreg.c   | 14 +++++---------
>  drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
>  include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>  kernel/locking/mutex-debug.c | 11 +++++++++++
>  9 files changed, 122 insertions(+), 74 deletions(-)

Doesn't apply to v6.8.

What base was used for this?
George Stark March 22, 2024, 10:19 a.m. UTC | #2
Hello Lee

On 3/21/24 21:11, Lee Jones wrote:
> On Thu, 14 Mar 2024, George Stark wrote:
> 
>> 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 subsystem 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.

...

>>    locking/mutex: introduce devm_mutex_init()
>>    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 initialization
>>    leds: an30259a: use devm_mutex_init() for mutex initialization
>>
>>   drivers/leds/leds-an30259a.c | 14 ++++----------
>>   drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
>>   drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
>>   drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
>>   drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
>>   drivers/leds/leds-mlxreg.c   | 14 +++++---------
>>   drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
>>   include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>>   kernel/locking/mutex-debug.c | 11 +++++++++++
>>   9 files changed, 122 insertions(+), 74 deletions(-)
> 
> Doesn't apply to v6.8.
> 
> What base was used for this?

I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
and v7 was applied cleanly. linux-next is ok too.

v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
v7's patch #2 depends on it
Lee Jones March 22, 2024, 10:43 a.m. UTC | #3
On Fri, 22 Mar 2024, George Stark wrote:

> Hello Lee
> 
> On 3/21/24 21:11, Lee Jones wrote:
> > On Thu, 14 Mar 2024, George Stark wrote:
> > 
> > > 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 subsystem 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.
> 
> ...
> 
> > >    locking/mutex: introduce devm_mutex_init()
> > >    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 initialization
> > >    leds: an30259a: use devm_mutex_init() for mutex initialization
> > > 
> > >   drivers/leds/leds-an30259a.c | 14 ++++----------
> > >   drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
> > >   drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
> > >   drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
> > >   drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
> > >   drivers/leds/leds-mlxreg.c   | 14 +++++---------
> > >   drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
> > >   include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
> > >   kernel/locking/mutex-debug.c | 11 +++++++++++
> > >   9 files changed, 122 insertions(+), 74 deletions(-)
> > 
> > Doesn't apply to v6.8.
> > 
> > What base was used for this?
> 
> I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> and v7 was applied cleanly. linux-next is ok too.
> 
> v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
> v7's patch #2 depends on it

No problem.  I'll wait for v6.9-rc1.
George Stark March 29, 2024, 1:43 p.m. UTC | #4
Hello Lee

On 3/22/24 13:43, Lee Jones wrote:
> On Fri, 22 Mar 2024, George Stark wrote:
> 
>> Hello Lee
>>
>> On 3/21/24 21:11, Lee Jones wrote:
>>> On Thu, 14 Mar 2024, George Stark wrote:
>>>
>>>> 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 subsystem 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.
>>
>> ...
>>
>>>>     locking/mutex: introduce devm_mutex_init()
>>>>     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 initialization
>>>>     leds: an30259a: use devm_mutex_init() for mutex initialization
>>>>
>>>>    drivers/leds/leds-an30259a.c | 14 ++++----------
>>>>    drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
>>>>    drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
>>>>    drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
>>>>    drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
>>>>    drivers/leds/leds-mlxreg.c   | 14 +++++---------
>>>>    drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
>>>>    include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>>>>    kernel/locking/mutex-debug.c | 11 +++++++++++
>>>>    9 files changed, 122 insertions(+), 74 deletions(-)
>>>
>>> Doesn't apply to v6.8.
>>>
>>> What base was used for this?
>>
>> I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>> and v7 was applied cleanly. linux-next is ok too.
>>
>> v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
>> v7's patch #2 depends on it
> 
> No problem.  I'll wait for v6.9-rc1.
> 

Just checked the v7 patch series on released 6.9-rc1 and it's applied
cleanly. If anything i can help please let me know.
Lee Jones April 11, 2024, 10:59 a.m. UTC | #5
On Thu, 14 Mar 2024 23:18:48 +0300, George Stark wrote:
> 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().
> 
> [...]

Applied, thanks!

[1/8] locking/mutex: introduce devm_mutex_init()
      commit: c1a17857f8e333aa597ce0099c8728888e0c5037
[2/8] leds: aw2013: use devm API to cleanup module's resources
      commit: aa2fb50d9f17185e799a5969d0d357842d07450b
[3/8] leds: aw200xx: use devm API to cleanup module's resources
      commit: 517d9d3c408369cfa552652ac06294410c570095
[4/8] leds: lp3952: use devm API to cleanup module's resources
      commit: f8cf710c971e021e5ca39832adde32dfa241e081
[5/8] leds: lm3532: use devm API to cleanup module's resources
      commit: d265d86a2725e38c4f13ef79fc4e685b7234e7a3
[6/8] leds: nic78bx: use devm API to cleanup module's resources
      commit: 917676a8961e18e4dd5b17411317e3261473762a
[7/8] leds: mlxreg: use devm_mutex_init() for mutex initialization
      commit: 0548081cb25fbd0eb3a605cb6647dd166368da32
[8/8] leds: an30259a: use devm_mutex_init() for mutex initialization
      commit: c898cf6eb464d4172c79e925ea4d8007e856e10c

--
Lee Jones [李琼斯]
Lee Jones April 11, 2024, 11 a.m. UTC | #6
On Thu, 11 Apr 2024, Lee Jones wrote:

> On Thu, 14 Mar 2024 23:18:48 +0300, George Stark wrote:
> > 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().
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/8] locking/mutex: introduce devm_mutex_init()
>       commit: c1a17857f8e333aa597ce0099c8728888e0c5037
> [2/8] leds: aw2013: use devm API to cleanup module's resources
>       commit: aa2fb50d9f17185e799a5969d0d357842d07450b
> [3/8] leds: aw200xx: use devm API to cleanup module's resources
>       commit: 517d9d3c408369cfa552652ac06294410c570095
> [4/8] leds: lp3952: use devm API to cleanup module's resources
>       commit: f8cf710c971e021e5ca39832adde32dfa241e081
> [5/8] leds: lm3532: use devm API to cleanup module's resources
>       commit: d265d86a2725e38c4f13ef79fc4e685b7234e7a3
> [6/8] leds: nic78bx: use devm API to cleanup module's resources
>       commit: 917676a8961e18e4dd5b17411317e3261473762a
> [7/8] leds: mlxreg: use devm_mutex_init() for mutex initialization
>       commit: 0548081cb25fbd0eb3a605cb6647dd166368da32
> [8/8] leds: an30259a: use devm_mutex_init() for mutex initialization
>       commit: c898cf6eb464d4172c79e925ea4d8007e856e10c

Submitted for build testing.

I'll follow-up with a pull-request once everything is in order.
Lee Jones April 11, 2024, 1:45 p.m. UTC | #7
On Fri, 29 Mar 2024, George Stark wrote:

> Hello Lee
> 
> On 3/22/24 13:43, Lee Jones wrote:
> > On Fri, 22 Mar 2024, George Stark wrote:
> > 
> > > Hello Lee
> > > 
> > > On 3/21/24 21:11, Lee Jones wrote:
> > > > On Thu, 14 Mar 2024, George Stark wrote:
> > > > 
> > > > > 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 subsystem 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.
> > > 
> > > ...
> > > 
> > > > >     locking/mutex: introduce devm_mutex_init()
> > > > >     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 initialization
> > > > >     leds: an30259a: use devm_mutex_init() for mutex initialization
> > > > > 
> > > > >    drivers/leds/leds-an30259a.c | 14 ++++----------
> > > > >    drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
> > > > >    drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
> > > > >    drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
> > > > >    drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
> > > > >    drivers/leds/leds-mlxreg.c   | 14 +++++---------
> > > > >    drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
> > > > >    include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
> > > > >    kernel/locking/mutex-debug.c | 11 +++++++++++
> > > > >    9 files changed, 122 insertions(+), 74 deletions(-)
> > > > 
> > > > Doesn't apply to v6.8.
> > > > 
> > > > What base was used for this?
> > > 
> > > I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > > and v7 was applied cleanly. linux-next is ok too.
> > > 
> > > v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
> > > v7's patch #2 depends on it
> > 
> > No problem.  I'll wait for v6.9-rc1.
> > 
> 
> Just checked the v7 patch series on released 6.9-rc1 and it's applied
> cleanly. If anything i can help please let me know.

It applies, but doesn't seem to build:

make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc' allmodconfig
make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc'
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-an30259a.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw200xx.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw2013.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-lm3532.ko] undefined!
ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-mlxreg.ko] undefined!
make[3]: *** [/builds/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1

Did you forget to export it?
George Stark April 11, 2024, 2:20 p.m. UTC | #8
Hello Lee

On 4/11/24 16:45, Lee Jones wrote:
> On Fri, 29 Mar 2024, George Stark wrote:
> 
>> Hello Lee
>>
>> On 3/22/24 13:43, Lee Jones wrote:
>>> On Fri, 22 Mar 2024, George Stark wrote:
>>>
>>>> Hello Lee
>>>>
>>>> On 3/21/24 21:11, Lee Jones wrote:
>>>>> On Thu, 14 Mar 2024, George Stark wrote:
>>>>>
>>>>>> 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 subsystem 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.
>>>>
>>>> ...
>>>>
>>>>>>      locking/mutex: introduce devm_mutex_init()
>>>>>>      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 initialization
>>>>>>      leds: an30259a: use devm_mutex_init() for mutex initialization
>>>>>>
>>>>>>     drivers/leds/leds-an30259a.c | 14 ++++----------
>>>>>>     drivers/leds/leds-aw200xx.c  | 32 +++++++++++++++++++++-----------
>>>>>>     drivers/leds/leds-aw2013.c   | 25 +++++++++++++------------
>>>>>>     drivers/leds/leds-lm3532.c   | 29 +++++++++++++++++------------
>>>>>>     drivers/leds/leds-lp3952.c   | 21 +++++++++++----------
>>>>>>     drivers/leds/leds-mlxreg.c   | 14 +++++---------
>>>>>>     drivers/leds/leds-nic78bx.c  | 23 +++++++++++++----------
>>>>>>     include/linux/mutex.h        | 27 +++++++++++++++++++++++++++
>>>>>>     kernel/locking/mutex-debug.c | 11 +++++++++++
>>>>>>     9 files changed, 122 insertions(+), 74 deletions(-)
>>>>>
>>>>> Doesn't apply to v6.8.
>>>>>
>>>>> What base was used for this?
>>>>
>>>> I've just pulled git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>>>> and v7 was applied cleanly. linux-next is ok too.
>>>>
>>>> v6.8 is lack of recent patch 6969d0a2ba1adc9ba6a49b9805f24080896c255c
>>>> v7's patch #2 depends on it
>>>
>>> No problem.  I'll wait for v6.9-rc1.
>>>
>>
>> Just checked the v7 patch series on released 6.9-rc1 and it's applied
>> cleanly. If anything i can help please let me know.
> 
> It applies, but doesn't seem to build:
> 
> make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc' allmodconfig
> make --silent --keep-going --jobs=8 O=../build ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- 'CC=sccache aarch64-linux-gnu-gcc' 'HOSTCC=sccache gcc'
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-an30259a.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw200xx.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-aw2013.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-lm3532.ko] undefined!
> ERROR: modpost: "__devm_mutex_init" [drivers/leds/leds-mlxreg.ko] undefined!
> make[3]: *** [/builds/linux/scripts/Makefile.modpost:145: Module.symvers] Error 1
> 
> Did you forget to export it?
> 

Yes, my bad. I tested *DEBUG* configs but without modules enabled.
Adding EXPORT_SYMBOL_GPL(__devm_mutex_init); fixes the problem,
v8 is on the way.