[PATCH 00/13] leds: Fix devm vs. non-devm ordering

[PATCH 00/13] leds: Fix devm vs. non-devm ordering

[Wang Yufen]

Fault-inject tests reports this issue:

DEBUG_LOCKS_WARN_ON(lock->magic != lock)
WARNING: CPU: 2 PID: 52 at kernel/locking/mutex.c:582 __mutex_lock+0x1366/0x15b0
Call Trace:
 <TASK>
   cr0014114_set_sync+0x2d/0x80 [leds_cr0014114 dbd1de3fefae3e163bcc08f4eeaa6d1b243203a9]
   set_brightness_delayed+0xc2/0x140
   process_one_work+0x651/0xc30
   worker_thread+0x30b/0x820
   kthread+0x1a0/0x1e0
   ret_from_fork+0x1f/0x30
 </TASK>

The issue occurs in the following scenarios:

.probe()
  mutex_init()
  led->brightness_set_blocking = callback  <-- e.g. cr0014114_set_sync()
  devm_led_classdev_register_ext()
  <-- dr->node.release = devm_led_classdev_release()
...

.remove()
  mutex_destroy(lock)         <-- lock destroy

worker_thread()
  set_brightness_work
    set_brightness_delayed()
      __led_set_brightness_blocking()
        led_cdev->brightness_set_blocking()
        <-- callback, e.g. cr0014114_set_sync()
          mutex_lock(lock)              <-- lock is used after destroy

.release()
  devm_led_classdev_release()
    led_classdev_unregister()
      <-- flush set_brightness_work

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Wang Yufen (13):
  leds: cr0014114: Fix devm vs. non-devm ordering
  leds: el15203000: Fix devm vs. non-devm ordering
  leds: lm3532: Fix devm vs. non-devm ordering
  leds: lm3692x: Fix devm vs. non-devm ordering
  leds: lm3697: Fix devm vs. non-devm ordering
  leds: lp50xx: Fix devm vs. non-devm ordering
  leds: lp8860: Fix devm vs. non-devm ordering
  leds: mlxreg: Fix devm vs. non-devm ordering
  leds: mt6323: Fix devm vs. non-devm ordering
  leds: powernv: Fix devm vs. non-devm ordering
  leds: sc27xx: Fix devm vs. non-devm ordering
  leds: spi-byte: Fix devm vs. non-devm ordering
  leds: rt8515: Fix devm vs. non-devm ordering

 drivers/leds/flash/leds-rt8515.c | 11 +++++++++--
 drivers/leds/leds-cr0014114.c    | 11 ++++++++++-
 drivers/leds/leds-el15203000.c   | 18 +++++++++++-------
 drivers/leds/leds-lm3532.c       | 10 ++++++++++
 drivers/leds/leds-lm3692x.c      | 11 ++++++++++-
 drivers/leds/leds-lm3697.c       | 13 ++++++++++---
 drivers/leds/leds-lp50xx.c       | 12 ++++++++++--
 drivers/leds/leds-lp8860.c       | 11 +++++++++--
 drivers/leds/leds-mlxreg.c       | 20 ++++++++++----------
 drivers/leds/leds-mt6323.c       | 11 +++++++++--
 drivers/leds/leds-powernv.c      | 12 +++++++++---
 drivers/leds/leds-sc27xx-bltc.c  | 27 +++++++++++----------------
 drivers/leds/leds-spi-byte.c     | 21 ++++++++++-----------
 13 files changed, 128 insertions(+), 60 deletions(-)

-- 
1.8.3.1

[PATCH 01/13] leds: cr0014114: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 9e50d5fb0d18 ("leds: add LED driver for CR0014114 board")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Oleh Kravchenko <oleg@kaa.org.ua>
---
 drivers/leds/leds-cr0014114.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
index c87686b..f3a0320 100644
--- a/drivers/leds/leds-cr0014114.c
+++ b/drivers/leds/leds-cr0014114.c
@@ -211,6 +211,11 @@ static int cr0014114_probe_dt(struct cr0014114 *priv)
 	return 0;
 }
 
