Received-SPF: pass (google.com: domain of linux-kernel+bounces-76920-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 22 Feb 2024 11:06:41 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Vidya Sagar <vidyas@nvidia.com>
Cc: bhelgaas@google.com, rafael@kernel.org, lenb@kernel.org,
	will@kernel.org, lpieralisi@kernel.org, kw@linux.com,
	robh@kernel.org, frowand.list@gmail.com, linux-pci@vger.kernel.org,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	treding@nvidia.com, jonathanh@nvidia.com, kthota@nvidia.com,
	mmaddireddy@nvidia.com, sagar.tv@gmail.com
Subject: Re: [PATCH V3] PCI: Add support for preserving boot configuration
Message-ID: <20240222170641.GA15593@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240222124110.2681455-1-vidyas@nvidia.com>

On Thu, Feb 22, 2024 at 06:11:10PM +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 PCIe host bridge
> device-tree node. It also unifies the ACPI and DT based boot flows
> in this regard.

> +/**
> + * of_pci_bridge_check_probe_only - Return true if the boot configuration
> + *                                  needs to be preserved

I don't like the "check_probe_only" name because it's a boolean
function but the name doesn't tell me what a true/false return value
means.  Something like "preserve_resources" would be better.  If you
want "probe_only", even removing the "check" would help.

> + * @node: Device tree node with the domain information.
> + *
> + * This function looks for "linux,pci-probe-only" property for a given
> + * PCIe controller's node and returns true if found. Having this property
> + * for a PCIe controller ensures that the kernel doesn't re-enumerate and
> + * reconfigure the BAR resources that are already done by the platform firmware.

This is generic PCI, not PCIe-specific (also in commit log and comment
below).

I think "enumeration" specifically refers to discovering what devices
are present, and the kernel always does that, so drop that part.
Reconfiguring BARs and bridge windows is what we want to prevent.

> + * NOTE: The scope of "linux,pci-probe-only" defined within a PCIe 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_check_probe_only(struct device_node *node)
> +{
> +	return of_property_read_bool(node, "linux,pci-probe-only");
> +}
> +EXPORT_SYMBOL_GPL(of_pci_bridge_check_probe_only);

Why does this need to be exported for modules and exposed via
include/linux/pci.h?

> +static void pci_check_config_preserve(struct pci_host_bridge *host_bridge)
> +{
> +	if (&host_bridge->dev) {

Checking &host_bridge->dev doesn't seem like the right way to
determine whether this is an ACPI host bridge.

> +		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);
> +	}