All of lore.kernel.org
 help / color / mirror / Atom feed
From: George Stark <gnstark@salutedevices.com>
To: <andy.shevchenko@gmail.com>, <pavel@ucw.cz>, <lee@kernel.org>,
	<vadimp@nvidia.com>, <mpe@ellerman.id.au>, <npiggin@gmail.com>,
	<christophe.leroy@csgroup.eu>, <hdegoede@redhat.com>,
	<mazziesaccount@gmail.com>, <peterz@infradead.org>,
	<mingo@redhat.com>, <will@kernel.org>, <longman@redhat.com>,
	<boqun.feng@gmail.com>, <nikitos.tr@gmail.com>,
	<kabel@kernel.org>
Cc: <linux-leds@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linuxppc-dev@lists.ozlabs.org>, <kernel@salutedevices.com>,
	George Stark <gnstark@salutedevices.com>
Subject: [PATCH v5 00/10] devm_led_classdev_register() usage problem
Date: Thu, 7 Mar 2024 05:40:24 +0300	[thread overview]
Message-ID: <20240307024034.1548605-1-gnstark@salutedevices.com> (raw)

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

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 oneline
	- 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 oneline
	- 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 initializtion
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
	- those pathes 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_mutext_init in mutex.h now

[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

George Stark (10):
  locking/mutex: move mutex_destroy() definition lower
  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 initializtion
  leds: an30259a: use devm_mutext_init for mutext initialization
  leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds

 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 +++++++++++++----------
 drivers/leds/leds-powernv.c  | 23 ++++++++---------------
 include/linux/mutex.h        | 27 +++++++++++++++++++++++----
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 10 files changed, 137 insertions(+), 93 deletions(-)

--
2.25.1


WARNING: multiple messages have this Message-ID (diff)
From: George Stark <gnstark@salutedevices.com>
To: <andy.shevchenko@gmail.com>, <pavel@ucw.cz>, <lee@kernel.org>,
	<vadimp@nvidia.com>, <mpe@ellerman.id.au>, <npiggin@gmail.com>,
	<christophe.leroy@csgroup.eu>, <hdegoede@redhat.com>,
	<mazziesaccount@gmail.com>, <peterz@infradead.org>,
	<mingo@redhat.com>, <will@kernel.org>, <longman@redhat.com>,
	<boqun.feng@gmail.com>, <nikitos.tr@gmail.com>,
	<kabel@kernel.org>
Cc: kernel@salutedevices.com, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
	George Stark <gnstark@salutedevices.com>
Subject: [PATCH v5 00/10] devm_led_classdev_register() usage problem
Date: Thu, 7 Mar 2024 05:40:24 +0300	[thread overview]
Message-ID: <20240307024034.1548605-1-gnstark@salutedevices.com> (raw)

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

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 oneline
	- 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 oneline
	- 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 initializtion
leds: an30259a: use devm_mutext_init for mutext initialization
leds: powernv: add LED_RETAIN_AT_SHUTDOWN flag for leds
	- those pathes 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_mutext_init in mutex.h now

[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

George Stark (10):
  locking/mutex: move mutex_destroy() definition lower
  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 initializtion
  leds: an30259a: use devm_mutext_init for mutext initialization
  leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds

 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 +++++++++++++----------
 drivers/leds/leds-powernv.c  | 23 ++++++++---------------
 include/linux/mutex.h        | 27 +++++++++++++++++++++++----
 kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
 10 files changed, 137 insertions(+), 93 deletions(-)

--
2.25.1


             reply	other threads:[~2024-03-07  2:40 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-07  2:40 George Stark [this message]
2024-03-07  2:40 ` [PATCH v5 00/10] devm_led_classdev_register() usage problem George Stark
2024-03-07  2:40 ` [PATCH v5 01/10] locking/mutex: move mutex_destroy() definition lower George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 02/10] locking/mutex: introduce devm_mutex_init George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  9:56   ` Marek Behún
2024-03-07  9:56     ` Marek Behún
2024-03-07 13:39     ` Waiman Long
2024-03-07 13:39       ` Waiman Long
2024-03-07 16:44       ` Marek Behún
2024-03-07 16:44         ` Marek Behún
2024-03-11 23:47         ` George Stark
2024-03-11 23:47           ` George Stark
2024-03-12  1:10           ` Waiman Long
2024-03-12  1:10             ` Waiman Long
2024-03-12  5:44             ` Christophe Leroy
2024-03-12  5:44               ` Christophe Leroy
2024-03-12  6:04           ` Christophe Leroy
2024-03-12  6:04             ` Christophe Leroy
2024-03-12 11:39             ` George Stark
2024-03-12 11:39               ` George Stark
2024-03-12 11:51               ` Christophe Leroy
2024-03-12 11:51                 ` Christophe Leroy
2024-03-12 15:30                 ` George Stark
2024-03-12 15:30                   ` George Stark
2024-03-12 18:17                   ` Christophe Leroy
2024-03-12 18:17                     ` Christophe Leroy
2024-03-07 10:34   ` Andy Shevchenko
2024-03-07 10:34     ` Andy Shevchenko
2024-03-12  0:01     ` George Stark
2024-03-12  0:01       ` George Stark
2024-03-12  5:41       ` Christophe Leroy
2024-03-12  5:41         ` Christophe Leroy
2024-03-12  8:58         ` Andy Shevchenko
2024-03-12  8:58           ` Andy Shevchenko
2024-03-07 13:50   ` Christophe Leroy
2024-03-07 13:50     ` Christophe Leroy
2024-03-11 23:31     ` George Stark
2024-03-11 23:31       ` George Stark
2024-03-07  2:40 ` [PATCH v5 03/10] leds: aw2013: use devm API to cleanup module's resources George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 04/10] leds: aw200xx: " George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 05/10] leds: lp3952: " George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 06/10] leds: lm3532: " George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 07/10] leds: nic78bx: " George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 08/10] leds: mlxreg: use devm_mutex_init for mutex initializtion George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 09/10] leds: an30259a: use devm_mutext_init for mutext initialization George Stark
2024-03-07  2:40   ` George Stark
2024-03-07  2:40 ` [PATCH v5 10/10] leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds George Stark
2024-03-07  2:40   ` George Stark

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240307024034.1548605-1-gnstark@salutedevices.com \
    --to=gnstark@salutedevices.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=christophe.leroy@csgroup.eu \
    --cc=hdegoede@redhat.com \
    --cc=kabel@kernel.org \
    --cc=kernel@salutedevices.com \
    --cc=lee@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=longman@redhat.com \
    --cc=mazziesaccount@gmail.com \
    --cc=mingo@redhat.com \
    --cc=mpe@ellerman.id.au \
    --cc=nikitos.tr@gmail.com \
    --cc=npiggin@gmail.com \
    --cc=pavel@ucw.cz \
    --cc=peterz@infradead.org \
    --cc=vadimp@nvidia.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.