[PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Benjamin Herrenschmidt]

Instead of the simpler

	pci_bus_size_bridges(bus);
	pci_bus_assign_resources(bus);

Use pci_assign_unassigned_root_bus_resources(). This should have no effect
as long as we are reassigning everything. Once we start honoring FW
resource allocations, this will bring up the "reallocation" feature
which can help making room for SR-IOV when necessary.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/arm64/kernel/pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index bb85e2f4603f..1419b1b4e9b9 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	if (!bus)
 		return NULL;
 
-	pci_bus_size_bridges(bus);
-	pci_bus_assign_resources(bus);
+	pci_assign_unassigned_root_bus_resources(bus);
 
 	list_for_each_entry(child, &bus->children, node)
 		pcie_bus_configure_settings(child);
-- 
2.17.1

[PATCH 2/4] pci: acpi: Read _DSM #5 from ACPI on root bridges

[Benjamin Herrenschmidt]

This reads _DSM #5 value on root bridges, and when 0, sets a "preserve_config"
flag in the host bridge structure indicating that the FW has requested that
the existing configuration be preserved.

The upcoming spec change to define _DSM #5 for host bridges states that
this should be the default behaviour, however doing so would be very
intrusive and break existing setups. So we leave the default to be
the existing behaviour.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/acpi/pci_root.c  | 23 +++++++++++++++++++++++
 include/linux/pci-acpi.h |  7 ++++---
 include/linux/pci.h      |  2 ++
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 39f5d172e84f..217b3916f0f1 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -881,6 +881,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	int node = acpi_get_node(device->handle);
 	struct pci_bus *bus;
 	struct pci_host_bridge *host_bridge;
+	union acpi_object *obj;
 
 	info->root = root;
 	info->bridge = device;
@@ -917,6 +918,28 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
 		host_bridge->native_ltr = 0;
 
+	/*
+	 * Invoke the PCI device specific method (_DSM) #5 'Ignore PCI Boot
+	 * Configuration', on the host bridge. This tells us whether the
+	 * firmware wants us to preserve or reassign the configuration of
+	 * the PCI resource tree for this root bridge.
+	 *
+	 * For now, we only care about the function being present and returning
+	 * 0, which we use to set a flag indicating that we'll preserve the
+	 * FW configuration.
+	 *
+	 * This diverges from the spec which states that 0 is also the default
+	 * in absence of _DSM #5. We do that today to work around the fact that
+	 * our arm64 code doesn't implement the right defaults otherwise. This
+	 * will be superseeded by a more thorough handling of _DSM #5 once the
+	 * resource survey code has been consolidated further.
+	 */
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
+	                        IGNORE_PCI_BOOT_CONFIG_DSM, NULL);
+	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
+		host_bridge->preserve_config = 1;
+	ACPI_FREE(obj);
+
 	pci_scan_child_bus(bus);
 	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
 				    info);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 8082b612f561..62b7fdcc661c 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -107,9 +107,10 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 #endif
 
 extern const guid_t pci_acpi_dsm_guid;
