[v4] PCI: Add support for preserving boot configuration

[PATCH V4] PCI: Add support for preserving boot configuration

Posted by Vidya Sagar 1 year, 11 months ago

Add support for preserving the boot configuration done by the
platform firmware per host bridge basis, based on the presence of
'linux,pci-probe-only' property in the respective PCI host bridge
device-tree node. It also unifies the ACPI and DT based boot flows
in this regard.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* Addressed Bjorn's review comments

V3:
* Unified ACPI and DT flows as part of addressing Bjorn's review comments

V2:
* Addressed issues reported by kernel test robot <lkp@intel.com>

 drivers/acpi/pci_root.c                  | 12 -------
 drivers/pci/controller/pci-host-common.c |  4 ---
 drivers/pci/of.c                         | 21 +++++++++++
 drivers/pci/probe.c                      | 46 ++++++++++++++++++------
 include/linux/of_pci.h                   |  6 ++++
 5 files changed, 62 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 84030804a763..ddc2b3e89111 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1008,7 +1008,6 @@ 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;
@@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL))
 		host_bridge->native_cxl_error = 0;
 
-	/*
-	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
-	 * exists and returns 0, we must preserve any PCI resource
-	 * assignments made by firmware for this host bridge.
-	 */
-	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
-				DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
-	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
-		host_bridge->preserve_config = 1;
-	ACPI_FREE(obj);
-
 	acpi_dev_power_up_children_with_adr(device);
 
 	pci_scan_child_bus(bus);
diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 6be3266cd7b5..e2602e38ae45 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
 	if (IS_ERR(cfg))
 		return PTR_ERR(cfg);
 
-	/* Do not reassign resources if probe only */
-	if (!pci_has_flag(PCI_PROBE_ONLY))
-		pci_add_flags(PCI_REASSIGN_ALL_BUS);
-
 	bridge->sysdata = cfg;
 	bridge->ops = (struct pci_ops *)&ops->pci_ops;
 	bridge->msi_domain = true;
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5ab..f0f1156040a5 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -258,6 +258,27 @@ void of_pci_check_probe_only(void)
 }
 EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
 
+/**
+ * of_pci_bridge_preserve_resources - Return true if the boot configuration
+ *                                    needs to be preserved
+ * @node: Device tree node with the domain information.
+ *
+ * This function looks for "linux,pci-probe-only" property for a given
+ * PCI controller's node and returns true if found. Having this property
+ * for a PCI controller ensures that the kernel doesn't reconfigure the
+ * BARs and bridge windows that are already done by the platform firmware.
+ * NOTE: The scope of "linux,pci-probe-only" defined within a PCI bridge device
+ *       is limited to the hierarchy under that particular bridge device. whereas
+ *       the scope of "linux,pci-probe-only" defined within chosen node is
+ *       system wide.
+ *
+ * Return: true if the property exists false otherwise.
+ */
+bool of_pci_bridge_preserve_resources(struct device_node *node)
+{
+	return of_property_read_bool(node, "linux,pci-probe-only");
+}
+
 /**
  * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI
  *                                           host bridge resources from DT
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 795534589b98..c57648084503 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -15,6 +15,7 @@
 #include <linux/cpumask.h>
 #include <linux/aer.h>
 #include <linux/acpi.h>
+#include <linux/pci-acpi.h>
 #include <linux/hypervisor.h>
 #include <linux/irqdomain.h>
 #include <linux/pm_runtime.h>
@@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
 	dev_set_msi_domain(&bus->dev, d);
 }
 
+static void pci_check_config_preserve(struct pci_host_bridge *host_bridge)
+{
+	if (ACPI_HANDLE(&host_bridge->dev)) {
+		union acpi_object *obj;
+
+		/*
+		 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
+		 * exists and returns 0, we must preserve any PCI resource
+		 * assignments made by firmware for this host bridge.
+		 */
+		obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1,
+					DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
+		if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
+			host_bridge->preserve_config = 1;
+		ACPI_FREE(obj);
+	}
+
+	if (host_bridge->dev.parent && host_bridge->dev.parent->of_node)
+		host_bridge->preserve_config =
+			of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node);
+}
+
 static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 {
 	struct device *parent = bridge->dev.parent;
@@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE)
 		dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n");
 
+	/* Check if the boot configuration by FW needs to be preserved */
+	pci_check_config_preserve(bridge);
+
 	/* Coalesce contiguous windows */
 	resource_list_for_each_entry_safe(window, n, &resources) {
 		if (list_is_last(&window->node, &resources))
@@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge)
 
 	bus = bridge->bus;
 
+	/* If we must preserve the resource configuration, claim now */
+	if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config)
+		pci_bus_claim_resources(bus);
+
 	/*
-	 * We insert PCI resources into the iomem_resource and
-	 * ioport_resource trees in either pci_bus_claim_resources()
-	 * or pci_bus_assign_resources().
+	 * Assign whatever was left unassigned. If we didn't claim above,
+	 * this will reassign everything.
 	 */
-	if (pci_has_flag(PCI_PROBE_ONLY)) {
-		pci_bus_claim_resources(bus);
-	} else {
-		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);
-	}
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
 
 	pci_bus_add_devices(bus);
 	return 0;
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 29658c0ee71f..3f3909a5d55d 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn);
 int of_pci_get_devfn(struct device_node *np);
 void of_pci_check_probe_only(void);
+bool of_pci_bridge_preserve_resources(struct device_node *node);
 #else
 static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn)
@@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np)
 }
 
 static inline void of_pci_check_probe_only(void) { }
+
+static inline bool of_pci_bridge_preserve_resources(struct device_node *node)
+{
+	return false;
+}
 #endif
 
 #if IS_ENABLED(CONFIG_OF_IRQ)
-- 
2.25.1

Re: [PATCH V4] PCI: Add support for preserving boot configuration

Posted by Rob Herring 1 year, 11 months ago

