[PATCH 0/5] leds: trigger: netdev: fixup preparation for LEDs hw control

[PATCH 0/5] leds: trigger: netdev: fixup preparation for LEDs hw control

[Christian Marangi]

This is a continue of [1]. It was decided to take a more gradual
approach to implement LEDs support for switch and phy starting with
basic support and then implementing the hw control part when we have all
the prereq done.

This is a small series in preparation for a bigger change adding support
for LEDs hw control.

Some minor fixup and improvements are required to add support for LEDs hw
control for the netdev trigger.

The main fixes are switching to mutex from spinlocks and adding namespace
to netdev trigger enum modes. These will be exposed in the future in the
generic linux include folder so LEDs driver may supports these specific
modes.

While working on htis it was also discovered a bug addressed in the first
patch of this series.

Changes from the old v8 series:
- Reword some commits and add more info on why the changes is needed
- Add fixup patch on dev rename

Christian Marangi (5):
  leds: trigger: netdev: recheck NETDEV_LED_MODE_LINKUP on dev rename
  leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
  leds: trigger: netdev: rename add namespace to netdev trigger enum
    modes
  leds: trigger: netdev: convert device attr to macro
  leds: trigger: netdev: use mutex instead of spinlocks

 drivers/leds/trigger/ledtrig-netdev.c | 151 ++++++++++----------------
 1 file changed, 59 insertions(+), 92 deletions(-)

-- 
2.39.2

[PATCH 1/5] leds: trigger: netdev: recheck NETDEV_LED_MODE_LINKUP on dev rename

[Christian Marangi]

Dev can be renamed also while up for supported device. We currently
wrongly clear the NETDEV_LED_MODE_LINKUP flag on NETDEV_CHANGENAME
event.

Fix this by rechecking if the carrier is ok on NETDEV_CHANGENAME and
correctly set the NETDEV_LED_MODE_LINKUP bit.

Fixes: 5f820ed52371 ("leds: trigger: netdev: fix handling on interface rename")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Cc: stable@vger.kernel.org # v5.5+
---
 drivers/leds/trigger/ledtrig-netdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..f4d670ec30bc 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -318,6 +318,9 @@ static int netdev_trig_notify(struct notifier_block *nb,
 	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
 	switch (evt) {
 	case NETDEV_CHANGENAME:
+		if (netif_carrier_ok(dev))
+			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+		fallthrough;
 	case NETDEV_REGISTER:
 		if (trigger_data->net_dev)
 			dev_put(trigger_data->net_dev);
-- 
2.39.2

Re: [PATCH 1/5] leds: trigger: netdev: recheck NETDEV_LED_MODE_LINKUP on dev rename

[Andrew Lunn]

On Wed, Apr 19, 2023 at 11:07:39PM +0200, Christian Marangi wrote:
> Dev can be renamed also while up for supported device. We currently
> wrongly clear the NETDEV_LED_MODE_LINKUP flag on NETDEV_CHANGENAME
> event.
> 
> Fix this by rechecking if the carrier is ok on NETDEV_CHANGENAME and
> correctly set the NETDEV_LED_MODE_LINKUP bit.
> 
> Fixes: 5f820ed52371 ("leds: trigger: netdev: fix handling on interface rename")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org # v5.5+

Since this is a fix, it should be posted on its own. It should then
get merged faster and backported to stable.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 1/5] leds: trigger: netdev: recheck NETDEV_LED_MODE_LINKUP on dev rename

[Lee Jones]

On Wed, 19 Apr 2023, Christian Marangi wrote:

> Dev can be renamed also while up for supported device. We currently
> wrongly clear the NETDEV_LED_MODE_LINKUP flag on NETDEV_CHANGENAME
> event.
> 
> Fix this by rechecking if the carrier is ok on NETDEV_CHANGENAME and
> correctly set the NETDEV_LED_MODE_LINKUP bit.
> 
> Fixes: 5f820ed52371 ("leds: trigger: netdev: fix handling on interface rename")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Cc: stable@vger.kernel.org # v5.5+
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH 2/5] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Christian Marangi]

Putting NETDEV_LED_MODE_LINKUP in the same list of the netdev trigger
modes is wrong as it's used to set the link state of the device and not
to set a blink mode as it's done by NETDEV_LED_LINK, NETDEV_LED_TX and
NETDEV_LED_RX. It's also wrong to put this state in the same bitmap of the
netdev trigger mode and should be external to it.

Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
that will be true or false based on the carrier link. No functional
change intended.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index f4d670ec30bc..d5c4e72b8261 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -50,10 +50,10 @@ struct led_netdev_data {
 	unsigned int last_activity;
 
 	unsigned long mode;
+	bool carrier_link_up;
 #define NETDEV_LED_LINK	0
 #define NETDEV_LED_TX	1
 #define NETDEV_LED_RX	2
-#define NETDEV_LED_MODE_LINKUP	3
 };
 
 enum netdev_led_attr {
@@ -73,9 +73,9 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!led_cdev->blink_brightness)
 		led_cdev->blink_brightness = led_cdev->max_brightness;
 
