On 2018-05-30 15:08:37 [+0100], John Garry wrote: > On 30/05/2018 14:45, Sebastian Andrzej Siewior wrote: > > excellent. So no more objections from your side or is this a complaint I > > didn't fully decode? > > I think the original code is not great since we're dropping the lock but > maintaining the irq posture as disabled. the patch does not change the locking as it exists today. > Do you plan to add a lockdep_assert_irqs_disabled() check in addition? It's > not needed if we work on the basis that the lock is always taken with irqs > disabled. And is this lockdep function even intended to be used outside core > kernel code? I don't plan to add lockdep_assert_irqs_disabled(). You should only need to use lockdep_assert_held() to verify that the lock is held as expected. Whether or not the interrupts need to be disabled is best verified with lockdep. Lockdep would even complain here if you intend to invoke spin_unlock() on a lock which was not acquired earlier (I'm just trying to say that lockdep_assert_held() is not required here either). > Personally I think that solving the issue in the original code would be > better, i.e. keeping irqs disabled needlessly. Or else the "TODO" can stay > (or be amended to improve clarity). I would like to get rid of that local_irq_save() here because it breaks -RT and is not the correct thing to do for !RT (but it does not break things). I posted v1 with just the removal of the local_irq_sav() and v2 with TODO removed as suggested here in the thread. The interrupts still remain disabled. So based on that it seems that you are in favour of the initial v1. > John Sebastian