+static void cr0014114_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int cr0014114_probe(struct spi_device *spi)
 {
 	struct cr0014114	*priv;
@@ -240,6 +245,11 @@ static int cr0014114_probe(struct spi_device *spi)
 	priv->delay	= jiffies -
 			  msecs_to_jiffies(CR_FW_DELAY_MSEC);
 
+	ret = devm_add_action_or_reset(&spi->dev, cr0014114_mutex_destroy,
+				       &priv->lock);
+	if (ret)
+		return ret;
+
 	priv->do_recount = true;
 	ret = cr0014114_sync(priv);
 	if (ret) {
@@ -271,7 +281,6 @@ static void cr0014114_remove(struct spi_device *spi)
 	struct cr0014114 *priv = spi_get_drvdata(spi);
 
 	cancel_delayed_work_sync(&priv->work);
-	mutex_destroy(&priv->lock);
 }
 
 static const struct of_device_id cr0014114_dt_ids[] = {
-- 
1.8.3.1

[PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: fc19967bcb8f ("leds: add LED driver for EL15203000 board")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Oleh Kravchenko <oleg@kaa.org.ua>
---
 drivers/leds/leds-el15203000.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
index 7e7b617..9be934e 100644
--- a/drivers/leds/leds-el15203000.c
+++ b/drivers/leds/leds-el15203000.c
@@ -287,10 +287,16 @@ static int el15203000_probe_dt(struct el15203000 *priv)
 	return ret;
 }
 
+static void el15203000_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int el15203000_probe(struct spi_device *spi)
 {
 	struct el15203000	*priv;
 	size_t			count;
+	int			ret;
 
 	count = device_get_child_node_count(&spi->dev);
 	if (!count) {
@@ -312,15 +318,14 @@ static int el15203000_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, priv);
 
+	ret = devm_add_action_or_reset(&spi->dev, el15203000_mutex_destroy,
+				       &priv->lock);
+	if (ret)
+		return ret;
+
 	return el15203000_probe_dt(priv);
 }
 
-static void el15203000_remove(struct spi_device *spi)
-{
-	struct el15203000 *priv = spi_get_drvdata(spi);
-
-	mutex_destroy(&priv->lock);
-}
 
 static const struct of_device_id el15203000_dt_ids[] = {
 	{ .compatible = "crane,el15203000", },
@@ -331,7 +336,6 @@ static void el15203000_remove(struct spi_device *spi)
 
 static struct spi_driver el15203000_driver = {
 	.probe		= el15203000_probe,
-	.remove		= el15203000_remove,
 	.driver = {
 		.name		= KBUILD_MODNAME,
 		.of_match_table	= el15203000_dt_ids,
-- 
1.8.3.1

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Oleh Kravchenko]

Hello all!

> 9 лист. 2022 р. о 10:48 Wang Yufen <wangyufen@huawei.com> написав(ла):
> 
> When non-devm resources are allocated they mustn't be followed by devm
> allocations, otherwise it will break the tear down ordering and might
> lead to crashes or other bugs during ->remove() stage. Fix this by
> wrapping mutex_destroy() call with devm_add_action_or_reset().
> 
> Fixes: fc19967bcb8f ("leds: add LED driver for EL15203000 board")
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> Cc: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
> drivers/leds/leds-el15203000.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
> index 7e7b617..9be934e 100644
> --- a/drivers/leds/leds-el15203000.c
> +++ b/drivers/leds/leds-el15203000.c
> @@ -287,10 +287,16 @@ static int el15203000_probe_dt(struct el15203000 *priv)
> return ret;
> }
> 
> +static void el15203000_mutex_destroy(void *lock)
> +{
> + mutex_destroy(lock);
> +}
> +
> static int el15203000_probe(struct spi_device *spi)
> {
> struct el15203000 *priv;
> size_t count;
> + int ret;
> 
> count = device_get_child_node_count(&spi->dev);
> if (!count) {
> @@ -312,15 +318,14 @@ static int el15203000_probe(struct spi_device *spi)
> 
> spi_set_drvdata(spi, priv);
> 
> + ret = devm_add_action_or_reset(&spi->dev, el15203000_mutex_destroy,
> +       &priv->lock);
> + if (ret)
> + return ret;
> +
> return el15203000_probe_dt(priv);
> }
> 
> -static void el15203000_remove(struct spi_device *spi)

Is remove() callback from struct spi_driver deprecated?

> -{
> - struct el15203000 *priv = spi_get_drvdata(spi);
> -
> - mutex_destroy(&priv->lock);
> -}
> 
> static const struct of_device_id el15203000_dt_ids[] = {
> { .compatible = "crane,el15203000", },
> @@ -331,7 +336,6 @@ static void el15203000_remove(struct spi_device *spi)
> 
> static struct spi_driver el15203000_driver = {
> .probe = el15203000_probe,
> - .remove = el15203000_remove,
> .driver = {
> .name = KBUILD_MODNAME,
> .of_match_table = el15203000_dt_ids,
> -- 
> 1.8.3.1
> 

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[wangyufen]

在 2022/11/9 17:39, Oleh Kravchenko 写道:
> Hello all!
>
>> 9 лист. 2022 р. о 10:48 Wang Yufen <wangyufen@huawei.com> написав(ла):
>>
>> When non-devm resources are allocated they mustn't be followed by devm
>> allocations, otherwise it will break the tear down ordering and might
>> lead to crashes or other bugs during ->remove() stage. Fix this by
>> wrapping mutex_destroy() call with devm_add_action_or_reset().
>>
>> Fixes: fc19967bcb8f ("leds: add LED driver for EL15203000 board")
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>> Cc: Oleh Kravchenko <oleg@kaa.org.ua>
>> ---
>> drivers/leds/leds-el15203000.c | 18 +++++++++++-------
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
>> index 7e7b617..9be934e 100644
>> --- a/drivers/leds/leds-el15203000.c
>> +++ b/drivers/leds/leds-el15203000.c
>> @@ -287,10 +287,16 @@ static int el15203000_probe_dt(struct el15203000 *priv)
>> return ret;
>> }
>>
>> +static void el15203000_mutex_destroy(void *lock)
>> +{
>> + mutex_destroy(lock);
>> +}
>> +
>> static int el15203000_probe(struct spi_device *spi)
>> {
>> struct el15203000 *priv;
>> size_t count;
>> + int ret;
>>
>> count = device_get_child_node_count(&spi->dev);
>> if (!count) {
>> @@ -312,15 +318,14 @@ static int el15203000_probe(struct spi_device *spi)
>>
>> spi_set_drvdata(spi, priv);
>>
>> + ret = devm_add_action_or_reset(&spi->dev, el15203000_mutex_destroy,
>> +       &priv->lock);
>> + if (ret)
>> + return ret;
>> +
>> return el15203000_probe_dt(priv);
>> }
>>
>> -static void el15203000_remove(struct spi_device *spi)
> Is remove() callback from struct spi_driver deprecated?

It is not that remove() callback is deprecated,
it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(),
remove() callback is unnecessary here.

>
>> -{
>> - struct el15203000 *priv = spi_get_drvdata(spi);
>> -
>> - mutex_destroy(&priv->lock);
>> -}
>>
>> static const struct of_device_id el15203000_dt_ids[] = {
>> { .compatible = "crane,el15203000", },
>> @@ -331,7 +336,6 @@ static void el15203000_remove(struct spi_device *spi)
>>
>> static struct spi_driver el15203000_driver = {
>> .probe = el15203000_probe,
>> - .remove = el15203000_remove,
>> .driver = {
>> .name = KBUILD_MODNAME,
>> .of_match_table = el15203000_dt_ids,
>> -- 
>> 1.8.3.1
>>
>

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Oleh Kravchenko]

> 9 лист. 2022 р. о 12:25 wangyufen <wangyufen@huawei.com> написав(ла):
> 
> 
> 在 2022/11/9 17:39, Oleh Kravchenko 写道:
>> Hello all!
>> 
>>> 9 лист. 2022 р. о 10:48 Wang Yufen <wangyufen@huawei.com> написав(ла):
>>> 
>>> return el15203000_probe_dt(priv);
>>> }
>>> 
>>> -static void el15203000_remove(struct spi_device *spi)
>> Is remove() callback from struct spi_driver deprecated?
> 
> It is not that remove() callback is deprecated,
> it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(),
> remove() callback is unnecessary here.

When remove() is called, the memory allocated by devm_*() is valid.
So what you try to fix here?

> 
>> 
>>> -{
>>> - struct el15203000 *priv = spi_get_drvdata(spi);
>>> -
>>> - mutex_destroy(&priv->lock);
>>> -}
>>> 
>>> static const struct of_device_id el15203000_dt_ids[] = {
>>> { .compatible = "crane,el15203000", },
>>> @@ -331,7 +336,6 @@ static void el15203000_remove(struct spi_device *spi)
>>> 
>>> static struct spi_driver el15203000_driver = {
>>> .probe = el15203000_probe,
>>> - .remove = el15203000_remove,
>>> .driver = {
>>> .name = KBUILD_MODNAME,
>>> .of_match_table = el15203000_dt_ids,
>>> -- 
>>> 1.8.3.1
>>> 
>> 

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Oleh Kravchenko]

Hello Wang,

> 11 лист. 2022 р. о 11:21 wangyufen <wangyufen@huawei.com> написав(ла):
> 
> 
> 在 2022/11/9 18:43, Oleh Kravchenko 写道:
>> 
>> 
>>> 9 лист. 2022 р. о 12:25 wangyufen <wangyufen@huawei.com> написав(ла):
>>> 
>>> 
>>> 在 2022/11/9 17:39, Oleh Kravchenko 写道:
>>> 
>>>>> -static void el15203000_remove(struct spi_device *spi)
>>>>> 
>>>> Is remove() callback from struct spi_driver deprecated?
>>>> 
>>> It is not that remove() callback is deprecated,
>>> it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(),
>>> remove() callback is unnecessary here.
>>> 
>> When remove() is called, the memory allocated by devm_*() is valid.
>> So what you try to fix here?
> 
> Fix the &priv->lock used after destroy, for details, please see patch #0
> LKML: Wang Yufen: [PATCH 00/13] leds: Fix devm vs. non-devm ordering

It doesn’t make any sense for me.
You saying that remove() called before devm_* allocation
if it true then set_brightness_delayed() will crash the system in anyway.

LED device has a parent SPI device; LED device can’t exist without SPI device.

So deallocation order should be next:
1. LED device devm_*()
2. SPI device remove()

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Wei Yongjun]

Hi Oleh,

On 2022/11/11 18:39, Oleh Kravchenko wrote:
> Hello Wang,
> 
>> 11 лист. 2022 р. о 11:21 wangyufen <wangyufen@huawei.com> написав(ла):
>>
>>
>> 在 2022/11/9 18:43, Oleh Kravchenko 写道:
>>>
>>>
>>>> 9 лист. 2022 р. о 12:25 wangyufen <wangyufen@huawei.com> написав(ла):
>>>>
>>>>
>>>> 在 2022/11/9 17:39, Oleh Kravchenko 写道:
>>>>
>>>>>> -static void el15203000_remove(struct spi_device *spi)
>>>>>>
>>>>> Is remove() callback from struct spi_driver deprecated?
>>>>>
>>>> It is not that remove() callback is deprecated,
>>>> it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(),
>>>> remove() callback is unnecessary here.
>>>>
>>> When remove() is called, the memory allocated by devm_*() is valid.
>>> So what you try to fix here?
>>
>> Fix the &priv->lock used after destroy, for details, please see patch #0
>> LKML: Wang Yufen: [PATCH 00/13] leds: Fix devm vs. non-devm ordering
> 
> It doesn’t make any sense for me.
> You saying that remove() called before devm_* allocation
> if it true then set_brightness_delayed() will crash the system in anyway.
> 
> LED device has a parent SPI device; LED device can’t exist without SPI device.
> 
> So deallocation order should be next:
> 1. LED device devm_*()
> 2. SPI device remove()

The allocation order is as follows:

el15203000_probe()
  mutex_init(&priv->lock);
  el15203000_probe_dt(priv)
    device_for_each_child_node(priv->dev, child) {
      ...
      led->ldev.brightness_set_blocking = el15203000_set_blocking;
      ...
      devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data);
         dr = devres_alloc(devm_led_classdev_release, sizeof(*dr), GFP_KERNEL);
         <-- dr->node.release = devm_led_classdev_release()
         ...
         devres_add(parent, dr);
         <-- add dr->node to &priv->dev->devres_head

And the full deallocation order should be this:

1. SPI device .remove callback
2. LED device devm_*()
3. SPI device deallocation

spi_unregister_device()
  device_del()
    bus_remove_device()
      device_release_driver_internal()
        __device_release_driver()
          ...
	  device_remove()
	    spi_remove()   <-- call el15203000_remove() here, mutex_destroy(&priv->lock), lock destroy
          ...
          device_unbind_cleanup()
            devres_release_all()
              release_nodes()
              <-- traverse spi->dev->devres_head list and call dr->node.release in sequence.
                   devm_led_classdev_release()
                     led_classdev_unregister()
                     <-- flush set_brightness_work here, before the work flush, set_brightness_work may be sched.
                     <-- that is el15203000_set_blocking()..-> mutex_lock(&led->priv->lock) is called,
                     <-- this leads to the priv->lock use after destroy.
 put_device(&spi->dev) <-- spi device is deallocation in here


Regards,
Wei Yongjun

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Wang Yufen]

在 2022/11/15 10:06, Wei Yongjun 写道:
> Hi Oleh,
> 
> On 2022/11/11 18:39, Oleh Kravchenko wrote:
>> Hello Wang,
>>
>>> 11 лист. 2022 р. о 11:21 wangyufen <wangyufen@huawei.com> написав(ла):
>>>
>>>
>>> 在 2022/11/9 18:43, Oleh Kravchenko 写道:
>>>>
>>>>
>>>>> 9 лист. 2022 р. о 12:25 wangyufen <wangyufen@huawei.com> написав(ла):
>>>>>
>>>>>
>>>>> 在 2022/11/9 17:39, Oleh Kravchenko 写道:
>>>>>
>>>>>>> -static void el15203000_remove(struct spi_device *spi)
>>>>>>>
>>>>>> Is remove() callback from struct spi_driver deprecated?
>>>>>>
>>>>> It is not that remove() callback is deprecated,
>>>>> it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(),
>>>>> remove() callback is unnecessary here.
>>>>>
>>>> When remove() is called, the memory allocated by devm_*() is valid.
>>>> So what you try to fix here?
>>>
>>> Fix the &priv->lock used after destroy, for details, please see patch #0
>>> LKML: Wang Yufen: [PATCH 00/13] leds: Fix devm vs. non-devm ordering
>>
>> It doesn’t make any sense for me.
>> You saying that remove() called before devm_* allocation
>> if it true then set_brightness_delayed() will crash the system in anyway.
>>
>> LED device has a parent SPI device; LED device can’t exist without SPI device.
>>
>> So deallocation order should be next:
>> 1. LED device devm_*()
>> 2. SPI device remove()
> 
> The allocation order is as follows:
> 
> el15203000_probe()
>    mutex_init(&priv->lock);
>    el15203000_probe_dt(priv)
>      device_for_each_child_node(priv->dev, child) {
>        ...
>        led->ldev.brightness_set_blocking = el15203000_set_blocking;
>        ...
>        devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data);
>           dr = devres_alloc(devm_led_classdev_release, sizeof(*dr), GFP_KERNEL);
>           <-- dr->node.release = devm_led_classdev_release()
>           ...
>           devres_add(parent, dr);
>           <-- add dr->node to &priv->dev->devres_head
> 
> And the full deallocation order should be this:
> 
> 1. SPI device .remove callback
> 2. LED device devm_*()
> 3. SPI device deallocation
> 
> spi_unregister_device()
>    device_del()
>      bus_remove_device()
>        device_release_driver_internal()
>          __device_release_driver()
>            ...
> 	  device_remove()
> 	    spi_remove()   <-- call el15203000_remove() here, mutex_destroy(&priv->lock), lock destroy
>            ...
>            device_unbind_cleanup()
>              devres_release_all()
>                release_nodes()
>                <-- traverse spi->dev->devres_head list and call dr->node.release in sequence.
>                     devm_led_classdev_release()
>                       led_classdev_unregister()
>                       <-- flush set_brightness_work here, before the work flush, set_brightness_work may be sched.
>                       <-- that is el15203000_set_blocking()..-> mutex_lock(&led->priv->lock) is called,
>                       <-- this leads to the priv->lock use after destroy.
>   put_device(&spi->dev) <-- spi device is deallocation in here
> 
>
Hi Oleh,

Judging from the deallocation order above, there is a issue that the 
&priv->lock used after destroy, right?

And thanks Wei for the detailed explanation.

Thanks,
Wang


> Regards,
> Wei Yongjun
> 

Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering

[Oleh Kravchenko]

Hello Wang,

22.11.22 03:10, Wang Yufen пише:
> 
> Hi Oleh,
> 
> Judging from the deallocation order above, there is a issue that the &priv->lock used after destroy, right?
> 
> And thanks Wei for the detailed explanation.
> 
> Thanks,
> Wang

Sorry, guys.
The last russian missile attack made my work impossible.
I will try to verify all when I have the ability.

[PATCH 03/13] leds: lm3532: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: bc1b8492c764 ("leds: lm3532: Introduce the lm3532 LED driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm3532.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/leds/leds-lm3532.c b/drivers/leds/leds-lm3532.c
index db64d44..a052966 100644
--- a/drivers/leds/leds-lm3532.c
+++ b/drivers/leds/leds-lm3532.c
@@ -663,6 +663,11 @@ static int lm3532_parse_node(struct lm3532_data *priv)
 	return ret;
 }
 
+static void lm3532_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int lm3532_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
@@ -693,6 +698,11 @@ static int lm3532_probe(struct i2c_client *client,
 	}
 
 	mutex_init(&drvdata->lock);
+	ret = devm_add_action_or_reset(&client->dev, lm3532_mutex_destroy,
+				       &drvdata->lock);
+	if (ret)
+		return ret;
+
 	i2c_set_clientdata(client, drvdata);
 
 	ret = lm3532_parse_node(drvdata);
-- 
1.8.3.1

[PATCH 04/13] leds: lm3692x: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 9699cb6bbef2 ("leds: lm3692x: Introduce LM3692x dual string driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm3692x.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 54b4662..aa1b0cc 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -456,6 +456,11 @@ static int lm3692x_probe_dt(struct lm3692x_led *led)
 	return ret;
 }
 
+static void lm3692x_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int lm3692x_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -472,6 +477,11 @@ static int lm3692x_probe(struct i2c_client *client,
 	led->model_id = id->driver_data;
 	i2c_set_clientdata(client, led);
 
+	ret = devm_add_action_or_reset(&client->dev, lm3692x_mutex_destroy,
+				       &led->lock);
+	if (ret)
+		return ret;
+
 	led->regmap = devm_regmap_init_i2c(client, &lm3692x_regmap_config);
 	if (IS_ERR(led->regmap)) {
 		ret = PTR_ERR(led->regmap);
@@ -496,7 +506,6 @@ static void lm3692x_remove(struct i2c_client *client)
 	struct lm3692x_led *led = i2c_get_clientdata(client);
 
 	lm3692x_leds_disable(led);
-	mutex_destroy(&led->lock);
 }
 
 static const struct i2c_device_id lm3692x_id[] = {
-- 
1.8.3.1

[PATCH 05/13] leds: lm3697: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 5c1d824cda9f ("leds: lm3697: Introduce the lm3697 driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lm3697.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 71231a6..a345333 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -299,6 +299,11 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 	return ret;
 }
 
+static void lm3697_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int lm3697_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -318,8 +323,12 @@ static int lm3697_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	mutex_init(&led->lock);
-	i2c_set_clientdata(client, led);
+	ret = devm_add_action_or_reset(&client->dev, lm3697_mutex_destroy,
+				       &led->lock);
+	if (ret)
+		return ret;
 
+	i2c_set_clientdata(client, led);
 	led->client = client;
 	led->dev = dev;
 	led->num_banks = count;
@@ -356,8 +365,6 @@ static void lm3697_remove(struct i2c_client *client)
 		if (ret)
 			dev_err(dev, "Failed to disable regulator\n");
 	}
-
-	mutex_destroy(&led->lock);
 }
 
 static const struct i2c_device_id lm3697_id[] = {
-- 
1.8.3.1

[PATCH 06/13] leds: lp50xx: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lp50xx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
index 28d6b39..fcec568e 100644
--- a/drivers/leds/leds-lp50xx.c
+++ b/drivers/leds/leds-lp50xx.c
@@ -521,6 +521,11 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
 	return ret;
 }
 
+static void lp50xx_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int lp50xx_probe(struct i2c_client *client)
 {
 	struct lp50xx *led;
@@ -539,6 +544,11 @@ static int lp50xx_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	mutex_init(&led->lock);
+	ret = devm_add_action_or_reset(&client->dev, lp50xx_mutex_destroy,
+				       &led->lock);
+	if (ret)
+		return ret;
+
 	led->client = client;
 	led->dev = &client->dev;
 	led->chip_info = device_get_match_data(&client->dev);
@@ -577,8 +587,6 @@ static void lp50xx_remove(struct i2c_client *client)
 		if (ret)
 			dev_err(led->dev, "Failed to disable regulator\n");
 	}
-
-	mutex_destroy(&led->lock);
 }
 
 static const struct i2c_device_id lp50xx_id[] = {
-- 
1.8.3.1

[PATCH 07/13] leds: lp8860: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: a2169c9b762a ("leds: lp8860: Various fixes to align with LED framework")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lp8860.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index e2b36d3..ca73d91 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -375,6 +375,11 @@ static int lp8860_init(struct lp8860_led *led)
 	.cache_type = REGCACHE_NONE,
 };
 
+static void lp8860_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int lp8860_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -408,6 +413,10 @@ static int lp8860_probe(struct i2c_client *client,
 	led->led_dev.brightness_set_blocking = lp8860_brightness_set;
 
 	mutex_init(&led->lock);
+	ret = devm_add_action_or_reset(&client->dev, lp8860_mutex_destroy,
+				       &led->lock);
+	if (ret)
+		return ret;
 
 	i2c_set_clientdata(client, led);
 
@@ -459,8 +468,6 @@ static void lp8860_remove(struct i2c_client *client)
 			dev_err(&led->client->dev,
 				"Failed to disable regulator\n");
 	}
-
-	mutex_destroy(&led->lock);
 }
 
 static const struct i2c_device_id lp8860_id[] = {
-- 
1.8.3.1

[PATCH 08/13] leds: mlxreg: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 386570d76f2f ("leds: add driver for support Mellanox regmap LEDs for BMC and x86 platform")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Vadim Pasternak <vadimp@mellanox.com>
---
 drivers/leds/leds-mlxreg.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
index b7855c9..b7c58fb 100644
--- a/drivers/leds/leds-mlxreg.c
+++ b/drivers/leds/leds-mlxreg.c
@@ -254,10 +254,16 @@ static int mlxreg_led_config(struct mlxreg_led_priv_data *priv)
 	return 0;
 }
 
+static void mlxreg_led_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int mlxreg_led_probe(struct platform_device *pdev)
 {
 	struct mlxreg_core_platform_data *led_pdata;
 	struct mlxreg_led_priv_data *priv;
+	int ret;
 
 	led_pdata = dev_get_platdata(&pdev->dev);
 	if (!led_pdata) {
@@ -273,24 +279,18 @@ static int mlxreg_led_probe(struct platform_device *pdev)
 	priv->pdev = pdev;
 	priv->pdata = led_pdata;
 
+	ret = devm_add_action_or_reset(&pdev->dev, mlxreg_led_mutex_destroy,
+				       &priv->access_lock);
+	if (ret)
+		return ret;
 	return mlxreg_led_config(priv);
 }
 
-static int mlxreg_led_remove(struct platform_device *pdev)
-{
-	struct mlxreg_led_priv_data *priv = dev_get_drvdata(&pdev->dev);
-
-	mutex_destroy(&priv->access_lock);
-
-	return 0;
-}
-
 static struct platform_driver mlxreg_led_driver = {
 	.driver = {
 	    .name = "leds-mlxreg",
 	},
 	.probe = mlxreg_led_probe,
-	.remove = mlxreg_led_remove,
 };
 
 module_platform_driver(mlxreg_led_driver);
-- 
1.8.3.1

[PATCH 09/13] leds: mt6323: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 216ec6cc4c19 ("leds: Add LED support for MT6323 PMIC")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Sean Wang <sean.wang@mediatek.com>
---
 drivers/leds/leds-mt6323.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
index f59e0e8..2dacaca 100644
--- a/drivers/leds/leds-mt6323.c
+++ b/drivers/leds/leds-mt6323.c
@@ -361,6 +361,11 @@ static int mt6323_led_set_dt_default(struct led_classdev *cdev,
 	return ret;
 }
 
+static void mt6323_led_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int mt6323_led_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -386,6 +391,10 @@ static int mt6323_led_probe(struct platform_device *pdev)
 	 */
 	leds->hw = hw;
 	mutex_init(&leds->lock);
+	ret = devm_add_action_or_reset(dev, mt6323_led_mutex_destroy,
+				       &leds->lock);
+	if (ret)
+		return ret;
 
 	status = MT6323_RG_DRV_32K_CK_PDN;
 	ret = regmap_update_bits(leds->hw->regmap, MT6323_TOP_CKPDN0,
@@ -464,8 +473,6 @@ static int mt6323_led_remove(struct platform_device *pdev)
 			   MT6323_RG_DRV_32K_CK_PDN_MASK,
 			   MT6323_RG_DRV_32K_CK_PDN);
 
-	mutex_destroy(&leds->lock);
-
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH 10/13] leds: powernv: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: 84ad6e5cd3e8 ("leds/powernv: Add driver for PowerNV platform")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 drivers/leds/leds-powernv.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
index 743e2cd..535f295 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -275,6 +275,11 @@ static int powernv_led_classdev(struct platform_device *pdev,
 	return rc;
 }
 
+static void powernv_led_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 /* Platform driver probe */
 static int powernv_led_probe(struct platform_device *pdev)
 {
@@ -298,6 +303,10 @@ static int powernv_led_probe(struct platform_device *pdev)
 	}
 
 	mutex_init(&powernv_led_common->lock);
+	ret = devm_add_action_or_reset(dev, powernv_led_mutex_destroy,
+				       &powernv_led_common->lock);
+	if (ret)
+		return ret;
 	powernv_led_common->max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
 
 	platform_set_drvdata(pdev, powernv_led_common);
@@ -317,9 +326,6 @@ static int powernv_led_remove(struct platform_device *pdev)
 	powernv_led_common = platform_get_drvdata(pdev);
 	powernv_led_common->led_disabled = true;
 
-	/* Destroy lock */
-	mutex_destroy(&powernv_led_common->lock);
-
 	dev_info(&pdev->dev, "PowerNV led module unregistered\n");
 	return 0;
 }
-- 
1.8.3.1

[PATCH 11/13] leds: sc27xx: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: e081c49e30ec ("leds: Add Spreadtrum SC27xx breathing light controller driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/leds/leds-sc27xx-bltc.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-sc27xx-bltc.c b/drivers/leds/leds-sc27xx-bltc.c
index e199ea1..43a821c 100644
--- a/drivers/leds/leds-sc27xx-bltc.c
+++ b/drivers/leds/leds-sc27xx-bltc.c
@@ -273,6 +273,11 @@ static int sc27xx_led_register(struct device *dev, struct sc27xx_led_priv *priv)
 	return 0;
 }
 
+static void sc27xx_led_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int sc27xx_led_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -297,6 +302,11 @@ static int sc27xx_led_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 	mutex_init(&priv->lock);
+	err = devm_add_action_or_reset(dev, sc27xx_led_mutex_destroy,
+				       &priv->lock);
+	if (err)
+		return err;
+
 	priv->base = base;
 	priv->regmap = dev_get_regmap(dev->parent, NULL);
 	if (!priv->regmap) {
@@ -309,13 +319,11 @@ static int sc27xx_led_probe(struct platform_device *pdev)
 		err = of_property_read_u32(child, "reg", &reg);
 		if (err) {
 			of_node_put(child);
-			mutex_destroy(&priv->lock);
 			return err;
 		}
 
 		if (reg >= SC27XX_LEDS_MAX || priv->leds[reg].active) {
 			of_node_put(child);
-			mutex_destroy(&priv->lock);
 			return -EINVAL;
 		}
 
@@ -323,19 +331,7 @@ static int sc27xx_led_probe(struct platform_device *pdev)
 		priv->leds[reg].active = true;
 	}
 
-	err = sc27xx_led_register(dev, priv);
-	if (err)
-		mutex_destroy(&priv->lock);
-
-	return err;
-}
-
-static int sc27xx_led_remove(struct platform_device *pdev)
-{
-	struct sc27xx_led_priv *priv = platform_get_drvdata(pdev);
-
-	mutex_destroy(&priv->lock);
-	return 0;
+	return sc27xx_led_register(dev, priv);
 }
 
 static const struct of_device_id sc27xx_led_of_match[] = {
@@ -350,7 +346,6 @@ static int sc27xx_led_remove(struct platform_device *pdev)
 		.of_match_table = sc27xx_led_of_match,
 	},
 	.probe = sc27xx_led_probe,
-	.remove = sc27xx_led_remove,
 };
 
 module_platform_driver(sc27xx_led_driver);
-- 
1.8.3.1

[PATCH 12/13] leds: spi-byte: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: e9a804d7a428 ("leds: spi-byte: add single byte SPI LED driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Christian Mauderer <oss@c-mauderer.de>
---
 drivers/leds/leds-spi-byte.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index 2bc5c99..a169780 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -78,6 +78,11 @@ static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
 	return ret;
 }
 
+static void spi_byte_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int spi_byte_probe(struct spi_device *spi)
 {
 	struct device_node *child;
@@ -101,6 +106,10 @@ static int spi_byte_probe(struct spi_device *spi)
 	strlcpy(led->name, name, sizeof(led->name));
 	led->spi = spi;
 	mutex_init(&led->mutex);
+	ret = devm_add_action_or_reset(dev, spi_byte_mutex_destroy,
+				       &led->mutex);
+	if (ret)
+		return ret;
 	led->cdef = device_get_match_data(dev);
 	led->ldev.name = led->name;
 	led->ldev.brightness = LED_OFF;
@@ -121,25 +130,15 @@ static int spi_byte_probe(struct spi_device *spi)
 					 led->ldev.brightness);
 
 	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
-	if (ret) {
-		mutex_destroy(&led->mutex);
+	if (ret)
 		return ret;
-	}
 	spi_set_drvdata(spi, led);
 
 	return 0;
 }
 
-static void spi_byte_remove(struct spi_device *spi)
-{
-	struct spi_byte_led	*led = spi_get_drvdata(spi);
-
-	mutex_destroy(&led->mutex);
-}
-
 static struct spi_driver spi_byte_driver = {
 	.probe		= spi_byte_probe,
-	.remove		= spi_byte_remove,
 	.driver = {
 		.name		= KBUILD_MODNAME,
 		.of_match_table	= spi_byte_dt_ids,
-- 
1.8.3.1

[PATCH 13/13] leds: rt8515: Fix devm vs. non-devm ordering

[Wang Yufen]

When non-devm resources are allocated they mustn't be followed by devm
allocations, otherwise it will break the tear down ordering and might
lead to crashes or other bugs during ->remove() stage. Fix this by
wrapping mutex_destroy() call with devm_add_action_or_reset().

Fixes: e1c6edcbea13 ("leds: rt8515: Add Richtek RT8515 LED driver")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/leds/flash/leds-rt8515.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/flash/leds-rt8515.c b/drivers/leds/flash/leds-rt8515.c
index 44904fde..f2d92ea 100644
--- a/drivers/leds/flash/leds-rt8515.c
+++ b/drivers/leds/flash/leds-rt8515.c
@@ -273,6 +273,11 @@ static void rt8515_determine_max_intensity(struct rt8515 *rt,
 	*max_intensity_setting = max_intensity;
 }
 
+static void rt8515_mutex_destroy(void *lock)
+{
+	mutex_destroy(lock);
+}
+
 static int rt8515_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -338,13 +343,16 @@ static int rt8515_probe(struct platform_device *pdev)
 	led->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH;
 
 	mutex_init(&rt->lock);
+	ret = devm_add_action_or_reset(dev, rt8515_mutex_destroy,
+				       &rt->lock);
+	if (ret)
+		return ret;
 
 	platform_set_drvdata(pdev, rt);
 
 	ret = devm_led_classdev_flash_register_ext(dev, fled, &init_data);
 	if (ret) {
 		fwnode_handle_put(child);
-		mutex_destroy(&rt->lock);
 		dev_err(dev, "can't register LED %s\n", led->name);
 		return ret;
 	}
@@ -373,7 +381,6 @@ static int rt8515_remove(struct platform_device *pdev)
 
 	rt8515_v4l2_flash_release(rt);
 	del_timer_sync(&rt->powerdown_timer);
-	mutex_destroy(&rt->lock);
 
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCH 00/13] leds: Fix devm vs. non-devm ordering

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

Hi!

> Fault-inject tests reports this issue:
> 
> DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> WARNING: CPU: 2 PID: 52 at kernel/locking/mutex.c:582 __mutex_lock+0x1366/0x15b0
> Call Trace:

Ok, so:

a) this does not happen in wild

b) you have not actually tested any of this

?

It looks reasonable, but the "Fixes:" tags mean -stable will pick this
up almost immediately.

Is anyone else willing to say this looks ok to him?

Any testers?

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 00/13] leds: Fix devm vs. non-devm ordering

