[RFC/RFT PATCH v2 0/3] PCI: generic device NUMA node detection

[RFC/RFT PATCH v2 0/3] PCI: generic device NUMA node detection

[Lorenzo Pieralisi]

This patch series is v2 of a previous posting:

v1 -> v2:

- Added missing call to acpi_pci_bus_find_numa_node()
- Rebased against v4.12-rc1

v1: https://lkml.org/lkml/2017/4/26/211

On the ARM64 architecture, the arch specific PCI pcibus_to_node() callback
can be triggered on struct pci_bus.sysdata structures that carry host
bridge specific data through different structures layout (ie sysdata
pointer points at different structs for different host bridges), therefore
it is not possible to have a unified pcibus_to_node() implementation.

Given that the device NUMA node is a property of the struct pci_bus (and
its associated struct device), move the struct pci_bus node assignment to
generic PCI code, where according to the platform firmware the NUMA node
for the device backing the struct pci_bus is retrieved and propagated
through the PCI bus hierarchy.

Lorenzo Pieralisi (3):
  PCI: Introduce pci_bus_find_numa_node()
  PCI: Add call to set-up NUMA node for struct pci_bus devices
  PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

 drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
 drivers/pci/pci.c      |  6 ++++++
 drivers/pci/probe.c    |  3 ++-
 include/linux/pci.h    |  7 +++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.10.0

[RFC/RFT PATCH v2 1/3] PCI: Introduce pci_bus_find_numa_node()

[Lorenzo Pieralisi]

Create an empty stub, to be populated by firmware methods, to
retrieve the NUMA node for a given pci_bus. No functional change
introduced.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c   | 6 ++++++
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..e9803c1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -12,6 +12,7 @@
 #include <linux/delay.h>
 #include <linux/dmi.h>
 #include <linux/init.h>
+#include <linux/numa.h>
 #include <linux/of.h>
 #include <linux/of_pci.h>
 #include <linux/pci.h>
@@ -5403,6 +5404,11 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
 #endif
 #endif
 
+int pci_bus_find_numa_node(struct pci_bus *bus)
+{
+	return NUMA_NO_NODE;
+}
+
 /**
  * pci_ext_cfg_avail - can we access extended PCI config space?
  *
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33c2b0b..ccbeefd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1472,6 +1472,7 @@ static inline int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
 #endif
 int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent);
 #endif
+int pci_bus_find_numa_node(struct pci_bus *bus);
 
 /* some architectures require additional setup to direct VGA traffic */
 typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode,
-- 
2.10.0

[RFC/RFT PATCH v2 2/3] PCI: Add call to set-up NUMA node for struct pci_bus devices

[Lorenzo Pieralisi]

The member dev in struct pci_bus represents the kernel struct
device representation of a PCI bus, inclusive of its NUMA node.
When a PCI host bridge is registered, initialize the corresponding
struct pci_bus.dev member NUMA node and add code in
pci_alloc_child_bus() to propagate the NUMA node set-up through the PCI
bus hierarchy (ie child nodes are in the same NUMA node as the parent).

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..76c089f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -770,6 +770,7 @@ int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	device_enable_async_suspend(bus->bridge);
 	pci_set_bus_of_node(bus);
 	pci_set_bus_msi_domain(bus);
+	set_dev_node(&bus->dev, pci_bus_find_numa_node(bus));
 
 	if (!parent)
 		set_dev_node(bus->bridge, pcibus_to_node(bus));
