Date:   Sun, 20 Mar 2022 12:23:56 +0100
From:   Mateusz Jończyk <mat.jonczyk@...pl>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        "Ira W . Snyder" <ira.snyder@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH] x86/pci: drop ServerWorks / Broadcom CNB20LE PCI host
 bridge driver

W dniu 18.03.2022 o 17:55, Bjorn Helgaas pisze:
> On Thu, Mar 10, 2022 at 08:34:19PM +0100, Mateusz Jończyk wrote:
>> The ServerWorks / Broadcom CNB20LE chipset was probably designed
>> for Intel Pentium III processors only. The dedicated driver for it
>> (implemented in arch/x86/pci/broadcom_bus.c) reads PCI windows
>> from this chipset, which is (if I understand correctly) needed mostly
>> for PCI hotplug and "option ROM mapping" [2]. This driver was written
>> without access to the documentation of CNB20LE and caused problems on
>> some servers, so it was soon disabled for all systems with ACPI enabled.
>> It appears that most systems with CNB20LE support ACPI and it is not
>> known whether the driver works correctly on most of those that don't.
>>
>> On such old platforms, PCI hotplug is typically not used and
>> "reliability" (i.e. that the system boots at all) is more important.
>> So, delete this driver as it can cause more harm then good and there is
>> little benefit in keeping it in the kernel.
>>
>> Details:
>>
>> I was unable to find a description of this chipset. However, it was
>> almost exclusively used with the Pentium III processor (this CPU model
>> was used in all references to it that I examined where the CPU model
>> was provided: dmesgs in [1] and [2]; [3] page 2; [4]-[7]).
>>
>> The CNB20LE driver was added to the kernel in 2010 (many years after
>> the introduction of Pentium III) in Linux 2.6.35 in
>> commit 3f6ea84a3035 ("PCI: read memory ranges out of Broadcom CNB20LE host bridge")
>> Soon, it caused problems on some Compaq Proliant DL320 servers [2] and
>> it was decided to disable the driver on all systems with ACPI enabled.
>> This went in
>> commit 30e664afb5cb ("x86/PCI: don't use native Broadcom CNB20LE driver when ACPI is available")
>>
>> However, most of these systems mentioned (all except for [3], where it is not
>> known and [7]) support ACPI. I think that dmesg from the original system
>> on which the driver was developed can be found in [7]. It is possible
>> that CNB30LE used the same PCI ID and was also detected by this driver
>> [7], but it probably also was used with Pentium III.
[snip]
>>
>> Signed-off-by: Mateusz Jończyk <mat.jonczyk@...pl>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Borislav Petkov <bp@...en8.de>
>> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>> Cc: x86@...nel.org
>> Cc: linux-pci@...r.kernel.org
>> Cc: "H. Peter Anvin" <hpa@...or.com>
>> Cc: Ira W. Snyder <ira.snyder@...il.com>
>> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
>>
>> ---
>>
>> Hello,
>>
>> When trying to document CONFIG_PCI_CNB20LE_QUIRK for Kconfig I realised
>> that this driver probably may be removed from the kernel.
>>
>> I am not sure what exactly the drawbacks of missing PCI bus window
>> information for this chipset are. In [2] there is a comment by Mr Bjorn
>> Helgaas:
>>         I think the possibilities are:
>>         [...]
>>         2) Ignore _CRS and make broadcom_bus.c do nothing.  This gets us
>>         back to the working situation of F13 [Fedora 13].  Since we
>>         don't have any host bridge information, things like PCI hotplug
>>         and option ROM mapping may not work, but that's the way it's
>>         always been on these boxes.
>>
>> I understand how host bridge windows are required for PCI hotplug, but I
>> don't know why they may be necessary for option ROM mapping.
> The windows are required for management of PCI MMIO space, i.e., we
> can't assign space to PCI BARs unless we know what space is available
> to be assigned.  Mapping option ROMs is one instance where we may
> need to assign space for a BAR.
>
> Presumably 3f6ea84a3035 ("PCI: read memory ranges out of Broadcom
> CNB20LE host bridge") was added because it fixed a problem. 

The main advertised feature of this driver is PCI hotplug support.
It is the only benefit mentioned in Kconfig and the description of this commit,
so I think it was the primary motivation for writing this driver.

> Would we
> also fix a problem by removing this? I hesitate to remove it on the
> grounds of "it might cause more harm than good" unless we have actual
> reports of problems.

This driver looks unused and broken, and was designed for very old hardware.
It is probably unnecessary to keep it in the kernel as PCI hotplug is typically
not used for hardware that is so old. There are no actual problem reports, though.

If this driver will not be removed or marked BROKEN yet, I think that
PCI_CNB20LE_QUIRK could be made to depend on X86_32.
I have pretty good data that this chipset works only with 32-bit processors.

Greetings,

Mateusz

> Bjorn
>
>> ---
>>  arch/x86/Kconfig            |  13 -----
>>  arch/x86/pci/Makefile       |   1 -
>>  arch/x86/pci/broadcom_bus.c | 112 ------------------------------------
>>  arch/x86/pci/bus_numa.c     |   2 +-
>>  4 files changed, 1 insertion(+), 127 deletions(-)
>>  delete mode 100644 arch/x86/pci/broadcom_bus.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 9f5bd41bf660..577a588fe1ae 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2656,19 +2656,6 @@ config MMCONF_FAM10H
>>  	def_bool y
>>  	depends on X86_64 && PCI_MMCONFIG && ACPI
>>  
>> -config PCI_CNB20LE_QUIRK
>> -	bool "Read CNB20LE Host Bridge Windows" if EXPERT
>> -	depends on PCI
>> -	help
>> -	  Read the PCI windows out of the CNB20LE host bridge. This allows
>> -	  PCI hotplug to work on systems with the CNB20LE chipset which do
>> -	  not have ACPI.
>> -
>> -	  There's no public spec for this chipset, and this functionality
>> -	  is known to be incomplete.
>> -
>> -	  You should say N unless you know you need this.
>> -
>>  config ISA_BUS
>>  	bool "ISA bus support on modern systems" if EXPERT
>>  	help
>> diff --git a/arch/x86/pci/Makefile b/arch/x86/pci/Makefile
>> index 48bcada5cabe..ca343d8c7964 100644
>> --- a/arch/x86/pci/Makefile
>> +++ b/arch/x86/pci/Makefile
>> @@ -22,6 +22,5 @@ obj-y				+= common.o early.o
>>  obj-y				+= bus_numa.o
>>  
>>  obj-$(CONFIG_AMD_NB)		+= amd_bus.o
>> -obj-$(CONFIG_PCI_CNB20LE_QUIRK)	+= broadcom_bus.o
>>  
>>  ccflags-$(CONFIG_PCI_DEBUG)	+= -DDEBUG
>> diff --git a/arch/x86/pci/broadcom_bus.c b/arch/x86/pci/broadcom_bus.c
>> deleted file mode 100644
>> index 2db73613cada..000000000000
>> --- a/arch/x86/pci/broadcom_bus.c
>> +++ /dev/null
>> @@ -1,112 +0,0 @@
>> -// SPDX-License-Identifier: GPL-2.0-or-later
>> -/*
>> - * Read address ranges from a Broadcom CNB20LE Host Bridge
>> - *
>> - * Copyright (c) 2010 Ira W. Snyder <iws@...o.caltech.edu>
>> - */
>> -
>> -#include <linux/acpi.h>
>> -#include <linux/delay.h>
>> -#include <linux/dmi.h>
>> -#include <linux/pci.h>
>> -#include <linux/init.h>
>> -#include <asm/pci_x86.h>
>> -#include <asm/pci-direct.h>
>> -
>> -#include "bus_numa.h"
>> -
>> -static void __init cnb20le_res(u8 bus, u8 slot, u8 func)
>> -{
>> -	struct pci_root_info *info;
>> -	struct pci_root_res *root_res;
>> -	struct resource res;
>> -	u16 word1, word2;
>> -	u8 fbus, lbus;
>> -
>> -	/* read the PCI bus numbers */
>> -	fbus = read_pci_config_byte(bus, slot, func, 0x44);
>> -	lbus = read_pci_config_byte(bus, slot, func, 0x45);
>> -	info = alloc_pci_root_info(fbus, lbus, 0, 0);
>> -
>> -	/*
>> -	 * Add the legacy IDE ports on bus 0
>> -	 *
>> -	 * These do not exist anywhere in the bridge registers, AFAICT. I do
>> -	 * not have the datasheet, so this is the best I can do.
>> -	 */
>> -	if (fbus == 0) {
>> -		update_res(info, 0x01f0, 0x01f7, IORESOURCE_IO, 0);
>> -		update_res(info, 0x03f6, 0x03f6, IORESOURCE_IO, 0);
>> -		update_res(info, 0x0170, 0x0177, IORESOURCE_IO, 0);
>> -		update_res(info, 0x0376, 0x0376, IORESOURCE_IO, 0);
>> -		update_res(info, 0xffa0, 0xffaf, IORESOURCE_IO, 0);
>> -	}
>> -
>> -	/* read the non-prefetchable memory window */
>> -	word1 = read_pci_config_16(bus, slot, func, 0xc0);
>> -	word2 = read_pci_config_16(bus, slot, func, 0xc2);
>> -	if (word1 != word2) {
>> -		res.start = ((resource_size_t) word1 << 16) | 0x0000;
>> -		res.end   = ((resource_size_t) word2 << 16) | 0xffff;
>> -		res.flags = IORESOURCE_MEM;
>> -		update_res(info, res.start, res.end, res.flags, 0);
>> -	}
>> -
>> -	/* read the prefetchable memory window */
>> -	word1 = read_pci_config_16(bus, slot, func, 0xc4);
>> -	word2 = read_pci_config_16(bus, slot, func, 0xc6);
>> -	if (word1 != word2) {
>> -		res.start = ((resource_size_t) word1 << 16) | 0x0000;
>> -		res.end   = ((resource_size_t) word2 << 16) | 0xffff;
>> -		res.flags = IORESOURCE_MEM | IORESOURCE_PREFETCH;
>> -		update_res(info, res.start, res.end, res.flags, 0);
>> -	}
>> -
>> -	/* read the IO port window */
>> -	word1 = read_pci_config_16(bus, slot, func, 0xd0);
>> -	word2 = read_pci_config_16(bus, slot, func, 0xd2);
>> -	if (word1 != word2) {
>> -		res.start = word1;
>> -		res.end   = word2;
>> -		res.flags = IORESOURCE_IO;
>> -		update_res(info, res.start, res.end, res.flags, 0);
>> -	}
>> -
>> -	/* print information about this host bridge */
>> -	res.start = fbus;
>> -	res.end   = lbus;
>> -	res.flags = IORESOURCE_BUS;
>> -	printk(KERN_INFO "CNB20LE PCI Host Bridge (domain 0000 %pR)\n", &res);
>> -
>> -	list_for_each_entry(root_res, &info->resources, list)
>> -		printk(KERN_INFO "host bridge window %pR\n", &root_res->res);
>> -}
>> -
>> -static int __init broadcom_postcore_init(void)
>> -{
>> -	u8 bus = 0, slot = 0;
>> -	u32 id;
>> -	u16 vendor, device;
>> -
>> -#ifdef CONFIG_ACPI
>> -	/*
>> -	 * We should get host bridge information from ACPI unless the BIOS
>> -	 * doesn't support it.
>> -	 */
>> -	if (!acpi_disabled && acpi_os_get_root_pointer())
>> -		return 0;
>> -#endif
>> -
>> -	id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID);
>> -	vendor = id & 0xffff;
>> -	device = (id >> 16) & 0xffff;
>> -
>> -	if (vendor == PCI_VENDOR_ID_SERVERWORKS &&
>> -	    device == PCI_DEVICE_ID_SERVERWORKS_LE) {
>> -		cnb20le_res(bus, slot, 0);
>> -		cnb20le_res(bus, slot, 1);
>> -	}
>> -	return 0;
>> -}
>> -
>> -postcore_initcall(broadcom_postcore_init);
>> diff --git a/arch/x86/pci/bus_numa.c b/arch/x86/pci/bus_numa.c
>> index 2752c02e3f0e..b4f5e668812e 100644
>> --- a/arch/x86/pci/bus_numa.c
>> +++ b/arch/x86/pci/bus_numa.c
>> @@ -59,7 +59,7 @@ void x86_pci_root_bus_resources(int bus, struct list_head *resources)
>>  default_resources:
>>  	/*
>>  	 * We don't have any host bridge aperture information from the
>> -	 * "native host bridge drivers," e.g., amd_bus or broadcom_bus,
>> +	 * "native host bridge drivers", e.g. amd_bus,
>>  	 * so fall back to the defaults historically used by pci_create_bus().
>>  	 */
>>  	printk(KERN_DEBUG "PCI: root bus %02x: using default resources\n", bus);
>>
>> base-commit: ffb217a13a2eaf6d5bd974fc83036a53ca69f1e2
>> -- 
>> 2.25.1
>>

Powered by blists - more mailing lists