linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] leds: trigger: fix potential deadlock with libata
@ 2020-11-02 10:41 Andrea Righi
  2020-11-25 12:46 ` Pavel Machek
  2021-03-06 20:39 ` Marc Kleine-Budde
  0 siblings, 2 replies; 13+ messages in thread
From: Andrea Righi @ 2020-11-02 10:41 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Boqun Feng, Dan Murphy, linux-leds, linux-kernel

We have the following potential deadlock condition:

 ========================================================
 WARNING: possible irq lock inversion dependency detected
 5.10.0-rc2+ #25 Not tainted
 --------------------------------------------------------
 swapper/3/0 just changed the state of lock:
 ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
 but this lock took another, HARDIRQ-READ-unsafe lock in the past:
  (&trig->leddev_list_lock){.+.?}-{2:2}

 and interrupts could create inverse lock ordering between them.

 other info that might help us debug this:
  Possible interrupt unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&trig->leddev_list_lock);
                                local_irq_disable();
                                lock(&host->lock);
                                lock(&trig->leddev_list_lock);
   <Interrupt>
     lock(&host->lock);

  *** DEADLOCK ***

 no locks held by swapper/3/0.

 the shortest dependencies between 2nd lock and 1st lock:
  -> (&trig->leddev_list_lock){.+.?}-{2:2} ops: 46 {
     HARDIRQ-ON-R at:
                       lock_acquire+0x15f/0x420
                       _raw_read_lock+0x42/0x90
                       led_trigger_event+0x2b/0x70
                       rfkill_global_led_trigger_worker+0x94/0xb0
                       process_one_work+0x240/0x560
                       worker_thread+0x58/0x3d0
                       kthread+0x151/0x170
                       ret_from_fork+0x1f/0x30
     IN-SOFTIRQ-R at:
                       lock_acquire+0x15f/0x420
                       _raw_read_lock+0x42/0x90
                       led_trigger_event+0x2b/0x70
                       kbd_bh+0x9e/0xc0
                       tasklet_action_common.constprop.0+0xe9/0x100
                       tasklet_action+0x22/0x30
                       __do_softirq+0xcc/0x46d
                       run_ksoftirqd+0x3f/0x70
                       smpboot_thread_fn+0x116/0x1f0
                       kthread+0x151/0x170
                       ret_from_fork+0x1f/0x30
     SOFTIRQ-ON-R at:
                       lock_acquire+0x15f/0x420
                       _raw_read_lock+0x42/0x90
                       led_trigger_event+0x2b/0x70
                       rfkill_global_led_trigger_worker+0x94/0xb0
                       process_one_work+0x240/0x560
                       worker_thread+0x58/0x3d0
                       kthread+0x151/0x170
                       ret_from_fork+0x1f/0x30
     INITIAL READ USE at:
                           lock_acquire+0x15f/0x420
                           _raw_read_lock+0x42/0x90
                           led_trigger_event+0x2b/0x70
                           rfkill_global_led_trigger_worker+0x94/0xb0
                           process_one_work+0x240/0x560
                           worker_thread+0x58/0x3d0
                           kthread+0x151/0x170
                           ret_from_fork+0x1f/0x30
   }
   ... key      at: [<ffffffff83da4c00>] __key.0+0x0/0x10
   ... acquired at:
    _raw_read_lock+0x42/0x90
    led_trigger_blink_oneshot+0x3b/0x90
    ledtrig_disk_activity+0x3c/0xa0
    ata_qc_complete+0x26/0x450
    ata_do_link_abort+0xa3/0xe0
    ata_port_freeze+0x2e/0x40
    ata_hsm_qc_complete+0x94/0xa0
    ata_sff_hsm_move+0x177/0x7a0
    ata_sff_pio_task+0xc7/0x1b0
    process_one_work+0x240/0x560
    worker_thread+0x58/0x3d0
    kthread+0x151/0x170
    ret_from_fork+0x1f/0x30

 -> (&host->lock){-...}-{2:2} ops: 69 {
    IN-HARDIRQ-W at:
                     lock_acquire+0x15f/0x420
                     _raw_spin_lock_irqsave+0x52/0xa0
                     ata_bmdma_interrupt+0x27/0x200
                     __handle_irq_event_percpu+0xd5/0x2b0
                     handle_irq_event+0x57/0xb0
                     handle_edge_irq+0x8c/0x230
                     asm_call_irq_on_stack+0xf/0x20
                     common_interrupt+0x100/0x1c0
                     asm_common_interrupt+0x1e/0x40
                     native_safe_halt+0xe/0x10
                     arch_cpu_idle+0x15/0x20
                     default_idle_call+0x59/0x1c0
                     do_idle+0x22c/0x2c0
                     cpu_startup_entry+0x20/0x30
                     start_secondary+0x11d/0x150
                     secondary_startup_64_no_verify+0xa6/0xab
    INITIAL USE at:
                    lock_acquire+0x15f/0x420
                    _raw_spin_lock_irqsave+0x52/0xa0
                    ata_dev_init+0x54/0xe0
                    ata_link_init+0x8b/0xd0
                    ata_port_alloc+0x1f1/0x210
                    ata_host_alloc+0xf1/0x130
                    ata_host_alloc_pinfo+0x14/0xb0
                    ata_pci_sff_prepare_host+0x41/0xa0
                    ata_pci_bmdma_prepare_host+0x14/0x30
                    piix_init_one+0x21f/0x600
                    local_pci_probe+0x48/0x80
                    pci_device_probe+0x105/0x1c0
                    really_probe+0x221/0x490
                    driver_probe_device+0xe9/0x160
                    device_driver_attach+0xb2/0xc0
                    __driver_attach+0x91/0x150
                    bus_for_each_dev+0x81/0xc0
                    driver_attach+0x1e/0x20
                    bus_add_driver+0x138/0x1f0
                    driver_register+0x91/0xf0
                    __pci_register_driver+0x73/0x80
                    piix_init+0x1e/0x2e
                    do_one_initcall+0x5f/0x2d0
                    kernel_init_freeable+0x26f/0x2cf
                    kernel_init+0xe/0x113
                    ret_from_fork+0x1f/0x30
  }
  ... key      at: [<ffffffff83d9fdc0>] __key.6+0x0/0x10
  ... acquired at:
    __lock_acquire+0x9da/0x2370
    lock_acquire+0x15f/0x420
    _raw_spin_lock_irqsave+0x52/0xa0
    ata_bmdma_interrupt+0x27/0x200
    __handle_irq_event_percpu+0xd5/0x2b0
    handle_irq_event+0x57/0xb0
    handle_edge_irq+0x8c/0x230
    asm_call_irq_on_stack+0xf/0x20
    common_interrupt+0x100/0x1c0
    asm_common_interrupt+0x1e/0x40
    native_safe_halt+0xe/0x10
    arch_cpu_idle+0x15/0x20
    default_idle_call+0x59/0x1c0
    do_idle+0x22c/0x2c0
    cpu_startup_entry+0x20/0x30
    start_secondary+0x11d/0x150
    secondary_startup_64_no_verify+0xa6/0xab

This lockdep splat is reported after:
commit e918188611f0 ("locking: More accurate annotations for read_lock()")

To clarify:
 - read-locks are recursive only in interrupt context (when
   in_interrupt() returns true)
 - after acquiring host->lock in CPU1, another cpu (i.e. CPU2) may call
   write_lock(&trig->leddev_list_lock) that would be blocked by CPU0
   that holds trig->leddev_list_lock in read-mode
 - when CPU1 (ata_ac_complete()) tries to read-lock
   trig->leddev_list_lock, it would be blocked by the write-lock waiter
   on CPU2 (because we are not in interrupt context, so the read-lock is
   not recursive)
 - at this point if an interrupt happens on CPU0 and
   ata_bmdma_interrupt() is executed it will try to acquire host->lock,
   that is held by CPU1, that is currently blocked by CPU2, so:

   * CPU0 blocked by CPU1
   * CPU1 blocked by CPU2
   * CPU2 blocked by CPU0

     *** DEADLOCK ***

The deadlock scenario is better represented by the following schema
(thanks to Boqun Feng <boqun.feng@gmail.com> for the schema and the
detailed explanation of the deadlock condition):

 CPU 0:                          CPU 1:                        CPU 2:
 -----                           -----                         -----
 led_trigger_event():
   read_lock(&trig->leddev_list_lock);
 				<workqueue>
 				ata_hsm_qc_complete():
 				  spin_lock_irqsave(&host->lock);
 								write_lock(&trig->leddev_list_lock);
 				  ata_port_freeze():
 				    ata_do_link_abort():
 				      ata_qc_complete():
 					ledtrig_disk_activity():
 					  led_trigger_blink_oneshot():
 					    read_lock(&trig->leddev_list_lock);
 					    // ^ not in in_interrupt() context, so could get blocked by CPU 2
 <interrupt>
   ata_bmdma_interrupt():
     spin_lock_irqsave(&host->lock);