[wangyufen]

在 2022/12/8 4:20, Pavel Machek 写道:
> Hi!
> 
>> Fault-inject tests reports this issue:
>>
>> DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>> WARNING: CPU: 2 PID: 52 at kernel/locking/mutex.c:582 __mutex_lock+0x1366/0x15b0
>> Call Trace:
> 
> Ok, so:
> 
> a) this does not happen in wild
> 
> b) you have not actually tested any of this
> 

Hi!

Sorry, I don't have an actual device, I tested it with bpf mock device. 
During the test on leds_cr0014114, the following issue occurs:

root@syzkaller:~# python3 -m kddv.cmds.test 
kddv.tests.leds.test_cr0014114.TestCR0014114
test_device_probe (kddv.tests.leds.test_cr0014114.TestCR0014114) ... [ 
61.307831] SPI driver leds_cr0014114 has no spi_device_id for 
crane,cr0014114
[   77.660222] ------------[ cut here ]------------
[   77.660822] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[   77.660860] WARNING: CPU: 0 PID: 95 at kernel/locking/mutex.c:582 
__mutex_lock+0x1366/0x15b0
[   77.662518] Modules linked in: leds_cr0014114 rtc_cmos
[   77.663190] CPU: 0 PID: 95 Comm: kworker/0:2 Tainted: G 
   N 6.1.0-rc6+ #59 b2d6fc177d3c6952297e89bfb954a5133622046e