-#define DEVICE_LABEL_DSM	0x07
-#define RESET_DELAY_DSM		0x08
-#define FUNCTION_DELAY_DSM	0x09
+#define IGNORE_PCI_BOOT_CONFIG_DSM	0x05
+#define DEVICE_LABEL_DSM		0x07
+#define RESET_DELAY_DSM			0x08
+#define FUNCTION_DELAY_DSM		0x09
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index dd436da7eccc..c50389b8df3c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -506,6 +506,8 @@ struct pci_host_bridge {
 	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
+        unsigned int    preserve_config:1;	/* Preserve FW resources setup */
+
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,
-- 
2.17.1

Re: [PATCH 2/4] pci: acpi: Read _DSM #5 from ACPI on root bridges

[Bjorn Helgaas]

Match the subject line convention for this file, e.g.,

  PCI/ACPI: Evaluate PCI Boot Configuration _DSM

On Sat, Jun 15, 2019 at 10:23:57AM +1000, Benjamin Herrenschmidt wrote:
> This reads _DSM #5 value on root bridges, and when 0, sets a "preserve_config"
> flag in the host bridge structure indicating that the FW has requested that
> the existing configuration be preserved.

Wrap to 75 or so, so this doesn't overflow 80 columns when "git log" adds
its indentation.

> The upcoming spec change to define _DSM #5 for host bridges states that
> this should be the default behaviour, however doing so would be very
> intrusive and break existing setups. So we leave the default to be
> the existing behaviour.

This paragraph is incorrect.  Just remove it completely.

I would silently fix stuff like this if I applied these, but you can
do it if you repost these.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/acpi/pci_root.c  | 23 +++++++++++++++++++++++
>  include/linux/pci-acpi.h |  7 ++++---
>  include/linux/pci.h      |  2 ++
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 39f5d172e84f..217b3916f0f1 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -881,6 +881,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	int node = acpi_get_node(device->handle);
>  	struct pci_bus *bus;
>  	struct pci_host_bridge *host_bridge;
> +	union acpi_object *obj;
>  
>  	info->root = root;
>  	info->bridge = device;
> @@ -917,6 +918,28 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
>  		host_bridge->native_ltr = 0;
>  
> +	/*
> +	 * Invoke the PCI device specific method (_DSM) #5 'Ignore PCI Boot
> +	 * Configuration', on the host bridge. This tells us whether the
> +	 * firmware wants us to preserve or reassign the configuration of
> +	 * the PCI resource tree for this root bridge.
> +	 *
> +	 * For now, we only care about the function being present and returning
> +	 * 0, which we use to set a flag indicating that we'll preserve the
> +	 * FW configuration.
> +	 *
> +	 * This diverges from the spec which states that 0 is also the default
> +	 * in absence of _DSM #5. We do that today to work around the fact that
> +	 * our arm64 code doesn't implement the right defaults otherwise. This
> +	 * will be superseeded by a more thorough handling of _DSM #5 once the
> +	 * resource survey code has been consolidated further.
> +	 */
> +	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
> +	                        IGNORE_PCI_BOOT_CONFIG_DSM, NULL);
> +	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
> +		host_bridge->preserve_config = 1;
> +	ACPI_FREE(obj);
> +
>  	pci_scan_child_bus(bus);
>  	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
>  				    info);
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 8082b612f561..62b7fdcc661c 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -107,9 +107,10 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
>  #endif
>  
>  extern const guid_t pci_acpi_dsm_guid;
> -#define DEVICE_LABEL_DSM	0x07
> -#define RESET_DELAY_DSM		0x08
> -#define FUNCTION_DELAY_DSM	0x09
> +#define IGNORE_PCI_BOOT_CONFIG_DSM	0x05
> +#define DEVICE_LABEL_DSM		0x07
> +#define RESET_DELAY_DSM			0x08
> +#define FUNCTION_DELAY_DSM		0x09
>  
>  #else	/* CONFIG_ACPI */
>  static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index dd436da7eccc..c50389b8df3c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -506,6 +506,8 @@ struct pci_host_bridge {
>  	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
> +        unsigned int    preserve_config:1;	/* Preserve FW resources setup */
> +
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
>  			const struct resource *res,
> -- 
> 2.17.1
> 

[PATCH 3/4] pci: Do not auto-enable PCI reallocation when _DSM #5 returns 0

[Benjamin Herrenschmidt]

This prevents auto-enabling of bridges reallocation when the FW tells
us that the initial configuration must be preserved for a given host
bridge.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/pci/setup-bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0cdd5ff389de..049a5602b942 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1684,10 +1684,16 @@ static enum enable_type pci_realloc_detect(struct pci_bus *bus,
 					   enum enable_type enable_local)
 {
 	bool unassigned = false;
+	struct pci_host_bridge *hb;
 
 	if (enable_local != undefined)
 		return enable_local;
 
+	/* Don't realloc if ACPI tells us not to */
+	hb = pci_find_host_bridge(bus);
+	if (hb->preserve_config)
+		return auto_disabled;
+
 	pci_walk_bus(bus, iov_resources_unassigned, &unassigned);
 	if (unassigned)
 		return auto_enabled;
-- 
2.17.1

Re: [PATCH 3/4] pci: Do not auto-enable PCI reallocation when _DSM #5 returns 0

[Bjorn Helgaas]

  PCI: Don't auto-realloc if we're preserving firmware config

On Sat, Jun 15, 2019 at 10:23:58AM +1000, Benjamin Herrenschmidt wrote:
> This prevents auto-enabling of bridges reallocation when the FW tells
> us that the initial configuration must be preserved for a given host
> bridge.

"Prevent auto-enabling ..." to follow usual style of imperative mood in
commit logs.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/pci/setup-bus.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 0cdd5ff389de..049a5602b942 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1684,10 +1684,16 @@ static enum enable_type pci_realloc_detect(struct pci_bus *bus,
>  					   enum enable_type enable_local)
>  {
>  	bool unassigned = false;
> +	struct pci_host_bridge *hb;

Conventional variable names are "bridge" or "host".

>  	if (enable_local != undefined)
>  		return enable_local;
>  
> +	/* Don't realloc if ACPI tells us not to */

I'd drop the comment, since there might be other mechanisms, e.g., DT,
someday.

> +	hb = pci_find_host_bridge(bus);
> +	if (hb->preserve_config)
> +		return auto_disabled;
> +
>  	pci_walk_bus(bus, iov_resources_unassigned, &unassigned);
>  	if (unassigned)
>  		return auto_enabled;
> -- 
> 2.17.1
> 

[PATCH 4/4] arm64: pci: acpi: Preserve PCI resources configuration when asked by ACPI

[Benjamin Herrenschmidt]

When _DSM #5 returns 0 for a host bridge, we need to claim the existing
resources rather than reassign everything.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/arm64/kernel/pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 1419b1b4e9b9..a2c608a3fc41 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -168,6 +168,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	struct acpi_pci_generic_root_info *ri;
 	struct pci_bus *bus, *child;
 	struct acpi_pci_root_ops *root_ops;
+	struct pci_host_bridge *hb;
 
 	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
 	if (!ri)
@@ -193,6 +194,16 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	if (!bus)
 		return NULL;
 
+	hb = pci_find_host_bridge(bus);
+
+	/* If ACPI tells us to preserve the resource configuration, claim now */
+	if (hb->preserve_config)
+		pci_bus_claim_resources(bus);
+
+	/*
+	 * Assign whatever was left unassigned. If we didn't claim above, this will
+	 * reassign everything.
+	 */
 	pci_assign_unassigned_root_bus_resources(bus);
 
 	list_for_each_entry(child, &bus->children, node)
-- 
2.17.1

Re: [PATCH 4/4] arm64: pci: acpi: Preserve PCI resources configuration when asked by ACPI

[Lorenzo Pieralisi]

On Sat, Jun 15, 2019 at 10:23:59AM +1000, Benjamin Herrenschmidt wrote:
> When _DSM #5 returns 0 for a host bridge, we need to claim the existing

Nit: technically we do not know whether it is a _DSM #5 setting
hb->preserve_config or not, it is just a matter of rewording the log.

> resources rather than reassign everything.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/arm64/kernel/pci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1419b1b4e9b9..a2c608a3fc41 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -168,6 +168,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	struct acpi_pci_generic_root_info *ri;
>  	struct pci_bus *bus, *child;
>  	struct acpi_pci_root_ops *root_ops;
> +	struct pci_host_bridge *hb;
>  
>  	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
>  	if (!ri)
> @@ -193,6 +194,16 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!bus)
>  		return NULL;
>  
> +	hb = pci_find_host_bridge(bus);

to_pci_host_bridge(bus->bridge) would do but it is probably better to
leave it as-is.

> +
> +	/* If ACPI tells us to preserve the resource configuration, claim now */
> +	if (hb->preserve_config)
> +		pci_bus_claim_resources(bus);
> +
> +	/*
> +	 * Assign whatever was left unassigned. If we didn't claim above, this will
> +	 * reassign everything.
> +	 */
>  	pci_assign_unassigned_root_bus_resources(bus);
>  
>  	list_for_each_entry(child, &bus->children, node)

This is fine as far as we acknowledge that claiming resources
on a bus is what should be done to make them immutable.

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Re: [PATCH 4/4] arm64: pci: acpi: Preserve PCI resources configuration when asked by ACPI

[Benjamin Herrenschmidt]

On Fri, 2019-06-21 at 15:57 +0100, Lorenzo Pieralisi wrote:
> >        pci_assign_unassigned_root_bus_resources(bus);
> >   
> >        list_for_each_entry(child, &bus->children, node)
> 
> This is fine as far as we acknowledge that claiming resources
> on a bus is what should be done to make them immutable.

Well, as immuatable as it gets today. It's not perfect but it's a step
in the right direction. With the previous change in the series that
prevents auto-realloc when preserve_config is set, it will be
equivalent, in the current state of the code.

As part of my ongoing rework, I plan to look at making
IORESOURCE_PCI_FIXED more generally useful/robust, in which case we
could add that on top as well. That said, if we go down that path, I'm
keen on also adding a cmdline arg to ignore _DSM #5, if anything as a
test/diag tool when chasing problems caused by buggy BIOSes.

Cheers,
Ben.

Re: [PATCH 4/4] arm64: pci: acpi: Preserve PCI resources configuration when asked by ACPI

[Ard Biesheuvel]

On Sat, 15 Jun 2019 at 02:30, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> When _DSM #5 returns 0 for a host bridge, we need to claim the existing
> resources rather than reassign everything.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  arch/arm64/kernel/pci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1419b1b4e9b9..a2c608a3fc41 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -168,6 +168,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>         struct acpi_pci_generic_root_info *ri;
>         struct pci_bus *bus, *child;
>         struct acpi_pci_root_ops *root_ops;
> +       struct pci_host_bridge *hb;
>
>         ri = kzalloc(sizeof(*ri), GFP_KERNEL);
>         if (!ri)
> @@ -193,6 +194,16 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>         if (!bus)
>                 return NULL;
>
> +       hb = pci_find_host_bridge(bus);
> +
> +       /* If ACPI tells us to preserve the resource configuration, claim now */
> +       if (hb->preserve_config)
> +               pci_bus_claim_resources(bus);
> +
> +       /*
> +        * Assign whatever was left unassigned. If we didn't claim above, this will
> +        * reassign everything.
> +        */
>         pci_assign_unassigned_root_bus_resources(bus);
>
>         list_for_each_entry(child, &bus->children, node)
> --
> 2.17.1
>

Re: [PATCH 4/4] arm64: pci: acpi: Preserve PCI resources configuration when asked by ACPI

[Bjorn Helgaas]

  arm64: PCI: Preserve firmware configuration when necessary

On Sat, Jun 15, 2019 at 10:23:59AM +1000, Benjamin Herrenschmidt wrote:
> When _DSM #5 returns 0 for a host bridge, we need to claim the existing
> resources rather than reassign everything.

Use imperative mood.  I'd remove the reference to _DSM #5.  This patch
does not directly reference _DSM, and it's conceivable a kernel
command line parameter or other mechanism could set
host->preserve_config.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/arm64/kernel/pci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 1419b1b4e9b9..a2c608a3fc41 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -168,6 +168,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	struct acpi_pci_generic_root_info *ri;
>  	struct pci_bus *bus, *child;
>  	struct acpi_pci_root_ops *root_ops;
> +	struct pci_host_bridge *hb;
>  
>  	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
>  	if (!ri)
> @@ -193,6 +194,16 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!bus)
>  		return NULL;
>  
> +	hb = pci_find_host_bridge(bus);
> +
> +	/* If ACPI tells us to preserve the resource configuration, claim now */
> +	if (hb->preserve_config)
> +		pci_bus_claim_resources(bus);
> +
> +	/*
> +	 * Assign whatever was left unassigned. If we didn't claim above, this will
> +	 * reassign everything.

Wrap the comment so it fits in 80 columns (unless local arch/arm64 style
allows wider lines, but I don't see any other wide lines in the file).

This series generally looks good to me.

> +	 */
>  	pci_assign_unassigned_root_bus_resources(bus);
>  
>  	list_for_each_entry(child, &bus->children, node)
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Lorenzo Pieralisi]