Fix by using read_lock_irqsave/irqrestore() in led_trigger_event(), so
that no interrupt can happen in between, preventing the deadlock
condition.

Link: https://lore.kernel.org/lkml/20201101092614.GB3989@xps-13-7390/
Fixes: eb25cb9956cc ("leds: convert IDE trigger to common disk trigger")
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
---
 drivers/leds/led-triggers.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 91da90cfb11d..16d1a93a10a8 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
 			enum led_brightness brightness)
 {
 	struct led_classdev *led_cdev;
+	unsigned long flags;
 
 	if (!trig)
 		return;
 
-	read_lock(&trig->leddev_list_lock);
+	read_lock_irqsave(&trig->leddev_list_lock, flags);
 	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
 		led_set_brightness(led_cdev, brightness);
-	read_unlock(&trig->leddev_list_lock);
+	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
 }
 EXPORT_SYMBOL_GPL(led_trigger_event);
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2020-11-02 10:41 [PATCH] leds: trigger: fix potential deadlock with libata Andrea Righi
@ 2020-11-25 12:46 ` Pavel Machek
  2020-11-25 14:01   ` Boqun Feng
  2020-11-25 14:15   ` Andrea Righi
  2021-03-06 20:39 ` Marc Kleine-Budde
  1 sibling, 2 replies; 13+ messages in thread
From: Pavel Machek @ 2020-11-25 12:46 UTC (permalink / raw)
  To: Andrea Righi; +Cc: Boqun Feng, Dan Murphy, linux-leds, linux-kernel

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

Hi!

> We have the following potential deadlock condition:
> 
>  ========================================================
>  WARNING: possible irq lock inversion dependency detected
>  5.10.0-rc2+ #25 Not tainted
>  --------------------------------------------------------
>  swapper/3/0 just changed the state of lock:
>  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
>  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
>   (&trig->leddev_list_lock){.+.?}-{2:2}
> 
>  and interrupts could create inverse lock ordering between them.
> 
>  other info that might help us debug this:
>   Possible interrupt unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(&trig->leddev_list_lock);
>                                 local_irq_disable();
>                                 lock(&host->lock);
>                                 lock(&trig->leddev_list_lock);
>    <Interrupt>
>      lock(&host->lock);
> 
>   *** DEADLOCK ***
> 
>  no locks held by swapper/3/0.
> 
>  the shortest dependencies between 2nd lock and 1st lock:
>   -> (&trig->leddev_list_lock){.+.?}-{2:2} ops: 46 {
>      HARDIRQ-ON-R at:
>                        lock_acquire+0x15f/0x420
>                        _raw_read_lock+0x42/0x90
>                        led_trigger_event+0x2b/0x70
>                        rfkill_global_led_trigger_worker+0x94/0xb0
>                        process_one_work+0x240/0x560
>                        worker_thread+0x58/0x3d0
>                        kthread+0x151/0x170
>                        ret_from_fork+0x1f/0x30
>      IN-SOFTIRQ-R at:
>                        lock_acquire+0x15f/0x420
>                        _raw_read_lock+0x42/0x90
>                        led_trigger_event+0x2b/0x70
>                        kbd_bh+0x9e/0xc0
>                        tasklet_action_common.constprop.0+0xe9/0x100
>                        tasklet_action+0x22/0x30
>                        __do_softirq+0xcc/0x46d
>                        run_ksoftirqd+0x3f/0x70
>                        smpboot_thread_fn+0x116/0x1f0
>                        kthread+0x151/0x170
>                        ret_from_fork+0x1f/0x30
>      SOFTIRQ-ON-R at:
>                        lock_acquire+0x15f/0x420
>                        _raw_read_lock+0x42/0x90
>                        led_trigger_event+0x2b/0x70
>                        rfkill_global_led_trigger_worker+0x94/0xb0
>                        process_one_work+0x240/0x560
>                        worker_thread+0x58/0x3d0
>                        kthread+0x151/0x170
>                        ret_from_fork+0x1f/0x30
>      INITIAL READ USE at:
>                            lock_acquire+0x15f/0x420
>                            _raw_read_lock+0x42/0x90
>                            led_trigger_event+0x2b/0x70
>                            rfkill_global_led_trigger_worker+0x94/0xb0
>                            process_one_work+0x240/0x560
>                            worker_thread+0x58/0x3d0
>                            kthread+0x151/0x170
>                            ret_from_fork+0x1f/0x30
>    }
>    ... key      at: [<ffffffff83da4c00>] __key.0+0x0/0x10
>    ... acquired at:
>     _raw_read_lock+0x42/0x90
>     led_trigger_blink_oneshot+0x3b/0x90
>     ledtrig_disk_activity+0x3c/0xa0
>     ata_qc_complete+0x26/0x450
>     ata_do_link_abort+0xa3/0xe0
>     ata_port_freeze+0x2e/0x40
>     ata_hsm_qc_complete+0x94/0xa0
>     ata_sff_hsm_move+0x177/0x7a0
>     ata_sff_pio_task+0xc7/0x1b0
>     process_one_work+0x240/0x560
>     worker_thread+0x58/0x3d0
>     kthread+0x151/0x170
>     ret_from_fork+0x1f/0x30
> 
>  -> (&host->lock){-...}-{2:2} ops: 69 {
>     IN-HARDIRQ-W at:
>                      lock_acquire+0x15f/0x420
>                      _raw_spin_lock_irqsave+0x52/0xa0
>                      ata_bmdma_interrupt+0x27/0x200
>                      __handle_irq_event_percpu+0xd5/0x2b0
>                      handle_irq_event+0x57/0xb0
>                      handle_edge_irq+0x8c/0x230
>                      asm_call_irq_on_stack+0xf/0x20
>                      common_interrupt+0x100/0x1c0
>                      asm_common_interrupt+0x1e/0x40
>                      native_safe_halt+0xe/0x10
>                      arch_cpu_idle+0x15/0x20
>                      default_idle_call+0x59/0x1c0
>                      do_idle+0x22c/0x2c0
>                      cpu_startup_entry+0x20/0x30
>                      start_secondary+0x11d/0x150
>                      secondary_startup_64_no_verify+0xa6/0xab
>     INITIAL USE at:
>                     lock_acquire+0x15f/0x420
>                     _raw_spin_lock_irqsave+0x52/0xa0
>                     ata_dev_init+0x54/0xe0
>                     ata_link_init+0x8b/0xd0
>                     ata_port_alloc+0x1f1/0x210
>                     ata_host_alloc+0xf1/0x130
>                     ata_host_alloc_pinfo+0x14/0xb0
>                     ata_pci_sff_prepare_host+0x41/0xa0
>                     ata_pci_bmdma_prepare_host+0x14/0x30
>                     piix_init_one+0x21f/0x600
>                     local_pci_probe+0x48/0x80
>                     pci_device_probe+0x105/0x1c0
>                     really_probe+0x221/0x490
>                     driver_probe_device+0xe9/0x160
>                     device_driver_attach+0xb2/0xc0
>                     __driver_attach+0x91/0x150
>                     bus_for_each_dev+0x81/0xc0
>                     driver_attach+0x1e/0x20
>                     bus_add_driver+0x138/0x1f0
>                     driver_register+0x91/0xf0
>                     __pci_register_driver+0x73/0x80
>                     piix_init+0x1e/0x2e
>                     do_one_initcall+0x5f/0x2d0
>                     kernel_init_freeable+0x26f/0x2cf
>                     kernel_init+0xe/0x113
>                     ret_from_fork+0x1f/0x30
>   }
>   ... key      at: [<ffffffff83d9fdc0>] __key.6+0x0/0x10
>   ... acquired at:
>     __lock_acquire+0x9da/0x2370
>     lock_acquire+0x15f/0x420
>     _raw_spin_lock_irqsave+0x52/0xa0
>     ata_bmdma_interrupt+0x27/0x200
>     __handle_irq_event_percpu+0xd5/0x2b0
>     handle_irq_event+0x57/0xb0
>     handle_edge_irq+0x8c/0x230
>     asm_call_irq_on_stack+0xf/0x20
>     common_interrupt+0x100/0x1c0
>     asm_common_interrupt+0x1e/0x40
>     native_safe_halt+0xe/0x10
>     arch_cpu_idle+0x15/0x20
>     default_idle_call+0x59/0x1c0
>     do_idle+0x22c/0x2c0
>     cpu_startup_entry+0x20/0x30
>     start_secondary+0x11d/0x150
>     secondary_startup_64_no_verify+0xa6/0xab
> 
> This lockdep splat is reported after:
> commit e918188611f0 ("locking: More accurate annotations for read_lock()")
> 
> To clarify:
>  - read-locks are recursive only in interrupt context (when
>    in_interrupt() returns true)
>  - after acquiring host->lock in CPU1, another cpu (i.e. CPU2) may call
>    write_lock(&trig->leddev_list_lock) that would be blocked by CPU0
>    that holds trig->leddev_list_lock in read-mode
>  - when CPU1 (ata_ac_complete()) tries to read-lock
>    trig->leddev_list_lock, it would be blocked by the write-lock waiter
>    on CPU2 (because we are not in interrupt context, so the read-lock is
>    not recursive)
>  - at this point if an interrupt happens on CPU0 and
>    ata_bmdma_interrupt() is executed it will try to acquire host->lock,
>    that is held by CPU1, that is currently blocked by CPU2, so:
> 
>    * CPU0 blocked by CPU1
>    * CPU1 blocked by CPU2
>    * CPU2 blocked by CPU0
> 
>      *** DEADLOCK ***
> 
> The deadlock scenario is better represented by the following schema
> (thanks to Boqun Feng <boqun.feng@gmail.com> for the schema and the
> detailed explanation of the deadlock condition):
> 
>  CPU 0:                          CPU 1:                        CPU 2:
>  -----                           -----                         -----
>  led_trigger_event():
>    read_lock(&trig->leddev_list_lock);
>  				<workqueue>
>  				ata_hsm_qc_complete():
>  				  spin_lock_irqsave(&host->lock);
>  								write_lock(&trig->leddev_list_lock);
>  				  ata_port_freeze():
>  				    ata_do_link_abort():
>  				      ata_qc_complete():
>  					ledtrig_disk_activity():
>  					  led_trigger_blink_oneshot():
>  					    read_lock(&trig->leddev_list_lock);
>  					    // ^ not in in_interrupt() context, so could get blocked by CPU 2
>  <interrupt>
>    ata_bmdma_interrupt():
>      spin_lock_irqsave(&host->lock);
> 
> Fix by using read_lock_irqsave/irqrestore() in led_trigger_event(), so
> that no interrupt can happen in between, preventing the deadlock
> condition.
> 
> Link: https://lore.kernel.org/lkml/20201101092614.GB3989@xps-13-7390/
> Fixes: eb25cb9956cc ("leds: convert IDE trigger to common disk trigger")
> Signed-off-by: Andrea Righi <andrea.righi@canonical.com>

