[PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Hans de Goede]

Hi All,

Here is a patch series to fix an oops about sleeping in led_trigger_blink()
+ one other small bugfix.

Patches 1-3 should arguably have a:

Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")

tag, but Fixes tags tend to lead to patches getting automatically added
to the stable series and I would prefer to see this series get some
significant testing time in mainline first, so I have chosen to omit
the tag.

Regards,

Hans


Hans de Goede (4):
  leds: Change led_trigger_blink[_oneshot]() delay parameters to
    pass-by-value
  leds: Fix set_brightness_delayed() race
  leds: Fix oops about sleeping in led_trigger_blink()
  leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger

 drivers/leds/led-core.c                  | 81 ++++++++++++++++++++----
 drivers/leds/led-triggers.c              | 17 ++---
 drivers/leds/trigger/ledtrig-disk.c      |  9 +--
 drivers/leds/trigger/ledtrig-mtd.c       |  8 +--
 drivers/net/arcnet/arcnet.c              |  8 +--
 drivers/power/supply/power_supply_leds.c |  5 +-
 drivers/usb/common/led.c                 |  4 +-
 include/linux/leds.h                     | 43 ++++++++++---
 net/mac80211/led.c                       |  2 +-
 net/mac80211/led.h                       |  8 +--
 net/netfilter/xt_LED.c                   |  3 +-
 11 files changed, 125 insertions(+), 63 deletions(-)

-- 
2.39.1

[PATCH 1/4] leds: Change led_trigger_blink[_oneshot]() delay parameters to pass-by-value

[Hans de Goede]

led_blink_set[_oneshot]()'s delay_on and delay_off function parameters
are pass by reference, so that hw-blink implementations can report
back the actual achieved delays when the values have been rounded
to something the hw supports.

This is really only interesting for the sysfs API / the timer trigger.
Other triggers don't really care about this and none of the callers of
led_trigger_blink[_oneshot]() do anything with the returned delay values.

Change the led_trigger_blink[_oneshot]() delay parameters to pass-by-value,
there are 2 reasons for this:

1. led_cdev->blink_set() may sleep, while led_trigger_blink() may not.
So on hw where led_cdev->blink_set() sleeps the call needs to be deferred
to a workqueue, in which case the actual achieved delays are unknown
(this is a preparation patch for the deferring).

2. Since the callers don't care about the actual achieved delays, allowing
callers to directly pass a value leads to simpler code for most callers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-triggers.c              | 16 ++++++++--------
 drivers/leds/trigger/ledtrig-disk.c      |  9 +++------
 drivers/leds/trigger/ledtrig-mtd.c       |  8 ++------
 drivers/net/arcnet/arcnet.c              |  8 ++------
 drivers/power/supply/power_supply_leds.c |  5 +----
 drivers/usb/common/led.c                 |  4 +---
 include/linux/leds.h                     | 16 ++++++++--------
 net/mac80211/led.c                       |  2 +-
 net/mac80211/led.h                       |  8 ++------
 net/netfilter/xt_LED.c                   |  3 +--
 10 files changed, 29 insertions(+), 50 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 072491d3e17b..e06361165e9b 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -393,8 +393,8 @@ void led_trigger_event(struct led_trigger *trig,
 EXPORT_SYMBOL_GPL(led_trigger_event);
 
 static void led_trigger_blink_setup(struct led_trigger *trig,
-			     unsigned long *delay_on,
-			     unsigned long *delay_off,
+			     unsigned long delay_on,
+			     unsigned long delay_off,
 			     int oneshot,
 			     int invert)
 {
@@ -406,25 +406,25 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
 	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,
+			led_blink_set_oneshot(led_cdev, &delay_on, &delay_off,
 					      invert);
 		else
-			led_blink_set(led_cdev, delay_on, delay_off);
+			led_blink_set(led_cdev, &delay_on, &delay_off);
 	}
 	rcu_read_unlock();
 }
 
 void led_trigger_blink(struct led_trigger *trig,
-		       unsigned long *delay_on,
-		       unsigned long *delay_off)
+		       unsigned long delay_on,
+		       unsigned long delay_off)
 {
 	led_trigger_blink_setup(trig, delay_on, delay_off, 0, 0);
 }
 EXPORT_SYMBOL_GPL(led_trigger_blink);
 
 void led_trigger_blink_oneshot(struct led_trigger *trig,
-			       unsigned long *delay_on,
-			       unsigned long *delay_off,
+			       unsigned long delay_on,
+			       unsigned long delay_off,
 			       int invert)
 {
 	led_trigger_blink_setup(trig, delay_on, delay_off, 1, invert);
diff --git a/drivers/leds/trigger/ledtrig-disk.c b/drivers/leds/trigger/ledtrig-disk.c
index 0b7dfbd04273..e9b87ee944f2 100644
--- a/drivers/leds/trigger/ledtrig-disk.c
+++ b/drivers/leds/trigger/ledtrig-disk.c
@@ -19,16 +19,13 @@ DEFINE_LED_TRIGGER(ledtrig_disk_write);
 
 void ledtrig_disk_activity(bool write)
 {
-	unsigned long blink_delay = BLINK_DELAY;
-
-	led_trigger_blink_oneshot(ledtrig_disk,
-				  &blink_delay, &blink_delay, 0);
+	led_trigger_blink_oneshot(ledtrig_disk, BLINK_DELAY, BLINK_DELAY, 0);
 	if (write)
 		led_trigger_blink_oneshot(ledtrig_disk_write,
-					  &blink_delay, &blink_delay, 0);
+					  BLINK_DELAY, BLINK_DELAY, 0);
 	else
 		led_trigger_blink_oneshot(ledtrig_disk_read,
-					  &blink_delay, &blink_delay, 0);
+					  BLINK_DELAY, BLINK_DELAY, 0);
 }
 EXPORT_SYMBOL(ledtrig_disk_activity);
 