On Fri, Feb 23, 2024 at 01:30:21PM +0530, Vidya Sagar wrote:
> Add support for preserving the boot configuration done by the
> platform firmware per host bridge basis, based on the presence of
> 'linux,pci-probe-only' property in the respective PCI host bridge
> device-tree node. It also unifies the ACPI and DT based boot flows
> in this regard.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * Addressed Bjorn's review comments
> 
> V3:
> * Unified ACPI and DT flows as part of addressing Bjorn's review comments
> 
> V2:
> * Addressed issues reported by kernel test robot <lkp@intel.com>
> 
>  drivers/acpi/pci_root.c                  | 12 -------
>  drivers/pci/controller/pci-host-common.c |  4 ---
>  drivers/pci/of.c                         | 21 +++++++++++
>  drivers/pci/probe.c                      | 46 ++++++++++++++++++------
>  include/linux/of_pci.h                   |  6 ++++
>  5 files changed, 62 insertions(+), 27 deletions(-)

One more thing.

> @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge)
>  
>  	bus = bridge->bus;
>  
> +	/* If we must preserve the resource configuration, claim now */
> +	if (pci_has_flag(PCI_PROBE_ONLY) || bridge->preserve_config)
> +		pci_bus_claim_resources(bus);

No reason to check PCI_PROBE_ONLY if you set preserve_config based on 
/chosen as well. IOW, we should deprecate PCI_PROBE_ONLY flag in favor 
of the per host bridge setting.

Rob

Re: [PATCH V4] PCI: Add support for preserving boot configuration

Posted by Rob Herring 1 year, 11 months ago

On Fri, Feb 23, 2024 at 01:30:21PM +0530, Vidya Sagar wrote:
> Add support for preserving the boot configuration done by the
> platform firmware per host bridge basis, based on the presence of
> 'linux,pci-probe-only' property in the respective PCI host bridge
> device-tree node. It also unifies the ACPI and DT based boot flows
> in this regard.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * Addressed Bjorn's review comments
> 
> V3:
> * Unified ACPI and DT flows as part of addressing Bjorn's review comments
> 
> V2:
> * Addressed issues reported by kernel test robot <lkp@intel.com>
> 
>  drivers/acpi/pci_root.c                  | 12 -------
>  drivers/pci/controller/pci-host-common.c |  4 ---
>  drivers/pci/of.c                         | 21 +++++++++++
>  drivers/pci/probe.c                      | 46 ++++++++++++++++++------
>  include/linux/of_pci.h                   |  6 ++++
>  5 files changed, 62 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 84030804a763..ddc2b3e89111 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1008,7 +1008,6 @@ 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;
> @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL))
>  		host_bridge->native_cxl_error = 0;
>  
> -	/*
> -	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> -	 * exists and returns 0, we must preserve any PCI resource
> -	 * assignments made by firmware for this host bridge.
> -	 */
> -	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
> -				DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
> -	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
> -		host_bridge->preserve_config = 1;
> -	ACPI_FREE(obj);
> -
>  	acpi_dev_power_up_children_with_adr(device);
>  
>  	pci_scan_child_bus(bus);
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index 6be3266cd7b5..e2602e38ae45 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
>  	if (IS_ERR(cfg))
>  		return PTR_ERR(cfg);
>  
> -	/* Do not reassign resources if probe only */
> -	if (!pci_has_flag(PCI_PROBE_ONLY))
> -		pci_add_flags(PCI_REASSIGN_ALL_BUS);
> -
>  	bridge->sysdata = cfg;
>  	bridge->ops = (struct pci_ops *)&ops->pci_ops;
>  	bridge->msi_domain = true;
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 51e3dd0ea5ab..f0f1156040a5 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -258,6 +258,27 @@ void of_pci_check_probe_only(void)
>  }
>  EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
>  
> +/**
> + * of_pci_bridge_preserve_resources - Return true if the boot configuration
> + *                                    needs to be preserved
> + * @node: Device tree node with the domain information.
> + *
> + * This function looks for "linux,pci-probe-only" property for a given
> + * PCI controller's node and returns true if found. Having this property
> + * for a PCI controller ensures that the kernel doesn't reconfigure the
> + * BARs and bridge windows that are already done by the platform firmware.
> + * NOTE: The scope of "linux,pci-probe-only" defined within a PCI bridge device
> + *       is limited to the hierarchy under that particular bridge device. whereas
> + *       the scope of "linux,pci-probe-only" defined within chosen node is
> + *       system wide.
> + *
> + * Return: true if the property exists false otherwise.
> + */
> +bool of_pci_bridge_preserve_resources(struct device_node *node)
> +{
> +	return of_property_read_bool(node, "linux,pci-probe-only");

This is the wrong type. The existing "linux,pci-probe-only" is a u32 and 
non-zero value means probe-only. This would return true for 
'linux,pci-probe-only = <0>'.

Also, this should also check chosen. If you make this work accepting 
NULL for node, then of_pci_check_probe_only() can be re-implemented to 
use it.



Rob

[PATCH V5] PCI: Add support for preserving boot configuration

Posted by Vidya Sagar 1 year, 10 months ago

Add support for preserving the boot configuration done by the
platform firmware per host bridge basis, based on the presence of
'linux,pci-probe-only' property in the respective PCI host bridge
device-tree node. It also unifies the ACPI and DT based boot flows
in this regard.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V5:
* Addressed Rob's review comments

V4:
* Addressed Bjorn's review comments

V3:
* Unified ACPI and DT flows as part of addressing Bjorn's review comments

V2:
* Addressed issues reported by kernel test robot <lkp@intel.com>

 drivers/acpi/pci_root.c                  | 12 -----
 drivers/pci/controller/pci-host-common.c |  4 --
 drivers/pci/of.c                         | 57 +++++++++++++++++++-----
 drivers/pci/probe.c                      | 46 ++++++++++++++-----
 include/linux/of_pci.h                   |  6 +++
 5 files changed, 88 insertions(+), 37 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 84030804a763..ddc2b3e89111 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1008,7 +1008,6 @@ 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;
@@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL))
 		host_bridge->native_cxl_error = 0;
 