I'd hate to see this in stable 3 days after Linus merges it...

Do these need _irqsave, too?

drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);

Best regards,
								Pavel

> ---
>  drivers/leds/led-triggers.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 91da90cfb11d..16d1a93a10a8 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
>  			enum led_brightness brightness)
>  {
>  	struct led_classdev *led_cdev;
> +	unsigned long flags;
>  
>  	if (!trig)
>  		return;
>  
> -	read_lock(&trig->leddev_list_lock);
> +	read_lock_irqsave(&trig->leddev_list_lock, flags);
>  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
>  		led_set_brightness(led_cdev, brightness);
> -	read_unlock(&trig->leddev_list_lock);
> +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_event);
>  

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2020-11-25 12:46 ` Pavel Machek
@ 2020-11-25 14:01   ` Boqun Feng
  2020-11-25 14:15   ` Andrea Righi
  1 sibling, 0 replies; 13+ messages in thread
From: Boqun Feng @ 2020-11-25 14:01 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Andrea Righi, Dan Murphy, linux-leds, linux-kernel

On Wed, Nov 25, 2020 at 01:46:48PM +0100, Pavel Machek wrote:
> Hi!
> 
> > We have the following potential deadlock condition:
> > 
> >  ========================================================
> >  WARNING: possible irq lock inversion dependency detected
> >  5.10.0-rc2+ #25 Not tainted
> >  --------------------------------------------------------
> >  swapper/3/0 just changed the state of lock:
> >  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
> >  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
> >   (&trig->leddev_list_lock){.+.?}-{2:2}
> > 
> >  and interrupts could create inverse lock ordering between them.
> > 
> >  other info that might help us debug this:
> >   Possible interrupt unsafe locking scenario:
> > 
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(&trig->leddev_list_lock);
> >                                 local_irq_disable();
> >                                 lock(&host->lock);
> >                                 lock(&trig->leddev_list_lock);
> >    <Interrupt>
> >      lock(&host->lock);
> > 
> >   *** DEADLOCK ***
> > 
> >  no locks held by swapper/3/0.
> > 
> >  the shortest dependencies between 2nd lock and 1st lock:
> >   -> (&trig->leddev_list_lock){.+.?}-{2:2} ops: 46 {
> >      HARDIRQ-ON-R at:
> >                        lock_acquire+0x15f/0x420
> >                        _raw_read_lock+0x42/0x90
> >                        led_trigger_event+0x2b/0x70
> >                        rfkill_global_led_trigger_worker+0x94/0xb0
> >                        process_one_work+0x240/0x560
> >                        worker_thread+0x58/0x3d0
> >                        kthread+0x151/0x170
> >                        ret_from_fork+0x1f/0x30
> >      IN-SOFTIRQ-R at:
> >                        lock_acquire+0x15f/0x420
> >                        _raw_read_lock+0x42/0x90
> >                        led_trigger_event+0x2b/0x70
> >                        kbd_bh+0x9e/0xc0
> >                        tasklet_action_common.constprop.0+0xe9/0x100
> >                        tasklet_action+0x22/0x30
> >                        __do_softirq+0xcc/0x46d
> >                        run_ksoftirqd+0x3f/0x70
> >                        smpboot_thread_fn+0x116/0x1f0
> >                        kthread+0x151/0x170
> >                        ret_from_fork+0x1f/0x30
> >      SOFTIRQ-ON-R at:
> >                        lock_acquire+0x15f/0x420
> >                        _raw_read_lock+0x42/0x90
> >                        led_trigger_event+0x2b/0x70
> >                        rfkill_global_led_trigger_worker+0x94/0xb0
> >                        process_one_work+0x240/0x560
> >                        worker_thread+0x58/0x3d0
> >                        kthread+0x151/0x170
> >                        ret_from_fork+0x1f/0x30
> >      INITIAL READ USE at:
> >                            lock_acquire+0x15f/0x420
> >                            _raw_read_lock+0x42/0x90
> >                            led_trigger_event+0x2b/0x70
> >                            rfkill_global_led_trigger_worker+0x94/0xb0
> >                            process_one_work+0x240/0x560
> >                            worker_thread+0x58/0x3d0
> >                            kthread+0x151/0x170
> >                            ret_from_fork+0x1f/0x30
> >    }
> >    ... key      at: [<ffffffff83da4c00>] __key.0+0x0/0x10
> >    ... acquired at:
> >     _raw_read_lock+0x42/0x90
> >     led_trigger_blink_oneshot+0x3b/0x90
> >     ledtrig_disk_activity+0x3c/0xa0
> >     ata_qc_complete+0x26/0x450
> >     ata_do_link_abort+0xa3/0xe0
> >     ata_port_freeze+0x2e/0x40
> >     ata_hsm_qc_complete+0x94/0xa0
> >     ata_sff_hsm_move+0x177/0x7a0
> >     ata_sff_pio_task+0xc7/0x1b0
> >     process_one_work+0x240/0x560
> >     worker_thread+0x58/0x3d0
> >     kthread+0x151/0x170
> >     ret_from_fork+0x1f/0x30
> > 
> >  -> (&host->lock){-...}-{2:2} ops: 69 {
> >     IN-HARDIRQ-W at:
> >                      lock_acquire+0x15f/0x420
> >                      _raw_spin_lock_irqsave+0x52/0xa0
> >                      ata_bmdma_interrupt+0x27/0x200
> >                      __handle_irq_event_percpu+0xd5/0x2b0
> >                      handle_irq_event+0x57/0xb0
> >                      handle_edge_irq+0x8c/0x230
> >                      asm_call_irq_on_stack+0xf/0x20
> >                      common_interrupt+0x100/0x1c0
> >                      asm_common_interrupt+0x1e/0x40
> >                      native_safe_halt+0xe/0x10
> >                      arch_cpu_idle+0x15/0x20
> >                      default_idle_call+0x59/0x1c0
> >                      do_idle+0x22c/0x2c0
> >                      cpu_startup_entry+0x20/0x30
> >                      start_secondary+0x11d/0x150
> >                      secondary_startup_64_no_verify+0xa6/0xab
> >     INITIAL USE at:
> >                     lock_acquire+0x15f/0x420
> >                     _raw_spin_lock_irqsave+0x52/0xa0
> >                     ata_dev_init+0x54/0xe0
> >                     ata_link_init+0x8b/0xd0
> >                     ata_port_alloc+0x1f1/0x210
> >                     ata_host_alloc+0xf1/0x130
> >                     ata_host_alloc_pinfo+0x14/0xb0
> >                     ata_pci_sff_prepare_host+0x41/0xa0
> >                     ata_pci_bmdma_prepare_host+0x14/0x30
> >                     piix_init_one+0x21f/0x600
> >                     local_pci_probe+0x48/0x80
> >                     pci_device_probe+0x105/0x1c0
> >                     really_probe+0x221/0x490
> >                     driver_probe_device+0xe9/0x160
> >                     device_driver_attach+0xb2/0xc0
> >                     __driver_attach+0x91/0x150
> >                     bus_for_each_dev+0x81/0xc0
> >                     driver_attach+0x1e/0x20
> >                     bus_add_driver+0x138/0x1f0
> >                     driver_register+0x91/0xf0
> >                     __pci_register_driver+0x73/0x80
> >                     piix_init+0x1e/0x2e
> >                     do_one_initcall+0x5f/0x2d0
> >                     kernel_init_freeable+0x26f/0x2cf
> >                     kernel_init+0xe/0x113
> >                     ret_from_fork+0x1f/0x30
> >   }
> >   ... key      at: [<ffffffff83d9fdc0>] __key.6+0x0/0x10
> >   ... acquired at:
> >     __lock_acquire+0x9da/0x2370
> >     lock_acquire+0x15f/0x420
> >     _raw_spin_lock_irqsave+0x52/0xa0
> >     ata_bmdma_interrupt+0x27/0x200
> >     __handle_irq_event_percpu+0xd5/0x2b0
> >     handle_irq_event+0x57/0xb0
> >     handle_edge_irq+0x8c/0x230
> >     asm_call_irq_on_stack+0xf/0x20
> >     common_interrupt+0x100/0x1c0
> >     asm_common_interrupt+0x1e/0x40
> >     native_safe_halt+0xe/0x10
> >     arch_cpu_idle+0x15/0x20
> >     default_idle_call+0x59/0x1c0
> >     do_idle+0x22c/0x2c0
> >     cpu_startup_entry+0x20/0x30
> >     start_secondary+0x11d/0x150
> >     secondary_startup_64_no_verify+0xa6/0xab
> > 
> > This lockdep splat is reported after:
> > commit e918188611f0 ("locking: More accurate annotations for read_lock()")
> > 
> > To clarify:
> >  - read-locks are recursive only in interrupt context (when
> >    in_interrupt() returns true)
> >  - after acquiring host->lock in CPU1, another cpu (i.e. CPU2) may call
> >    write_lock(&trig->leddev_list_lock) that would be blocked by CPU0
> >    that holds trig->leddev_list_lock in read-mode
> >  - when CPU1 (ata_ac_complete()) tries to read-lock
> >    trig->leddev_list_lock, it would be blocked by the write-lock waiter
> >    on CPU2 (because we are not in interrupt context, so the read-lock is
> >    not recursive)
> >  - at this point if an interrupt happens on CPU0 and
> >    ata_bmdma_interrupt() is executed it will try to acquire host->lock,
> >    that is held by CPU1, that is currently blocked by CPU2, so:
> > 
> >    * CPU0 blocked by CPU1
> >    * CPU1 blocked by CPU2
> >    * CPU2 blocked by CPU0
> > 
> >      *** DEADLOCK ***
> > 
> > The deadlock scenario is better represented by the following schema
> > (thanks to Boqun Feng <boqun.feng@gmail.com> for the schema and the
> > detailed explanation of the deadlock condition):
> > 
> >  CPU 0:                          CPU 1:                        CPU 2:
> >  -----                           -----                         -----
> >  led_trigger_event():
> >    read_lock(&trig->leddev_list_lock);
> >  				<workqueue>
> >  				ata_hsm_qc_complete():
> >  				  spin_lock_irqsave(&host->lock);
> >  								write_lock(&trig->leddev_list_lock);
> >  				  ata_port_freeze():
> >  				    ata_do_link_abort():
> >  				      ata_qc_complete():
> >  					ledtrig_disk_activity():
> >  					  led_trigger_blink_oneshot():
> >  					    read_lock(&trig->leddev_list_lock);
> >  					    // ^ not in in_interrupt() context, so could get blocked by CPU 2
> >  <interrupt>
> >    ata_bmdma_interrupt():
> >      spin_lock_irqsave(&host->lock);
> > 
> > Fix by using read_lock_irqsave/irqrestore() in led_trigger_event(), so
> > that no interrupt can happen in between, preventing the deadlock
> > condition.
> > 
> > Link: https://lore.kernel.org/lkml/20201101092614.GB3989@xps-13-7390/
> > Fixes: eb25cb9956cc ("leds: convert IDE trigger to common disk trigger")
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> 
> I'd hate to see this in stable 3 days after Linus merges it...
> 
> Do these need _irqsave, too?
> 
> drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
> drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
> drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
> drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
> 

I think so, if you mean the read_{un,}lock in led_trigger_blink_setup()
also need to convert to irq-safe version ;-)

Regards,
Boqun

> Best regards,
> 								Pavel
> 
> > ---
> >  drivers/leds/led-triggers.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index 91da90cfb11d..16d1a93a10a8 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> >  			enum led_brightness brightness)
> >  {
> >  	struct led_classdev *led_cdev;
> > +	unsigned long flags;
> >  
> >  	if (!trig)
> >  		return;
> >  
> > -	read_lock(&trig->leddev_list_lock);
> > +	read_lock_irqsave(&trig->leddev_list_lock, flags);
> >  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> >  		led_set_brightness(led_cdev, brightness);
> > -	read_unlock(&trig->leddev_list_lock);
> > +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> >  }
> >  EXPORT_SYMBOL_GPL(led_trigger_event);
> >  
> 
> -- 
> http://www.livejournal.com/~pavelmachek



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2020-11-25 12:46 ` Pavel Machek
  2020-11-25 14:01   ` Boqun Feng
@ 2020-11-25 14:15   ` Andrea Righi
  2020-11-25 15:20     ` Andrea Righi
  1 sibling, 1 reply; 13+ messages in thread
From: Andrea Righi @ 2020-11-25 14:15 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Boqun Feng, Dan Murphy, linux-leds, linux-kernel

Hi Pavel,

On Wed, Nov 25, 2020 at 01:46:48PM +0100, Pavel Machek wrote:
> Hi!
> 
> > We have the following potential deadlock condition:
> > 
> >  ========================================================
> >  WARNING: possible irq lock inversion dependency detected
> >  5.10.0-rc2+ #25 Not tainted
> >  --------------------------------------------------------
> >  swapper/3/0 just changed the state of lock:
> >  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
> >  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
> >   (&trig->leddev_list_lock){.+.?}-{2:2}
> > 
> >  and interrupts could create inverse lock ordering between them.
> > 
> >  other info that might help us debug this:
> >   Possible interrupt unsafe locking scenario:
> > 
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(&trig->leddev_list_lock);
> >                                 local_irq_disable();
> >                                 lock(&host->lock);
> >                                 lock(&trig->leddev_list_lock);
> >    <Interrupt>
> >      lock(&host->lock);
> > 
> >   *** DEADLOCK ***
> > 
> >  no locks held by swapper/3/0.
> > 
> >  the shortest dependencies between 2nd lock and 1st lock:
> >   -> (&trig->leddev_list_lock){.+.?}-{2:2} ops: 46 {
> >      HARDIRQ-ON-R at:
> >                        lock_acquire+0x15f/0x420
> >                        _raw_read_lock+0x42/0x90
> >                        led_trigger_event+0x2b/0x70
> >                        rfkill_global_led_trigger_worker+0x94/0xb0
> >                        process_one_work+0x240/0x560
> >                        worker_thread+0x58/0x3d0
> >                        kthread+0x151/0x170
> >                        ret_from_fork+0x1f/0x30
> >      IN-SOFTIRQ-R at:
> >                        lock_acquire+0x15f/0x420
> >                        _raw_read_lock+0x42/0x90
> >                        led_trigger_event+0x2b/0x70
> >                        kbd_bh+0x9e/0xc0
> >                        tasklet_action_common.constprop.0+0xe9/0x100
> >                        tasklet_action+0x22/0x30
> >                        __do_softirq+0xcc/0x46d
> >                        run_ksoftirqd+0x3f/0x70
> >                        smpboot_thread_fn+0x116/0x1f0
> >                        kthread+0x151/0x170
> >                        ret_from_fork+0x1f/0x30
> >      SOFTIRQ-ON-R at:
> >                        lock_acquire+0x15f/0x420
> >                        _raw_read_lock+0x42/0x90
> >                        led_trigger_event+0x2b/0x70
> >                        rfkill_global_led_trigger_worker+0x94/0xb0
> >                        process_one_work+0x240/0x560
> >                        worker_thread+0x58/0x3d0
> >                        kthread+0x151/0x170
> >                        ret_from_fork+0x1f/0x30
> >      INITIAL READ USE at:
> >                            lock_acquire+0x15f/0x420
> >                            _raw_read_lock+0x42/0x90
> >                            led_trigger_event+0x2b/0x70
> >                            rfkill_global_led_trigger_worker+0x94/0xb0
> >                            process_one_work+0x240/0x560
> >                            worker_thread+0x58/0x3d0
> >                            kthread+0x151/0x170
> >                            ret_from_fork+0x1f/0x30
> >    }
> >    ... key      at: [<ffffffff83da4c00>] __key.0+0x0/0x10
> >    ... acquired at:
> >     _raw_read_lock+0x42/0x90
> >     led_trigger_blink_oneshot+0x3b/0x90
> >     ledtrig_disk_activity+0x3c/0xa0
> >     ata_qc_complete+0x26/0x450
> >     ata_do_link_abort+0xa3/0xe0
> >     ata_port_freeze+0x2e/0x40
> >     ata_hsm_qc_complete+0x94/0xa0
> >     ata_sff_hsm_move+0x177/0x7a0
> >     ata_sff_pio_task+0xc7/0x1b0
> >     process_one_work+0x240/0x560
> >     worker_thread+0x58/0x3d0
> >     kthread+0x151/0x170
> >     ret_from_fork+0x1f/0x30
> > 
> >  -> (&host->lock){-...}-{2:2} ops: 69 {
> >     IN-HARDIRQ-W at:
> >                      lock_acquire+0x15f/0x420
> >                      _raw_spin_lock_irqsave+0x52/0xa0
> >                      ata_bmdma_interrupt+0x27/0x200
> >                      __handle_irq_event_percpu+0xd5/0x2b0
> >                      handle_irq_event+0x57/0xb0
> >                      handle_edge_irq+0x8c/0x230
> >                      asm_call_irq_on_stack+0xf/0x20
> >                      common_interrupt+0x100/0x1c0
> >                      asm_common_interrupt+0x1e/0x40
> >                      native_safe_halt+0xe/0x10
> >                      arch_cpu_idle+0x15/0x20
> >                      default_idle_call+0x59/0x1c0
> >                      do_idle+0x22c/0x2c0
> >                      cpu_startup_entry+0x20/0x30
> >                      start_secondary+0x11d/0x150
> >                      secondary_startup_64_no_verify+0xa6/0xab
> >     INITIAL USE at:
> >                     lock_acquire+0x15f/0x420
> >                     _raw_spin_lock_irqsave+0x52/0xa0
> >                     ata_dev_init+0x54/0xe0
> >                     ata_link_init+0x8b/0xd0
> >                     ata_port_alloc+0x1f1/0x210
> >                     ata_host_alloc+0xf1/0x130
> >                     ata_host_alloc_pinfo+0x14/0xb0
> >                     ata_pci_sff_prepare_host+0x41/0xa0
> >                     ata_pci_bmdma_prepare_host+0x14/0x30
> >                     piix_init_one+0x21f/0x600
> >                     local_pci_probe+0x48/0x80
> >                     pci_device_probe+0x105/0x1c0
> >                     really_probe+0x221/0x490
> >                     driver_probe_device+0xe9/0x160
> >                     device_driver_attach+0xb2/0xc0
> >                     __driver_attach+0x91/0x150
> >                     bus_for_each_dev+0x81/0xc0
> >                     driver_attach+0x1e/0x20
> >                     bus_add_driver+0x138/0x1f0
> >                     driver_register+0x91/0xf0
> >                     __pci_register_driver+0x73/0x80
> >                     piix_init+0x1e/0x2e
> >                     do_one_initcall+0x5f/0x2d0
> >                     kernel_init_freeable+0x26f/0x2cf
> >                     kernel_init+0xe/0x113
> >                     ret_from_fork+0x1f/0x30
> >   }
> >   ... key      at: [<ffffffff83d9fdc0>] __key.6+0x0/0x10
> >   ... acquired at:
> >     __lock_acquire+0x9da/0x2370
> >     lock_acquire+0x15f/0x420
> >     _raw_spin_lock_irqsave+0x52/0xa0
> >     ata_bmdma_interrupt+0x27/0x200
> >     __handle_irq_event_percpu+0xd5/0x2b0
> >     handle_irq_event+0x57/0xb0
> >     handle_edge_irq+0x8c/0x230
> >     asm_call_irq_on_stack+0xf/0x20
> >     common_interrupt+0x100/0x1c0
> >     asm_common_interrupt+0x1e/0x40
> >     native_safe_halt+0xe/0x10
> >     arch_cpu_idle+0x15/0x20
> >     default_idle_call+0x59/0x1c0
> >     do_idle+0x22c/0x2c0
> >     cpu_startup_entry+0x20/0x30
> >     start_secondary+0x11d/0x150
> >     secondary_startup_64_no_verify+0xa6/0xab
> > 
> > This lockdep splat is reported after:
> > commit e918188611f0 ("locking: More accurate annotations for read_lock()")
> > 
> > To clarify:
> >  - read-locks are recursive only in interrupt context (when
> >    in_interrupt() returns true)
> >  - after acquiring host->lock in CPU1, another cpu (i.e. CPU2) may call
> >    write_lock(&trig->leddev_list_lock) that would be blocked by CPU0
> >    that holds trig->leddev_list_lock in read-mode
> >  - when CPU1 (ata_ac_complete()) tries to read-lock
> >    trig->leddev_list_lock, it would be blocked by the write-lock waiter
> >    on CPU2 (because we are not in interrupt context, so the read-lock is
> >    not recursive)
> >  - at this point if an interrupt happens on CPU0 and
> >    ata_bmdma_interrupt() is executed it will try to acquire host->lock,
> >    that is held by CPU1, that is currently blocked by CPU2, so:
> > 
> >    * CPU0 blocked by CPU1
> >    * CPU1 blocked by CPU2
> >    * CPU2 blocked by CPU0
> > 
> >      *** DEADLOCK ***
> > 
> > The deadlock scenario is better represented by the following schema
> > (thanks to Boqun Feng <boqun.feng@gmail.com> for the schema and the
> > detailed explanation of the deadlock condition):
> > 
> >  CPU 0:                          CPU 1:                        CPU 2:
> >  -----                           -----                         -----
> >  led_trigger_event():
> >    read_lock(&trig->leddev_list_lock);
> >  				<workqueue>
> >  				ata_hsm_qc_complete():
> >  				  spin_lock_irqsave(&host->lock);
> >  								write_lock(&trig->leddev_list_lock);
> >  				  ata_port_freeze():
> >  				    ata_do_link_abort():
> >  				      ata_qc_complete():
> >  					ledtrig_disk_activity():
> >  					  led_trigger_blink_oneshot():
> >  					    read_lock(&trig->leddev_list_lock);
> >  					    // ^ not in in_interrupt() context, so could get blocked by CPU 2
> >  <interrupt>
> >    ata_bmdma_interrupt():
> >      spin_lock_irqsave(&host->lock);
> > 
> > Fix by using read_lock_irqsave/irqrestore() in led_trigger_event(), so
> > that no interrupt can happen in between, preventing the deadlock
> > condition.
> > 
> > Link: https://lore.kernel.org/lkml/20201101092614.GB3989@xps-13-7390/
> > Fixes: eb25cb9956cc ("leds: convert IDE trigger to common disk trigger")
> > Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
> 
> I'd hate to see this in stable 3 days after Linus merges it...
> 
> Do these need _irqsave, too?
> 
> drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
> drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
> drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
> drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
> 
> Best regards,

I think also led_trigger_blink_setup() needs to use irqsave/irqrestore,
in fact:

$ git grep "led_trigger_blink("
drivers/leds/led-triggers.c:void led_trigger_blink(struct led_trigger *trig,
drivers/power/supply/power_supply_leds.c:               led_trigger_blink(psy->charging_blink_full_solid_trig,
include/linux/leds.h:void led_trigger_blink(struct led_trigger *trigger, unsigned long *delay_on,
include/linux/leds.h:static inline void led_trigger_blink(struct led_trigger *trigger,

power_supply_leds.c is using led_trigger_blink() from a workqueue
context, so potentially the same deadlock condition can also happen.

Let me know if you want me to send a new patch to include also this
case.

-Andrea

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2020-11-25 14:15   ` Andrea Righi
@ 2020-11-25 15:20     ` Andrea Righi
  0 siblings, 0 replies; 13+ messages in thread
From: Andrea Righi @ 2020-11-25 15:20 UTC (permalink / raw)
  To: Pavel Machek; +Cc: Boqun Feng, Dan Murphy, linux-leds, linux-kernel

On Wed, Nov 25, 2020 at 03:15:18PM +0100, Andrea Righi wrote:
...
> > I'd hate to see this in stable 3 days after Linus merges it...
> > 
> > Do these need _irqsave, too?
> > 
> > drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
> > drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
> > drivers/leds/led-triggers.c:   read_lock(&trig->leddev_list_lock);
> > drivers/leds/led-triggers.c:   read_unlock(&trig->leddev_list_lock);
> > 
> > Best regards,
> 
> I think also led_trigger_blink_setup() needs to use irqsave/irqrestore,
> in fact:
> 
> $ git grep "led_trigger_blink("
> drivers/leds/led-triggers.c:void led_trigger_blink(struct led_trigger *trig,
> drivers/power/supply/power_supply_leds.c:               led_trigger_blink(psy->charging_blink_full_solid_trig,
> include/linux/leds.h:void led_trigger_blink(struct led_trigger *trigger, unsigned long *delay_on,
> include/linux/leds.h:static inline void led_trigger_blink(struct led_trigger *trigger,
> 
> power_supply_leds.c is using led_trigger_blink() from a workqueue
> context, so potentially the same deadlock condition can also happen.
> 
> Let me know if you want me to send a new patch to include also this
> case.

Just sent (and tested) a v2 of this patch that changes also
led_trigger_blink_setup().

-Andrea

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2020-11-02 10:41 [PATCH] leds: trigger: fix potential deadlock with libata Andrea Righi
  2020-11-25 12:46 ` Pavel Machek