-	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
+	if (!trigger_data->carrier_link_up) {
 		led_set_brightness(led_cdev, LED_OFF);
-	else {
+	} else {
 		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);
@@ -131,10 +131,9 @@ static ssize_t device_name_store(struct device *dev,
 		trigger_data->net_dev =
 		    dev_get_by_name(&init_net, trigger_data->device_name);
 
-	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	trigger_data->carrier_link_up = false;
 	if (trigger_data->net_dev != NULL)
-		if (netif_carrier_ok(trigger_data->net_dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+		trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
 
 	trigger_data->last_activity = 0;
 
@@ -315,11 +314,10 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	spin_lock_bh(&trigger_data->lock);
 
-	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+	trigger_data->carrier_link_up = false;
 	switch (evt) {
 	case NETDEV_CHANGENAME:
-		if (netif_carrier_ok(dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+		trigger_data->carrier_link_up = netif_carrier_ok(dev);
 		fallthrough;
 	case NETDEV_REGISTER:
 		if (trigger_data->net_dev)
@@ -333,8 +331,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 		break;
 	case NETDEV_UP:
 	case NETDEV_CHANGE:
-		if (netif_carrier_ok(dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+		trigger_data->carrier_link_up = netif_carrier_ok(dev);
 		break;
 	}
 
-- 
2.39.2

Re: [PATCH 2/5] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Andrew Lunn]

On Wed, Apr 19, 2023 at 11:07:40PM +0200, Christian Marangi wrote:
> Putting NETDEV_LED_MODE_LINKUP in the same list of the netdev trigger
> modes is wrong as it's used to set the link state of the device and not
> to set a blink mode as it's done by NETDEV_LED_LINK, NETDEV_LED_TX and
> NETDEV_LED_RX. It's also wrong to put this state in the same bitmap of the
> netdev trigger mode and should be external to it.
> 
> Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
> that will be true or false based on the carrier link. No functional
> change intended.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 2/5] leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode

[Lee Jones]

On Wed, 19 Apr 2023, Christian Marangi wrote:

> Putting NETDEV_LED_MODE_LINKUP in the same list of the netdev trigger
> modes is wrong as it's used to set the link state of the device and not
> to set a blink mode as it's done by NETDEV_LED_LINK, NETDEV_LED_TX and
> NETDEV_LED_RX. It's also wrong to put this state in the same bitmap of the
> netdev trigger mode and should be external to it.
> 
> Drop NETDEV_LED_MODE_LINKUP from mode list and convert to a simple bool
> that will be true or false based on the carrier link. No functional
> change intended.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH 3/5] leds: trigger: netdev: rename add namespace to netdev trigger enum modes

[Christian Marangi]

Rename NETDEV trigger enum modes to a more symbolic name and add a
namespace to them.

Also add __TRIGGER_NETDEV_MAX to identify the max modes of the netdev
trigger.

This is a cleanup to drop the define and no behaviour change are
intended.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 58 ++++++++++++---------------
 1 file changed, 25 insertions(+), 33 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5c4e72b8261..0d4649e7a84d 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -51,15 +51,15 @@ struct led_netdev_data {
 
 	unsigned long mode;
 	bool carrier_link_up;
-#define NETDEV_LED_LINK	0
-#define NETDEV_LED_TX	1
-#define NETDEV_LED_RX	2
 };
 
-enum netdev_led_attr {
-	NETDEV_ATTR_LINK,
-	NETDEV_ATTR_TX,
-	NETDEV_ATTR_RX
+enum led_trigger_netdev_modes {
+	TRIGGER_NETDEV_LINK = 0,
+	TRIGGER_NETDEV_TX,
+	TRIGGER_NETDEV_RX,
+
+	/* keep last */
+	__TRIGGER_NETDEV_MAX,
 };
 
 static void set_baseline_state(struct led_netdev_data *trigger_data)
@@ -76,7 +76,7 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!trigger_data->carrier_link_up) {
 		led_set_brightness(led_cdev, LED_OFF);
 	} else {
-		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+		if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);
 		else
@@ -85,8 +85,8 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 		/* If we are looking for RX/TX start periodically
 		 * checking stats
 		 */
-		if (test_bit(NETDEV_LED_TX, &trigger_data->mode) ||
-		    test_bit(NETDEV_LED_RX, &trigger_data->mode))
+		if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
+		    test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
 			schedule_delayed_work(&trigger_data->work, 0);
 	}
 }
@@ -146,20 +146,16 @@ static ssize_t device_name_store(struct device *dev,
 static DEVICE_ATTR_RW(device_name);
 
 static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
-	enum netdev_led_attr attr)
+				    enum led_trigger_netdev_modes attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	int bit;
 
 	switch (attr) {
-	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
-		break;
-	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
-		break;
-	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+	case TRIGGER_NETDEV_LINK:
+	case TRIGGER_NETDEV_TX:
+	case TRIGGER_NETDEV_RX:
+		bit = attr;
 		break;
 	default:
 		return -EINVAL;
@@ -169,7 +165,7 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 }
 
 static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
-	size_t size, enum netdev_led_attr attr)
+				     size_t size, enum led_trigger_netdev_modes attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	unsigned long state;
@@ -181,14 +177,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 		return ret;
 
 	switch (attr) {
-	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
-		break;
-	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
-		break;
-	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+	case TRIGGER_NETDEV_LINK:
+	case TRIGGER_NETDEV_TX:
+	case TRIGGER_NETDEV_RX:
+		bit = attr;
 		break;
 	default:
 		return -EINVAL;
@@ -360,21 +352,21 @@ static void netdev_trig_work(struct work_struct *work)
 	}
 
 	/* If we are not looking for RX/TX then return  */
-	if (!test_bit(NETDEV_LED_TX, &trigger_data->mode) &&
-	    !test_bit(NETDEV_LED_RX, &trigger_data->mode))
+	if (!test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) &&
+	    !test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
 		return;
 
 	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
 	new_activity =
-	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
+	    (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ?
 		dev_stats->tx_packets : 0) +
-	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
+	    (test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ?
 		dev_stats->rx_packets : 0);
 
 	if (trigger_data->last_activity != new_activity) {
 		led_stop_software_blink(trigger_data->led_cdev);
 
-		invert = test_bit(NETDEV_LED_LINK, &trigger_data->mode);
+		invert = test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode);
 		interval = jiffies_to_msecs(
 				atomic_read(&trigger_data->interval));
 		/* base state is ON (link present) */
-- 
2.39.2

Re: [PATCH 3/5] leds: trigger: netdev: rename add namespace to netdev trigger enum modes

[Andrew Lunn]

On Wed, Apr 19, 2023 at 11:07:41PM +0200, Christian Marangi wrote:
> Rename NETDEV trigger enum modes to a more symbolic name and add a
> namespace to them.
> 
> Also add __TRIGGER_NETDEV_MAX to identify the max modes of the netdev
> trigger.
> 
> This is a cleanup to drop the define and no behaviour change are
> intended.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>  
>  static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
> -	enum netdev_led_attr attr)
> +				    enum led_trigger_netdev_modes attr)
>  {
> @@ -169,7 +165,7 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
>  }
>  
>  static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
> -	size_t size, enum netdev_led_attr attr)
> +				     size_t size, enum led_trigger_netdev_modes attr)