-	/*
-	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
-	 * exists and returns 0, we must preserve any PCI resource
-	 * assignments made by firmware for this host bridge.
-	 */
-	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
-				DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
-	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
-		host_bridge->preserve_config = 1;
-	ACPI_FREE(obj);
-
 	acpi_dev_power_up_children_with_adr(device);
 
 	pci_scan_child_bus(bus);
diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 6be3266cd7b5..e2602e38ae45 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
 	if (IS_ERR(cfg))
 		return PTR_ERR(cfg);
 
-	/* Do not reassign resources if probe only */
-	if (!pci_has_flag(PCI_PROBE_ONLY))
-		pci_add_flags(PCI_REASSIGN_ALL_BUS);
-
 	bridge->sysdata = cfg;
 	bridge->ops = (struct pci_ops *)&ops->pci_ops;
 	bridge->msi_domain = true;
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5ab..e6da3654f9ac 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
  */
 void of_pci_check_probe_only(void)
 {
-	u32 val;
+	bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen);
+
+	if (is_preserve_config)
+		pci_add_flags(PCI_PROBE_ONLY);
+	else
+		pci_clear_flags(PCI_PROBE_ONLY);
+
+	pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled");
+}
+EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
+
+/**
+ * of_pci_bridge_preserve_resources - Return true if the boot configuration
+ *                                    needs to be preserved
+ * @node: Device tree node.
+ *
+ * This function looks for "linux,pci-probe-only" property for a given
+ * PCI controller's node and returns true if found. It will also look in the
+ * chosen node if the property is not found in the given controller's node.
+ * Having this property ensures that the kernel doesn't reconfigure the
+ * BARs and bridge windows that are already done by the platform firmware.
+ *
+ * Return: true if the property exists false otherwise.
+ */
+bool of_pci_bridge_preserve_resources(struct device_node *node)
+{
+	u32 val = 0;
 	int ret;
 
-	ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val);
+	if (!node) {
+		pr_warn("device node is NULL, trying with of_chosen\n");
+		node = of_chosen;
+	}
+
+retry:
+	ret = of_property_read_u32(node, "linux,pci-probe-only", &val);
 	if (ret) {
-		if (ret == -ENODATA || ret == -EOVERFLOW)
-			pr_warn("linux,pci-probe-only without valid value, ignoring\n");
-		return;
+		if (ret == -ENODATA || ret == -EOVERFLOW) {
+			pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node);
+			return false;
+		}
+		if (ret == -EINVAL) {
+			if (node == of_chosen)
+				return false;
+
+			node = of_chosen;
+			goto retry;
+		}
 	}
 
 	if (val)
-		pci_add_flags(PCI_PROBE_ONLY);
+		return true;
 	else
-		pci_clear_flags(PCI_PROBE_ONLY);
-
-	pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled");
+		return false;
 }
-EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
 
 /**
  * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 795534589b98..b0e0226a8da8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -15,6 +15,7 @@
 #include <linux/cpumask.h>
 #include <linux/aer.h>
 #include <linux/acpi.h>
+#include <linux/pci-acpi.h>
 #include <linux/hypervisor.h>
 #include <linux/irqdomain.h>
 #include <linux/pm_runtime.h>
@@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
 	dev_set_msi_domain(&bus->dev, d);
 }
 
+static void pci_check_config_preserve(struct pci_host_bridge *host_bridge)
+{
+	if (ACPI_HANDLE(&host_bridge->dev)) {
+		union acpi_object *obj;
+
+		/*
+		 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
+		 * exists and returns 0, we must preserve any PCI resource
+		 * assignments made by firmware for this host bridge.
+		 */
+		obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1,
+					DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
+		if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
+			host_bridge->preserve_config = 1;
+		ACPI_FREE(obj);
+	}
+
+	if (host_bridge->dev.parent && host_bridge->dev.parent->of_node)
+		host_bridge->preserve_config =
+			of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node);
+}
+
 static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 {
 	struct device *parent = bridge->dev.parent;
@@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE)
 		dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n");
 
+	/* Check if the boot configuration by FW needs to be preserved */
+	pci_check_config_preserve(bridge);
+
 	/* Coalesce contiguous windows */
 	resource_list_for_each_entry_safe(window, n, &resources) {
 		if (list_is_last(&window->node, &resources))
@@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge)
 
 	bus = bridge->bus;
 
+	/* If we must preserve the resource configuration, claim now */
+	if (bridge->preserve_config)
+		pci_bus_claim_resources(bus);
+
 	/*
-	 * We insert PCI resources into the iomem_resource and
-	 * ioport_resource trees in either pci_bus_claim_resources()
-	 * or pci_bus_assign_resources().
+	 * Assign whatever was left unassigned. If we didn't claim above,
+	 * this will reassign everything.
 	 */
-	if (pci_has_flag(PCI_PROBE_ONLY)) {
-		pci_bus_claim_resources(bus);
-	} else {
-		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);
-	}
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
 
 	pci_bus_add_devices(bus);
 	return 0;
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 29658c0ee71f..3f3909a5d55d 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn);
 int of_pci_get_devfn(struct device_node *np);
 void of_pci_check_probe_only(void);
+bool of_pci_bridge_preserve_resources(struct device_node *node);
 #else
 static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
 					     unsigned int devfn)
@@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np)
 }
 
 static inline void of_pci_check_probe_only(void) { }
+
+static inline bool of_pci_bridge_preserve_resources(struct device_node *node)
+{
+	return false;
+}
 #endif
 
 #if IS_ENABLED(CONFIG_OF_IRQ)
-- 
2.25.1

Re: [PATCH V5] PCI: Add support for preserving boot configuration

Posted by Bjorn Helgaas 1 year, 10 months ago

On Mon, Apr 01, 2024 at 01:20:31PM +0530, Vidya Sagar wrote:
> Add support for preserving the boot configuration done by the
> platform firmware per host bridge basis, based on the presence of
> 'linux,pci-probe-only' property in the respective PCI host bridge
> device-tree node. It also unifies the ACPI and DT based boot flows
> in this regard.

