| Message ID | 20220503151633.18760-8-ansuelsmth@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Adds support for PHY LEDs with offload triggers | expand |
> @@ -400,7 +400,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); I'm not sure you can convert a spin_lock_bh() in a mutex_lock(). Did you check this? What context is the notifier called in? Andrew
On Thu, May 05, 2022 at 03:10:21AM +0200, Andrew Lunn wrote: > > @@ -400,7 +400,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); > > I'm not sure you can convert a spin_lock_bh() in a mutex_lock(). > > Did you check this? What context is the notifier called in? > > Andrew I had to do this because qca8k use completion to set the value and that can sleep... Mhhh any idea how to handle this?
On Thu, May 05, 2022 at 03:29:09PM +0200, Ansuel Smith wrote: > On Thu, May 05, 2022 at 03:10:21AM +0200, Andrew Lunn wrote: > > > @@ -400,7 +400,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); > > > > I'm not sure you can convert a spin_lock_bh() in a mutex_lock(). > > > > Did you check this? What context is the notifier called in? > > > > Andrew > > I had to do this because qca8k use completion to set the value and that > can sleep... Mhhh any idea how to handle this? First step is to define what the lock is protecting. Once you know that, you should be able to see what you can do without actually holding the lock. Do you need the lock when actually setting the LED? Or is the lock protecting state information inside trigger_data? Can you make a copy of what you need from trigger_data while holding the lock, release it and then set the LED? Maybe a nested mutex and a spin lock? The spin lock protecting trigger_data, and the mutex protecting setting the LED? Andrew
On Thu, May 05, 2022 at 04:21:54PM +0200, Andrew Lunn wrote: > On Thu, May 05, 2022 at 03:29:09PM +0200, Ansuel Smith wrote: > > On Thu, May 05, 2022 at 03:10:21AM +0200, Andrew Lunn wrote: > > > > @@ -400,7 +400,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); > > > > > > I'm not sure you can convert a spin_lock_bh() in a mutex_lock(). > > > > > > Did you check this? What context is the notifier called in? > > > > > > Andrew > > > > I had to do this because qca8k use completion to set the value and that > > can sleep... Mhhh any idea how to handle this? > > First step is to define what the lock is protecting. Once you know > that, you should be able to see what you can do without actually > holding the lock. >
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index ed019cb5867c..a471e0cde836 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" @@ -38,7 +38,7 @@ struct led_netdev_data { enum led_blink_modes blink_mode; - spinlock_t lock; + struct mutex lock; struct delayed_work work; struct notifier_block notifier; @@ -183,9 +183,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; } @@ -206,7 +206,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); @@ -231,7 +231,7 @@ static ssize_t device_name_store(struct device *dev, trigger_data->net_dev = old_net; memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ); - spin_unlock_bh(&trigger_data->lock); + mutex_unlock(&trigger_data->lock); return -EINVAL; } @@ -242,7 +242,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; } @@ -400,7 +400,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) { @@ -423,7 +423,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; } @@ -484,7 +484,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;
Some LEDs may require to sleep to apply their hardware rules. Convert to mutex lock to fix warning for sleeping unser spinlock softirq. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/leds/trigger/ledtrig-netdev.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)