[PATCH V3 0/3] x86/pci Fix numa_node info for AMD hostbridge and misc clean up.

[PATCH V3 0/3] x86/pci Fix numa_node info for AMD hostbridge and misc clean up.

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This in the V3 of the patch set which trys to fixed up numa_node for AMD hostbridges.
This topic was discussed in the following threads:

V1: https://lkml.org/lkml/2014/3/5/898
V2: https://lkml.org/lkml/2014/4/18/623

Change from V2:
As requested by Bjorn, the patch set will only add supports for family15h, and 
also deprecating the mechanism, as future platforms should be relying on the ACPI info.
He has also requested to keep the changes to the minimum. Therefore, the V2 patch 2 and 3
has been removed.

Myron Stowe (2):
  ACPI/PCI: Warn if we have to "guess" host bridge node information
  PCI: Remove unnecessary 'quirk_amd_nb_node'

Suravee Suthikulpanit (1):
  x86/PCI: Fix PCI root numa_node info on AMD family15h

 arch/x86/kernel/quirks.c | 58 -------------------------------------
 arch/x86/pci/acpi.c      |  6 +++-
 arch/x86/pci/amd_bus.c   | 75 +++++++++++++++++++++++++++++-------------------
 3 files changed, 51 insertions(+), 88 deletions(-)

-- 
1.9.0

[PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

This patch fixes the numa_node information in sysfs for PCI root
on AMD family15h platforms (currently showing -1) by adding
the hostbridge in the list of probed devices to be used for initializing
pci_root_info structue.

This mechanism is now deprecated in favor of info in ACPI.
Therefore, this patch also adds note stating the deprecation.

Reference(s):
  https://bugzilla.kernel.org/show_bug.cgi?id=72051
  Advanced Micro Devices (AMD), "BIOS and Kernel Developer's
  Guide (BKDG) for AMD Family 15h Models 00h-0fh Processors."  Section
  3.4 Device [1F:18]h Function 1 Configuration Registers; D18F1x[EC:E0]
  Configuration Map, 42301 Rev 3.12 - October 11, 2012.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
Tested-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Robert Richter <rric@kernel.org>
Cc: Daniel J Blueman <daniel@numascale.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
---
 arch/x86/pci/amd_bus.c | 75 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 29 deletions(-)

diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
index e88f4c5..7c251c2 100644
--- a/arch/x86/pci/amd_bus.c
+++ b/arch/x86/pci/amd_bus.c
@@ -11,27 +11,33 @@
 
 #include "bus_numa.h"
 
-/*
- * This discovers the pcibus <-> node mapping on AMD K8.
- * also get peer root bus resource for io,mmio
- */
+#define AMD_NB_F0_NODE_ID			0x60
+#define AMD_NB_F0_UNIT_ID			0x64
+#define AMD_NB_F1_CONFIG_MAP_REG		0xe0
 
-struct pci_hostbridge_probe {
+#define RANGE_NUM				16
+#define AMD_NB_F1_CONFIG_MAP_RANGES		4
+
+struct amd_hostbridge {
 	u32 bus;
 	u32 slot;
-	u32 vendor;
 	u32 device;
 };
 
-static struct pci_hostbridge_probe pci_probes[] __initdata = {
-	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1100 },
-	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
-	{ 0xff, 0, PCI_VENDOR_ID_AMD, 0x1200 },
-	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1300 },
+/*
+ * IMPORTANT NOTE:
+ * hb_probes[] and early_root_info_init() is in maintenance mode.
+ * It only supports K8, Fam10h, Fam11h, and Fam15h_00h-0fh .
+ * Future processor will rely on information in ACPI.
+ */
+static struct amd_hostbridge hb_probes[] __initdata = {
+	{ 0, 0x18, 0x1100 }, /* K8 */
+	{ 0, 0x18, 0x1200 }, /* Family10h */
+	{ 0xff, 0, 0x1200 }, /* Family10h */
+	{ 0, 0x18, 0x1300 }, /* Family11h */
+	{ 0, 0x18, 0x1600 }, /* Family15h */
 };
 
-#define RANGE_NUM 16
-
 static struct pci_root_info __init *find_pci_root_info(int node, int link)
 {
 	struct pci_root_info *info;
@@ -45,12 +51,12 @@ static struct pci_root_info __init *find_pci_root_info(int node, int link)
 }
 
 /**
- * early_fill_mp_bus_to_node()
+ * early_root_info_init()
  * called before pcibios_scan_root and pci_scan_bus
- * fills the mp_bus_to_cpumask array based according to the LDT Bus Number
- * Registers found in the K8 northbridge
+ * fills the mp_bus_to_cpumask array based according
+ * to the LDT Bus Number Registers found in the northbridge.
  */
-static int __init early_fill_mp_bus_info(void)
+static int __init early_root_info_init(void)
 {
 	int i;
 	unsigned bus;
@@ -75,19 +81,21 @@ static int __init early_fill_mp_bus_info(void)
 		return -1;
 
 	found = false;
-	for (i = 0; i < ARRAY_SIZE(pci_probes); i++) {
+	for (i = 0; i < ARRAY_SIZE(hb_probes); i++) {
 		u32 id;
 		u16 device;
 		u16 vendor;
 
-		bus = pci_probes[i].bus;
-		slot = pci_probes[i].slot;
+		bus = hb_probes[i].bus;
+		slot = hb_probes[i].slot;
 		id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID);
-
 		vendor = id & 0xffff;
 		device = (id>>16) & 0xffff;
-		if (pci_probes[i].vendor == vendor &&
-		    pci_probes[i].device == device) {
+
+		if (vendor != PCI_VENDOR_ID_AMD)
+			continue;
+
+		if (hb_probes[i].device == device) {
 			found = true;
 			break;
 		}
@@ -96,10 +104,11 @@ static int __init early_fill_mp_bus_info(void)
 	if (!found)
 		return 0;
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < AMD_NB_F1_CONFIG_MAP_RANGES; i++) {
 		int min_bus;
 		int max_bus;
-		reg = read_pci_config(bus, slot, 1, 0xe0 + (i << 2));
+		reg = read_pci_config(bus, slot, 1,
+				AMD_NB_F1_CONFIG_MAP_REG + (i << 2));
 
 		/* Check if that register is enabled for bus range */
 		if ((reg & 7) != 3)
@@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
 		info = alloc_pci_root_info(min_bus, max_bus, node, link);
 	}
 
+	/*
+	 * The following code is only supported until Fam11h.
+	 * Newer processors will depend on ACPI MCFG table instead.
+	 */
+	if (boot_cpu_data.x86 > 0x11)
+		return 0;
+
 	/* get the default node and link for left over res */
-	reg = read_pci_config(bus, slot, 0, 0x60);
+	reg = read_pci_config(bus, slot, 0, AMD_NB_F0_NODE_ID);
 	def_node = (reg >> 8) & 0x07;
-	reg = read_pci_config(bus, slot, 0, 0x64);
+	reg = read_pci_config(bus, slot, 0, AMD_NB_F0_UNIT_ID);
 	def_link = (reg >> 8) & 0x03;
 
 	memset(range, 0, sizeof(range));
@@ -363,7 +379,7 @@ static int __init pci_io_ecs_init(void)
 	int cpu;
 
 	/* assume all cpus from fam10h have IO ECS */
-        if (boot_cpu_data.x86 < 0x10)
+	if (boot_cpu_data.x86 < 0x10)
 		return 0;
 
 	/* Try the PCI method first. */
@@ -387,7 +403,8 @@ static int __init amd_postcore_init(void)
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		return 0;
 
-	early_fill_mp_bus_info();
+	early_root_info_init();
+
 	pci_io_ecs_init();
 
 	return 0;
-- 
1.9.0

[PATCH V3 2/3] ACPI/PCI: Warn if we have to "guess" host bridge node information

[suravee.suthikulpanit]

From: Myron Stowe <myron.stowe@redhat.com>

The vast majority of platforms are not supplying ACPI _PXM (proximity)
information corresponding to host bridge (PNP0A03/PNP0A08) devices
resulting in sysfs "numa_node" values of -1 (NUMA_NO_NODE) [1]:
  # for i in /sys/devices/pci0000\:00/*/numa_node; do cat $i; done | uniq
  -1

  # find /sys/ -name "numa_node" | while read fname; do cat $fname; \
    done | uniq
  -1

AMD based platforms provide a fall-back for this situation via amd_bus.c.
These platforms snoop out the information by directly reading specific
registers from the Northbridge and caching them via 'alloc_pci_root_info'.

Later during boot processing when host bridges are discovered -
'pci_acpi_scan_root' - the kernel looks for their corresponding ACPI _PXM
method - drivers/acpi/numa.c::acpi_get_node().  If the BIOS supplied a
_PXM method then that node (proximity) value is associated.  If the BIOS
did not supply a _PXM method *and* the platform is AMD based, the
fall-back cached values obtained directly from the Northbridge are used;
otherwise, "NUMA_NO_NODE" is associated.

There are a number of issues with this fall-back mechanism the most
notable being that amd_bus.c extracts a 3-bit number from a CPU register
and uses it as the node number.  The node numbers used by Linux are
logical and there's no reason they need to be identical to settings in the
CPU registers.  So if we have some node information obtained in the normal
way (from _PXM, SLIT, SRAT, etc.) and some from amd_bus.c, there's no
reason to believe they will be compatible.

This patch warns when this situation occurs:
  pci_root PNP0A08:00: [Firmware Bug]: no _PXM; falling back to node 0 from hardware (may be inconsistent with ACPI node numbers)

[1] https://bugzilla.kernel.org/show_bug.cgi?id=72051

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/pci/acpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 01edac6..5075371 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -489,8 +489,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	}
 
 	node = acpi_get_node(device->handle);
-	if (node == NUMA_NO_NODE)
+	if (node == NUMA_NO_NODE) {
 		node = x86_pci_root_bus_node(busnum);
+		if (node != 0 && node != NUMA_NO_NODE)
+			dev_info(&device->dev, FW_BUG "no _PXM; falling back to node %d from hardware (may be inconsistent with ACPI node numbers)\n",
+				node);
+	}
 
 	if (node != NUMA_NO_NODE && !node_online(node))
 		node = NUMA_NO_NODE;
-- 
1.9.0

[PATCH V3 3/3] PCI: Remove unnecessary 'quirk_amd_nb_node'

[suravee.suthikulpanit]

From: Myron Stowe <myron.stowe@redhat.com>

The quirk is used for fixing up the numa_node information in sysfs for AMD hostbridge
devices (i.e. PCI device 00:[18-1f].x).  However, this is currently unused.
and is becoming maintenance burden. Therefore, it is removed.

Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Robert Richter <rric@kernel.org>
Cc: Daniel J Blueman <daniel@numascale.com>
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 arch/x86/kernel/quirks.c | 58 ------------------------------------------------
 1 file changed, 58 deletions(-)

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index ff898bb..abbd6bc 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -514,64 +514,6 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS,
 
 #endif
 
-#if defined(CONFIG_PCI) && defined(CONFIG_NUMA)
-/* Set correct numa_node information for AMD NB functions */
-static void quirk_amd_nb_node(struct pci_dev *dev)
-{
-	struct pci_dev *nb_ht;
-	unsigned int devfn;
-	u32 node;
-	u32 val;
-
-	devfn = PCI_DEVFN(PCI_SLOT(dev->devfn), 0);
-	nb_ht = pci_get_slot(dev->bus, devfn);
-	if (!nb_ht)
-		return;
-
-	pci_read_config_dword(nb_ht, 0x60, &val);
-	node = pcibus_to_node(dev->bus) | (val & 7);
-	/*
-	 * Some hardware may return an invalid node ID,
-	 * so check it first:
-	 */
-	if (node_online(node))
-		set_dev_node(&dev->dev, node);
-	pci_dev_put(nb_ht);
-}
-
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB_ADDRMAP,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB_MEMCTL,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB_MISC,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_HT,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MAP,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_DRAM,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_MISC,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_10H_NB_LINK,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F0,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F1,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F2,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F3,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F4,
-			quirk_amd_nb_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F5,
-			quirk_amd_nb_node);
-
-#endif
-
 #ifdef CONFIG_PCI
 /*
  * Processor does not ensure DRAM scrub read/write sequence
-- 
1.9.0

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Robert Richter]

On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
> @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
>  		info = alloc_pci_root_info(min_bus, max_bus, node, link);
>  	}
>  
> +	/*
> +	 * The following code is only supported until Fam11h.
> +	 * Newer processors will depend on ACPI MCFG table instead.
> +	 */
> +	if (boot_cpu_data.x86 > 0x11)
> +		return 0;
> +
>  	/* get the default node and link for left over res */

As this is the only substantial change of your patch, I would better
drop ther rest or at least split it in two patches. Should this change
also be for stable?

-Robert

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Robert Richter]