>  drivers/acpi/pci_root.c                  | 12 -----
>  drivers/pci/controller/pci-host-common.c |  4 --
>  drivers/pci/of.c                         | 57 +++++++++++++++++++-----
>  drivers/pci/probe.c                      | 46 ++++++++++++++-----
>  include/linux/of_pci.h                   |  6 +++
>  5 files changed, 88 insertions(+), 37 deletions(-)

What does this apply to?  I tried v6.9-rc1:

  $ git checkout -b wip/2404-vidya-preserve-boot-v5 v6.9-rc1
  Switched to a new branch 'wip/2404-vidya-preserve-boot-v5'

  $ git am m/v5_20240401_vidyas_pci_add_support_for_preserving_boot_configuration.mbx
  Applying: PCI: Add support for preserving boot configuration
  error: patch failed: drivers/acpi/pci_root.c:1050
  error: drivers/acpi/pci_root.c: patch does not apply
  Patch failed at 0001 PCI: Add support for preserving boot configuration

> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 84030804a763..ddc2b3e89111 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -1008,7 +1008,6 @@ 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;
> @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  	if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL))
>  		host_bridge->native_cxl_error = 0;
>  
> -	/*
> -	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> -	 * exists and returns 0, we must preserve any PCI resource
> -	 * assignments made by firmware for this host bridge.
> -	 */
> -	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
> -				DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
> -	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
> -		host_bridge->preserve_config = 1;
> -	ACPI_FREE(obj);
> -
>  	acpi_dev_power_up_children_with_adr(device);
>  
>  	pci_scan_child_bus(bus);
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index 6be3266cd7b5..e2602e38ae45 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
>  	if (IS_ERR(cfg))
>  		return PTR_ERR(cfg);
>  
> -	/* Do not reassign resources if probe only */
> -	if (!pci_has_flag(PCI_PROBE_ONLY))
> -		pci_add_flags(PCI_REASSIGN_ALL_BUS);
> -
>  	bridge->sysdata = cfg;
>  	bridge->ops = (struct pci_ops *)&ops->pci_ops;
>  	bridge->msi_domain = true;
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 51e3dd0ea5ab..e6da3654f9ac 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
>   */
>  void of_pci_check_probe_only(void)
>  {
> -	u32 val;
> +	bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen);

No need for a bool here:

  if (of_pci_bridge_preserve_resources(of_chosen))
    pci_add_flags(PCI_PROBE_ONLY);

I think it would make more sense to add
of_pci_bridge_preserve_resources() *above* of_pci_check_probe_only()
since the usual order is to define functions earlier in the file than
the calls.

> +
> +	if (is_preserve_config)
> +		pci_add_flags(PCI_PROBE_ONLY);
> +	else
> +		pci_clear_flags(PCI_PROBE_ONLY);

Not related to *this* patch, but I see that of_pci_check_probe_only()
already clears PCI_PROBE_ONLY (added by f81c11af617c ("of/pci: Add
of_pci_check_probe_only to parse "linux,pci-probe-only"").

I'm concerned about clearing PCI_PROBE_ONLY because some platforms set
this unconditionally, and I don't think they necessarily have
"linux,pci-probe-only" in DT.

Apparently none of them currently calls of_pci_check_probe_only() so
PCI_PROBE_ONLY remains set, but clearing it here feels like a landmine
waiting for somebody to move this into a unified call path.

I guess if we were to drop pci_clear_flags(), that should be a
separate patch in case it breaks something.

> +	pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled");
> +}
> +EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
> +
> +/**
> + * of_pci_bridge_preserve_resources - Return true if the boot configuration
> + *                                    needs to be preserved
> + * @node: Device tree node.
> + *
> + * This function looks for "linux,pci-probe-only" property for a given
> + * PCI controller's node and returns true if found. It will also look in the
> + * chosen node if the property is not found in the given controller's node.
> + * Having this property ensures that the kernel doesn't reconfigure the
> + * BARs and bridge windows that are already done by the platform firmware.
> + *
> + * Return: true if the property exists false otherwise.
> + */
> +bool of_pci_bridge_preserve_resources(struct device_node *node)
> +{
> +	u32 val = 0;
>  	int ret;
>  
> -	ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val);
> +	if (!node) {
> +		pr_warn("device node is NULL, trying with of_chosen\n");
> +		node = of_chosen;
> +	}
> +
> +retry:
> +	ret = of_property_read_u32(node, "linux,pci-probe-only", &val);
>  	if (ret) {
> -		if (ret == -ENODATA || ret == -EOVERFLOW)
> -			pr_warn("linux,pci-probe-only without valid value, ignoring\n");
> -		return;
> +		if (ret == -ENODATA || ret == -EOVERFLOW) {
> +			pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node);
> +			return false;
> +		}
> +		if (ret == -EINVAL) {
> +			if (node == of_chosen)
> +				return false;
> +
> +			node = of_chosen;
> +			goto retry;
> +		}
>  	}
>  
>  	if (val)
> -		pci_add_flags(PCI_PROBE_ONLY);
> +		return true;
>  	else
> -		pci_clear_flags(PCI_PROBE_ONLY);
> -
> -	pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled");
> +		return false;
>  }
> -EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
>  
>  /**
>   * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 795534589b98..b0e0226a8da8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -15,6 +15,7 @@
>  #include <linux/cpumask.h>
>  #include <linux/aer.h>
>  #include <linux/acpi.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/hypervisor.h>
>  #include <linux/irqdomain.h>
>  #include <linux/pm_runtime.h>
> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
>  	dev_set_msi_domain(&bus->dev, d);
>  }
>  
> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge)
> +{
> +	if (ACPI_HANDLE(&host_bridge->dev)) {
> +		union acpi_object *obj;
> +
> +		/*
> +		 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> +		 * exists and returns 0, we must preserve any PCI resource
> +		 * assignments made by firmware for this host bridge.
> +		 */
> +		obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1,
> +					DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
> +		if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
> +			host_bridge->preserve_config = 1;
> +		ACPI_FREE(obj);
> +	}
> +
> +	if (host_bridge->dev.parent && host_bridge->dev.parent->of_node)
> +		host_bridge->preserve_config =
> +			of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node);
> +}
> +
>  static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  {
>  	struct device *parent = bridge->dev.parent;
> @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  	if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE)
>  		dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n");
>  
> +	/* Check if the boot configuration by FW needs to be preserved */
> +	pci_check_config_preserve(bridge);

