Re: [PATCH RESEND 3/4] leds: Fix oops about sleeping in led_trigger_blink()

[Lee Jones <lee@kernel.org>]

On Wed, 10 May 2023, Hans de Goede wrote:

> led_trigger_blink() calls led_blink_set() from a RCU read-side critical
> section so led_blink_set() must not sleep. Note sleeping was not allowed
> before the switch to RCU either because a spinlock was held before.
> 
> led_blink_set() does not sleep when sw-blinking is used, but
> many LED controller drivers with hw blink support have a blink_set
> function which may sleep, leading to an oops like this one:
> 
> [  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
> <snip>
> [  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 ]---
> 
> Add a new led_blink_set_nosleep() function which defers the actual
> led_blink_set() call to a workqueue when necessary to fix this.
> 
> This also fixes an existing race where a pending led_set_brightness() has
> been deferred to set_brightness_work and might then race with a later
> led_cdev->blink_set() call. Note this race is only an issue with triggers
> mixing led_trigger_event() and led_trigger_blink() calls, sysfs API
> calls and led_trigger_blink_oneshot() are not affected.
> 
> Note rather then adding a separate blink_set_blocking callback this uses
> the presence of the already existing brightness_set_blocking callback to
> detect if the blinking call should be deferred to set_brightness_work.
> 
> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-core.c     | 24 ++++++++++++++++++++++++
>  drivers/leds/led-triggers.c |  2 +-
>  include/linux/leds.h        | 24 ++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 1 deletion(-)

Applied, thanks

-- 
Lee Jones [李琼斯]