diff mbox series

[RFC,leds,+,net-next,2/7] leds: trigger: netdev: simplify the driver by using bit field members

Message ID 20201030114435.20169-3-kabel@kernel.org (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
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 
Use bit fields members in struct led_netdev_data instead of one mode
member and set_bit/clear_bit/test_bit functions. These functions are
suitable for longer or variable length bit arrays.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 69 ++++++++++++---------------
 1 file changed, 30 insertions(+), 39 deletions(-)

Comments

Jacek Anaszewski Oct. 30, 2020, 10:37 p.m. UTC | #1 
Hi Marek,

Bitops are guaranteed to be atomic and this was done for a reason.

On 10/30/20 12:44 PM, Marek Behún wrote:
> Use bit fields members in struct led_netdev_data instead of one mode
> member and set_bit/clear_bit/test_bit functions. These functions are
> suitable for longer or variable length bit arrays.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>   drivers/leds/trigger/ledtrig-netdev.c | 69 ++++++++++++---------------
>   1 file changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 4f6b73e3b491..8f013b6df4fa 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -49,11 +49,11 @@ struct led_netdev_data {
>   	atomic_t interval;
>   	unsigned int last_activity;
>   
> -	unsigned long mode;
> -#define NETDEV_LED_LINK	0
> -#define NETDEV_LED_TX	1
> -#define NETDEV_LED_RX	2
> -#define NETDEV_LED_MODE_LINKUP	3
> +	unsigned link:1;
> +	unsigned tx:1;
> +	unsigned rx:1;
> +
> +	unsigned linkup:1;
>   };
>   
>   enum netdev_led_attr {
> @@ -73,10 +73,10 @@ 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->linkup)
>   		led_set_brightness(led_cdev, LED_OFF);
[...]
Marek Behún Oct. 30, 2020, 11:45 p.m. UTC | #2 
On Fri, 30 Oct 2020 23:37:52 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Marek,
> 
> Bitops are guaranteed to be atomic and this was done for a reason.

Hmm okay...
Sooo, netdev_trig_work cannot be executed at the same time as the
link/linkup/rx/tx changing stuff from netdev_trig_notify,
interval_store or netdev_led_attr_store, because all these functions
ensure cancelation of netdev_trig_work by calling
cancel_delayed_work_sync. Doesn't this somehow prevent the need for
memory barriers provided by atomic bitops?

BTW Jacek what do you think about the other patches?

Marek
Jacek Anaszewski Nov. 1, 2020, 4:40 p.m. UTC | #3 
On 10/31/20 12:45 AM, Marek Behún wrote:
> On Fri, 30 Oct 2020 23:37:52 +0100
> Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
>> Hi Marek,
>>
>> Bitops are guaranteed to be atomic and this was done for a reason.
> 
> Hmm okay...
> Sooo, netdev_trig_work cannot be executed at the same time as the
> link/linkup/rx/tx changing stuff from netdev_trig_notify,
> interval_store or netdev_led_attr_store, because all these functions
> ensure cancelation of netdev_trig_work by calling
> cancel_delayed_work_sync. Doesn't this somehow prevent the need for
> memory barriers provided by atomic bitops?

That's true. But unless there is proven decline in performance related
to use of bitops, I don't see any gain in removing those from this
trigger. They improve code readability.

> BTW Jacek what do you think about the other patches?

I like the idea, but I'd need to spend more time reviewing it.
One thing coming to mind at first glance - it would be good to get rid
of blink_set op at this occasion since this is just another case of
trigger offloading. It would however need touching many drivers, so
that could possibly be done as a follow-up.
Pavel Machek March 1, 2021, 10:30 a.m. UTC | #4 
On Fri 2020-10-30 12:44:30, Marek Behún wrote:
> Use bit fields members in struct led_netdev_data instead of one mode
> member and set_bit/clear_bit/test_bit functions. These functions are
> suitable for longer or variable length bit arrays.

They also provide atomicity guarantees. If you can explain why this is
safe, we can do this, but it needs _way_ better changelog.

								Pavel

> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 69 ++++++++++++---------------
>  1 file changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 4f6b73e3b491..8f013b6df4fa 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -49,11 +49,11 @@ struct led_netdev_data {
>  	atomic_t interval;
>  	unsigned int last_activity;
>  
> -	unsigned long mode;
> -#define NETDEV_LED_LINK	0
> -#define NETDEV_LED_TX	1
> -#define NETDEV_LED_RX	2
> -#define NETDEV_LED_MODE_LINKUP	3
> +	unsigned link:1;
> +	unsigned tx:1;
> +	unsigned rx:1;
> +
> +	unsigned linkup:1;
>  };
>  
>  enum netdev_led_attr {
> @@ -73,10 +73,10 @@ 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->linkup)
>  		led_set_brightness(led_cdev, LED_OFF);
>  	else {
> -		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
> +		if (trigger_data->link)
>  			led_set_brightness(led_cdev,
>  					   led_cdev->blink_brightness);
>  		else
> @@ -85,8 +85,7 @@ 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 (trigger_data->tx || trigger_data->rx)
>  			schedule_delayed_work(&trigger_data->work, 0);
>  	}
>  }
> @@ -131,10 +130,10 @@ 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->linkup = 0;
>  	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->linkup = 1;
>  
>  	trigger_data->last_activity = 0;
>  
> @@ -150,23 +149,24 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
>  	enum netdev_led_attr attr)
>  {
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> -	int bit;
> +	int val;
>  
>  	switch (attr) {
>  	case NETDEV_ATTR_LINK:
> -		bit = NETDEV_LED_LINK;
> +		val = trigger_data->link;
>  		break;
>  	case NETDEV_ATTR_TX:
> -		bit = NETDEV_LED_TX;
> +		val = trigger_data->tx;
>  		break;
>  	case NETDEV_ATTR_RX:
> -		bit = NETDEV_LED_RX;
> +		val = trigger_data->rx;
>  		break;
>  	default:
> -		return -EINVAL;
> +		/* unreachable */
> +		break;
>  	}
>  
> -	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
> +	return sprintf(buf, "%u\n", val);
>  }
>  
>  static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
> @@ -175,33 +175,28 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
>  	unsigned long state;
>  	int ret;
> -	int bit;
>  
>  	ret = kstrtoul(buf, 0, &state);
>  	if (ret)
>  		return ret;
>  
> +	cancel_delayed_work_sync(&trigger_data->work);
> +
>  	switch (attr) {
>  	case NETDEV_ATTR_LINK:
> -		bit = NETDEV_LED_LINK;
> +		trigger_data->link = state;
>  		break;
>  	case NETDEV_ATTR_TX:
> -		bit = NETDEV_LED_TX;
> +		trigger_data->tx = state;
>  		break;
>  	case NETDEV_ATTR_RX:
> -		bit = NETDEV_LED_RX;
> +		trigger_data->rx = state;
>  		break;
>  	default:
> -		return -EINVAL;
> +		/* unreachable */
> +		break;
>  	}
>  
> -	cancel_delayed_work_sync(&trigger_data->work);
> -
> -	if (state)
> -		set_bit(bit, &trigger_data->mode);
> -	else
> -		clear_bit(bit, &trigger_data->mode);
> -
>  	set_baseline_state(trigger_data);
>  
>  	return size;
> @@ -315,7 +310,7 @@ 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->linkup = 0;
>  	switch (evt) {
>  	case NETDEV_CHANGENAME:
>  	case NETDEV_REGISTER:
> @@ -331,7 +326,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
>  	case NETDEV_UP:
>  	case NETDEV_CHANGE:
>  		if (netif_carrier_ok(dev))
> -			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
> +			trigger_data->linkup = 1;
>  		break;
>  	}
>  
> @@ -360,21 +355,17 @@ 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 (!trigger_data->tx && !trigger_data->rx)
>  		return;
>  
>  	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
> -	new_activity =
> -	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
> -		dev_stats->tx_packets : 0) +
> -	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
> -		dev_stats->rx_packets : 0);
> +	new_activity = (trigger_data->tx ? dev_stats->tx_packets : 0) +
> +		       (trigger_data->rx ? 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 = trigger_data->link;
>  		interval = jiffies_to_msecs(
>  				atomic_read(&trigger_data->interval));
>  		/* base state is ON (link present) */
> -- 
> 2.26.2
diff mbox series

Patch

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 4f6b73e3b491..8f013b6df4fa 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -49,11 +49,11 @@  struct led_netdev_data {
 	atomic_t interval;
 	unsigned int last_activity;
 
-	unsigned long mode;
-#define NETDEV_LED_LINK	0
-#define NETDEV_LED_TX	1
-#define NETDEV_LED_RX	2
-#define NETDEV_LED_MODE_LINKUP	3
+	unsigned link:1;
+	unsigned tx:1;
+	unsigned rx:1;
+
+	unsigned linkup:1;
 };
 
 enum netdev_led_attr {
@@ -73,10 +73,10 @@  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->linkup)
 		led_set_brightness(led_cdev, LED_OFF);
 	else {
-		if (test_bit(NETDEV_LED_LINK, &trigger_data->mode))
+		if (trigger_data->link)
 			led_set_brightness(led_cdev,
 					   led_cdev->blink_brightness);
 		else
@@ -85,8 +85,7 @@  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 (trigger_data->tx || trigger_data->rx)
 			schedule_delayed_work(&trigger_data->work, 0);
 	}
 }