I really have an allergic reaction to functions named "..._check_..."
We can tell that the function has something to do with preserving
configuration, and it's void so obviously the side effects are the
important thing, but there's no clue in the caller about what the side
effects are.

I'd prefer something like:

  bridge->preserve_config = pci_must_preserve_config(bridge);

where pci_must_preserve_config() has no side effects, returns bool,
and the action is at the caller.

>  	/* Coalesce contiguous windows */
>  	resource_list_for_each_entry_safe(window, n, &resources) {
>  		if (list_is_last(&window->node, &resources))
> @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge)
>  
>  	bus = bridge->bus;
>  
> +	/* If we must preserve the resource configuration, claim now */
> +	if (bridge->preserve_config)
> +		pci_bus_claim_resources(bus);
> +
>  	/*
> -	 * We insert PCI resources into the iomem_resource and
> -	 * ioport_resource trees in either pci_bus_claim_resources()
> -	 * or pci_bus_assign_resources().
> +	 * Assign whatever was left unassigned. If we didn't claim above,
> +	 * this will reassign everything.
>  	 */
> -	if (pci_has_flag(PCI_PROBE_ONLY)) {
> -		pci_bus_claim_resources(bus);
> -	} else {
> -		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);
> -	}
> +	list_for_each_entry(child, &bus->children, node)
> +		pcie_bus_configure_settings(child);
>  
>  	pci_bus_add_devices(bus);
>  	return 0;
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index 29658c0ee71f..3f3909a5d55d 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
>  					     unsigned int devfn);
>  int of_pci_get_devfn(struct device_node *np);
>  void of_pci_check_probe_only(void);
> +bool of_pci_bridge_preserve_resources(struct device_node *node);

This looks like it should be in drivers/pci/pci.h since it's not used
outside drivers/pci/.

>  #else
>  static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
>  					     unsigned int devfn)
> @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np)
>  }
>  
>  static inline void of_pci_check_probe_only(void) { }
> +
> +static inline bool of_pci_bridge_preserve_resources(struct device_node *node)
> +{
> +	return false;
> +}
>  #endif
>  
>  #if IS_ENABLED(CONFIG_OF_IRQ)
> -- 
> 2.25.1
>

Re: [PATCH V5] PCI: Add support for preserving boot configuration

Posted by Vidya Sagar 1 year, 9 months ago

Replied inline...