diff --git a/drivers/leds/trigger/ledtrig-mtd.c b/drivers/leds/trigger/ledtrig-mtd.c
index 8fa763c2269b..bbe6876a249d 100644
--- a/drivers/leds/trigger/ledtrig-mtd.c
+++ b/drivers/leds/trigger/ledtrig-mtd.c
@@ -22,12 +22,8 @@ DEFINE_LED_TRIGGER(ledtrig_nand);
 
 void ledtrig_mtd_activity(void)
 {
-	unsigned long blink_delay = BLINK_DELAY;
-
-	led_trigger_blink_oneshot(ledtrig_mtd,
-				  &blink_delay, &blink_delay, 0);
-	led_trigger_blink_oneshot(ledtrig_nand,
-				  &blink_delay, &blink_delay, 0);
+	led_trigger_blink_oneshot(ledtrig_mtd, BLINK_DELAY, BLINK_DELAY, 0);
+	led_trigger_blink_oneshot(ledtrig_nand, BLINK_DELAY, BLINK_DELAY, 0);
 }
 EXPORT_SYMBOL(ledtrig_mtd_activity);
 
diff --git a/drivers/net/arcnet/arcnet.c b/drivers/net/arcnet/arcnet.c
index 1bad1866ae46..99265667538c 100644
--- a/drivers/net/arcnet/arcnet.c
+++ b/drivers/net/arcnet/arcnet.c
@@ -196,13 +196,10 @@ static void arcnet_dump_packet(struct net_device *dev, int bufnum,
 void arcnet_led_event(struct net_device *dev, enum arcnet_led_event event)
 {
 	struct arcnet_local *lp = netdev_priv(dev);
-	unsigned long led_delay = 350;
-	unsigned long tx_delay = 50;
 
 	switch (event) {
 	case ARCNET_LED_EVENT_RECON:
-		led_trigger_blink_oneshot(lp->recon_led_trig,
-					  &led_delay, &led_delay, 0);
+		led_trigger_blink_oneshot(lp->recon_led_trig, 350, 350, 0);
 		break;
 	case ARCNET_LED_EVENT_OPEN:
 		led_trigger_event(lp->tx_led_trig, LED_OFF);
@@ -213,8 +210,7 @@ void arcnet_led_event(struct net_device *dev, enum arcnet_led_event event)
 		led_trigger_event(lp->recon_led_trig, LED_OFF);
 		break;
 	case ARCNET_LED_EVENT_TX:
-		led_trigger_blink_oneshot(lp->tx_led_trig,
-					  &tx_delay, &tx_delay, 0);
+		led_trigger_blink_oneshot(lp->tx_led_trig, 50, 50, 0);
 		break;
 	}
 }
diff --git a/drivers/power/supply/power_supply_leds.c b/drivers/power/supply/power_supply_leds.c
index 702bf83f6e6d..e2f554e4e4e6 100644
--- a/drivers/power/supply/power_supply_leds.c
+++ b/drivers/power/supply/power_supply_leds.c
@@ -22,8 +22,6 @@
 static void power_supply_update_bat_leds(struct power_supply *psy)
 {
 	union power_supply_propval status;
-	unsigned long delay_on = 0;
-	unsigned long delay_off = 0;
 
 	if (power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS, &status))
 		return;
@@ -42,8 +40,7 @@ static void power_supply_update_bat_leds(struct power_supply *psy)
 		led_trigger_event(psy->charging_full_trig, LED_FULL);
 		led_trigger_event(psy->charging_trig, LED_FULL);
 		led_trigger_event(psy->full_trig, LED_OFF);
-		led_trigger_blink(psy->charging_blink_full_solid_trig,
-			&delay_on, &delay_off);
+		led_trigger_blink(psy->charging_blink_full_solid_trig, 0, 0);
 		break;
 	default:
 		led_trigger_event(psy->charging_full_trig, LED_OFF);
diff --git a/drivers/usb/common/led.c b/drivers/usb/common/led.c
index 0865dd44a80a..1de18d90b134 100644
--- a/drivers/usb/common/led.c
+++ b/drivers/usb/common/led.c
@@ -14,8 +14,6 @@
 
 #define BLINK_DELAY 30
 
-static unsigned long usb_blink_delay = BLINK_DELAY;
-
 DEFINE_LED_TRIGGER(ledtrig_usb_gadget);
 DEFINE_LED_TRIGGER(ledtrig_usb_host);
 
@@ -32,7 +30,7 @@ void usb_led_activity(enum usb_led_event ev)
 		break;
 	}
 	/* led_trigger_blink_oneshot() handles trig == NULL gracefully */
-	led_trigger_blink_oneshot(trig, &usb_blink_delay, &usb_blink_delay, 0);
+	led_trigger_blink_oneshot(trig, BLINK_DELAY, BLINK_DELAY, 0);
 }
 EXPORT_SYMBOL_GPL(usb_led_activity);
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d71201a968b6..6006786cafdc 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -415,11 +415,11 @@ void led_trigger_register_simple(const char *name,
 				struct led_trigger **trigger);
 void led_trigger_unregister_simple(struct led_trigger *trigger);
 void led_trigger_event(struct led_trigger *trigger,  enum led_brightness event);
-void led_trigger_blink(struct led_trigger *trigger, unsigned long *delay_on,
-		       unsigned long *delay_off);
+void led_trigger_blink(struct led_trigger *trigger, unsigned long delay_on,
+		       unsigned long delay_off);
 void led_trigger_blink_oneshot(struct led_trigger *trigger,
-			       unsigned long *delay_on,
-			       unsigned long *delay_off,
+			       unsigned long delay_on,
+			       unsigned long delay_off,
 			       int invert);
 void led_trigger_set_default(struct led_classdev *led_cdev);
 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger);