On 08.05.14 10:59:05, Robert Richter wrote:
> On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
> > @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
> >  		info = alloc_pci_root_info(min_bus, max_bus, node, link);
> >  	}
> >  
> > +	/*
> > +	 * The following code is only supported until Fam11h.
> > +	 * Newer processors will depend on ACPI MCFG table instead.
> > +	 */
> > +	if (boot_cpu_data.x86 > 0x11)
> > +		return 0;
> > +
> >  	/* get the default node and link for left over res */
> 
> As this is the only substantial change of your patch, I would better
> drop ther rest or at least split it in two patches. Should this change
> also be for stable?

Of course adding the hostbridge must be also part of the patch, didn't
note this due to the other noise. See why the split would be good?

> 
> -Robert

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Suravee Suthikulanit]

On 5/8/2014 4:01 AM, Robert Richter wrote:
> On 08.05.14 10:59:05, Robert Richter wrote:
>> On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
>>> @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
>>>   		info = alloc_pci_root_info(min_bus, max_bus, node, link);
>>>   	}
>>>
>>> +	/*
>>> +	 * The following code is only supported until Fam11h.
>>> +	 * Newer processors will depend on ACPI MCFG table instead.
>>> +	 */
>>> +	if (boot_cpu_data.x86 > 0x11)
>>> +		return 0;
>>> +
>>>   	/* get the default node and link for left over res */
>>
>> As this is the only substantial change of your patch, I would better
>> drop ther rest or at least split it in two patches. Should this change
>> also be for stable?
>
> Of course adding the hostbridge must be also part of the patch, didn't
> note this due to the other noise. See why the split would be good?
>
>>
>> -Robert