On 11-04-2024 02:20, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Apr 01, 2024 at 01:20:31PM +0530, Vidya Sagar wrote:
>> Add support for preserving the boot configuration done by the
>> platform firmware per host bridge basis, based on the presence of
>> 'linux,pci-probe-only' property in the respective PCI host bridge
>> device-tree node. It also unifies the ACPI and DT based boot flows
>> in this regard.
>>   drivers/acpi/pci_root.c                  | 12 -----
>>   drivers/pci/controller/pci-host-common.c |  4 --
>>   drivers/pci/of.c                         | 57 +++++++++++++++++++-----
>>   drivers/pci/probe.c                      | 46 ++++++++++++++-----
>>   include/linux/of_pci.h                   |  6 +++
>>   5 files changed, 88 insertions(+), 37 deletions(-)
> What does this apply to?  I tried v6.9-rc1:
>
>    $ git checkout -b wip/2404-vidya-preserve-boot-v5 v6.9-rc1
>    Switched to a new branch 'wip/2404-vidya-preserve-boot-v5'
>
>    $ git am m/v5_20240401_vidyas_pci_add_support_for_preserving_boot_configuration.mbx
>    Applying: PCI: Add support for preserving boot configuration
>    error: patch failed: drivers/acpi/pci_root.c:1050
>    error: drivers/acpi/pci_root.c: patch does not apply
>    Patch failed at 0001 PCI: Add support for preserving boot configuration
I'm going to rebase this patch on top of linux-next and send.
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 84030804a763..ddc2b3e89111 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -1008,7 +1008,6 @@ 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;
>> @@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>        if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL))
>>                host_bridge->native_cxl_error = 0;
>>
>> -     /*
>> -      * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
>> -      * exists and returns 0, we must preserve any PCI resource
>> -      * assignments made by firmware for this host bridge.
>> -      */
>> -     obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
>> -                             DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
>> -     if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
>> -             host_bridge->preserve_config = 1;
>> -     ACPI_FREE(obj);
>> -
>>        acpi_dev_power_up_children_with_adr(device);
>>
>>        pci_scan_child_bus(bus);
>> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
>> index 6be3266cd7b5..e2602e38ae45 100644
>> --- a/drivers/pci/controller/pci-host-common.c
>> +++ b/drivers/pci/controller/pci-host-common.c
>> @@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
>>        if (IS_ERR(cfg))
>>                return PTR_ERR(cfg);
>>
>> -     /* Do not reassign resources if probe only */
>> -     if (!pci_has_flag(PCI_PROBE_ONLY))
>> -             pci_add_flags(PCI_REASSIGN_ALL_BUS);
>> -
>>        bridge->sysdata = cfg;
>>        bridge->ops = (struct pci_ops *)&ops->pci_ops;
>>        bridge->msi_domain = true;
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index 51e3dd0ea5ab..e6da3654f9ac 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -239,24 +239,61 @@ EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
>>    */
>>   void of_pci_check_probe_only(void)
>>   {
>> -     u32 val;
>> +     bool is_preserve_config = of_pci_bridge_preserve_resources(of_chosen);
> No need for a bool here:
>
>    if (of_pci_bridge_preserve_resources(of_chosen))
>      pci_add_flags(PCI_PROBE_ONLY);
>
> I think it would make more sense to add
> of_pci_bridge_preserve_resources() *above* of_pci_check_probe_only()
> since the usual order is to define functions earlier in the file than
> the calls.
I'll take care of it in the next patch.
>> +
>> +     if (is_preserve_config)
>> +             pci_add_flags(PCI_PROBE_ONLY);
>> +     else
>> +             pci_clear_flags(PCI_PROBE_ONLY);
> Not related to *this* patch, but I see that of_pci_check_probe_only()
> already clears PCI_PROBE_ONLY (added by f81c11af617c ("of/pci: Add
> of_pci_check_probe_only to parse "linux,pci-probe-only"").
>
> I'm concerned about clearing PCI_PROBE_ONLY because some platforms set
> this unconditionally, and I don't think they necessarily have
> "linux,pci-probe-only" in DT.
>
> Apparently none of them currently calls of_pci_check_probe_only() so
> PCI_PROBE_ONLY remains set, but clearing it here feels like a landmine
> waiting for somebody to move this into a unified call path.
>
> I guess if we were to drop pci_clear_flags(), that should be a
> separate patch in case it breaks something.
As far as the current patch is concerned, since it is not going to do anything
different compared to what already exists, I'll leave it at it for now and
ponder over moving pci_clear_flags() to a different patch later.
>
>> +     pr_info("PROBE_ONLY %s\n", is_preserve_config ? "enabled" : "disabled");
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
>> +
>> +/**
>> + * of_pci_bridge_preserve_resources - Return true if the boot configuration
>> + *                                    needs to be preserved
>> + * @node: Device tree node.
>> + *
>> + * This function looks for "linux,pci-probe-only" property for a given
>> + * PCI controller's node and returns true if found. It will also look in the
>> + * chosen node if the property is not found in the given controller's node.
>> + * Having this property ensures that the kernel doesn't reconfigure the
>> + * BARs and bridge windows that are already done by the platform firmware.
>> + *
>> + * Return: true if the property exists false otherwise.
>> + */
>> +bool of_pci_bridge_preserve_resources(struct device_node *node)
>> +{
>> +     u32 val = 0;
>>        int ret;
>>
>> -     ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val);
>> +     if (!node) {
>> +             pr_warn("device node is NULL, trying with of_chosen\n");
>> +             node = of_chosen;
>> +     }
>> +
>> +retry:
>> +     ret = of_property_read_u32(node, "linux,pci-probe-only", &val);
>>        if (ret) {
>> -             if (ret == -ENODATA || ret == -EOVERFLOW)
>> -                     pr_warn("linux,pci-probe-only without valid value, ignoring\n");
>> -             return;
>> +             if (ret == -ENODATA || ret == -EOVERFLOW) {
>> +                     pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n", node);
>> +                     return false;
>> +             }
>> +             if (ret == -EINVAL) {
>> +                     if (node == of_chosen)
>> +                             return false;
>> +
>> +                     node = of_chosen;
>> +                     goto retry;
>> +             }
>>        }
>>
>>        if (val)
>> -             pci_add_flags(PCI_PROBE_ONLY);
>> +             return true;
>>        else
>> -             pci_clear_flags(PCI_PROBE_ONLY);
>> -
>> -     pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled");
>> +             return false;
>>   }
>> -EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
>>
>>   /**
>>    * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 795534589b98..b0e0226a8da8 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/cpumask.h>
>>   #include <linux/aer.h>
>>   #include <linux/acpi.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/hypervisor.h>
>>   #include <linux/irqdomain.h>
>>   #include <linux/pm_runtime.h>
>> @@ -877,6 +878,28 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
>>        dev_set_msi_domain(&bus->dev, d);
>>   }
>>
>> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge)
>> +{
>> +     if (ACPI_HANDLE(&host_bridge->dev)) {
>> +             union acpi_object *obj;
>> +
>> +             /*
>> +              * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
>> +              * exists and returns 0, we must preserve any PCI resource
>> +              * assignments made by firmware for this host bridge.
>> +              */
>> +             obj = acpi_evaluate_dsm(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid, 1,
>> +                                     DSM_PCI_PRESERVE_BOOT_CONFIG, NULL);
>> +             if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0)
>> +                     host_bridge->preserve_config = 1;
>> +             ACPI_FREE(obj);
>> +     }
>> +
>> +     if (host_bridge->dev.parent && host_bridge->dev.parent->of_node)
>> +             host_bridge->preserve_config =
>> +                     of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node);
>> +}
>> +
>>   static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>>   {
>>        struct device *parent = bridge->dev.parent;
>> @@ -971,6 +994,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>>        if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE)
>>                dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n");
>>
>> +     /* Check if the boot configuration by FW needs to be preserved */
>> +     pci_check_config_preserve(bridge);
> I really have an allergic reaction to functions named "..._check_..."
> We can tell that the function has something to do with preserving
> configuration, and it's void so obviously the side effects are the
> important thing, but there's no clue in the caller about what the side
> effects are.
>
> I'd prefer something like:
>
>    bridge->preserve_config = pci_must_preserve_config(bridge);
>
> where pci_must_preserve_config() has no side effects, returns bool,
> and the action is at the caller.
I'll take care of it in the next patch.
>>        /* Coalesce contiguous windows */
>>        resource_list_for_each_entry_safe(window, n, &resources) {
>>                if (list_is_last(&window->node, &resources))
>> @@ -3080,20 +3106,18 @@ int pci_host_probe(struct pci_host_bridge *bridge)
>>
>>        bus = bridge->bus;
>>
>> +     /* If we must preserve the resource configuration, claim now */
>> +     if (bridge->preserve_config)
>> +             pci_bus_claim_resources(bus);
>> +
>>        /*
>> -      * We insert PCI resources into the iomem_resource and
>> -      * ioport_resource trees in either pci_bus_claim_resources()
>> -      * or pci_bus_assign_resources().
>> +      * Assign whatever was left unassigned. If we didn't claim above,
>> +      * this will reassign everything.
>>         */
>> -     if (pci_has_flag(PCI_PROBE_ONLY)) {
>> -             pci_bus_claim_resources(bus);
>> -     } else {
>> -             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);
>> -     }
>> +     list_for_each_entry(child, &bus->children, node)
>> +             pcie_bus_configure_settings(child);
>>
>>        pci_bus_add_devices(bus);
>>        return 0;
>> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
>> index 29658c0ee71f..3f3909a5d55d 100644
>> --- a/include/linux/of_pci.h
>> +++ b/include/linux/of_pci.h
>> @@ -13,6 +13,7 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
>>                                             unsigned int devfn);
>>   int of_pci_get_devfn(struct device_node *np);
>>   void of_pci_check_probe_only(void);
>> +bool of_pci_bridge_preserve_resources(struct device_node *node);
> This looks like it should be in drivers/pci/pci.h since it's not used
> outside drivers/pci/.
Agree. I'll take care of it in the next patch.
>>   #else
>>   static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
>>                                             unsigned int devfn)
>> @@ -26,6 +27,11 @@ static inline int of_pci_get_devfn(struct device_node *np)
>>   }
>>
>>   static inline void of_pci_check_probe_only(void) { }
>> +
>> +static inline bool of_pci_bridge_preserve_resources(struct device_node *node)
>> +{
>> +     return false;
>> +}
>>   #endif
>>
>>   #if IS_ENABLED(CONFIG_OF_IRQ)
>> --
>> 2.25.1
>>