@@ -131,10 +130,10 @@  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->linkup = 0;
 	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->linkup = 1;
 
 	trigger_data->last_activity = 0;
 
@@ -150,23 +149,24 @@  static ssize_t netdev_led_attr_show(struct device *dev, char *buf,
 	enum netdev_led_attr attr)
 {
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
-	int bit;
+	int val;
 
 	switch (attr) {
 	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
+		val = trigger_data->link;
 		break;
 	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
+		val = trigger_data->tx;
 		break;
 	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+		val = trigger_data->rx;
 		break;
 	default:
-		return -EINVAL;
+		/* unreachable */
+		break;
 	}
 
-	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
+	return sprintf(buf, "%u\n", val);
 }
 
 static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
@@ -175,33 +175,28 @@  static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	unsigned long state;
 	int ret;
-	int bit;
 
 	ret = kstrtoul(buf, 0, &state);
 	if (ret)
 		return ret;
 
+	cancel_delayed_work_sync(&trigger_data->work);
+
 	switch (attr) {
 	case NETDEV_ATTR_LINK:
-		bit = NETDEV_LED_LINK;
+		trigger_data->link = state;
 		break;
 	case NETDEV_ATTR_TX:
-		bit = NETDEV_LED_TX;
+		trigger_data->tx = state;
 		break;
 	case NETDEV_ATTR_RX:
-		bit = NETDEV_LED_RX;
+		trigger_data->rx = state;
 		break;
 	default:
-		return -EINVAL;
+		/* unreachable */
+		break;
 	}
 