@@ -469,11 +469,11 @@ static inline void led_trigger_unregister_simple(struct led_trigger *trigger) {}
 static inline void led_trigger_event(struct led_trigger *trigger,
 				enum led_brightness event) {}
 static inline void led_trigger_blink(struct led_trigger *trigger,
-				      unsigned long *delay_on,
-				      unsigned long *delay_off) {}
+				      unsigned long delay_on,
+				      unsigned long delay_off) {}
 static inline void led_trigger_blink_oneshot(struct led_trigger *trigger,
-				      unsigned long *delay_on,
-				      unsigned long *delay_off,
+				      unsigned long delay_on,
+				      unsigned long delay_off,
 				      int invert) {}
 static inline void led_trigger_set_default(struct led_classdev *led_cdev) {}
 static inline int led_trigger_set(struct led_classdev *led_cdev,
diff --git a/net/mac80211/led.c b/net/mac80211/led.c
index 6de8d0ad5497..2dc732147e85 100644
--- a/net/mac80211/led.c
+++ b/net/mac80211/led.c
@@ -282,7 +282,7 @@ static void tpt_trig_timer(struct timer_list *t)
 		}
 	}
 
-	led_trigger_blink(&local->tpt_led, &on, &off);
+	led_trigger_blink(&local->tpt_led, on, off);
 }
 
 const char *
diff --git a/net/mac80211/led.h b/net/mac80211/led.h
index b71a1428d883..d25f13346b82 100644
--- a/net/mac80211/led.h
+++ b/net/mac80211/led.h
@@ -13,22 +13,18 @@
 static inline void ieee80211_led_rx(struct ieee80211_local *local)
 {
 #ifdef CONFIG_MAC80211_LEDS
-	unsigned long led_delay = MAC80211_BLINK_DELAY;
-
 	if (!atomic_read(&local->rx_led_active))
 		return;
-	led_trigger_blink_oneshot(&local->rx_led, &led_delay, &led_delay, 0);
+	led_trigger_blink_oneshot(&local->rx_led, MAC80211_BLINK_DELAY, MAC80211_BLINK_DELAY, 0);
 #endif
 }
 
 static inline void ieee80211_led_tx(struct ieee80211_local *local)
 {
 #ifdef CONFIG_MAC80211_LEDS
-	unsigned long led_delay = MAC80211_BLINK_DELAY;
-
 	if (!atomic_read(&local->tx_led_active))
 		return;
-	led_trigger_blink_oneshot(&local->tx_led, &led_delay, &led_delay, 0);
+	led_trigger_blink_oneshot(&local->tx_led, MAC80211_BLINK_DELAY, MAC80211_BLINK_DELAY, 0);
 #endif
 }
 
diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
index 66b0f941d8fb..36c9720ad8d6 100644
--- a/net/netfilter/xt_LED.c
+++ b/net/netfilter/xt_LED.c
@@ -43,7 +43,6 @@ led_tg(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct xt_led_info *ledinfo = par->targinfo;
 	struct xt_led_info_internal *ledinternal = ledinfo->internal_data;
-	unsigned long led_delay = XT_LED_BLINK_DELAY;
 
 	/*
 	 * If "always blink" is enabled, and there's still some time until the
@@ -52,7 +51,7 @@ led_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	if ((ledinfo->delay > 0) && ledinfo->always_blink &&
 	    timer_pending(&ledinternal->timer))
 		led_trigger_blink_oneshot(&ledinternal->netfilter_led_trigger,
-					  &led_delay, &led_delay, 1);
+					  XT_LED_BLINK_DELAY, XT_LED_BLINK_DELAY, 1);
 	else
 		led_trigger_event(&ledinternal->netfilter_led_trigger, LED_FULL);
 
-- 
2.39.1

[PATCH 2/4] leds: Fix set_brightness_delayed() race

[Hans de Goede]

When a trigger wants to switch from blinking to LED on it needs to call:
	led_set_brightness(LED_OFF);
	led_set_brightness(LED_FULL);

To first call disables blinking and the second then turns the LED on
(the power-supply charging-blink-full-solid triggers do this).

These calls happen immediately after each other, so it is possible
that set_brightness_delayed() from the first call has not run yet
when the led_set_brightness(LED_FULL) call finishes.

If this race hits then this is causing problems for both
sw- and hw-blinking:

For sw-blinking set_brightness_delayed() clears delayed_set_value
when LED_BLINK_DISABLE is set causing the led_set_brightness(LED_FULL)
call effects to get lost when hitting the race, resulting in the LED
turning off instead of on.

For hw-blinking if the race hits delayed_set_value has been
set to LED_FULL by the time set_brightness_delayed() runs.
So led_cdev->brightness_set_blocking() is never called with
LED_OFF as argument and the hw-blinking is never disabled leaving
the LED blinking instead of on.

Fix both issues by adding LED_SET_BRIGHTNESS and LED_SET_BRIGHTNESS_OFF
work_flags making this 2 separate actions to be run by
set_brightness_delayed().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-core.c | 57 +++++++++++++++++++++++++++++++----------
 include/linux/leds.h    |  3 +++
 2 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 4a97cb745788..e61acc785410 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -114,21 +114,14 @@ static void led_timer_function(struct timer_list *t)
 	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
 }
 
-static void set_brightness_delayed(struct work_struct *ws)
+static void set_brightness_delayed_set_brightness(struct led_classdev *led_cdev,
+						  unsigned int value)
 {
-	struct led_classdev *led_cdev =
-		container_of(ws, struct led_classdev, set_brightness_work);
 	int ret = 0;
 
-	if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
-		led_cdev->delayed_set_value = LED_OFF;
-		led_stop_software_blink(led_cdev);
-	}
-
-	ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
+	ret = __led_set_brightness(led_cdev, value);
 	if (ret == -ENOTSUPP)
-		ret = __led_set_brightness_blocking(led_cdev,
-					led_cdev->delayed_set_value);
+		ret = __led_set_brightness_blocking(led_cdev, value);
 	if (ret < 0 &&
 	    /* LED HW might have been unplugged, therefore don't warn */
 	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -137,6 +130,30 @@ static void set_brightness_delayed(struct work_struct *ws)
 			"Setting an LED's brightness failed (%d)\n", ret);
 }
 
