| Message ID | 20240507095111.27109-1-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: trigger: Call synchronize_rcu() before calling trig->activate() | expand |
On Tue, 07 May 2024, Hans de Goede wrote: > Some triggers call led_trigger_event() from their activate() callback > to initialize the brightness of the LED for which the trigger is being > activated. > > In order for the LEDs initial state to be set correctly this requires that > the led_trigger_event() call uses the new version of trigger->led_cdevs, > which has the new LED. > > AFAICT led_trigger_event() will always use the new version when it is > running on the same CPU as where the list_add_tail_rcu() call was made, > which is why the missing synchronize_rcu() has not lead to bug reports. > But if activate() is pre-empted, sleeps or uses a worker then > the led_trigger_event() call may run on another CPU which may still use > the old trigger->led_cdevs list. > > Add a synchronize_rcu() call to ensure that any led_trigger_event() calls > done from activate() always use the new list. > > Triggers using led_trigger_event() from their activate() callback are: > net/bluetooth/leds.c, net/rfkill/core.c and drivers/tty/vt/keyboard.c. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/leds/led-triggers.c | 7 +++++++ > 1 file changed, 7 insertions(+) Looks like there have been a few changes to led_trigger_set() since this was authored. Please rebase and resubmit. Thanks.
On Tue, 07 May 2024 11:51:11 +0200, Hans de Goede wrote: > Some triggers call led_trigger_event() from their activate() callback > to initialize the brightness of the LED for which the trigger is being > activated. > > In order for the LEDs initial state to be set correctly this requires that > the led_trigger_event() call uses the new version of trigger->led_cdevs, > which has the new LED. > > [...] Applied, thanks! [1/1] leds: trigger: Call synchronize_rcu() before calling trig->activate() commit: 47b8a4059f8a8b2407ab22fad8775842c54e59c6 -- Lee Jones [李琼斯]
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index 6d535a7fd075..018fe1eed4db 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -194,6 +194,13 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) spin_unlock(&trig->leddev_list_lock); led_cdev->trigger = trig; + /* + * Some activate() calls use led_trigger_event() to initialize + * the brightness of the LED for which the trigger is being set. + * Ensure the led_cdev is visible on trig->led_cdevs for this. + */ + synchronize_rcu(); + if (trig->activate) ret = trig->activate(led_cdev); else
Some triggers call led_trigger_event() from their activate() callback to initialize the brightness of the LED for which the trigger is being activated. In order for the LEDs initial state to be set correctly this requires that the led_trigger_event() call uses the new version of trigger->led_cdevs, which has the new LED. AFAICT led_trigger_event() will always use the new version when it is running on the same CPU as where the list_add_tail_rcu() call was made, which is why the missing synchronize_rcu() has not lead to bug reports. But if activate() is pre-empted, sleeps or uses a worker then the led_trigger_event() call may run on another CPU which may still use the old trigger->led_cdevs list. Add a synchronize_rcu() call to ensure that any led_trigger_event() calls done from activate() always use the new list. Triggers using led_trigger_event() from their activate() callback are: net/bluetooth/leds.c, net/rfkill/core.c and drivers/tty/vt/keyboard.c. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/leds/led-triggers.c | 7 +++++++ 1 file changed, 7 insertions(+)