-	cancel_delayed_work_sync(&trigger_data->work);
-
-	if (state)
-		set_bit(bit, &trigger_data->mode);
-	else
-		clear_bit(bit, &trigger_data->mode);
-
 	set_baseline_state(trigger_data);
 
 	return size;
@@ -315,7 +310,7 @@  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->linkup = 0;
 	switch (evt) {
 	case NETDEV_CHANGENAME:
 	case NETDEV_REGISTER:
@@ -331,7 +326,7 @@  static int netdev_trig_notify(struct notifier_block *nb,
 	case NETDEV_UP:
 	case NETDEV_CHANGE:
 		if (netif_carrier_ok(dev))
-			set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+			trigger_data->linkup = 1;
 		break;
 	}
 
@@ -360,21 +355,17 @@  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 (!trigger_data->tx && !trigger_data->rx)
 		return;
 
 	dev_stats = dev_get_stats(trigger_data->net_dev, &temp);
-	new_activity =
-	    (test_bit(NETDEV_LED_TX, &trigger_data->mode) ?
-		dev_stats->tx_packets : 0) +
-	    (test_bit(NETDEV_LED_RX, &trigger_data->mode) ?
-		dev_stats->rx_packets : 0);
+	new_activity = (trigger_data->tx ? dev_stats->tx_packets : 0) +
+		       (trigger_data->rx ? 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 = trigger_data->link;
 		interval = jiffies_to_msecs(
 				atomic_read(&trigger_data->interval));
 		/* base state is ON (link present) */