From: Lee Jones <lee@kernel.org>
To: George Stark <gnstark@salutedevices.com>
Cc: andy.shevchenko@gmail.com, pavel@ucw.cz, 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, linux-leds@vger.kernel.org,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
kernel@salutedevices.com
Subject: Re: [PATCH v4 00/10] devm_led_classdev_register() usage problem
Date: Thu, 21 Dec 2023 15:11:11 +0000 [thread overview]
Message-ID: <20231221151111.GJ10102@google.com> (raw)
In-Reply-To: <20231214173614.2820929-1-gnstark@salutedevices.com>
On Thu, 14 Dec 2023, 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 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>
>
> [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/#m63126025f5d1bdcef69bcad50f2e58274d42e2d7
>
> George Stark (10):
> leds: aw2013: unlock mutex before destroying it
> locking: 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 | 15 +++++----------
> drivers/leds/leds-aw200xx.c | 33 ++++++++++++++++++++++-----------
> drivers/leds/leds-aw2013.c | 27 +++++++++++++++------------
> drivers/leds/leds-lm3532.c | 30 ++++++++++++++++++------------
> drivers/leds/leds-lp3952.c | 21 +++++++++++----------
> drivers/leds/leds-mlxreg.c | 17 ++++++-----------
> drivers/leds/leds-nic78bx.c | 25 +++++++++++++------------
> drivers/leds/leds-powernv.c | 23 ++++++++---------------
FYI: I'll conduct my review once the locking side is settled.
> include/linux/mutex.h | 23 +++++++++++++++++++++++
> kernel/locking/mutex-debug.c | 22 ++++++++++++++++++++++
> 10 files changed, 143 insertions(+), 93 deletions(-)
>
> --
> 2.25.1
>
>
--
Lee Jones [李琼斯]
next prev parent reply other threads:[~2023-12-21 15:11 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-14 17:36 [PATCH v4 00/10] devm_led_classdev_register() usage problem George Stark
2023-12-14 17:36 ` [PATCH v4 01/10] leds: aw2013: unlock mutex before destroying it George Stark
2024-02-23 15:07 ` (subset) " Lee Jones
2023-12-14 17:36 ` [PATCH v4 02/10] locking: introduce devm_mutex_init George Stark
2023-12-14 18:48 ` Waiman Long
2023-12-14 19:53 ` Christophe Leroy
2023-12-14 21:48 ` Waiman Long
2023-12-15 5:46 ` Christophe Leroy
2023-12-17 1:05 ` George Stark
2023-12-17 9:31 ` Christophe Leroy
2023-12-18 13:26 ` George Stark
2023-12-14 19:47 ` Christophe Leroy
2023-12-15 6:22 ` [PATCH RFC v4-bis] " Christophe Leroy
2023-12-15 15:58 ` Andy Shevchenko
2023-12-15 17:51 ` Christophe Leroy
2023-12-16 1:30 ` Waiman Long
2023-12-14 17:36 ` [PATCH v4 03/10] leds: aw2013: use devm API to cleanup module's resources George Stark
2023-12-14 17:36 ` [PATCH v4 04/10] leds: aw200xx: " George Stark
2023-12-14 17:36 ` [PATCH v4 05/10] leds: lp3952: " George Stark
2023-12-14 17:36 ` [PATCH v4 06/10] leds: lm3532: " George Stark
2023-12-14 17:36 ` [PATCH v4 07/10] leds: nic78bx: " George Stark
2023-12-14 17:36 ` [PATCH v4 08/10] leds: mlxreg: use devm_mutex_init for mutex initializtion George Stark
2023-12-14 17:36 ` [PATCH v4 09/10] leds: an30259a: use devm_mutext_init for mutext initialization George Stark
2023-12-14 17:36 ` [PATCH v4 10/10] leds: powernv: use LED_RETAIN_AT_SHUTDOWN flag for leds George Stark
2023-12-21 15:11 ` Lee Jones [this message]
2024-02-09 17:11 ` [PATCH v4 00/10] devm_led_classdev_register() usage problem Andy Shevchenko
2024-02-11 23:52 ` [DMARC error][SPF error] " George Stark
2024-02-12 9:53 ` Andy Shevchenko
2024-02-13 0:14 ` George Stark
2024-02-13 10:55 ` Andy Shevchenko
2024-02-13 23:56 ` George Stark
2024-02-09 17:17 ` Andy Shevchenko
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=20231221151111.GJ10102@google.com \
--to=lee@kernel.org \
--cc=andy.shevchenko@gmail.com \
--cc=boqun.feng@gmail.com \
--cc=christophe.leroy@csgroup.eu \
--cc=gnstark@salutedevices.com \
--cc=hdegoede@redhat.com \
--cc=kernel@salutedevices.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).