On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote:
> Instead of the simpler
> 
> 	pci_bus_size_bridges(bus);
> 	pci_bus_assign_resources(bus);
> 
> Use pci_assign_unassigned_root_bus_resources(). This should have no effect
> as long as we are reassigning everything. Once we start honoring FW
> resource allocations, this will bring up the "reallocation" feature
> which can help making room for SR-IOV when necessary.

I would like to add more details on why we want to make this change,
I will update the log when we merge it, it is a bit too late for v5.3,
even if in theory no functional change is intended.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/arm64/kernel/pci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index bb85e2f4603f..1419b1b4e9b9 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!bus)
>  		return NULL;
>  
> -	pci_bus_size_bridges(bus);
> -	pci_bus_assign_resources(bus);
> +	pci_assign_unassigned_root_bus_resources(bus);

These hunks should be identical, minus the additional resource size
handling and realloc policy (which are *missing* features in current
code). We must document this change in the log.

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

>  	list_for_each_entry(child, &bus->children, node)
>  		pcie_bus_configure_settings(child);
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Benjamin Herrenschmidt]

On Thu, 2019-06-20 at 18:13 +0100, Lorenzo Pieralisi wrote:
> On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote:
> > Instead of the simpler
> > 
> > 	pci_bus_size_bridges(bus);
> > 	pci_bus_assign_resources(bus);
> > 
> > Use pci_assign_unassigned_root_bus_resources(). This should have no effect
> > as long as we are reassigning everything. Once we start honoring FW
> > resource allocations, this will bring up the "reallocation" feature
> > which can help making room for SR-IOV when necessary.
> 
> I would like to add more details on why we want to make this change,
> I will update the log when we merge it, it is a bit too late for v5.3,
> even if in theory no functional change is intended.