These whitespace changes should not really be in this patch. But...

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 3/5] leds: trigger: netdev: rename add namespace to netdev trigger enum modes

[Lee Jones]

On Wed, 19 Apr 2023, Christian Marangi wrote:

> Rename NETDEV trigger enum modes to a more symbolic name and add a
> namespace to them.
> 
> Also add __TRIGGER_NETDEV_MAX to identify the max modes of the netdev
> trigger.
> 
> This is a cleanup to drop the define and no behaviour change are
> intended.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 58 ++++++++++++---------------
>  1 file changed, 25 insertions(+), 33 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH 4/5] leds: trigger: netdev: convert device attr to macro

[Christian Marangi]

Convert link tx and rx device attr to a common macro to reduce common
code and in preparation for additional attr.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 57 ++++++++-------------------
 1 file changed, 16 insertions(+), 41 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 0d4649e7a84d..5a47913c813c 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -198,47 +198,22 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	return size;
 }
 
-static ssize_t link_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_LINK);
-}
-
-static ssize_t link_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t size)
-{
-	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_LINK);
-}
-
-static DEVICE_ATTR_RW(link);
-
-static ssize_t tx_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_TX);
-}
-
-static ssize_t tx_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t size)
-{
-	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_TX);
-}
-
-static DEVICE_ATTR_RW(tx);
-
-static ssize_t rx_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return netdev_led_attr_show(dev, buf, NETDEV_ATTR_RX);
-}
-
-static ssize_t rx_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t size)
-{
-	return netdev_led_attr_store(dev, buf, size, NETDEV_ATTR_RX);
-}
-
-static DEVICE_ATTR_RW(rx);
+#define DEFINE_NETDEV_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+		struct device_attribute *attr, char *buf) \
+	{ \
+		return netdev_led_attr_show(dev, buf, trigger); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+		struct device_attribute *attr, const char *buf, size_t size) \
+	{ \
+		return netdev_led_attr_store(dev, buf, size, trigger); \
+	} \
+	static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_NETDEV_TRIGGER(link, TRIGGER_NETDEV_LINK);
+DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX);
+DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX);
 
 static ssize_t interval_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