@ 2021-03-06 20:39 ` Marc Kleine-Budde
  2021-03-07  2:02   ` Boqun Feng
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Marc Kleine-Budde @ 2021-03-06 20:39 UTC (permalink / raw)
  To: Andrea Righi
  Cc: Pavel Machek, Boqun Feng, Dan Murphy, linux-leds, linux-kernel,
	kernel, schuchmann

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

Hello *,

On 02.11.2020 11:41:52, Andrea Righi wrote:
> We have the following potential deadlock condition:
> 
>  ========================================================
>  WARNING: possible irq lock inversion dependency detected
>  5.10.0-rc2+ #25 Not tainted
>  --------------------------------------------------------
>  swapper/3/0 just changed the state of lock:
>  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
>  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
>   (&trig->leddev_list_lock){.+.?}-{2:2}
> 
>  and interrupts could create inverse lock ordering between them.

[...]

> ---
>  drivers/leds/led-triggers.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 91da90cfb11d..16d1a93a10a8 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
>  			enum led_brightness brightness)
>  {
>  	struct led_classdev *led_cdev;
> +	unsigned long flags;
>  
>  	if (!trig)
>  		return;
>  
> -	read_lock(&trig->leddev_list_lock);
> +	read_lock_irqsave(&trig->leddev_list_lock, flags);
>  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
>  		led_set_brightness(led_cdev, brightness);
> -	read_unlock(&trig->leddev_list_lock);
> +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_event);