[   77.664587] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS 1.13.0-1ubuntu1.1 04/01/2014
[   77.665760] Workqueue: events set_brightness_delayed
[   77.666428] RIP: 0010:__mutex_lock+0x1366/0x15b0
[   77.667035] Code: e8 fc 30 88 e8 3b ed 8a fe 8b 15 cd d6 eb 01 85 d2 
0f 85 9b ed ff ff 48 c7 c6 40 98 a6 86 48 c7 c7 c0 96 a6 86 e8 d3 e2 f4 
ff <0f> 0b e9 81 ed ff ff e8 0e 42 ff ff 85 c0 0f 84 b7 fe ff ff 80 3d
[   77.669233] RSP: 0018:ffff88810bbbfba0 EFLAGS: 00010282
[   77.669876] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
0000000000000000
[   77.670750] RDX: ffff88810bba3600 RSI: 0000000000000000 RDI: 
ffffed1021777f66
[   77.671628] RBP: ffff88810bbbfce0 R08: ffffffff8492d14f R09: 
ffffed1023604efa
[   77.672503] R10: ffff88811b0277cb R11: ffffed1023604ef9 R12: 
ffff888111a7a0e8
[   77.673376] R13: 0000000000000000 R14: ffff88811b038a20 R15: 
ffff88811b0389c0
[   77.674258] FS:  0000000000000000(0000) GS:ffff88811b000000(0000) 
knlGS:0000000000000000
[   77.675250] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   77.675969] CR2: 00007fc05d4229d0 CR3: 0000000109ba4003 CR4: 
0000000000370ef0
[   77.676828] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[   77.677706] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 
0000000000000400
[   77.678579] Call Trace:
[   77.678913]  <TASK>
[   77.679200]  ? cr0014114_set_sync+0x2d/0x80 [leds_cr0014114 
04aa6b9a9c145895781b235c6c924b67f752bc72]
[   77.680324]  ? mutex_lock_io_nested+0x13d0/0x13d0
[   77.680930]  ? lock_acquire+0x175/0x410
[   77.681422]  ? lock_is_held_type+0xd7/0x130
[   77.681975]  ? cr0014114_set_sync+0x2d/0x80 [leds_cr0014114 
04aa6b9a9c145895781b235c6c924b67f752bc72]
[   77.683103]  cr0014114_set_sync+0x2d/0x80 [leds_cr0014114 
04aa6b9a9c145895781b235c6c924b67f752bc72]
[   77.684214]  ? cr0014114_sync+0x360/0x360 [leds_cr0014114 
04aa6b9a9c145895781b235c6c924b67f752bc72]
[   77.685326]  set_brightness_delayed+0xc5/0x140
[   77.685919]  process_one_work+0x654/0xc30
[   77.686436]  ? pwq_dec_nr_in_flight+0x140/0x140
[   77.687046]  ? rwlock_bug.part.0+0x50/0x50
[   77.687576]  worker_thread+0x30b/0x820
[   77.688076]  ? preempt_count_sub+0xf/0xc0
[   77.688599]  ? process_one_work+0xc30/0xc30
[   77.689149]  kthread+0x1a2/0x1e0
[   77.689575]  ? kthread_complete_and_exit+0x40/0x40
[   77.690202]  ret_from_fork+0x22/0x30
[   77.690679]  </TASK>
[   77.691005] irq event stamp: 25531
[   77.691435] hardirqs last  enabled at (25531): [<ffffffff8645deaf>] 
_raw_spin_unlock_irq+0x1f/0x50
[   77.692533] hardirqs last disabled at (25530): [<ffffffff8645dc61>] 
_raw_spin_lock_irq+0x41/0x50
[   77.693605] softirqs last  enabled at (25522): [<ffffffff868003dc>] 
__do_softirq+0x3dc/0x58d
[   77.694644] softirqs last disabled at (25517): [<ffffffff848d3ad3>] 
irq_exit_rcu+0xd3/0x100
[   77.695672] ---[ end trace 0000000000000000 ]---