+static void set_brightness_delayed(struct work_struct *ws)
+{
+	struct led_classdev *led_cdev =
+		container_of(ws, struct led_classdev, set_brightness_work);
+
+	if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
+		led_stop_software_blink(led_cdev);
+		set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
+	}
+
+	/*
+	 * Triggers may call led_set_brightness(LED_OFF),
+	 * led_set_brightness(LED_FULL) in quick succession to disable blinking
+	 * and turn the LED on. Both actions may have been scheduled to run
+	 * before this work item runs once. To make sure this works properly
+	 * handle LED_SET_BRIGHTNESS_OFF first.
+	 */
+	if (test_and_clear_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags))
+		set_brightness_delayed_set_brightness(led_cdev, LED_OFF);
+
+	if (test_and_clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags))
+		set_brightness_delayed_set_brightness(led_cdev, led_cdev->delayed_set_value);
+}
+
 static void led_set_software_blink(struct led_classdev *led_cdev,
 				   unsigned long delay_on,
 				   unsigned long delay_off)
@@ -271,8 +288,22 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
 	if (!__led_set_brightness(led_cdev, value))
 		return;
 
-	/* If brightness setting can sleep, delegate it to a work queue task */
-	led_cdev->delayed_set_value = value;
+	/*
+	 * Brightness setting can sleep, delegate it to a work queue task.
+	 * value 0 / LED_OFF is special, since it also disables hw-blinking
+	 * (sw-blink disable is handled in led_set_brightness()).
+	 * To avoid a hw-blink-disable getting lost when a second brightness
+	 * change is done immediately afterwards (before the work runs),
+	 * it uses a separate work_flag.
+	 */
+	if (value) {
+		led_cdev->delayed_set_value = value;
+		set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+	} else {
+		clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+		set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
+	}
+
 	schedule_work(&led_cdev->set_brightness_work);
 }
 EXPORT_SYMBOL_GPL(led_set_brightness_nopm);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6006786cafdc..eb68aea79c6d 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -116,6 +116,9 @@ struct led_classdev {
 #define LED_BLINK_INVERT		3
 #define LED_BLINK_BRIGHTNESS_CHANGE 	4
 #define LED_BLINK_DISABLE		5
+	/* Brightness off also disables hw-blinking so it is a separate action */
+#define LED_SET_BRIGHTNESS_OFF		6
+#define LED_SET_BRIGHTNESS		7
 
 	/* Set LED brightness level
 	 * Must not sleep. Use brightness_set_blocking for drivers
-- 
2.39.1

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

[Hans de Goede]

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.

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

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index e61acc785410..b9b1295833c9 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -152,6 +152,13 @@ static void set_brightness_delayed(struct work_struct *ws)
 
 	if (test_and_clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags))
 		set_brightness_delayed_set_brightness(led_cdev, led_cdev->delayed_set_value);
+
+	if (test_and_clear_bit(LED_SET_BLINK, &led_cdev->work_flags)) {
+		unsigned long delay_on = led_cdev->delayed_delay_on;
+		unsigned long delay_off = led_cdev->delayed_delay_off;
+
+		led_blink_set(led_cdev, &delay_on, &delay_off);
+	}
 }
 
 static void led_set_software_blink(struct led_classdev *led_cdev,
@@ -246,6 +253,22 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
 }
 EXPORT_SYMBOL_GPL(led_blink_set_oneshot);
 
+void led_blink_set_nosleep(struct led_classdev *led_cdev, unsigned long delay_on,
+			   unsigned long delay_off)
+{
+	/* If necessary delegate to a work queue task. */
+	if (led_cdev->blink_set && led_cdev->brightness_set_blocking) {
+		led_cdev->delayed_delay_on = delay_on;
+		led_cdev->delayed_delay_off = delay_off;
+		set_bit(LED_SET_BLINK, &led_cdev->work_flags);
+		schedule_work(&led_cdev->set_brightness_work);
+		return;
+	}
+
+	led_blink_set(led_cdev, &delay_on, &delay_off);
+}
+EXPORT_SYMBOL_GPL(led_blink_set_nosleep);
+
 void led_stop_software_blink(struct led_classdev *led_cdev)
 {
 	del_timer_sync(&led_cdev->blink_timer);
@@ -301,6 +324,7 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, unsigned int value)
 		set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
 	} else {
 		clear_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
+		clear_bit(LED_SET_BLINK, &led_cdev->work_flags);
 		set_bit(LED_SET_BRIGHTNESS_OFF, &led_cdev->work_flags);
 	}
 
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index e06361165e9b..8214d3f7bc5f 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -409,7 +409,7 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
 			led_blink_set_oneshot(led_cdev, &delay_on, &delay_off,
 					      invert);
 		else
-			led_blink_set(led_cdev, &delay_on, &delay_off);
+			led_blink_set_nosleep(led_cdev, delay_on, delay_off);
 	}
 	rcu_read_unlock();
 }
