On 30/05/2018 12:22, Sebastian Andrzej Siewior wrote:
On 2018-05-30 12:16:23 [+0100], John Garry wrote:
On 30/05/2018 11:16, Sebastian Andrzej Siewior wrote:
On 2018-05-30 10:34:12 [+0100], John Garry wrote:
Sorry, but personally I don't see much value in this change. I think it's
better for safety to be consistent in how we lock & unlock the spinlock,
i.e. use irqsave variant (or similar).
The lock should do irqsave() and unlock irqrestore(). This
local_irqsave() + unlock() is not correct.
Aren't they the same, i.e. local_irq_save()+spin_lock() =
spin_lock_irqsave()? Both give state of lock held, interrupts and preemption
disabled.
Yes, there are the same on vanilla and not on RT. However my point is
that the code does this instead:
local_irq_save();
spin_unlock();
Ah, I just noticed that this is spin_unlock().
So about the "TODO", which you mention "I *assumed* that the intention
was to audit the code for this
spin_unlock_irq(ap->lock);
change instead. But if this is or was never intended than I could indeed
remove the TODO comment."
As I see, we're dropping the lock but maintaining the irq posture for
holding that lock (disabled), which seems inefficient.
John