@@ -861,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
-
+	set_dev_node(&child->dev, dev_to_node(&parent->dev));
 	/*
 	 * Set up the primary, secondary and subordinate
 	 * bus numbers.
-- 
2.10.0

[RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Lorenzo Pieralisi]

The introduction of pci_bus_find_numa_node(pci_bus) allows at PCI
host bridge registration to detect the NUMA node for a given
struct pci_bus.dev. Implement an ACPI method that, through
the struct pci_bus.bridge ACPI companion, retrieve and return
the NUMA node corresponding to a given struct pci_bus.dev.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
 drivers/pci/pci.c      |  2 +-
 include/linux/pci.h    |  6 ++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0018603..915da79 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -10,6 +10,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/irqdomain.h>
+#include <linux/numa.h>
 #include <linux/pci.h>
 #include <linux/msi.h>
 #include <linux/pci_hotplug.h>
@@ -853,6 +854,25 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus)
 	return irq_find_matching_fwnode(fwnode, DOMAIN_BUS_PCI_MSI);
 }
 
+int acpi_pci_bus_find_numa_node(struct pci_bus *bus)
+{
+	int node = NUMA_NO_NODE;
+	struct device *bridge = get_device(bus->bridge);
+
+	if (!bridge)
+		return node;
+
+	if (has_acpi_companion(bridge)) {
+		struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
+
+		node = acpi_get_node(adev->handle);
+	}
+
+	put_device(bridge);
+
+	return node;
+}
+
 static int __init acpi_pci_init(void)
 {
 	int ret;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e9803c1..451342d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5406,7 +5406,7 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
 
 int pci_bus_find_numa_node(struct pci_bus *bus)
 {
-	return NUMA_NO_NODE;
+	return acpi_disabled ? NUMA_NO_NODE : acpi_pci_bus_find_numa_node(bus);
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ccbeefd..ed89220 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1472,6 +1472,12 @@ static inline int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
 #endif
 int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent);
 #endif
+#ifdef CONFIG_ACPI
+int acpi_pci_bus_find_numa_node(struct pci_bus *bus);
+#else
+static inline int acpi_pci_bus_find_numa_node(struct pci_bus *bus)
+{ return NUMA_NO_NODE; }
+#endif
 int pci_bus_find_numa_node(struct pci_bus *bus);
 
 /* some architectures require additional setup to direct VGA traffic */
-- 
2.10.0

Re: [RFC/RFT PATCH v2 0/3] PCI: generic device NUMA node detection

[Vadim Lomovtsev]

On Mon, May 15, 2017 at 02:22:02PM +0100, Lorenzo Pieralisi wrote:
> This patch series is v2 of a previous posting:
> 
> v1 -> v2:
> 
> - Added missing call to acpi_pci_bus_find_numa_node()
> - Rebased against v4.12-rc1
> 
> v1: https://lkml.org/lkml/2017/4/26/211
> 
> On the ARM64 architecture, the arch specific PCI pcibus_to_node() callback
> can be triggered on struct pci_bus.sysdata structures that carry host
> bridge specific data through different structures layout (ie sysdata
> pointer points at different structs for different host bridges), therefore
> it is not possible to have a unified pcibus_to_node() implementation.
> 
> Given that the device NUMA node is a property of the struct pci_bus (and
> its associated struct device), move the struct pci_bus node assignment to
> generic PCI code, where according to the platform firmware the NUMA node
> for the device backing the struct pci_bus is retrieved and propagated
> through the PCI bus hierarchy.
> 
> Lorenzo Pieralisi (3):
>   PCI: Introduce pci_bus_find_numa_node()
>   PCI: Add call to set-up NUMA node for struct pci_bus devices
>   PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation
> 
>  drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
>  drivers/pci/pci.c      |  6 ++++++
>  drivers/pci/probe.c    |  3 ++-
>  include/linux/pci.h    |  7 +++++++
>  4 files changed, 35 insertions(+), 1 deletion(-)
>

Tested at dual-socketed system with Cavium ThunderX SoC.
Tested with 4.11 kernel.
No issues observed, numa_node value set appropriately for pci devices.

Tested-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>

> -- 
> 2.10.0
> 


WBR,
Vadim

Re: [RFC/RFT PATCH v2 0/3] PCI: generic device NUMA node detection

[Robert Richter]

