Hi, On Thu, 29 Oct 2020 at 18:13, Marek Behún <kabel@xxxxxxxxxx> 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. IIRC that is what I was seeing on my platform and I used the same locking mechanism for consistency, though my memory is hazy. > > 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. Go for it, patches welcome! Sounds interesting. > > Marek
