| Message ID | 20201030114435.20169-3-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | netdev trigger offloading and LEDs on Marvell PHYs | expand |
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); [...] -- Best regards, Jacek Anaszewski
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
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. -- Best regards, Jacek Anaszewski
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 -- http://www.livejournal.com/~pavelmachek
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) */
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(-)