"leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

"leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Hans de Goede]

Hi Johannes, et al., 

Sorry to bring the bearer of bad news, but your commit 2a5a8fa8b23
("leds: trigger: use RCU to protect the led_cdevs list"),
is causing the following RCU warning when used with blinking
triggers on I2C LED controllers which support hw blinking.

The specific problem is drivers/leds/led-triggers.c:
led_trigger_blink_setup() which does:

        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);
        }
        rcu_read_unlock();

And that led_blink_set() call then hits this path:

        if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
            led_cdev->blink_set &&
            !led_cdev->blink_set(led_cdev, delay_on, delay_off))
                return;

Which calls directly into the LED controller driver which
talks to the LED controller over I2C which may sleep.

AFAICT (not much RCU experience) the problem basically is that in
a rcu_read_lock() section the code may not sleep. While
led_blink_set() can sleep. For completeness sake I've added
the full backtrace at the end of this email, but it really is not
that interesting.

From the original commit I understand that RCU was chosen
because of other hairy issues. For non blinking triggers this
works fine, because led_set_brightness() is guaranteed to
not sleep using a workqueue to delay the set brightness
in case it would sleep.

We could do something similar for led_set_blink, at least
when called from led_trigger_blink[_oneshot], to avoid
this issue.

But I wonder if someone has any better ideas ?

Regards,

Hans