diff --git a/include/linux/leds.h b/include/linux/leds.h
index eb68aea79c6d..3c93dec3f4e2 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -119,6 +119,7 @@ struct led_classdev {
 	/* Brightness off also disables hw-blinking so it is a separate action */
 #define LED_SET_BRIGHTNESS_OFF		6
 #define LED_SET_BRIGHTNESS		7
+#define LED_SET_BLINK			8
 
 	/* Set LED brightness level
 	 * Must not sleep. Use brightness_set_blocking for drivers
@@ -142,6 +143,10 @@ struct led_classdev {
 	 * match the values specified exactly.
 	 * Deactivate blinking again when the brightness is set to LED_OFF
 	 * via the brightness_set() callback.
+	 * For led_blink_set_nosleep() the LED core assumes that blink_set
+	 * implementations, of drivers which do not use brightness_set_blocking,
+	 * will not sleep. Therefor if brightness_set_blocking is not set
+	 * this function must not sleep!
 	 */
 	int		(*blink_set)(struct led_classdev *led_cdev,
 				     unsigned long *delay_on,
@@ -165,6 +170,8 @@ struct led_classdev {
 
 	struct work_struct	set_brightness_work;
 	int			delayed_set_value;
+	unsigned long		delayed_delay_on;
+	unsigned long		delayed_delay_off;
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	/* Protects the trigger data below */
@@ -257,12 +264,27 @@ struct led_classdev *__must_check devm_of_led_get(struct device *dev,
  * software blinking if there is no hardware blinking or if
  * the LED refuses the passed values.
  *
+ * This function may sleep!
+ *
  * Note that if software blinking is active, simply calling
  * led_cdev->brightness_set() will not stop the blinking,
  * use led_classdev_brightness_set() instead.
  */
 void led_blink_set(struct led_classdev *led_cdev, unsigned long *delay_on,
 		   unsigned long *delay_off);
+
+/**
+ * led_blink_set_nosleep - set blinking, guaranteed to not sleep
+ * @led_cdev: the LED to start blinking
+ * @delay_on: the time it should be on (in ms)
+ * @delay_off: the time it should ble off (in ms)
+ *
+ * This function makes the LED blink and is guaranteed to not sleep. Otherwise
+ * this is the same as led_blink_set(), see led_blink_set() for details.
+ */
+void led_blink_set_nosleep(struct led_classdev *led_cdev, unsigned long delay_on,
+			   unsigned long delay_off);
+
 /**
  * led_blink_set_oneshot - do a oneshot software blink
  * @led_cdev: the LED to start blinking
@@ -276,6 +298,8 @@ void led_blink_set(struct led_classdev *led_cdev, unsigned long *delay_on,
  *
  * If invert is set, led blinks for delay_off first, then for
  * delay_on and leave the led on after the on-off cycle.
+ *
+ * This function is guaranteed not to sleep.
  */
 void led_blink_set_oneshot(struct led_classdev *led_cdev,
 			   unsigned long *delay_on, unsigned long *delay_off,
-- 
2.39.1

[PATCH 4/4] leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger

[Hans de Goede]

Not all triggers use LED_INIT_DEFAULT_TRIGGER, which means that it
will not get cleared when the default trigger is a trigger which
does not use it such as "default-on".

If the default trigger then later gets replaced by a trigger which
does check LED_INIT_DEFAULT_TRIGGER, such as "timer" then that trigger
will behave as if it is the default trigger which it should not do.

To fix this clear the LED_INIT_DEFAULT_TRIGGER flag when clearing
the current trigger, so that it will not be set for any subsequently
set (non default) triggers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/leds/led-triggers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 8214d3f7bc5f..6a5e1f41f9a4 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -185,6 +185,7 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 		led_cdev->trigger = NULL;
 		led_cdev->trigger_data = NULL;
 		led_cdev->activated = false;
+		led_cdev->flags &= ~LED_INIT_DEFAULT_TRIGGER;
 		led_set_brightness(led_cdev, LED_OFF);
 	}
 	if (trig) {
-- 
2.39.1

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Jacek Anaszewski]

Hi Hans,

Generally the series looks good to me, but it would be good
to get some Tested-by(s).

Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

On 4/12/23 23:58, Hans de Goede wrote:
> Hi All,
> 
> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
> + one other small bugfix.
> 
> Patches 1-3 should arguably have a:
> 
> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> 
> tag, but Fixes tags tend to lead to patches getting automatically added
> to the stable series and I would prefer to see this series get some
> significant testing time in mainline first, so I have chosen to omit
> the tag.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (4):
>    leds: Change led_trigger_blink[_oneshot]() delay parameters to
>      pass-by-value
>    leds: Fix set_brightness_delayed() race
>    leds: Fix oops about sleeping in led_trigger_blink()
>    leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger
> 
>   drivers/leds/led-core.c                  | 81 ++++++++++++++++++++----
>   drivers/leds/led-triggers.c              | 17 ++---
>   drivers/leds/trigger/ledtrig-disk.c      |  9 +--
>   drivers/leds/trigger/ledtrig-mtd.c       |  8 +--
>   drivers/net/arcnet/arcnet.c              |  8 +--
>   drivers/power/supply/power_supply_leds.c |  5 +-
>   drivers/usb/common/led.c                 |  4 +-
>   include/linux/leds.h                     | 43 ++++++++++---
>   net/mac80211/led.c                       |  2 +-
>   net/mac80211/led.h                       |  8 +--
>   net/netfilter/xt_LED.c                   |  3 +-
>   11 files changed, 125 insertions(+), 63 deletions(-)
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Lee Jones]

On Wed, 12 Apr 2023, Hans de Goede wrote:

> Hi All,
> 
> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
> + one other small bugfix.
> 
> Patches 1-3 should arguably have a:
> 
> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> 
> tag, but Fixes tags tend to lead to patches getting automatically added
> to the stable series and I would prefer to see this series get some
> significant testing time in mainline first, so I have chosen to omit
> the tag.

With subjects with the word "fix" in it, they will be hoovered up by the
Stable auto-picker anyway.

> Hans de Goede (4):
>   leds: Change led_trigger_blink[_oneshot]() delay parameters to
>     pass-by-value
>   leds: Fix set_brightness_delayed() race
>   leds: Fix oops about sleeping in led_trigger_blink()
>   leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger
> 
>  drivers/leds/led-core.c                  | 81 ++++++++++++++++++++----
>  drivers/leds/led-triggers.c              | 17 ++---
>  drivers/leds/trigger/ledtrig-disk.c      |  9 +--
>  drivers/leds/trigger/ledtrig-mtd.c       |  8 +--
>  drivers/net/arcnet/arcnet.c              |  8 +--
>  drivers/power/supply/power_supply_leds.c |  5 +-
>  drivers/usb/common/led.c                 |  4 +-
>  include/linux/leds.h                     | 43 ++++++++++---
>  net/mac80211/led.c                       |  2 +-
>  net/mac80211/led.h                       |  8 +--
>  net/netfilter/xt_LED.c                   |  3 +-
>  11 files changed, 125 insertions(+), 63 deletions(-)
> 
> -- 
> 2.39.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Hans de Goede]

Hi Lee,

On 4/20/23 13:36, Lee Jones wrote:
> On Wed, 12 Apr 2023, Hans de Goede wrote:
> 
>> Hi All,
>>
>> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
>> + one other small bugfix.
>>
>> Patches 1-3 should arguably have a:
>>
>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
>>
>> tag, but Fixes tags tend to lead to patches getting automatically added
>> to the stable series and I would prefer to see this series get some
>> significant testing time in mainline first, so I have chosen to omit
>> the tag.
> 
> With subjects with the word "fix" in it, they will be hoovered up by the
> Stable auto-picker anyway.

Ok, in that case patch 3 should have:

Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")

Patches 1-2 are more preparation patches for this. Patch 2 does
fix another race, but I'm not sure we ever hit that.

Can you add the fixes tag while merging these, or do you
want a v2 of this series ?

Regards,

Hans






> 
>> Hans de Goede (4):
>>   leds: Change led_trigger_blink[_oneshot]() delay parameters to
>>     pass-by-value
>>   leds: Fix set_brightness_delayed() race
>>   leds: Fix oops about sleeping in led_trigger_blink()
>>   leds: Clear LED_INIT_DEFAULT_TRIGGER when clearing current trigger
>>
>>  drivers/leds/led-core.c                  | 81 ++++++++++++++++++++----
>>  drivers/leds/led-triggers.c              | 17 ++---
>>  drivers/leds/trigger/ledtrig-disk.c      |  9 +--
>>  drivers/leds/trigger/ledtrig-mtd.c       |  8 +--
>>  drivers/net/arcnet/arcnet.c              |  8 +--
>>  drivers/power/supply/power_supply_leds.c |  5 +-
>>  drivers/usb/common/led.c                 |  4 +-
>>  include/linux/leds.h                     | 43 ++++++++++---
>>  net/mac80211/led.c                       |  2 +-
>>  net/mac80211/led.h                       |  8 +--
>>  net/netfilter/xt_LED.c                   |  3 +-
>>  11 files changed, 125 insertions(+), 63 deletions(-)
>>
>> -- 
>> 2.39.1
>>
> 

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Lee Jones]

On Thu, 20 Apr 2023, Hans de Goede wrote:

> Hi Lee,
> 
> On 4/20/23 13:36, Lee Jones wrote:
> > On Wed, 12 Apr 2023, Hans de Goede wrote:
> > 
> >> Hi All,
> >>
> >> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
> >> + one other small bugfix.
> >>
> >> Patches 1-3 should arguably have a:
> >>
> >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> >>
> >> tag, but Fixes tags tend to lead to patches getting automatically added
> >> to the stable series and I would prefer to see this series get some
> >> significant testing time in mainline first, so I have chosen to omit
> >> the tag.
> > 
> > With subjects with the word "fix" in it, they will be hoovered up by the
> > Stable auto-picker anyway.
> 
> Ok, in that case patch 3 should have:
> 
> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> 
> Patches 1-2 are more preparation patches for this. Patch 2 does
> fix another race, but I'm not sure we ever hit that.
> 
> Can you add the fixes tag while merging these, or do you
> want a v2 of this series ?

I'm holding out for either a Pavel review or some Tested-by's suggested
by Jacek.

-- 
Lee Jones [李琼斯]

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Hans de Goede]