-- 
2.39.2

Re: [PATCH 4/5] leds: trigger: netdev: convert device attr to macro

[Andrew Lunn]

On Wed, Apr 19, 2023 at 11:07:42PM +0200, Christian Marangi wrote:
> Convert link tx and rx device attr to a common macro to reduce common
> code and in preparation for additional attr.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 4/5] leds: trigger: netdev: convert device attr to macro

[Lee Jones]

On Wed, 19 Apr 2023, Christian Marangi wrote:

> Convert link tx and rx device attr to a common macro to reduce common
> code and in preparation for additional attr.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 57 ++++++++-------------------
>  1 file changed, 16 insertions(+), 41 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

[PATCH 5/5] leds: trigger: netdev: use mutex instead of spinlocks

[Christian Marangi]

Some LEDs may require to sleep while doing some operation like setting
brightness and other cleanup.

For this reason, using a spinlock will cause a sleep under spinlock
warning.

It should be safe to convert this to a sleepable lock since:
- sysfs read/write can sleep
- netdev_trig_work is a work queue and can sleep
- netdev _trig_notify can sleep

The spinlock was used when brightness didn't support sleeping, but this
changed and now it supported with brightness_set_blocking().

Convert to mutex lock to permit sleeping using brightness_set_blocking().

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/trigger/ledtrig-netdev.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 5a47913c813c..115f2bae9eee 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,7 +20,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/timer.h>
 #include "../leds.h"
 
@@ -37,7 +37,7 @@
  */
 
 struct led_netdev_data {
-	spinlock_t lock;
+	struct mutex lock;
 
 	struct delayed_work work;
 	struct notifier_block notifier;
@@ -97,9 +97,9 @@ static ssize_t device_name_show(struct device *dev,
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	ssize_t len;
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 	len = sprintf(buf, "%s\n", trigger_data->device_name);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return len;
 }
@@ -115,7 +115,7 @@ static ssize_t device_name_store(struct device *dev,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
@@ -138,7 +138,7 @@ static ssize_t device_name_store(struct device *dev,
 	trigger_data->last_activity = 0;
 
 	set_baseline_state(trigger_data);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return size;
 }
@@ -279,7 +279,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	trigger_data->carrier_link_up = false;
 	switch (evt) {
@@ -304,7 +304,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	set_baseline_state(trigger_data);
 
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return NOTIFY_DONE;
 }
@@ -365,7 +365,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
-	spin_lock_init(&trigger_data->lock);
+	mutex_init(&trigger_data->lock);
 
 	trigger_data->notifier.notifier_call = netdev_trig_notify;
 	trigger_data->notifier.priority = 10;
-- 
2.39.2

Re: [PATCH 5/5] leds: trigger: netdev: use mutex instead of spinlocks

[Andrew Lunn]

On Wed, Apr 19, 2023 at 11:07:43PM +0200, Christian Marangi wrote:
> Some LEDs may require to sleep while doing some operation like setting
> brightness and other cleanup.
> 
> For this reason, using a spinlock will cause a sleep under spinlock
> warning.
> 
> It should be safe to convert this to a sleepable lock since:
> - sysfs read/write can sleep
> - netdev_trig_work is a work queue and can sleep
> - netdev _trig_notify can sleep
> 
> The spinlock was used when brightness didn't support sleeping, but this
> changed and now it supported with brightness_set_blocking().
> 
> Convert to mutex lock to permit sleeping using brightness_set_blocking().
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 5/5] leds: trigger: netdev: use mutex instead of spinlocks

[Lee Jones]

On Wed, 19 Apr 2023, Christian Marangi wrote:

> Some LEDs may require to sleep while doing some operation like setting
> brightness and other cleanup.
> 
> For this reason, using a spinlock will cause a sleep under spinlock
> warning.
> 
> It should be safe to convert this to a sleepable lock since:
> - sysfs read/write can sleep
> - netdev_trig_work is a work queue and can sleep
> - netdev _trig_notify can sleep
> 
> The spinlock was used when brightness didn't support sleeping, but this
> changed and now it supported with brightness_set_blocking().
> 
> Convert to mutex lock to permit sleeping using brightness_set_blocking().
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]