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 <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
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;
--
2.34.1
> @@ -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?
--
Ansuel
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.
>
From what I can see in the code, the lock is really used for the
work. It there to handle the device_name store/show and to not remove
the dev while a work is in progress...
But I can also see that on store and on netdev_trig the work is
cancelled, so in theory the problem of "removing dev while a work is in
progress" should never happen (as we cancel the work before anyway).
So I see the only real use for the lock is the device_name_show.
> 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?
I need to check what can I do to move the configuration phase outside
the lock.
Just to make sure the spinlock ot mutex conversion is not doable cause
we are locking unter a netdev notify or for other reason?
>
> Andrew
--
Ansuel