meanwhile this patch hit v5.10.x stable and caused a performance
degradation on our use case:

It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
controller. CAN stands for Controller Area Network and here used to
connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
Transport Protocol) transfer is running. With this patch, we see CAN
frames delayed for ~6ms, the usual gap between CAN frames is 240µs.

Reverting this patch, restores the old performance.

What is the best way to solve this dilemma? Identify the critical path
in our use case? Is there a way we can get around the irqsave in
led_trigger_event()?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-06 20:39 ` Marc Kleine-Budde
@ 2021-03-07  2:02   ` Boqun Feng
  2021-03-07  7:14     ` Andrea Righi
  2021-03-08 14:56     ` Marc Kleine-Budde
  2021-03-07 16:13   ` Pavel Machek
  2021-03-07 16:26   ` Pavel Machek
  2 siblings, 2 replies; 13+ messages in thread
From: Boqun Feng @ 2021-03-07  2:02 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Andrea Righi, Pavel Machek, Dan Murphy, linux-leds, linux-kernel,
	kernel, schuchmann

On Sat, Mar 06, 2021 at 09:39:54PM +0100, Marc Kleine-Budde wrote:
> Hello *,
> 
> On 02.11.2020 11:41:52, Andrea Righi wrote:
> > We have the following potential deadlock condition:
> > 
> >  ========================================================
> >  WARNING: possible irq lock inversion dependency detected
> >  5.10.0-rc2+ #25 Not tainted
> >  --------------------------------------------------------
> >  swapper/3/0 just changed the state of lock:
> >  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
> >  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
> >   (&trig->leddev_list_lock){.+.?}-{2:2}
> > 
> >  and interrupts could create inverse lock ordering between them.
> 
> [...]
> 
> > ---
> >  drivers/leds/led-triggers.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index 91da90cfb11d..16d1a93a10a8 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> >  			enum led_brightness brightness)
> >  {
> >  	struct led_classdev *led_cdev;
> > +	unsigned long flags;
> >  
> >  	if (!trig)
> >  		return;
> >  
> > -	read_lock(&trig->leddev_list_lock);
> > +	read_lock_irqsave(&trig->leddev_list_lock, flags);
> >  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> >  		led_set_brightness(led_cdev, brightness);
> > -	read_unlock(&trig->leddev_list_lock);
> > +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> >  }
> >  EXPORT_SYMBOL_GPL(led_trigger_event);
> 
> meanwhile this patch hit v5.10.x stable and caused a performance
> degradation on our use case:
> 
> It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> controller. CAN stands for Controller Area Network and here used to
> connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> Transport Protocol) transfer is running. With this patch, we see CAN
> frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
> 
> Reverting this patch, restores the old performance.
> 
> What is the best way to solve this dilemma? Identify the critical path
> in our use case? Is there a way we can get around the irqsave in
> led_trigger_event()?
> 