Ok. The why is that a subsequent patch will turn the code into

	if (claim)
		claim()
	pci_assign_unassigned*

And I wanted the patch replacing the existing 2 calls with the above
separate and bisectable in case we missed some odd effect of the
change.

Cheers,
Ben.

> 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  arch/arm64/kernel/pci.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index bb85e2f4603f..1419b1b4e9b9 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >  	if (!bus)
> >  		return NULL;
> >  
> > -	pci_bus_size_bridges(bus);
> > -	pci_bus_assign_resources(bus);
> > +	pci_assign_unassigned_root_bus_resources(bus);
> 
> These hunks should be identical, minus the additional resource size
> handling and realloc policy (which are *missing* features in current
> code). We must document this change in the log.
> 
> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> >  	list_for_each_entry(child, &bus->children, node)
> >  		pcie_bus_configure_settings(child);
> > -- 
> > 2.17.1
> > 

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Bjorn Helgaas]

Match the subject line convention, e.g.,

  arm64: PCI: Use pci_assign_unassigned_root_bus_resources()

But the function name doesn't really tell us anything unless we already
know how everything works.  I think the point is that
pci_assign_unassigned_root_bus_resources() gives us the possibility of
reallocating things if necessary.  A subject that hints at that would be
good.

On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote:
> Instead of the simpler
> 
> 	pci_bus_size_bridges(bus);
> 	pci_bus_assign_resources(bus);
> 
> Use pci_assign_unassigned_root_bus_resources(). This should have no
> effect as long as we are reassigning everything. 

  pci_bus_size_bridges(bus) == __pci_bus_size_bridges(bus, NULL)
  pci_bus_assign_resources(bus) == __pci_bus_assign_resources(bus, NULL, NULL)

