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;
Date:   Thu, 14 Oct 2021 00:13:57 +0200
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Myron Stowe <myron.stowe@redhat.com>,
        Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>, linux-pci@vger.kernel.org,
        x86@kernel.org, linux-kernel@vger.kernel.org,
        Benoit =?utf-8?B?R3LDqWdvaXJl?= <benoitg@coeus.ca>,
        Hui Wang <hui.wang@canonical.com>
Subject: Re: [PATCH v2] x86/PCI: Ignore E820 reservations for bridge windows
 on newer systems
Message-ID: <YWdaJVhoPDrg3Tsd@rocinante>
References: <20211011090531.244762-1-hdegoede@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211011090531.244762-1-hdegoede@redhat.com>
Precedence: bulk

Hi Hans,

Thank you for sending the patch over!

[...]
> [    0.000000] BIOS-e820: [mem 0x000000004bc50000-0x00000000cfffffff] reserved
> [    0.557473] pci_bus 0000:00: root bus resource [mem 0x65400000-0xbfffffff window]

A very small nitpick: we usually remove time/date stamps from kernel ring
buffer outputs keeping only the relevant message parts left.

[...]
> Old systems are defined here as BIOS year < 2018, this was chosen to
> make sure that pci_use_e820 will not be set on the currently affected
> systems, while at the same time also taking into account that the
> systems for which the E820 checking was orignally added may have

A tiny typo of "originally" in the sentence above.

[...]
> @@ -232,3 +236,9 @@ static inline void mmio_config_writel(void __iomem *pos, u32 val)
>  # define x86_default_pci_init_irq	NULL
>  # define x86_default_pci_fixup_irqs	NULL
>  #endif
> +
> +#if defined CONFIG_PCI && defined CONFIG_ACPI

I know that Mika already asked about this, and you responded, so I can only
added: brackets, let's add brackets, most definitely. :)

[...]
> +/* Consumed in arch/x86/kernel/resource.c */
> +bool pci_use_e820 = false;

A small nitpick: not sure if this comment is needed as probably most people
working with this code would use "git grep" and likes to list occurrences
where the variables is used.  But, this is highly subjective, thus there is
probably nothing to change here.

> +	printk(KERN_INFO "PCI: %s E820 reservations for host bridge windows\n",
> +	       pci_use_e820 ? "Honoring" : "Ignoring");

I know you followed the existing style, which is very much appreciated, but
if and where possible, we should move to newer API/style and replace the
printk() above with pr_info().  New code should not be adding old style if
it can be helped (checkpatch.pl would warn about this too).  What do yo you
think?

	Krzysztof