Probably, we can change from rwlock to rcu here, POC code as follow,
only compile tested. Marc, could you see whether this help the
performance on your platform? Please note that I haven't test it in a
running kernel and I'm not that familir with led subsystem, so use it
with caution ;-)

(While at it, I think maybe we miss the leddev_list_lock in net/mac80211
in the patch)

Regards,
Boqun
------->8
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 4e7b78a84149..ae68ccab6cc9 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -171,10 +171,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 
 	/* Remove any existing trigger */
 	if (led_cdev->trigger) {
-		write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
-		list_del(&led_cdev->trig_list);
-		write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock,
+		spin_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
+		list_del_rcu(&led_cdev->trig_list);
+		spin_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock,
 			flags);
+		/* Wait for the readers gone */
+		synchronize_rcu();
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
 		if (led_cdev->trigger->deactivate)
@@ -186,9 +188,9 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		led_set_brightness(led_cdev, LED_OFF);
 	}
 	if (trig) {
-		write_lock_irqsave(&trig->leddev_list_lock, flags);
-		list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
-		write_unlock_irqrestore(&trig->leddev_list_lock, flags);
+		spin_lock_irqsave(&trig->leddev_list_lock, flags);
+		list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
+		spin_unlock_irqrestore(&trig->leddev_list_lock, flags);
 		led_cdev->trigger = trig;
 
 		if (trig->activate)
@@ -223,9 +225,12 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		trig->deactivate(led_cdev);
 err_activate:
 
-	write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
-	list_del(&led_cdev->trig_list);
-	write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+	spin_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
+	list_del_rcu(&led_cdev->trig_list);
+	spin_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
+
+	/* XXX could use call_rcu() here? */
+	synchronize_rcu();
 	led_cdev->trigger = NULL;
 	led_cdev->trigger_data = NULL;
 	led_set_brightness(led_cdev, LED_OFF);
@@ -285,7 +290,7 @@ int led_trigger_register(struct led_trigger *trig)
 	struct led_classdev *led_cdev;
 	struct led_trigger *_trig;
 
-	rwlock_init(&trig->leddev_list_lock);
+	spin_lock_init(&trig->leddev_list_lock);
 	INIT_LIST_HEAD(&trig->led_cdevs);
 
 	down_write(&triggers_list_lock);
@@ -378,15 +383,14 @@ void led_trigger_event(struct led_trigger *trig,
 			enum led_brightness brightness)
 {
 	struct led_classdev *led_cdev;
-	unsigned long flags;
 
 	if (!trig)
 		return;
 
-	read_lock_irqsave(&trig->leddev_list_lock, flags);
-	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
+	rcu_read_lock();
+	list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list)
 		led_set_brightness(led_cdev, brightness);
-	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(led_trigger_event);
 
@@ -397,20 +401,19 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
 			     int invert)
 {
 	struct led_classdev *led_cdev;
-	unsigned long flags;
 
 	if (!trig)
 		return;
 
-	read_lock_irqsave(&trig->leddev_list_lock, flags);
-	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) {
 		if (oneshot)
 			led_blink_set_oneshot(led_cdev, delay_on, delay_off,
 					      invert);
 		else
 			led_blink_set(led_cdev, delay_on, delay_off);
 	}
