2017-10-22 11:20 UTC+02:00, Peter Zijlstra <[email protected]>: > On Fri, Oct 20, 2017 at 02:56:04AM +0200, Frederic Weisbecker wrote: >> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c >> index 03505ff..b4f3a55 100644 >> --- a/arch/x86/entry/common.c >> +++ b/arch/x86/entry/common.c >> @@ -186,7 +186,7 @@ __visible inline void prepare_exit_to_usermode(struct >> pt_regs *regs) >> >> addr_limit_user_check(); >> >> - if (IS_ENABLED(CONFIG_PROVE_LOCKING) && WARN_ON(!irqs_disabled())) >> + if (!lockdep_assert_irqs_disabled()) >> local_irq_disable(); >> >> lockdep_sys_exit(); > > So this is the only site that ever uses the return value; and for this > you've chosen the wrong value for !CONFIG_PROVE_LOCKING (namely 1), > resulting in an unconditional CLI here for !lockdep kernels.
Hmm, it should be the opposite. The assertion maps to 1 for !CONFIG_PROVE_LOCKING but we are testing with "!". > How about we replace that whole thing with a simple: > > lockdep_assert_irqs_disabled(); > > And leave it at that, allowing us to remove the return value thing > entirely. > > The whole if !disabled, disable logic is uber paranoid programming, but > I don't think we've ever seen that WARN trigger, and if it does (and > then burns the kernel) we at least know wtf happend. I certainly don't mind personally, as long as we get the warning. Now I've seen different opinions on matters such as this. Some prefer to have the auto-correction, some don't. I'll try as you say and see if anybody gets angry :)