On 15.05.17 14:22:02, Lorenzo Pieralisi wrote:
> This patch series is v2 of a previous posting:
> 
> v1 -> v2:
> 
> - Added missing call to acpi_pci_bus_find_numa_node()
> - Rebased against v4.12-rc1
> 
> v1: https://lkml.org/lkml/2017/4/26/211
> 
> On the ARM64 architecture, the arch specific PCI pcibus_to_node() callback
> can be triggered on struct pci_bus.sysdata structures that carry host
> bridge specific data through different structures layout (ie sysdata
> pointer points at different structs for different host bridges), therefore
> it is not possible to have a unified pcibus_to_node() implementation.
> 
> Given that the device NUMA node is a property of the struct pci_bus (and
> its associated struct device), move the struct pci_bus node assignment to
> generic PCI code, where according to the platform firmware the NUMA node
> for the device backing the struct pci_bus is retrieved and propagated
> through the PCI bus hierarchy.
> 
> Lorenzo Pieralisi (3):
>   PCI: Introduce pci_bus_find_numa_node()
>   PCI: Add call to set-up NUMA node for struct pci_bus devices
>   PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation
> 
>  drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
>  drivers/pci/pci.c      |  6 ++++++
>  drivers/pci/probe.c    |  3 ++-
>  include/linux/pci.h    |  7 +++++++
>  4 files changed, 35 insertions(+), 1 deletion(-)

For the whole series:

Reviewed-by: Robert Richter <rrichter@cavium.com>
Tested-by: Robert Richter <rrichter@cavium.com>

Re: [RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Robert Richter]

On 15.05.17 14:22:05, Lorenzo Pieralisi wrote:
> The introduction of pci_bus_find_numa_node(pci_bus) allows at PCI
> host bridge registration to detect the NUMA node for a given
> struct pci_bus.dev. Implement an ACPI method that, through
> the struct pci_bus.bridge ACPI companion, retrieve and return
> the NUMA node corresponding to a given struct pci_bus.dev.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
>  drivers/pci/pci.c      |  2 +-
>  include/linux/pci.h    |  6 ++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e9803c1..451342d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5406,7 +5406,7 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
>  
>  int pci_bus_find_numa_node(struct pci_bus *bus)
>  {
> -	return NUMA_NO_NODE;
> +	return acpi_disabled ? NUMA_NO_NODE : acpi_pci_bus_find_numa_node(bus);

I looked into how this works with devicetree.

With ACPI it is set directly in pci_register_host_bridge() with
set_dev_node().

For the DT case the set_dev_node call sets it to NUMA_NO_NODE first.
Since in DT the bridge is a platform device which has the node id
assigned already (if there is one), the bus' node id is set later in
device_add() when deriving it from the parent device which is the
bridge. So this should work.

I have tested it both DT and ACPI.

Thanks,

-Robert

>  }
>  
>  /**

Re: [RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Lorenzo Pieralisi]

On Tue, May 16, 2017 at 05:15:29PM +0200, Robert Richter wrote:
> On 15.05.17 14:22:05, Lorenzo Pieralisi wrote:
> > The introduction of pci_bus_find_numa_node(pci_bus) allows at PCI
> > host bridge registration to detect the NUMA node for a given
> > struct pci_bus.dev. Implement an ACPI method that, through
> > the struct pci_bus.bridge ACPI companion, retrieve and return
> > the NUMA node corresponding to a given struct pci_bus.dev.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
> >  drivers/pci/pci.c      |  2 +-
> >  include/linux/pci.h    |  6 ++++++
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> > 
> 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index e9803c1..451342d 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5406,7 +5406,7 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> >  
> >  int pci_bus_find_numa_node(struct pci_bus *bus)
> >  {
> > -	return NUMA_NO_NODE;
> > +	return acpi_disabled ? NUMA_NO_NODE : acpi_pci_bus_find_numa_node(bus);
> 
> I looked into how this works with devicetree.
> 
> With ACPI it is set directly in pci_register_host_bridge() with
> set_dev_node().
> 
> For the DT case the set_dev_node call sets it to NUMA_NO_NODE first.
> Since in DT the bridge is a platform device which has the node id
> assigned already (if there is one), the bus' node id is set later in
> device_add() when deriving it from the parent device which is the
> bridge. So this should work.

Which also means that the node propagation for bus->dev in patch 2 can
be probably removed :), the problem with ACPI is setting the node
for the host bridge which in DT is done by default at platform device
creation, the rest is done by the core already there is not any need
to propagate it again when child busses are created (they take their
node from the parent).

Thanks for the heads-up and testing,
Lorenzo

> I have tested it both DT and ACPI.
> 
> Thanks,
> 
> -Robert
> 
> >  }
> >  
> >  /**

Re: [RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Lorenzo Pieralisi]

On Tue, May 16, 2017 at 07:02:00PM +0100, Lorenzo Pieralisi wrote:
> On Tue, May 16, 2017 at 05:15:29PM +0200, Robert Richter wrote:
> > On 15.05.17 14:22:05, Lorenzo Pieralisi wrote:
> > > The introduction of pci_bus_find_numa_node(pci_bus) allows at PCI
> > > host bridge registration to detect the NUMA node for a given
> > > struct pci_bus.dev. Implement an ACPI method that, through
> > > the struct pci_bus.bridge ACPI companion, retrieve and return
> > > the NUMA node corresponding to a given struct pci_bus.dev.
> > > 
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/pci-acpi.c | 20 ++++++++++++++++++++
> > >  drivers/pci/pci.c      |  2 +-
> > >  include/linux/pci.h    |  6 ++++++
> > >  3 files changed, 27 insertions(+), 1 deletion(-)
> > > 
> > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index e9803c1..451342d 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5406,7 +5406,7 @@ int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
> > >  
> > >  int pci_bus_find_numa_node(struct pci_bus *bus)
> > >  {
> > > -	return NUMA_NO_NODE;
> > > +	return acpi_disabled ? NUMA_NO_NODE : acpi_pci_bus_find_numa_node(bus);
> > 
> > I looked into how this works with devicetree.
> > 
> > With ACPI it is set directly in pci_register_host_bridge() with
> > set_dev_node().
> > 
> > For the DT case the set_dev_node call sets it to NUMA_NO_NODE first.
> > Since in DT the bridge is a platform device which has the node id
> > assigned already (if there is one), the bus' node id is set later in
> > device_add() when deriving it from the parent device which is the
> > bridge. So this should work.
> 
> Which also means that the node propagation for bus->dev in patch 2 can
> be probably removed :), the problem with ACPI is setting the node
> for the host bridge which in DT is done by default at platform device
> creation, the rest is done by the core already there is not any need
> to propagate it again when child busses are created (they take their
> node from the parent).

More explicitly, I think the whole series should work also with the diff
below applied on top of it. Side note: for consistency, I do not think
that adding a DT counterpart to pci_bus_find_numa_node() would hurt.

Thanks !
Lorenzo

-- >8 --
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 76c089f..cf0692c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	 */
 	child->dev.class = &pcibus_class;
 	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
