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
