Hi Marek,
On 10/29/20 7:13 PM, Marek Behún wrote:
On Thu, 29 Oct 2020 18:45:29 +0100
Pavel Machek <pavel@xxxxxx> 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.
register_netdevice_notifier() registers raw notifier chain,
whose callbacks are not called from atomic context and there are
no restrictions on callbacks. See include/linux/notifier.h.
So it looks like the spin_lock_bh() can be safely changed to
mutex_lock().
--
Best regards,
Jacek Anaszewski
