diff mbox series

[RFC,leds,+,net-next,4/7] leds: trigger: netdev: support HW offloading

Message ID 20201030114435.20169-5-kabel@kernel.org
State New
Headers show
Series netdev trigger offloading and LEDs on Marvell PHYs | expand

Commit Message

Marek Behún Oct. 30, 2020, 11:44 a.m. UTC 
Add support for HW offloading of the netdev trigger.

We need to change spinlock to mutex, because if spinlock is used, the
trigger_offload() method cannot sleep, which can happen for ethernet
PHYs.

We can change the spinlock to mutex because, according to Jacek:
  register_netdevice_notifier() registers raw notifier chain,
  whose callbacks are not called from atomic context and there are
  no restrictions on callbacks. See include/linux/notifier.h.

Move struct led_trigger_data into global include directory, into file
linux/ledtrig.h, so that drivers wanting to offload the trigger can
access its settings.

Also export the netdev_led_trigger variable and declare it in ledtrih.h
so that drivers may check whether the LED is set to this trigger.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 48 ++++++++++-----------------
 include/linux/ledtrig.h               | 40 ++++++++++++++++++++++
 2 files changed, 57 insertions(+), 31 deletions(-)
 create mode 100644 include/linux/ledtrig.h

Comments

Marek Behún Nov. 5, 2020, 2:44 p.m. UTC | #1 
On Fri, 30 Oct 2020 12:44:32 +0100
Marek Behún <kabel@kernel.org> wrote:

> +	if (!led_trigger_offload(led_cdev))
> +		return;
> +

The offload method has to read the settings, so this code (better: the
whole set_baseline_state function) has to be always called with the
mutex locked. (I am talking about the mutex which was a spinlock before
this patch.)

I will fix this in next version.

Marek
Pavel Machek March 1, 2021, 10:44 a.m. UTC | #2 
Hi!

> Add support for HW offloading of the netdev trigger.
> 
> We need to change spinlock to mutex, because if spinlock is used, the
> trigger_offload() method cannot sleep, which can happen for ethernet
> PHYs.

Is that bugfix or just needed for offloading? Should be separate patch
in any case.

> Move struct led_trigger_data into global include directory, into file
> linux/ledtrig.h, so that drivers wanting to offload the trigger can
> access its settings.

Separate...

> @@ -327,12 +310,14 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  	case NETDEV_CHANGE:
>  		if (netif_carrier_ok(dev))
>  			trigger_data->linkup = 1;
> +		reset = !trigger_data->led_cdev->offloaded;
>  		break;
>  	}
>  
> -	set_baseline_state(trigger_data);
> +	if (reset)
> +		set_baseline_state(trigger_data);
>  
> -	spin_unlock_bh(&trigger_data->lock);
> +	mutex_unlock(&trigger_data->lock);
>  
>  	return NOTIFY_DONE;
>  }

Is this the only thing it saves? Because.. that would not be worth
it.

Best regards,
									Pavel
diff mbox series

Patch

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 8f013b6df4fa..d4fa031d3a9e 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -10,17 +10,16 @@ 
 //  Copyright 2005-2006 Openedhand Ltd.
 //  Author: Richard Purdie <rpurdie@openedhand.com>
 
-#include <linux/atomic.h>
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
+#include <linux/ledtrig.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
 
@@ -36,26 +35,6 @@ 
  *
  */
 
-struct led_netdev_data {
-	spinlock_t lock;
-
-	struct delayed_work work;
-	struct notifier_block notifier;
-
-	struct led_classdev *led_cdev;
-	struct net_device *net_dev;
-
-	char device_name[IFNAMSIZ];
-	atomic_t interval;
-	unsigned int last_activity;
-
-	unsigned link:1;
-	unsigned tx:1;
-	unsigned rx:1;
-
-	unsigned linkup:1;
-};
-
 enum netdev_led_attr {
 	NETDEV_ATTR_LINK,
 	NETDEV_ATTR_TX,
@@ -73,6 +52,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 (!led_trigger_offload(led_cdev))
+		return;
+
 	if (!trigger_data->linkup)
 		led_set_brightness(led_cdev, LED_OFF);
 	else {
@@ -96,9 +78,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;
 }
@@ -114,7 +96,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 +120,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;
 }
@@ -295,6 +277,7 @@  static int netdev_trig_notify(struct notifier_block *nb,
 		netdev_notifier_info_to_dev((struct netdev_notifier_info *)dv);
 	struct led_netdev_data *trigger_data =
 		container_of(nb, struct led_netdev_data, notifier);
+	bool reset = true;
 
 	if (evt != NETDEV_UP && evt != NETDEV_DOWN && evt != NETDEV_CHANGE
 	    && evt != NETDEV_REGISTER && evt != NETDEV_UNREGISTER
@@ -308,7 +291,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->linkup = 0;
 	switch (evt) {
@@ -327,12 +310,14 @@  static int netdev_trig_notify(struct notifier_block *nb,
 	case NETDEV_CHANGE:
 		if (netif_carrier_ok(dev))
 			trigger_data->linkup = 1;
+		reset = !trigger_data->led_cdev->offloaded;
 		break;
 	}
 
-	set_baseline_state(trigger_data);
+	if (reset)
+		set_baseline_state(trigger_data);
 
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return NOTIFY_DONE;
 }
@@ -389,7 +374,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;
@@ -423,12 +408,13 @@  static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 	kfree(trigger_data);
 }
 
-static struct led_trigger netdev_led_trigger = {
+struct led_trigger netdev_led_trigger = {
 	.name = "netdev",
 	.activate = netdev_trig_activate,
 	.deactivate = netdev_trig_deactivate,
 	.groups = netdev_trig_groups,
 };
+EXPORT_SYMBOL_GPL(netdev_led_trigger);
 
 static int __init netdev_trig_init(void)
 {
diff --git a/include/linux/ledtrig.h b/include/linux/ledtrig.h
new file mode 100644
index 000000000000..593b2bee6ca0
--- /dev/null
+++ b/include/linux/ledtrig.h
@@ -0,0 +1,40 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * LED trigger shared structures
+ */
+
+#ifndef __LINUX_LEDTRIG_H__
+#define __LINUX_LEDTRIG_H__
+
+#include <linux/atomic.h>
+#include <linux/leds.h>
+#include <linux/mutex.h>
+#include <linux/netdevice.h>
+
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV)
+
+struct led_netdev_data {
+	struct mutex lock;
+
+	struct delayed_work work;
+	struct notifier_block notifier;
+
+	struct led_classdev *led_cdev;
+	struct net_device *net_dev;
+
+	char device_name[IFNAMSIZ];
+	atomic_t interval;
+	unsigned int last_activity;
+
+	unsigned link:1;
+	unsigned tx:1;
+	unsigned rx:1;
+
+	unsigned linkup:1;
+};
+
+extern struct led_trigger netdev_led_trigger;
+
+#endif /* IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV) */
+
+#endif /* __LINUX_LEDTRIG_H__ */