-	set_dev_node(&child->dev, dev_to_node(&parent->dev));
+
 	/*
 	 * Set up the primary, secondary and subordinate
 	 * bus numbers.

Re: [RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Robert Richter]

On 17.05.17 14:46:54, Lorenzo Pieralisi wrote:

> More explicitly, I think the whole series should work also with the diff
> below applied on top of it. Side note: for consistency, I do not think
> that adding a DT counterpart to pci_bus_find_numa_node() would hurt.
> 
> Thanks !
> Lorenzo
> 
> -- >8 --
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 76c089f..cf0692c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  	 */
>  	child->dev.class = &pcibus_class;
>  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> -	set_dev_node(&child->dev, dev_to_node(&parent->dev));
> +

Hmm, in device_add() there is already:

	/* use parent numa_node */
	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
        	set_dev_node(dev, dev_to_node(parent));

So there are cases where the device has a different node than the
parent. I am not sure if we can assume for pci that it maps always.

And since device_add() is called later anyway, the above change might
not necessary at all. But at least we must assign the node id to the
bridge, which is the parent. Maybe just have in
acpi_pci_root_create():

	bridge = get_device(bus->bridge);
	adev = to_acpi_device_node(bridge->fwnode);
	set_dev_node(bridge, acpi_get_node(adev->handle));

-Robert