Robert,

I have already added the hostbridge for family15h in this patch.

+static struct amd_hostbridge hb_probes[] __initdata = {
+	{ 0, 0x18, 0x1100 }, /* K8 */
+	{ 0, 0x18, 0x1200 }, /* Family10h */
+	{ 0xff, 0, 0x1200 }, /* Family10h */
+	{ 0, 0x18, 0x1300 }, /* Family11h */
+	{ 0, 0x18, 0x1600 }, /* Family15h */ <--- HERE
  };

The rest of the changes are mostly comments, some minor renaming of 
variables for clarity, and replace hardcode values with preprocessor 
macro.  If needed, I can split them.

Suravee

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Robert Richter]

On 08.05.14 09:39:55, Suravee Suthikulanit wrote:
> On 5/8/2014 4:01 AM, Robert Richter wrote:
> >On 08.05.14 10:59:05, Robert Richter wrote:
> >>On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
> >>>@@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
> >>>  		info = alloc_pci_root_info(min_bus, max_bus, node, link);
> >>>  	}
> >>>
> >>>+	/*
> >>>+	 * The following code is only supported until Fam11h.
> >>>+	 * Newer processors will depend on ACPI MCFG table instead.
> >>>+	 */
> >>>+	if (boot_cpu_data.x86 > 0x11)
> >>>+		return 0;
> >>>+
> >>>  	/* get the default node and link for left over res */
> >>
> >>As this is the only substantial change of your patch, I would better
> >>drop ther rest or at least split it in two patches. Should this change
> >>also be for stable?
> >
> >Of course adding the hostbridge must be also part of the patch, didn't
> >note this due to the other noise. See why the split would be good?
> >
> >>
> >>-Robert
> 
> 
> Robert,
> 
> I have already added the hostbridge for family15h in this patch.
> 
> +static struct amd_hostbridge hb_probes[] __initdata = {
> +	{ 0, 0x18, 0x1100 }, /* K8 */
> +	{ 0, 0x18, 0x1200 }, /* Family10h */
> +	{ 0xff, 0, 0x1200 }, /* Family10h */
> +	{ 0, 0x18, 0x1300 }, /* Family11h */
> +	{ 0, 0x18, 0x1600 }, /* Family15h */ <--- HERE

Yes, I noticed that, but later, thus my 2nd mail.

>  };
> 
> The rest of the changes are mostly comments, some minor renaming of
> variables for clarity, and replace hardcode values with preprocessor macro.
> If needed, I can split them.

I just would drop it, you just need the fam15h device and the cpu mode
check.

-Robert

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Suravee Suthikulanit]

