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

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

[Hans de Goede]

Hi All,

This is a resend with al the subsys maintainers of files touched
by the small led_trigger_blink() / led_trigger_blink_oneshot()
API/prototype change in the first patch in this series.

ARCNet, power-supply, USB, MAC80211 and netfilter maintainers,
may we please have your ack for merging patch 1/4 through Lee's
LED tree: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/
?

Orginal cover letter:

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

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

[PATCH RESEND 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.

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-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 c39bbf17a25b..c3dc22d184e2 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -433,11 +433,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);
@@ -487,11 +487,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.40.1

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

[Sebastian Reichel]

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

Hi,

On Wed, May 10, 2023 at 06:22:31PM +0200, 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.
> 
> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> ---
>  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 c39bbf17a25b..c3dc22d184e2 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -433,11 +433,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);
> @@ -487,11 +487,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.40.1
> 

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

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

[Florian Westphal]

Hans de Goede <hdegoede@redhat.com> 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.

For netfilter:

Acked-by: Florian Westphal <fw@strlen.de>

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

[Greg Kroah-Hartman]

On Wed, May 10, 2023 at 06:22:31PM +0200, 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.
> 
> Reviewed-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Tested-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Lee Jones]

On Wed, 10 May 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.
> 
> 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-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(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH RESEND 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().

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 | 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 c3dc22d184e2..de813fe96a20 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -124,6 +124,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.40.1

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

[Lee Jones]

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

> 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().
> 
> 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 | 57 +++++++++++++++++++++++++++++++----------
>  include/linux/leds.h    |  3 +++
>  2 files changed, 47 insertions(+), 13 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH RESEND 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.

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

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 de813fe96a20..50b2f8f153fb 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -127,6 +127,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
@@ -150,6 +151,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,
@@ -173,6 +178,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 */
@@ -275,12 +282,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_set_brightness() 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
@@ -294,6 +316,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.40.1

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

[Lee Jones]

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 [李琼斯]

[PATCH RESEND 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.

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

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

[Lee Jones]

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

> 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.
> 
> 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-triggers.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks

-- 
Lee Jones [李琼斯]