-	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
+	rcu_read_unlock();
 }
 
 void led_trigger_blink(struct led_trigger *trig,
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6a8d6409c993..5acc0e8a9f18 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -356,7 +356,7 @@ struct led_trigger {
 	struct led_hw_trigger_type *trigger_type;
 
 	/* LEDs under control by this trigger (for simple triggers) */
-	rwlock_t	  leddev_list_lock;
+	spinlock_t	  leddev_list_lock;
 	struct list_head  led_cdevs;
 
 	/* Link to next registered trigger */
diff --git a/net/mac80211/led.c b/net/mac80211/led.c
index b275c8853074..5ec5070fe210 100644
--- a/net/mac80211/led.c
+++ b/net/mac80211/led.c
@@ -283,10 +283,10 @@ static void tpt_trig_timer(struct timer_list *t)
 		}
 	}
 
-	read_lock(&local->tpt_led.leddev_list_lock);
+	rcu_read_lock();
 	list_for_each_entry(led_cdev, &local->tpt_led.led_cdevs, trig_list)
 		led_blink_set(led_cdev, &on, &off);
-	read_unlock(&local->tpt_led.leddev_list_lock);
+	rcu_read_unlock();
 }
 
 const char *
@@ -349,10 +349,10 @@ static void ieee80211_stop_tpt_led_trig(struct ieee80211_local *local)
 	tpt_trig->running = false;
 	del_timer_sync(&tpt_trig->timer);
 
-	read_lock(&local->tpt_led.leddev_list_lock);
+	rcu_read_lock();
 	list_for_each_entry(led_cdev, &local->tpt_led.led_cdevs, trig_list)
 		led_set_brightness(led_cdev, LED_OFF);
-	read_unlock(&local->tpt_led.leddev_list_lock);
+	rcu_read_unlock();
 }
 
 void ieee80211_mod_tpt_led_trig(struct ieee80211_local *local,

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-07  2:02   ` Boqun Feng
@ 2021-03-07  7:14     ` Andrea Righi
  2021-03-08 14:56     ` Marc Kleine-Budde
  1 sibling, 0 replies; 13+ messages in thread
From: Andrea Righi @ 2021-03-07  7:14 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Marc Kleine-Budde, Pavel Machek, Dan Murphy, linux-leds,
	linux-kernel, kernel, schuchmann

On Sun, Mar 07, 2021 at 10:02:32AM +0800, Boqun Feng wrote:
> On Sat, Mar 06, 2021 at 09:39:54PM +0100, Marc Kleine-Budde wrote:
> > Hello *,
> > 
> > On 02.11.2020 11:41:52, Andrea Righi wrote:
> > > We have the following potential deadlock condition:
> > > 
> > >  ========================================================
> > >  WARNING: possible irq lock inversion dependency detected
> > >  5.10.0-rc2+ #25 Not tainted
> > >  --------------------------------------------------------
> > >  swapper/3/0 just changed the state of lock:
> > >  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
> > >  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
> > >   (&trig->leddev_list_lock){.+.?}-{2:2}
> > > 
> > >  and interrupts could create inverse lock ordering between them.
> > 
> > [...]
> > 
> > > ---
> > >  drivers/leds/led-triggers.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > > index 91da90cfb11d..16d1a93a10a8 100644
> > > --- a/drivers/leds/led-triggers.c
> > > +++ b/drivers/leds/led-triggers.c
> > > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> > >  			enum led_brightness brightness)
> > >  {
> > >  	struct led_classdev *led_cdev;
> > > +	unsigned long flags;
> > >  
> > >  	if (!trig)
> > >  		return;
> > >  
> > > -	read_lock(&trig->leddev_list_lock);
> > > +	read_lock_irqsave(&trig->leddev_list_lock, flags);
> > >  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> > >  		led_set_brightness(led_cdev, brightness);
> > > -	read_unlock(&trig->leddev_list_lock);
> > > +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> > >  }
> > >  EXPORT_SYMBOL_GPL(led_trigger_event);
> > 
> > meanwhile this patch hit v5.10.x stable and caused a performance
> > degradation on our use case:
> > 
> > It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> > controller. CAN stands for Controller Area Network and here used to
> > connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> > Transport Protocol) transfer is running. With this patch, we see CAN
> > frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
> > 
> > Reverting this patch, restores the old performance.
> > 
> > What is the best way to solve this dilemma? Identify the critical path
> > in our use case? Is there a way we can get around the irqsave in
> > led_trigger_event()?
> > 
> 
> Probably, we can change from rwlock to rcu here, POC code as follow,
> only compile tested. Marc, could you see whether this help the
> performance on your platform? Please note that I haven't test it in a
> running kernel and I'm not that familir with led subsystem, so use it
> with caution ;-)

If we don't want to touch the led subsystem at all maybe we could try to
fix the problem in libata, we just need to prevent calling
led_trigger_blink_oneshot() with &host->lock held from
ata_qc_complete(), maybe doing the led blinking from another context (a
workqueue for example)?

-Andrea

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-06 20:39 ` Marc Kleine-Budde
  2021-03-07  2:02   ` Boqun Feng
@ 2021-03-07 16:13   ` Pavel Machek
  2021-03-07 19:04     ` Hans de Goede
  2021-03-07 16:26   ` Pavel Machek
  2 siblings, 1 reply; 13+ messages in thread
From: Pavel Machek @ 2021-03-07 16:13 UTC (permalink / raw)
  To: Marc Kleine-Budde, hdegoede
  Cc: Andrea Righi, Boqun Feng, Dan Murphy, linux-leds, linux-kernel,
	kernel, schuchmann

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

Hi!

> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> >  			enum led_brightness brightness)
> >  {
> >  	struct led_classdev *led_cdev;
> > +	unsigned long flags;
> >  
> >  	if (!trig)
> >  		return;
> >  
> > -	read_lock(&trig->leddev_list_lock);
> > +	read_lock_irqsave(&trig->leddev_list_lock, flags);
> >  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> >  		led_set_brightness(led_cdev, brightness);
> > -	read_unlock(&trig->leddev_list_lock);
> > +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> >  }
> >  EXPORT_SYMBOL_GPL(led_trigger_event);
> 
> meanwhile this patch hit v5.10.x stable and caused a performance
> degradation on our use case:
> 
> It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> controller. CAN stands for Controller Area Network and here used to
> connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> Transport Protocol) transfer is running. With this patch, we see CAN
> frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
> 
> Reverting this patch, restores the old performance.
> 
> What is the best way to solve this dilemma? Identify the critical path
> in our use case? Is there a way we can get around the irqsave in
> led_trigger_event()?

Hans was pushing for this patch, perhaps he has some ideas...
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-06 20:39 ` Marc Kleine-Budde
  2021-03-07  2:02   ` Boqun Feng
  2021-03-07 16:13   ` Pavel Machek
@ 2021-03-07 16:26   ` Pavel Machek
  2 siblings, 0 replies; 13+ messages in thread
From: Pavel Machek @ 2021-03-07 16:26 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Andrea Righi, Boqun Feng, Dan Murphy, linux-leds, linux-kernel,
	kernel, schuchmann

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

Hi!

> > ---
> >  drivers/leds/led-triggers.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index 91da90cfb11d..16d1a93a10a8 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> >  			enum led_brightness brightness)
> >  {
> >  	struct led_classdev *led_cdev;
> > +	unsigned long flags;
> >  
> >  	if (!trig)
> >  		return;
> >  
> > -	read_lock(&trig->leddev_list_lock);
> > +	read_lock_irqsave(&trig->leddev_list_lock, flags);
> >  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> >  		led_set_brightness(led_cdev, brightness);
> > -	read_unlock(&trig->leddev_list_lock);
> > +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> >  }
> >  EXPORT_SYMBOL_GPL(led_trigger_event);
> 
> meanwhile this patch hit v5.10.x stable and caused a performance
> degradation on our use case:
> 
> It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> controller. CAN stands for Controller Area Network and here used to
> connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> Transport Protocol) transfer is running. With this patch, we see CAN
> frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
> 
> Reverting this patch, restores the old performance.
> 
> What is the best way to solve this dilemma? Identify the critical path
> in our use case? Is there a way we can get around the irqsave in
> led_trigger_event()?

6ms is quite long. Are you actively using any triggers? Do you have
LED blinking on CAN access?