[  832.605062] ------------[ cut here ]------------
[  832.605085] Voluntary context switch within RCU read-side critical section!
[  832.605119] WARNING: CPU: 2 PID: 370 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x4ee/0x690
[  832.605175] Modules linked in: snd_seq_dummy snd_hrtimer joydev wacom bq25890_charger(E) bq27xxx_battery_i2c(E) i2c_hid_of(E) goodix_ts hideep lp855x_bl bq27xxx_battery(E) snd_soc_sst_byt_cht_nocodec qrtr brcmfmac_wcc mei_pxp dwc3 mei_hdcp intel_powerclamp coretemp kvm_intel intel_rapl_msr gpio_keys x86_android_tablets(E) udc_core ulpi brcmfmac kvm brcmutil irqbypass i2c_cht_wc intel_cstate snd_sof_acpi_intel_byt cfg80211 snd_sof_acpi snd_sof_intel_atom leds_cht_wcove(E) snd_sof_xtensa_dsp snd_sof pcspkr extcon_intel_cht_wc snd_sof_utils lpc_ich snd_intel_sst_acpi intel_xhci_usb_role_switch snd_soc_acpi_intel_match snd_intel_sst_core snd_soc_sst_atom_hifi2_platform hid_sensor_magn_3d snd_soc_acpi snd_intel_dspcfg snd_intel_sdw_acpi hid_sensor_gyro_3d hid_sensor_accel_3d hid_sensor_incl_3d hid_sensor_rotation hid_sensor_als hid_sensor_trigger hid_sensor_iio_common industrialio_triggered_buffer kfifo_buf industrialio snd_soc_core snd_compress ac97_bus hci_uart(E) btqca(E) btrtl(E) btbcm(E) snd_pcm_dmaengine btintel(E)
[  832.605676]  snd_hdmi_lpe_audio intel_hid sparse_keymap intel_soc_pmic_bxtwc snd_seq bluetooth snd_seq_device ov5693 snd_pcm atomisp_ov2722(C) v4l2_fwnode v4l2_async atomisp_gmin_platform(C) videodev spi_pxa2xx_platform mc ecdh_generic snd_timer rfkill_gpio processor_thermal_device_pci_legacy snd rfkill dw_dmac processor_thermal_device soundcore processor_thermal_rfim processor_thermal_mbox mei_txe processor_thermal_rapl vfat fat int3400_thermal intel_rapl_common int3403_thermal mei soc_button_array int3406_thermal acpi_thermal_rel int340x_thermal_zone dptf_power(E) intel_soc_dts_iosf intel_int0002_vgpio dwc3_pci acpi_pad(E) zram hid_sensor_hub intel_ishtp_hid i915(E) crct10dif_pclmul mmc_block crc32_pclmul crc32c_intel i2c_algo_bit drm_buddy(E) drm_display_helper(E) ghash_clmulni_intel sha512_ssse3 wdat_wdt intel_ish_ipc cec video(E) intel_ishtp sdhci_acpi ttm(E) sdhci wmi i2c_hid_acpi(E) i2c_hid(E) mmc_core pwm_lpss_platform pwm_lpss ip6_tables ip_tables i2c_dev fuse
[  832.606213] CPU: 2 PID: 370 Comm: kworker/2:4 Tainted: G        WC  E      6.3.0-rc4+ #179
[  832.606237] Hardware name: Intel Corporation CHERRYVIEW D1 PLATFORM/Cherry Trail Tablet, BIOS CHTTYETI.X64.0514.R1B.1701240934 01/24/2017
[  832.606251] Workqueue: events power_supply_changed_work
[  832.606281] RIP: 0010:rcu_note_context_switch+0x4ee/0x690
[  832.606311] Code: 49 89 3f 49 83 bc 24 98 00 00 00 00 0f 85 66 fe ff ff e9 58 fe ff ff 48 c7 c7 48 e4 75 83 c6 05 b9 0c b2 01 01 e8 92 4b f5 ff <0f> 0b e9 70 fb ff ff a9 ff ff ff 7f 0f 84 2c fc ff ff 65 48 8b 3c
[  832.606328] RSP: 0018:ffffbd0480737908 EFLAGS: 00010092
[  832.606348] RAX: 000000000000003f RBX: ffff9ff8fa933900 RCX: 0000000000000000
[  832.606362] RDX: 0000000000000003 RSI: ffffffff837b6da0 RDI: 00000000ffffffff
[  832.606374] RBP: 0000000000000000 R08: 0000000000000000 R09: ffffbd04807377c0
[  832.606386] R10: 0000000000000003 R11: ffffffff83b44168 R12: ffff9ff8fa932a80
[  832.606398] R13: ffff9ff8867a0000 R14: ffffbd0480737a78 R15: ffff9ff8810e5060
[  832.606411] FS:  0000000000000000(0000) GS:ffff9ff8fa900000(0000) knlGS:0000000000000000
[  832.606427] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  832.606439] CR2: 00007f1e96b510c8 CR3: 0000000155a20000 CR4: 00000000001006e0
[  832.606453] Call Trace:
[  832.606466]  <TASK>
[  832.606487]  __schedule+0x9f/0x1480
[  832.606527]  schedule+0x5d/0xe0
[  832.606549]  schedule_timeout+0x79/0x140
[  832.606572]  ? __pfx_process_timeout+0x10/0x10
[  832.606599]  wait_for_completion_timeout+0x6f/0x140
[  832.606627]  i2c_dw_xfer+0x101/0x460
[  832.606659]  ? psi_group_change+0x168/0x400
[  832.606680]  __i2c_transfer+0x172/0x6d0
[  832.606709]  i2c_smbus_xfer_emulated+0x27d/0x9c0
[  832.606732]  ? __schedule+0x430/0x1480
[  832.606753]  ? preempt_count_add+0x6a/0xa0
[  832.606778]  ? get_nohz_timer_target+0x18/0x190
[  832.606796]  ? lock_timer_base+0x61/0x80
[  832.606817]  ? preempt_count_add+0x6a/0xa0
[  832.606842]  __i2c_smbus_xfer+0xa2/0x3f0
[  832.606862]  i2c_smbus_xfer+0x66/0xf0
[  832.606882]  i2c_smbus_read_byte_data+0x41/0x70
[  832.606901]  ? _raw_spin_unlock_irqrestore+0x23/0x40
[  832.606922]  ? __pm_runtime_suspend+0x46/0xc0
[  832.606946]  cht_wc_byte_reg_read+0x2e/0x60
[  832.606972]  _regmap_read+0x5c/0x120
[  832.606997]  _regmap_update_bits+0x96/0xc0
[  832.607023]  regmap_update_bits_base+0x5b/0x90
[  832.607053]  cht_wc_leds_brightness_get+0x412/0x910 [leds_cht_wcove]
[  832.607094]  led_blink_setup+0x28/0x100
[  832.607119]  led_trigger_blink+0x40/0x70
[  832.607145]  power_supply_update_leds+0x1b7/0x1c0
[  832.607174]  power_supply_changed_work+0x67/0xe0
[  832.607198]  process_one_work+0x1c8/0x3c0
[  832.607222]  worker_thread+0x4d/0x380
[  832.607243]  ? __pfx_worker_thread+0x10/0x10
[  832.607258]  kthread+0xe9/0x110
[  832.607279]  ? __pfx_kthread+0x10/0x10
[  832.607300]  ret_from_fork+0x2c/0x50
[  832.607337]  </TASK>
[  832.607344] ---[ end trace 0000000000000000 ]---