Re: [PATCH V5] PCI: Add support for preserving boot configuration

Posted by Rob Herring 1 year, 10 months ago

On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote:
> Add support for preserving the boot configuration done by the
> platform firmware per host bridge basis, based on the presence of
> 'linux,pci-probe-only' property in the respective PCI host bridge
> device-tree node. It also unifies the ACPI and DT based boot flows
> in this regard.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V5:
> * Addressed Rob's review comments
> 
> V4:
> * Addressed Bjorn's review comments
> 
> V3:
> * Unified ACPI and DT flows as part of addressing Bjorn's review comments
> 
> V2:
> * Addressed issues reported by kernel test robot <lkp@intel.com>
> 
>  drivers/acpi/pci_root.c                  | 12 -----
>  drivers/pci/controller/pci-host-common.c |  4 --
>  drivers/pci/of.c                         | 57 +++++++++++++++++++-----
>  drivers/pci/probe.c                      | 46 ++++++++++++++-----
>  include/linux/of_pci.h                   |  6 +++
>  5 files changed, 88 insertions(+), 37 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH V5] PCI: Add support for preserving boot configuration

Posted by Vidya Sagar 1 year, 10 months ago

Thanks Rob for the review.
Bjorn, do you have any further comments for this patch?

On 02-04-2024 21:31, Rob Herring wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote:
>> Add support for preserving the boot configuration done by the
>> platform firmware per host bridge basis, based on the presence of
>> 'linux,pci-probe-only' property in the respective PCI host bridge
>> device-tree node. It also unifies the ACPI and DT based boot flows
>> in this regard.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>> V5:
>> * Addressed Rob's review comments
>>
>> V4:
>> * Addressed Bjorn's review comments
>>
>> V3:
>> * Unified ACPI and DT flows as part of addressing Bjorn's review comments
>>
>> V2:
>> * Addressed issues reported by kernel test robot <lkp@intel.com>
>>
>>   drivers/acpi/pci_root.c                  | 12 -----
>>   drivers/pci/controller/pci-host-common.c |  4 --
>>   drivers/pci/of.c                         | 57 +++++++++++++++++++-----
>>   drivers/pci/probe.c                      | 46 ++++++++++++++-----
>>   include/linux/of_pci.h                   |  6 +++
>>   5 files changed, 88 insertions(+), 37 deletions(-)
>>
> Reviewed-by: Rob Herring <robh@kernel.org>
>

[PATCH V6] PCI: Add support for preserving boot configuration

Posted by Vidya Sagar 1 year, 9 months ago

Add support for preserving the boot configuration done by the
platform firmware per host bridge basis, based on the presence of
'linux,pci-probe-only' property in the respective PCI host bridge
device-tree node. It also unifies the ACPI and DT based boot flows
in this regard.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V5:
* Addressed Rob's review comments

V4:
* Addressed Bjorn's review comments

V3:
* Unified ACPI and DT flows as part of addressing Bjorn's review comments

V2:
* Addressed issues reported by kernel test robot <lkp@intel.com>

 drivers/acpi/pci_root.c                  | 12 ------
 drivers/pci/controller/pci-host-common.c |  4 --
 drivers/pci/of.c                         | 54 +++++++++++++++++++-----
 drivers/pci/pci.h                        |  7 +++
 drivers/pci/probe.c                      | 48 ++++++++++++++++-----
 5 files changed, 88 insertions(+), 37 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 58b89b8d950e..ddc2b3e89111 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -1008,7 +1008,6 @@ 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;
@@ -1050,17 +1049,6 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	if (!(root->osc_ext_control_set & OSC_CXL_ERROR_REPORTING_CONTROL))
 		host_bridge->native_cxl_error = 0;
 
-	/*
-	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
-	 * exists and returns 0, we must preserve any PCI resource
-	 * assignments made by firmware for this host bridge.
-	 */
-	obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(bus->bridge), &pci_acpi_dsm_guid, 1,
-				      DSM_PCI_PRESERVE_BOOT_CONFIG, NULL, ACPI_TYPE_INTEGER);
-	if (obj && obj->integer.value == 0)
-		host_bridge->preserve_config = 1;
-	ACPI_FREE(obj);
-
 	acpi_dev_power_up_children_with_adr(device);
 
 	pci_scan_child_bus(bus);
diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 45b71806182d..c96b2de163b5 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -73,10 +73,6 @@ int pci_host_common_probe(struct platform_device *pdev)
 	if (IS_ERR(cfg))
 		return PTR_ERR(cfg);
 