>  	/*
>  	 * Set up the primary, secondary and subordinate
>  	 * bus numbers.

Re: [RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Lorenzo Pieralisi]

On Wed, May 17, 2017 at 04:35:58PM +0200, Robert Richter wrote:
> On 17.05.17 14:46:54, Lorenzo Pieralisi wrote:
> 
> > More explicitly, I think the whole series should work also with the diff
> > below applied on top of it. Side note: for consistency, I do not think
> > that adding a DT counterpart to pci_bus_find_numa_node() would hurt.
> > 
> > Thanks !
> > Lorenzo
> > 
> > -- >8 --
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 76c089f..cf0692c 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >  	 */
> >  	child->dev.class = &pcibus_class;
> >  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> > -	set_dev_node(&child->dev, dev_to_node(&parent->dev));
> > +
> 
> Hmm, in device_add() there is already:
> 
> 	/* use parent numa_node */
> 	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
>         	set_dev_node(dev, dev_to_node(parent));
> 
> So there are cases where the device has a different node than the
> parent. I am not sure if we can assume for pci that it maps always.
> 
> And since device_add() is called later anyway, the above change might
> not necessary at all.

That's why I _removed_ the set_dev_node() in the diff above (that applies
to patch (2)), I do not think it is a) correct and b) necessary to
propagate the NUMA node from bus to a child bus given that device_add()
takes care of that already.

I should post a v3 (with the diff above applied) so that we are all on
the same page and we can test it.

> But at least we must assign the node id to the
> bridge, which is the parent. Maybe just have in
> acpi_pci_root_create():
> 
> 	bridge = get_device(bus->bridge);
> 	adev = to_acpi_device_node(bridge->fwnode);
> 	set_dev_node(bridge, acpi_get_node(adev->handle));

I do not think that's enough, I need to check again but I think that
also the bus->dev should have its NUMA node set for things to work (and
allow the NUMA node to propagate correctly through device_add())
otherwise pcibus_to_node() would fail for devices sitting on the root
bus, right ?

I will check again and post v3 shortly.

Thanks !
Lorenzo

> 
> -Robert
> 
> 
> >  	/*
> >  	 * Set up the primary, secondary and subordinate
> >  	 * bus numbers.

Re: [RFC/RFT PATCH v2 3/3] PCI/ACPI: Add ACPI pci_bus_find_numa_node() implementation

[Robert Richter]

On 17.05.17 17:04:24, Lorenzo Pieralisi wrote:
> On Wed, May 17, 2017 at 04:35:58PM +0200, Robert Richter wrote:
> > On 17.05.17 14:46:54, Lorenzo Pieralisi wrote:
> > 
> > > More explicitly, I think the whole series should work also with the diff
> > > below applied on top of it. Side note: for consistency, I do not think
> > > that adding a DT counterpart to pci_bus_find_numa_node() would hurt.
> > > 
> > > Thanks !
> > > Lorenzo
> > > 
> > > -- >8 --
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 76c089f..cf0692c 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -862,7 +862,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> > >  	 */
> > >  	child->dev.class = &pcibus_class;
> > >  	dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
> > > -	set_dev_node(&child->dev, dev_to_node(&parent->dev));
> > > +
> > 
> > Hmm, in device_add() there is already:
> > 
> > 	/* use parent numa_node */
> > 	if (parent && (dev_to_node(dev) == NUMA_NO_NODE))
> >         	set_dev_node(dev, dev_to_node(parent));
> > 
> > So there are cases where the device has a different node than the
> > parent. I am not sure if we can assume for pci that it maps always.
> > 
> > And since device_add() is called later anyway, the above change might
> > not necessary at all.
> 
> That's why I _removed_ the set_dev_node() in the diff above (that applies
> to patch (2)), I do not think it is a) correct and b) necessary to
> propagate the NUMA node from bus to a child bus given that device_add()
> takes care of that already.

Ah, right, you removed it instead.

> 
> I should post a v3 (with the diff above applied) so that we are all on
> the same page and we can test it.
> 
> > But at least we must assign the node id to the
> > bridge, which is the parent. Maybe just have in
> > acpi_pci_root_create():
> > 
> > 	bridge = get_device(bus->bridge);
> > 	adev = to_acpi_device_node(bridge->fwnode);
> > 	set_dev_node(bridge, acpi_get_node(adev->handle));
> 
> I do not think that's enough, I need to check again but I think that
> also the bus->dev should have its NUMA node set for things to work (and
> allow the NUMA node to propagate correctly through device_add())
> otherwise pcibus_to_node() would fail for devices sitting on the root
> bus, right ?

Yeah, maybe another hop is in between and the node id for bus->dev is
used. Which need to be set then.

> 
> I will check again and post v3 shortly.

Thanks,

-Robert