Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754515AbcDYLT1 (ORCPT ); Mon, 25 Apr 2016 07:19:27 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:36558 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754254AbcDYLTZ (ORCPT ); Mon, 25 Apr 2016 07:19:25 -0400 Date: Mon, 25 Apr 2016 12:19:23 +0100 From: Matt Fleming To: Mark Rutland Cc: Ard Biesheuvel , Laszlo Ersek , "linux-efi@vger.kernel.org" , Catalin Marinas , "hpa@zytor.com" , Leif Lindholm , "linux-arm-kernel@lists.infradead.org" , Russell King - ARM Linux , "linux-kernel@vger.kernel.org" , "mingo@redhat.com" , "tglx@linutronix.de" , Will Deacon Subject: Re: [PATCHv2 0/6] efi: detect erroneous firmware IRQ manipulation Message-ID: <20160425111923.GS2829@codeblueprint.co.uk> References: <1461333083-15529-1-git-send-email-mark.rutland@arm.com> <20160424212241.GO2829@codeblueprint.co.uk> <20160425101527.GP2829@codeblueprint.co.uk> <20160425102821.GQ2829@codeblueprint.co.uk> <20160425104009.GD25087@leverpostej> <20160425105153.GR2829@codeblueprint.co.uk> <20160425110455.GF25087@leverpostej> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160425110455.GF25087@leverpostej> User-Agent: Mutt/1.5.24+41 (02bc14ed1569) (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2122 Lines: 59 On Mon, 25 Apr, at 12:04:55PM, Mark Rutland wrote: > On Mon, Apr 25, 2016 at 11:51:53AM +0100, Matt Fleming wrote: > > On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote: > > > > > > It looks like irqs_disabled_flags() will do what you expect, and ignore > > > everything but the interrupt flag. > > > > > > However, for ARM that will ignore the other exceptions we've seen FW > > > erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with > > > those is just as disastrous. > > > > Bah, right. > > > > > Would you be happy with an arch_efi_call_check_flags(before, after), > > > instead? That way we can make the flags we check arch-specific. > > > > Could we just make the flag mask arch-specific instead of the call > > since the rest of efi_call_virt_check_flags() is good? > > Yup, I meant that arch_efi_call_check_flags would only do the flag > comparison, only replacing the bit currently in the WARN_ON_ONCE(). > > > Something like this (uncompiled, untested, half-baked), > > > > --- > > > > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c > > index c38b1cfc26e2..057d00bee7d6 100644 > > --- a/drivers/firmware/efi/runtime-wrappers.c > > +++ b/drivers/firmware/efi/runtime-wrappers.c > > @@ -25,9 +25,12 @@ > > static void efi_call_virt_check_flags(unsigned long flags, const char *call) > > { > > unsigned long cur_flags; > > + bool mismatch; > > > > local_save_flags(cur_flags); > > - if (!WARN_ON_ONCE(cur_flags != flags)) > > + > > + mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK; > > + if (!WARN_ON_ONCE(mismatch)) > > return; > > This style also works for me. Cool. One thing that occurred to me after I sent it is that technically we should either, 1) make 'mismatch' an int or 2) do mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK) Either is fine with me, I just don't want to leave the implicit conversion to C's type system. > Should I respin patch 6 as a series doing the above? > > I assume that the first 5 patches are fine as-is. Yep, they're fine. Sure, go ahead and respin patch 6.