Hi Lee,

On 4/20/23 15:56, Lee Jones wrote:
> On Thu, 20 Apr 2023, Hans de Goede wrote:
> 
>> Hi Lee,
>>
>> On 4/20/23 13:36, Lee Jones wrote:
>>> On Wed, 12 Apr 2023, Hans de Goede wrote:
>>>
>>>> Hi All,
>>>>
>>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
>>>> + one other small bugfix.
>>>>
>>>> Patches 1-3 should arguably have a:
>>>>
>>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
>>>>
>>>> tag, but Fixes tags tend to lead to patches getting automatically added
>>>> to the stable series and I would prefer to see this series get some
>>>> significant testing time in mainline first, so I have chosen to omit
>>>> the tag.
>>>
>>> With subjects with the word "fix" in it, they will be hoovered up by the
>>> Stable auto-picker anyway.
>>
>> Ok, in that case patch 3 should have:
>>
>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
>>
>> Patches 1-2 are more preparation patches for this. Patch 2 does
>> fix another race, but I'm not sure we ever hit that.
>>
>> Can you add the fixes tag while merging these, or do you
>> want a v2 of this series ?
> 
> I'm holding out for either a Pavel review or some Tested-by's suggested
> by Jacek.

Hmm, ok. I have asked Yauhen to give this a test since they have hit
the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove
driver too.

But Yauhen has the same hw as me (I have already tested this on
3 different laptop models).

Note that Jacek did already give his Reviewed-by:

Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

I think the bug this fixes was never an issue before because only
very few triggers use regular blinking (rather then one-shot
blinking which always uses the sw-blink implementation).

To hit this you need to use one of the few triggers which
actually use regular-blinking in combination with a
driver which supports hw-blinking and where its blink_set
callbavck may sleep. It looks to me like no-one has hit
this combination before. Which is why there are no bug reports
for the issue and which also is why finding testers is going
to be tricky.

I think that the best thing to do here is add this series to -next
early in the upcoming cycle, so that it gets the maximum testing
time possible in -next.

Regards,

Hans

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Lee Jones]

On Tue, 25 Apr 2023, Hans de Goede wrote:

> Hi Lee,
> 
> On 4/20/23 15:56, Lee Jones wrote:
> > On Thu, 20 Apr 2023, Hans de Goede wrote:
> > 
> >> Hi Lee,
> >>
> >> On 4/20/23 13:36, Lee Jones wrote:
> >>> On Wed, 12 Apr 2023, Hans de Goede wrote:
> >>>
> >>>> Hi All,
> >>>>
> >>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
> >>>> + one other small bugfix.
> >>>>
> >>>> Patches 1-3 should arguably have a:
> >>>>
> >>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> >>>>
> >>>> tag, but Fixes tags tend to lead to patches getting automatically added
> >>>> to the stable series and I would prefer to see this series get some
> >>>> significant testing time in mainline first, so I have chosen to omit
> >>>> the tag.
> >>>
> >>> With subjects with the word "fix" in it, they will be hoovered up by the
> >>> Stable auto-picker anyway.
> >>
> >> Ok, in that case patch 3 should have:
> >>
> >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> >>
> >> Patches 1-2 are more preparation patches for this. Patch 2 does
> >> fix another race, but I'm not sure we ever hit that.
> >>
> >> Can you add the fixes tag while merging these, or do you
> >> want a v2 of this series ?
> > 
> > I'm holding out for either a Pavel review or some Tested-by's suggested
> > by Jacek.
> 
> Hmm, ok. I have asked Yauhen to give this a test since they have hit
> the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove
> driver too.
> 
> But Yauhen has the same hw as me (I have already tested this on
> 3 different laptop models).
> 
> Note that Jacek did already give his Reviewed-by:
> 
> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> 
> I think the bug this fixes was never an issue before because only
> very few triggers use regular blinking (rather then one-shot
> blinking which always uses the sw-blink implementation).
> 
> To hit this you need to use one of the few triggers which
> actually use regular-blinking in combination with a
> driver which supports hw-blinking and where its blink_set
> callbavck may sleep. It looks to me like no-one has hit
> this combination before. Which is why there are no bug reports
> for the issue and which also is why finding testers is going
> to be tricky.
> 
> I think that the best thing to do here is add this series to -next
> early in the upcoming cycle, so that it gets the maximum testing
> time possible in -next.

Agree.  Let's revisit this once the merge-window closes.

-- 
Lee Jones [李琼斯]

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Yauhen Kharuzhy]

On Wed, Apr 26, 2023 at 03:34:39PM +0100, Lee Jones wrote:
> On Tue, 25 Apr 2023, Hans de Goede wrote:
> 
> > Hi Lee,
> > 
> > On 4/20/23 15:56, Lee Jones wrote:
> > > On Thu, 20 Apr 2023, Hans de Goede wrote:
> > > 
> > >> Hi Lee,
> > >>
> > >> On 4/20/23 13:36, Lee Jones wrote:
> > >>> On Wed, 12 Apr 2023, Hans de Goede wrote:
> > >>>
> > >>>> Hi All,
> > >>>>
> > >>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
> > >>>> + one other small bugfix.
> > >>>>
> > >>>> Patches 1-3 should arguably have a:
> > >>>>
> > >>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> > >>>>
> > >>>> tag, but Fixes tags tend to lead to patches getting automatically added
> > >>>> to the stable series and I would prefer to see this series get some
> > >>>> significant testing time in mainline first, so I have chosen to omit
> > >>>> the tag.
> > >>>
> > >>> With subjects with the word "fix" in it, they will be hoovered up by the
> > >>> Stable auto-picker anyway.
> > >>
> > >> Ok, in that case patch 3 should have:
> > >>
> > >> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
> > >>
> > >> Patches 1-2 are more preparation patches for this. Patch 2 does
> > >> fix another race, but I'm not sure we ever hit that.
> > >>
> > >> Can you add the fixes tag while merging these, or do you
> > >> want a v2 of this series ?
> > > 
> > > I'm holding out for either a Pavel review or some Tested-by's suggested
> > > by Jacek.
> > 
> > Hmm, ok. I have asked Yauhen to give this a test since they have hit
> > the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove
> > driver too.
> > 
> > But Yauhen has the same hw as me (I have already tested this on
> > 3 different laptop models).
> > 
> > Note that Jacek did already give his Reviewed-by:
> > 
> > Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > 
> > I think the bug this fixes was never an issue before because only
> > very few triggers use regular blinking (rather then one-shot
> > blinking which always uses the sw-blink implementation).
> > 
> > To hit this you need to use one of the few triggers which
> > actually use regular-blinking in combination with a
> > driver which supports hw-blinking and where its blink_set
> > callbavck may sleep. It looks to me like no-one has hit
> > this combination before. Which is why there are no bug reports
> > for the issue and which also is why finding testers is going
> > to be tricky.
> > 
> > I think that the best thing to do here is add this series to -next
> > early in the upcoming cycle, so that it gets the maximum testing
> > time possible in -next.
> 
> Agree.  Let's revisit this once the merge-window closes.

Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>

For entire series.


-- 
Yauhen Kharuzhy

Re: [PATCH 0/4] Fix oops about sleeping in led_trigger_blink()

[Hans de Goede]

Hi Lee,

On 4/26/23 16:34, Lee Jones wrote:
> On Tue, 25 Apr 2023, Hans de Goede wrote:
> 
>> Hi Lee,
>>
>> On 4/20/23 15:56, Lee Jones wrote:
>>> On Thu, 20 Apr 2023, Hans de Goede wrote:
>>>
>>>> Hi Lee,
>>>>
>>>> On 4/20/23 13:36, Lee Jones wrote:
>>>>> On Wed, 12 Apr 2023, Hans de Goede wrote:
>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>> Here is a patch series to fix an oops about sleeping in led_trigger_blink()
>>>>>> + one other small bugfix.
>>>>>>
>>>>>> Patches 1-3 should arguably have a:
>>>>>>
>>>>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
>>>>>>
>>>>>> tag, but Fixes tags tend to lead to patches getting automatically added
>>>>>> to the stable series and I would prefer to see this series get some
>>>>>> significant testing time in mainline first, so I have chosen to omit
>>>>>> the tag.
>>>>>
>>>>> With subjects with the word "fix" in it, they will be hoovered up by the
>>>>> Stable auto-picker anyway.
>>>>
>>>> Ok, in that case patch 3 should have:
>>>>
>>>> Fixes: 0b9536c95709 ("leds: Add ability to blink via simple trigger")
>>>>
>>>> Patches 1-2 are more preparation patches for this. Patch 2 does
>>>> fix another race, but I'm not sure we ever hit that.
>>>>
>>>> Can you add the fixes tag while merging these, or do you
>>>> want a v2 of this series ?
>>>
>>> I'm holding out for either a Pavel review or some Tested-by's suggested
>>> by Jacek.
>>
>> Hmm, ok. I have asked Yauhen to give this a test since they have hit
>> the oops/backtrace fixed by path 3/4 while testing the new leds-cht-wcove
>> driver too.
>>
>> But Yauhen has the same hw as me (I have already tested this on
>> 3 different laptop models).
>>
>> Note that Jacek did already give his Reviewed-by:
>>
>> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>>
>> I think the bug this fixes was never an issue before because only
>> very few triggers use regular blinking (rather then one-shot
>> blinking which always uses the sw-blink implementation).
>>
>> To hit this you need to use one of the few triggers which
>> actually use regular-blinking in combination with a
>> driver which supports hw-blinking and where its blink_set
>> callbavck may sleep. It looks to me like no-one has hit
>> this combination before. Which is why there are no bug reports
>> for the issue and which also is why finding testers is going
>> to be tricky.
>>
>> I think that the best thing to do here is add this series to -next
>> early in the upcoming cycle, so that it gets the maximum testing
>> time possible in -next.
> 
> Agree.  Let's revisit this once the merge-window closes.