and we have:

  pci_assign_unassigned_root_bus_resources()
  {
    ...
    __pci_bus_size_bridges(bus, add_list);
    __pci_bus_assign_resources(bus, add_list, &fail_head);

so I guess this should have no effect as long as we were able to
assign everything.  If we were unable to assign something, previously
we did nothing and left it unassigned, but after this patch, we will
attempt to do some reallocation.  Right?

> Once we start honoring FW resource allocations, this will bring up
> the "reallocation" feature which can help making room for SR-IOV
> when necessary.

I think this should be reordered so it's immediately before the patch
that checks hb->preserve_config, i.e., the patch that honors FW
assignments.

> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  arch/arm64/kernel/pci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index bb85e2f4603f..1419b1b4e9b9 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!bus)
>  		return NULL;
>  
> -	pci_bus_size_bridges(bus);
> -	pci_bus_assign_resources(bus);
> +	pci_assign_unassigned_root_bus_resources(bus);
>  
>  	list_for_each_entry(child, &bus->children, node)
>  		pcie_bus_configure_settings(child);
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Bjorn Helgaas]

On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote:
> Instead of the simpler
> 
> 	pci_bus_size_bridges(bus);
> 	pci_bus_assign_resources(bus);
> 
> Use pci_assign_unassigned_root_bus_resources(). This should have no effect
> as long as we are reassigning everything. Once we start honoring FW
> resource allocations, this will bring up the "reallocation" feature
> which can help making room for SR-IOV when necessary.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