Re: "leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Johannes Berg]

Hi,

> Sorry to bring the bearer of bad news, but your commit 2a5a8fa8b23
> ("leds: trigger: use RCU to protect the led_cdevs list"),
> is causing the following RCU warning when used with blinking
> triggers on I2C LED controllers which support hw blinking.

Err, well, surely that is a pre-existing driver bug then?

> The specific problem is drivers/leds/led-triggers.c:
> led_trigger_blink_setup() which does:
> 
>         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);
>         }
>         rcu_read_unlock();
> 
> And that led_blink_set() call then hits this path:
> 
>         if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
>             led_cdev->blink_set &&
>             !led_cdev->blink_set(led_cdev, delay_on, delay_off))
>                 return;
> 
> Which calls directly into the LED controller driver

Sure, so far so good.

> which talks to the LED controller over I2C which may sleep.

Which seems to me was already wrong before my patch, since the code was:

       read_lock_irqsave(&trig->leddev_list_lock, flags);
       list_for_each_entry(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);


Surely, the code wasn't allowed to sleep in an _irqsave() section? You'd
just see a different check complain, rather than about RCU, I guess.


So maybe you're bringing bad news, but I don't think it's for me ;-) I
don't see cht_wc_leds_brightness_get() or a driver/module called
leds_cht_wcove even in linux-next, so I guess you should look wherever
you got _that_ from.

johannes

Re: "leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Hans de Goede]

Hi Johannes,

Thank you for the quick reply.

On 4/11/23 21:38, Johannes Berg wrote:
> Hi,
> 
>> Sorry to bring the bearer of bad news, but your commit 2a5a8fa8b23
>> ("leds: trigger: use RCU to protect the led_cdevs list"),
>> is causing the following RCU warning when used with blinking
>> triggers on I2C LED controllers which support hw blinking.
> 
> Err, well, surely that is a pre-existing driver bug then?

So I just checked and the following LED drivers all have
a blink_set() implementation which calls mutex_lock()
and/or does I2C transfers:

leds-an30259a.c
leds-aw2013.c
leds-bd2802.c
leds-lp3944.c
leds-pca9532.c
leds-pca963x.c
leds-wm831x-status.c

And I only found one i2c LED controller driver which defers
I2C transfers to a workqueue itself (leds-tca6507.c).

And looking at: include/linux/leds.h

Then the brightness_set callback is explicitly marked as
"Must not sleep." (and there is a brightness_set_blocking
which e.g. I2C drivers can use instead).

So I believe that the intention has always been that
a driver's blink_set callback is allowed to sleep.

With that said you seem to be right that there seems to
be a long standing bug where led_trigger_blink[_oneshot]
calls led_classdev.blink_set() in a context where it may
not sleep.

But that is more of a LED (trigger) core bug then an
issue with the driver(s).

>> The specific problem is drivers/leds/led-triggers.c:
>> led_trigger_blink_setup() which does:
>>
>>         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);
>>         }
>>         rcu_read_unlock();
>>
>> And that led_blink_set() call then hits this path:
>>
>>         if (!test_bit(LED_BLINK_ONESHOT, &led_cdev->work_flags) &&
>>             led_cdev->blink_set &&
>>             !led_cdev->blink_set(led_cdev, delay_on, delay_off))
>>                 return;
>>
>> Which calls directly into the LED controller driver
> 
> Sure, so far so good.
> 
>> which talks to the LED controller over I2C which may sleep.
> 
> Which seems to me was already wrong before my patch, since the code was:
> 
>        read_lock_irqsave(&trig->leddev_list_lock, flags);
>        list_for_each_entry(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);
> 
> 
> Surely, the code wasn't allowed to sleep in an _irqsave() section? You'd
> just see a different check complain, rather than about RCU, I guess.

