Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <YfQpy5yGGqY8T0wW@jupiter.dyndns.org> <a7ef2455-ede5-2238-639b-b3a66842a04b@intel.com>
In-Reply-To: <a7ef2455-ede5-2238-639b-b3a66842a04b@intel.com>
From:   Brent Spillner <spillner@acm.org>
Date:   Fri, 28 Jan 2022 20:48:47 +0000
Message-ID: <CAGwJgaNa2u8vmxsnaSdpSH+ZO0e2GCYObSwC+j03843gXQ_vwg@mail.gmail.com>
Subject: Re: [PATCH] arch:x86:pci:irq.c: Improve log message when IRQ cannot
 be identified
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     bhelgaas@google.com, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org,
        hpa@zytor.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 28, 2022 at 6:00 PM Dave Hansen <dave.hansen@intel.com> wrote:
> Shockingly enough, that parameter is in the documentation:
> and double-shockingly, it's even called out as X86-32-only:

Right, seeing that is what convinced me that not customizing the log
message for x86_64 could be considered a (admittedly very minor) bug,
and perhaps worth fixing.

> Given that, do we really need to refer to the line numbers of the
> implementation which will probably be stale by the time this is merged
> anyway?

Understood, will change the commit message to just refer to the
command line documentation.

> Any chance you could make that, um, a bit more readable?  It's OK to add
> brackets to the else{} for readability even if they're not *strictly*
> necessary.
>
> It might also be nice to use
>
>         if (IS_ENABLED(CONFIG_FOO))
>                 ...
>
> rather than the #ifdefs.

I don't mind doing either of those if that's the maintainer consensus,
but would note that neither would be consistent with the surrounding
code. Prior to the patch, the .c files in arch/x86/pci contain a total
of 33 #ifdefs and just one IS_ENABLED(), and systematically avoid
superfluous braces around single-statement if/else/for bodies.
Granted, the code has other style problems and triggers a number of
checkpatch.pl warnings (although not in the region affected by this
patch), but I was trying to be as light a touch as possible.

> I'd also be perfectly OK having two different strings rather than
> relying on string concatenation and the #ifdefs.
>
> Is the "or enabling ACPI" message really necessary?

Not strictly necessary--- it seems fair to assume that anyone
disabling ACPI does so intentionally and with good reason--- but I
thought it might stimulate the right thought process for someone who
doesn't understand why their hardware isn't being properly detected,
as ACPI triggers some very different code paths through this driver.

It seems like the multiline string literal is your main pain point--- would

+#ifdef CONFIG_ACPI
+                       if (acpi_noirq)
+                               msg = "; consider removing acpi=noirq";
+                       else
+                               msg = "; recommend verifying UEFI/BIOS
IRQ options";
+#else
+                       msg = "; recommend verifying UEFI/BIOS IRQ
options or enabling ACPI";
+#endif

be OK without going to IS_ENABLED()?  (Personally, I think the #ifdef
style is more readable.)