The merge-window has closed now, and Yauhen has given their tested-by:

Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>

Lee, can you merge this now please so that this gets the maximum
possible time for testing in linux-next ?

Regards,

Hans

Re: [PATCH 1/4] leds: Change led_trigger_blink[_oneshot]() delay parameters to pass-by-value

[Lee Jones]

On Wed, 12 Apr 2023, Hans de Goede wrote:

> led_blink_set[_oneshot]()'s delay_on and delay_off function parameters
> are pass by reference, so that hw-blink implementations can report
> back the actual achieved delays when the values have been rounded
> to something the hw supports.
> 
> This is really only interesting for the sysfs API / the timer trigger.
> Other triggers don't really care about this and none of the callers of
> led_trigger_blink[_oneshot]() do anything with the returned delay values.
> 
> Change the led_trigger_blink[_oneshot]() delay parameters to pass-by-value,
> there are 2 reasons for this:
> 
> 1. led_cdev->blink_set() may sleep, while led_trigger_blink() may not.
> So on hw where led_cdev->blink_set() sleeps the call needs to be deferred
> to a workqueue, in which case the actual achieved delays are unknown
> (this is a preparation patch for the deferring).
> 
> 2. Since the callers don't care about the actual achieved delays, allowing
> callers to directly pass a value leads to simpler code for most callers.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/leds/led-triggers.c              | 16 ++++++++--------
>  drivers/leds/trigger/ledtrig-disk.c      |  9 +++------
>  drivers/leds/trigger/ledtrig-mtd.c       |  8 ++------
>  drivers/net/arcnet/arcnet.c              |  8 ++------
>  drivers/power/supply/power_supply_leds.c |  5 +----
>  drivers/usb/common/led.c                 |  4 +---
>  include/linux/leds.h                     | 16 ++++++++--------
>  net/mac80211/led.c                       |  2 +-
>  net/mac80211/led.h                       |  8 ++------
>  net/netfilter/xt_LED.c                   |  3 +--
>  10 files changed, 29 insertions(+), 50 deletions(-)

Just came to apply this and realised that you have touched other
subsystems without Cc'ing the appropriate maintainers.

Please can you [RESEND] this set and include them please?

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/4] leds: Change led_trigger_blink[_oneshot]() delay parameters to pass-by-value

[Hans de Goede]

Hi,

On 5/10/23 13:57, Lee Jones wrote:
> On Wed, 12 Apr 2023, Hans de Goede wrote:
> 
>> led_blink_set[_oneshot]()'s delay_on and delay_off function parameters
>> are pass by reference, so that hw-blink implementations can report
>> back the actual achieved delays when the values have been rounded
>> to something the hw supports.
>>
>> This is really only interesting for the sysfs API / the timer trigger.
>> Other triggers don't really care about this and none of the callers of
>> led_trigger_blink[_oneshot]() do anything with the returned delay values.
>>
>> Change the led_trigger_blink[_oneshot]() delay parameters to pass-by-value,
>> there are 2 reasons for this:
>>
>> 1. led_cdev->blink_set() may sleep, while led_trigger_blink() may not.
>> So on hw where led_cdev->blink_set() sleeps the call needs to be deferred
>> to a workqueue, in which case the actual achieved delays are unknown
>> (this is a preparation patch for the deferring).
>>
>> 2. Since the callers don't care about the actual achieved delays, allowing
>> callers to directly pass a value leads to simpler code for most callers.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/leds/led-triggers.c              | 16 ++++++++--------
>>  drivers/leds/trigger/ledtrig-disk.c      |  9 +++------
>>  drivers/leds/trigger/ledtrig-mtd.c       |  8 ++------
>>  drivers/net/arcnet/arcnet.c              |  8 ++------
>>  drivers/power/supply/power_supply_leds.c |  5 +----
>>  drivers/usb/common/led.c                 |  4 +---
>>  include/linux/leds.h                     | 16 ++++++++--------
>>  net/mac80211/led.c                       |  2 +-
>>  net/mac80211/led.h                       |  8 ++------
>>  net/netfilter/xt_LED.c                   |  3 +--
>>  10 files changed, 29 insertions(+), 50 deletions(-)
> 
> Just came to apply this and realised that you have touched other
> subsystems without Cc'ing the appropriate maintainers.

Good point, sorry about that.

> Please can you [RESEND] this set and include them please?

Ack, I'm rebasing them on 6.4-rc1, checking no new
users of led_trigger_blink() / led_trigger_blink_oneshot()
(which this set changes the prototype of) have shown up
and then I'll do a resend.

(if I have to make any changes other then the plain rebase
I'll make it a version 2 set instead.)

Regards,

Hans