Hmm, so the irqsave part of this was introduced by commit 27af8e2c90fb
("leds: trigger: fix potential deadlock with libata") but even before
then I think sleeping here was not allowed, given that an rwlock is
a spinlock variant the non irqsave version also leads to a section
where sleeping is not allowed I believe.

Further git archeology seems to indicate that this problem has existed
for a long long time already. I guess I'm the first user of a trigger
which calls led_trigger_blink[_oneshot] on a led_classdev with
a hw blink_set() implementation which sleeps. Or at least I'm
the first user to do so with various lock-debugging options
enabled ...

> So maybe you're bringing bad news, but I don't think it's for me ;-)

Yes it seems I have unearthed a bug which is much older then your
RCU changes.

> I don't see cht_wc_leds_brightness_get() or a driver/module called
> leds_cht_wcove even in linux-next, so I guess you should look wherever
> you got _that_ from.

Right that is part of a new LED driver I'm working on. I guess I could
fix things in the driver, but this seems to be a (trigger) core bug
so lets try to solve it there.

Regards,

Hans

Re: "leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Johannes Berg]

Hi,

> So I just checked and the following LED drivers all have
> a blink_set() implementation which calls mutex_lock()
> and/or does I2C transfers:

<snip>

Yay ...

> And looking at: include/linux/leds.h
> 
> Then the brightness_set callback is explicitly marked as
> "Must not sleep." (and there is a brightness_set_blocking
> which e.g. I2C drivers can use instead).
> 
> So I believe that the intention has always been that
> a driver's blink_set callback is allowed to sleep.

I guess I wouldn't really go that far from the lack of a comment saying
it cannot sleep :-)


> With that said you seem to be right that there seems to
> be a long standing bug where led_trigger_blink[_oneshot]
> calls led_classdev.blink_set() in a context where it may
> not sleep.
> 
> But that is more of a LED (trigger) core bug then an
> issue with the driver(s).

IMHO that's arguable, but I'm not going to quibble over it.

> Hmm, so the irqsave part of this was introduced by commit 27af8e2c90fb
> ("leds: trigger: fix potential deadlock with libata") 

Indeed.

> but even before
> then I think sleeping here was not allowed, given that an rwlock is
> a spinlock variant the non irqsave version also leads to a section
> where sleeping is not allowed I believe.

Right. Which goes back to 0b9536c95709 ("leds: Add ability to blink via
simple trigger") where blinking was made possible from a trigger ...

> Further git archeology seems to indicate that this problem has existed
> for a long long time already. I guess I'm the first user of a trigger
> which calls led_trigger_blink[_oneshot] on a led_classdev with
> a hw blink_set() implementation which sleeps. Or at least I'm
> the first user to do so with various lock-debugging options
> enabled ...

Right...

> but this seems to be a (trigger) core bug
> so lets try to solve it there.
> 

It's not so easy to fix I guess, other than maybe to defer to a
workqueue, but then you have the issue of cancelling?

Note that led_trigger_blink() also doesn't document when it's allowed to
be called, and at least in mac80211 (my code, yay) we're calling it from
a timer, so can't possibly sleep there. There's only one other caller in
the power supply code, but that can actually sleep.

I'd have a least thought of srcu if that timer weren't the case in
mac80211, but as is that doesn't help ...

So not sure. Clearly it's a long-standing issue, and given that many
drivers are affected probably better to fix it in the LED core, but I
don't really know my way around it very well either.

johannes

Re: "leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Hans de Goede]

Hi Johannes,

On 4/12/23 10:13, Johannes Berg wrote:
> Hi,
> 
>> So I just checked and the following LED drivers all have
>> a blink_set() implementation which calls mutex_lock()
>> and/or does I2C transfers:
> 
> <snip>
> 
> Yay ...

<snip>

> So not sure. Clearly it's a long-standing issue, and given that many
> drivers are affected probably better to fix it in the LED core, but I
> don't really know my way around it very well either.

I think I've come up with a solution for this.

I'll Cc you on the patch-set for this when it is ready.

Regards,

Hans