Can you verify if it is cli/sti taking too long, or if the
led_set_brightness takes too long?

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-07 16:13   ` Pavel Machek
@ 2021-03-07 19:04     ` Hans de Goede
  0 siblings, 0 replies; 13+ messages in thread
From: Hans de Goede @ 2021-03-07 19:04 UTC (permalink / raw)
  To: Pavel Machek, Marc Kleine-Budde
  Cc: Andrea Righi, Boqun Feng, Dan Murphy, linux-leds, linux-kernel,
	kernel, schuchmann

Hi,

On 3/7/21 5:13 PM, Pavel Machek wrote:
> Hi!
> 
>>> --- a/drivers/leds/led-triggers.c
>>> +++ b/drivers/leds/led-triggers.c
>>> @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
>>>  			enum led_brightness brightness)
>>>  {
>>>  	struct led_classdev *led_cdev;
>>> +	unsigned long flags;
>>>  
>>>  	if (!trig)
>>>  		return;
>>>  
>>> -	read_lock(&trig->leddev_list_lock);
>>> +	read_lock_irqsave(&trig->leddev_list_lock, flags);
>>>  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
>>>  		led_set_brightness(led_cdev, brightness);
>>> -	read_unlock(&trig->leddev_list_lock);
>>> +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
>>>  }
>>>  EXPORT_SYMBOL_GPL(led_trigger_event)
>>
>> meanwhile this patch hit v5.10.x stable and caused a performance
>> degradation on our use case:
>>
>> It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
>> controller. CAN stands for Controller Area Network and here used to
>> connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
>> Transport Protocol) transfer is running. With this patch, we see CAN
>> frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
>>
>> Reverting this patch, restores the old performance.
>>
>> What is the best way to solve this dilemma? Identify the critical path
>> in our use case? Is there a way we can get around the irqsave in
>> led_trigger_event()?
> 
> Hans was pushing for this patch, perhaps he has some ideas...

I was not pushing for this particular fix, I was asking about a fix
for the lockdep identified potential deadlock.

And you replied that this was already fixed in your for-next branch
when I asked, so all in all, other then reporting the potential deadlock
(after it was already fixed) I have very little do to with this patch.

With that all said, I must say that I'm surprised that switching from
read_lock() to read_lock_irqsave() causes such a hefty penalty, so I
wonder what is really going on here. Using the irqsave version disables
interrupts, but AFAIK only on the current core and only for the duration
of the led_set_brightness() call(s) . 

Is the system perhaps pinning IRQs to a specific CPU in combination with
a led_set_brightness() somehow taking much longer then it should?

Note that led_set_brightness() calls are not allowed to block, if they
block they should use the brightness_set_blocking callback in their
led_class_dev struct not the regular brightness_set callback. In which case
the LED-core will defer the actually setting of the LED to a workqueue.

So one thing which might be worthwhile to check is if any of the LED
drivers on the system in question are using the brightness_set callback,
where they should be using the blocking one.

Regards,

Hans


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-07  2:02   ` Boqun Feng
  2021-03-07  7:14     ` Andrea Righi
@ 2021-03-08 14:56     ` Marc Kleine-Budde
  2021-03-08 15:05       ` Marc Kleine-Budde
  1 sibling, 1 reply; 13+ messages in thread
From: Marc Kleine-Budde @ 2021-03-08 14:56 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Andrea Righi, Pavel Machek, Dan Murphy, linux-leds, linux-kernel,
	kernel, schuchmann

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

On 07.03.2021 10:02:32, Boqun Feng wrote:
> On Sat, Mar 06, 2021 at 09:39:54PM +0100, Marc Kleine-Budde wrote:
> > Hello *,
> > 
> > On 02.11.2020 11:41:52, Andrea Righi wrote:
> > > We have the following potential deadlock condition:
> > > 
> > >  ========================================================
> > >  WARNING: possible irq lock inversion dependency detected
> > >  5.10.0-rc2+ #25 Not tainted
> > >  --------------------------------------------------------
> > >  swapper/3/0 just changed the state of lock:
> > >  ffff8880063bd618 (&host->lock){-...}-{2:2}, at: ata_bmdma_interrupt+0x27/0x200
> > >  but this lock took another, HARDIRQ-READ-unsafe lock in the past:
> > >   (&trig->leddev_list_lock){.+.?}-{2:2}
> > > 
> > >  and interrupts could create inverse lock ordering between them.
> > 
> > [...]
> > 
> > > ---
> > >  drivers/leds/led-triggers.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > > index 91da90cfb11d..16d1a93a10a8 100644
> > > --- a/drivers/leds/led-triggers.c
> > > +++ b/drivers/leds/led-triggers.c
> > > @@ -378,14 +378,15 @@ void led_trigger_event(struct led_trigger *trig,
> > >  			enum led_brightness brightness)
> > >  {
> > >  	struct led_classdev *led_cdev;
> > > +	unsigned long flags;
> > >  
> > >  	if (!trig)
> > >  		return;
> > >  
> > > -	read_lock(&trig->leddev_list_lock);
> > > +	read_lock_irqsave(&trig->leddev_list_lock, flags);
> > >  	list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> > >  		led_set_brightness(led_cdev, brightness);
> > > -	read_unlock(&trig->leddev_list_lock);
> > > +	read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> > >  }
> > >  EXPORT_SYMBOL_GPL(led_trigger_event);
> > 
> > meanwhile this patch hit v5.10.x stable and caused a performance
> > degradation on our use case:
> > 
> > It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> > controller. CAN stands for Controller Area Network and here used to
> > connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> > Transport Protocol) transfer is running. With this patch, we see CAN
> > frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
> > 
> > Reverting this patch, restores the old performance.
> > 
> > What is the best way to solve this dilemma? Identify the critical path
> > in our use case? Is there a way we can get around the irqsave in
> > led_trigger_event()?
> > 
> 
> Probably, we can change from rwlock to rcu here, POC code as follow,
> only compile tested. Marc, could you see whether this help the
> performance on your platform? Please note that I haven't test it in a
> running kernel and I'm not that familir with led subsystem, so use it
> with caution ;-)
> 
> (While at it, I think maybe we miss the leddev_list_lock in net/mac80211
> in the patch)

I can confirm, this patch basically restores the old performance.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] leds: trigger: fix potential deadlock with libata
  2021-03-08 14:56     ` Marc Kleine-Budde
@ 2021-03-08 15:05       ` Marc Kleine-Budde
  0 siblings, 0 replies; 13+ messages in thread
From: Marc Kleine-Budde @ 2021-03-08 15:05 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Pavel Machek, linux-kernel, Andrea Righi, schuchmann, Dan Murphy,
	kernel, linux-leds

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

On 08.03.2021 15:56:53, Marc Kleine-Budde wrote:
> > > meanwhile this patch hit v5.10.x stable and caused a performance
> > > degradation on our use case:
> > > 
> > > It's an embedded ARM system, 4x Cortex A53, with an SPI attached CAN
> > > controller. CAN stands for Controller Area Network and here used to
> > > connect to some automotive equipment. Over CAN an ISOTP (a CAN-specific
> > > Transport Protocol) transfer is running. With this patch, we see CAN
> > > frames delayed for ~6ms, the usual gap between CAN frames is 240µs.
> > > 
> > > Reverting this patch, restores the old performance.
> > > 
> > > What is the best way to solve this dilemma? Identify the critical path
> > > in our use case? Is there a way we can get around the irqsave in
> > > led_trigger_event()?
> > > 
> > 
> > Probably, we can change from rwlock to rcu here, POC code as follow,
> > only compile tested. Marc, could you see whether this help the
> > performance on your platform? Please note that I haven't test it in a
> > running kernel and I'm not that familir with led subsystem, so use it
> > with caution ;-)
> > 
> > (While at it, I think maybe we miss the leddev_list_lock in net/mac80211
> > in the patch)
> 
> I can confirm, this patch basically restores the old performance.

Hmmm - it first looked OK, now it doesn't - let me do some better testing.

Sorry for the noise,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-03-08 15:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 10:41 [PATCH] leds: trigger: fix potential deadlock with libata Andrea Righi
2020-11-25 12:46 ` Pavel Machek
2020-11-25 14:01   ` Boqun Feng
2020-11-25 14:15   ` Andrea Righi
2020-11-25 15:20     ` Andrea Righi
2021-03-06 20:39 ` Marc Kleine-Budde
2021-03-07  2:02   ` Boqun Feng
2021-03-07  7:14     ` Andrea Righi
2021-03-08 14:56     ` Marc Kleine-Budde
2021-03-08 15:05       ` Marc Kleine-Budde
2021-03-07 16:13   ` Pavel Machek
2021-03-07 19:04     ` Hans de Goede
2021-03-07 16:26   ` Pavel Machek

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