Re: ledtrig netdev: what is the purpose of spinlock usage?

["Marek Behún" <kabel@kernel.org>]

On Thu, 29 Oct 2020 18:45:29 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > since you are the original author of netdev LED trigger, I guess this
> > question should go to you. Why are spinlocks used as locks in the
> > netdev trigger code? Is this for performance? Would it be a drastic
> > performance hit to use mutexes?  
> 
> Triggers can be called from interrupt context, IIRC, and many simple
> LEDs can be operated from interrupt context, too.
> 
> Thus you need spinlocks...
> 
> Best regards,
> 								Pavel

Pavel,

the set_baseline_state function in netdev trigger is guarded by a
spinlock only when reading/writing device_name attribute and in
netdev notify callback.

netdev_trig_notify can for example put the device away on
NETDEV_UNREGISTER event, and if someone was reading/writing the
device_name at the same time netdev_trig_notify is manipulating
netdevice pointer, it could break. So my guess for the lock is that it
is used because of this.

It is possible that netdev_trig_notify can be called from interrupt
context, I will have to look into this.

Anyway for the trigger_offload() method to be able to communicate with
PHYs I need the set_baseline_state function not to be called from
within spinlock. Otherwise the drivers implementing this method would
get too complicated. Would it be allright if on netdev event (link up,
link down, netdev changename, netdev unregister) the set_baseline_state
was called from a work, instead of directly from the spinlock?

I will send a patch proposing and explaining this.

Marek