After patch "leds: cr0014114: Fix devm vs. non-devm ordering" is 
installed, the test is successful.

root@syzkaller:~# python3 -m kddv.cmds.test 
kddv.tests.leds.test_cr0014114.TestCR0014114
test_device_probe (kddv.tests.leds.test_cr0014114.TestCR0014114) ... [ 
216.419070] SPI driver leds_cr0014114 has no spi_device_id for 
crane,cr0014114
ok


The bpf mock device test tool is provided by Yongjun. Currently, some of 
the tools have been submitted to the community 
(https://lwn.net/Articles/906236/). The whole test set is still in the 
preliminary validation phase, will send to the community after the 
further verification.

By the way, here's an introduction to the tool, but now only in Chinese:
https://github.com/ChinaLinuxKernel/CLK2022/blob/main/%E5%9F%BA%E4%BA%8EeBPF%E7%9A%84%E8%AE%BE%E5%A4%87%E9%A9%B1%E5%8A%A8%E6%B5%8B%E8%AF%95%E6%A1%86%E6%9E%B6.pdf

Thanks,
Wang

> ?
> 
> It looks reasonable, but the "Fixes:" tags mean -stable will pick this
> up almost immediately.
> 
> Is anyone else willing to say this looks ok to him?
> 
> Any testers?
> 
> Best regards,
> 								Pavel