On 5/8/2014 10:14 AM, Robert Richter wrote:
> On 08.05.14 09:39:55, Suravee Suthikulanit wrote:
>> On 5/8/2014 4:01 AM, Robert Richter wrote:
>>> On 08.05.14 10:59:05, Robert Richter wrote:
>>>> On 07.05.14 13:58:45, suravee.suthikulpanit@amd.com wrote:
>>>>> @@ -113,10 +122,17 @@ static int __init early_fill_mp_bus_info(void)
>>>>>   		info = alloc_pci_root_info(min_bus, max_bus, node, link);
>>>>>   	}
>>>>>
>>>>> +	/*
>>>>> +	 * The following code is only supported until Fam11h.
>>>>> +	 * Newer processors will depend on ACPI MCFG table instead.
>>>>> +	 */
>>>>> +	if (boot_cpu_data.x86 > 0x11)
>>>>> +		return 0;
>>>>> +
>>>>>   	/* get the default node and link for left over res */
>>>>
>>>> As this is the only substantial change of your patch, I would better
>>>> drop ther rest or at least split it in two patches. Should this change
>>>> also be for stable?
>>>
>>> Of course adding the hostbridge must be also part of the patch, didn't
>>> note this due to the other noise. See why the split would be good?
>>>
>>>>
>>>> -Robert
>>
>>
>> Robert,
>>
>> I have already added the hostbridge for family15h in this patch.
>>
>> +static struct amd_hostbridge hb_probes[] __initdata = {
>> +	{ 0, 0x18, 0x1100 }, /* K8 */
>> +	{ 0, 0x18, 0x1200 }, /* Family10h */
>> +	{ 0xff, 0, 0x1200 }, /* Family10h */
>> +	{ 0, 0x18, 0x1300 }, /* Family11h */
>> +	{ 0, 0x18, 0x1600 }, /* Family15h */ <--- HERE
>
> Yes, I noticed that, but later, thus my 2nd mail.
>
>>   };
>>
>> The rest of the changes are mostly comments, some minor renaming of
>> variables for clarity, and replace hardcode values with preprocessor macro.
>> If needed, I can split them.
>
> I just would drop it, you just need the fam15h device and the cpu mode
> check.
>
> -Robert
>
The reason I put it all these comments here is because it took us a 
while to discuss what to do with this file going forward. There were 
some confusions.  Therefore, I just want to document it here.

Also, the check for (boot_cpu_data.x86 > 0x11) was needed because it 
should not be done for family15h.

Suravee

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Robert Richter]

On 08.05.14 10:21:07, Suravee Suthikulanit wrote:
> The reason I put it all these comments here is because it took us a while to
> discuss what to do with this file going forward. There were some confusions.
> Therefore, I just want to document it here.
> 
> Also, the check for (boot_cpu_data.x86 > 0x11) was needed because it should
> not be done for family15h.

Yes, the only functional change of this patch is adding the bridge and
the family check, right? Basically:

+       { 0, 0x18, PCI_VENDOR_ID_AMD, 0x1600 },

and

+        /*
+         * The following code is only supported until Fam11h.
+         * Newer processors will depend on ACPI MCFG table instead.
+         */
+        if (boot_cpu_data.x86 > 0x11)
+                return 0;
+

This patch should stripped down to only those changes with a
split. And maybe this should be added to linux-stable?

All other rework is a different story... Can be done on top of this,
though I would drop it.

-Robert

Re: [PATCH V3 1/3] x86/PCI: Fix PCI root numa_node info on AMD family15h

[Myron Stowe]

On Thu, May 8, 2014 at 9:37 AM, Robert Richter <rric@kernel.org> wrote:
> On 08.05.14 10:21:07, Suravee Suthikulanit wrote:
>> The reason I put it all these comments here is because it took us a while to
>> discuss what to do with this file going forward. There were some confusions.
>> Therefore, I just want to document it here.

I agree.  Hopefully anyone getting into this in the future will be
able to find this thread in the archives as it has been enlightning

>>
>> Also, the check for (boot_cpu_data.x86 > 0x11) was needed because it should
>> not be done for family15h.
>
> Yes, the only functional change of this patch is adding the bridge and
> the family check, right? Basically:
>
> +       { 0, 0x18, PCI_VENDOR_ID_AMD, 0x1600 },
>
> and
>
> +        /*
> +         * The following code is only supported until Fam11h.
> +         * Newer processors will depend on ACPI MCFG table instead.
> +         */
> +        if (boot_cpu_data.x86 > 0x11)
> +                return 0;
> +
>
> This patch should stripped down to only those changes with a
> split. And maybe this should be added to linux-stable?
>
> All other rework is a different story... Can be done on top of this,
> though I would drop it.

I understand Robert's reasoning to split out the core changes (as
denoted above).  However, I *would* tend to follow on with an
additional subsequent patch that has the rest of the content as it
cleans this area up (as opposed to dropping it all together).  It
makes the code a little clearer and is basically little to no risk (no
functional change).

Myron
>
> -Robert
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html