-	/* Do not reassign resources if probe only */
-	if (!pci_has_flag(PCI_PROBE_ONLY))
-		pci_add_flags(PCI_REASSIGN_ALL_BUS);
-
 	bridge->sysdata = cfg;
 	bridge->ops = (struct pci_ops *)&ops->pci_ops;
 	bridge->msi_domain = true;
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5ab..aeacff742706 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -234,27 +234,61 @@ int of_get_pci_domain_nr(struct device_node *node)
 EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
 
 /**
- * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only
- *                           is present and valid
+ * of_pci_bridge_preserve_resources - Return true if the boot configuration
+ *                                    needs to be preserved
+ * @node: Device tree node.
+ *
+ * This function looks for "linux,pci-probe-only" property for a given
+ * PCI controller's node and returns true if found. It will also look in the
+ * chosen node if the property is not found in the given controller's node.
+ * Having this property ensures that the kernel doesn't reconfigure the
+ * BARs and bridge windows that are already done by the platform firmware.
+ *
+ * Return: true if the property exists false otherwise.
  */
-void of_pci_check_probe_only(void)
+bool of_pci_bridge_preserve_resources(struct device_node *node)
 {
-	u32 val;
+	u32 val = 0;
 	int ret;
 
-	ret = of_property_read_u32(of_chosen, "linux,pci-probe-only", &val);
+	if (!node) {
+		pr_warn("device node is NULL, trying with of_chosen\n");
+		node = of_chosen;
+	}
+
+retry:
+	ret = of_property_read_u32(node, "linux,pci-probe-only", &val);
 	if (ret) {
-		if (ret == -ENODATA || ret == -EOVERFLOW)
-			pr_warn("linux,pci-probe-only without valid value, ignoring\n");
-		return;
+		if (ret == -ENODATA || ret == -EOVERFLOW) {
+			pr_warn("Incorrect value for linux,pci-probe-only in %pOF, ignoring\n",
+				node);
+			return false;
+		}
+		if (ret == -EINVAL) {
+			if (node == of_chosen)
+				return false;
+
+			node = of_chosen;
+			goto retry;
+		}
 	}
 
 	if (val)
+		return true;
+	else
+		return false;
+}
+
+/**
+ * of_pci_check_probe_only - Setup probe only mode if linux,pci-probe-only
+ *                           is present and valid
+ */
+void of_pci_check_probe_only(void)
+{
+	if (of_pci_bridge_preserve_resources(of_chosen))
 		pci_add_flags(PCI_PROBE_ONLY);
 	else
 		pci_clear_flags(PCI_PROBE_ONLY);
-
-	pr_info("PROBE_ONLY %s\n", val ? "enabled" : "disabled");
 }
 EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..1da54ab18534 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -650,6 +650,7 @@ int of_pci_get_max_link_speed(struct device_node *node);
 u32 of_pci_get_slot_power_limit(struct device_node *node,
 				u8 *slot_power_limit_value,
 				u8 *slot_power_limit_scale);
+bool of_pci_bridge_preserve_resources(struct device_node *node);
 int pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
@@ -688,6 +689,12 @@ of_pci_get_slot_power_limit(struct device_node *node,
 	return 0;
 }
 
+static inline bool
+of_pci_bridge_preserve_resources(struct device_node *node)
+{
+	return false;
+}
+
 static inline int pci_set_of_node(struct pci_dev *dev) { return 0; }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 1325fbae2f28..ceb9129823ee 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -15,6 +15,7 @@
 #include <linux/cpumask.h>
 #include <linux/aer.h>
 #include <linux/acpi.h>
+#include <linux/pci-acpi.h>
 #include <linux/hypervisor.h>
 #include <linux/irqdomain.h>
 #include <linux/pm_runtime.h>
@@ -889,6 +890,30 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
 	dev_set_msi_domain(&bus->dev, d);
 }
 
+static bool pci_must_preserve_config(struct pci_host_bridge *host_bridge)
+{
+	if (ACPI_HANDLE(&host_bridge->dev)) {
+		union acpi_object *obj;
+
+		/*
+		 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
+		 * exists and returns 0, we must preserve any PCI resource
+		 * assignments made by firmware for this host bridge.
+		 */
+		obj = acpi_evaluate_dsm_typed(ACPI_HANDLE(&host_bridge->dev), &pci_acpi_dsm_guid,
+					      1, DSM_PCI_PRESERVE_BOOT_CONFIG, NULL,
+					      ACPI_TYPE_INTEGER);
+		if (obj && obj->integer.value == 0)
+			return true;
+		ACPI_FREE(obj);
+	}
+
+	if (host_bridge->dev.parent && host_bridge->dev.parent->of_node)
+		return of_pci_bridge_preserve_resources(host_bridge->dev.parent->of_node);
+
+	return false;
+}
+
 static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 {
 	struct device *parent = bridge->dev.parent;
@@ -983,6 +1008,9 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE)
 		dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n");
 
+	/* Check if the boot configuration by FW needs to be preserved */
+	bridge->preserve_config = pci_must_preserve_config(bridge);
+
 	/* Coalesce contiguous windows */
 	resource_list_for_each_entry_safe(window, n, &resources) {
 		if (list_is_last(&window->node, &resources))
@@ -3074,20 +3102,18 @@ int pci_host_probe(struct pci_host_bridge *bridge)
 
 	bus = bridge->bus;
 
+	/* If we must preserve the resource configuration, claim now */
+	if (bridge->preserve_config)
+		pci_bus_claim_resources(bus);
+
 	/*
-	 * We insert PCI resources into the iomem_resource and
-	 * ioport_resource trees in either pci_bus_claim_resources()
-	 * or pci_bus_assign_resources().
+	 * Assign whatever was left unassigned. If we didn't claim above,
+	 * this will reassign everything.
 	 */
-	if (pci_has_flag(PCI_PROBE_ONLY)) {
-		pci_bus_claim_resources(bus);
-	} else {
-		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);
-	}
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
 
 	pci_bus_add_devices(bus);
 	return 0;
-- 
2.25.1

[PATCH v7-incomplete 0/3] PCI: Add support for preserving boot configuration