I applied these to pci/resource, with my comments and acks from
Lorenzo and Ard.  Let me know if I was too aggressive or got something
wrong; I consider these branches malleable until the merge window.

Thanks for the first step on this long journey :)

> ---
>  arch/arm64/kernel/pci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index bb85e2f4603f..1419b1b4e9b9 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!bus)
>  		return NULL;
>  
> -	pci_bus_size_bridges(bus);
> -	pci_bus_assign_resources(bus);
> +	pci_assign_unassigned_root_bus_resources(bus);
>  
>  	list_for_each_entry(child, &bus->children, node)
>  		pcie_bus_configure_settings(child);
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Benjamin Herrenschmidt]

On Fri, 2019-06-21 at 15:48 -0500, Bjorn Helgaas wrote:
> On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt
> wrote:
> > Instead of the simpler
> > 
> > 	pci_bus_size_bridges(bus);
> > 	pci_bus_assign_resources(bus);
> > 
> > Use pci_assign_unassigned_root_bus_resources(). This should have no
> > effect
> > as long as we are reassigning everything. Once we start honoring FW
> > resource allocations, this will bring up the "reallocation" feature
> > which can help making room for SR-IOV when necessary.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> I applied these to pci/resource, with my comments and acks from
> Lorenzo and Ard.  Let me know if I was too aggressive or got
> something
> wrong; I consider these branches malleable until the merge window.
> 
> Thanks for the first step on this long journey :)

Heh thanks :-)

Cheers,
Ben.

> > ---
> >  arch/arm64/kernel/pci.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index bb85e2f4603f..1419b1b4e9b9 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct
> > acpi_pci_root *root)
> >  	if (!bus)
> >  		return NULL;
> >  
> > -	pci_bus_size_bridges(bus);
> > -	pci_bus_assign_resources(bus);
> > +	pci_assign_unassigned_root_bus_resources(bus);
> >  
> >  	list_for_each_entry(child, &bus->children, node)
> >  		pcie_bus_configure_settings(child);
> > -- 
> > 2.17.1
> > 

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Benjamin Herrenschmidt]

BTW...

You probably want to swap those 2:

2 hours	PCI/ACPI: Evaluate PCI Boot Configuration _DSM	Benjamin Herrenschmidt	3	-3/+18
2 hours	PCI: Don't auto-realloc if we're preserving firmware config

As "Don't auto-realloc..." tests a flag that is only created by "Evaluate PCI Boot..."

Cheers,
Ben.

Re: [PATCH 1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources()

[Bjorn Helgaas]

On Sat, Jun 22, 2019 at 09:00:50AM +1000, Benjamin Herrenschmidt wrote:
> BTW...
> 
> You probably want to swap those 2:
> 
> 2 hours	PCI/ACPI: Evaluate PCI Boot Configuration _DSM	Benjamin Herrenschmidt	3	-3/+18
> 2 hours	PCI: Don't auto-realloc if we're preserving firmware config
> 
> As "Don't auto-realloc..." tests a flag that is only created by "Evaluate PCI Boot..."

Ouch, thanks.  I don't know how I managed to swap those.