Hi Bjorn,

On 8/13/21 6:04 PM, Bjorn Helgaas wrote:

External email: Use caution opening links or attachments


[+cc Ben, Mika]

On Thu, Aug 05, 2021 at 09:59:15PM +0530, Amey Narkhede wrote:

quoted

From: Shanker Donthineni <redacted>

The pci_dev objects are created through two mechanisms 1) during PCI
bus scan and 2) from I/O Virtualization. The fwnode in pci_dev object
is being set at different places depends on the type of firmware used,
device creation mechanism, and acpi_pci_bridge_d3().

The software features which have a dependency on ACPI fwnode properties
and need to be handled before device_add() will not work. One use case,
the software has to check the existence of _RST method to support ACPI
based reset method.

This patch does the two changes in order to provide fwnode consistently.
 - Set ACPI and OF fwnodes from pci_setup_device().
 - Remove pci_set_acpi_fwnode() in acpi_pci_bridge_d3().

After this patch, ACPI/OF firmware properties are visible at the same
time during the early stage of pci_dev setup. And also call sites should
be able to use firmware agnostic functions device_property_xxx() for the
early PCI quirks in the future.

Signed-off-by: Shanker Donthineni <redacted>
Reviewed-by: Alex Williamson <redacted>
---
 drivers/pci/pci-acpi.c | 1 -
 drivers/pci/probe.c    | 7 ++++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index eaddbf701759..dae021322b3f 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c

@@ -952,7 +952,6 @@ static bool acpi_pci_bridge_d3(struct pci_dev *dev)
              return false;

      /* Assume D3 support if the bridge is power-manageable by ACPI. */
-     pci_set_acpi_fwnode(dev);
      adev = ACPI_COMPANION(&dev->dev);

I *think* the Root Port code farther down in this function is also now
unnecessary:

  acpi_pci_bridge_d3(...)
  {
    ...
    root = pcie_find_root_port(dev);
    adev = ACPI_COMPANION(&root->dev);
    if (root == dev) {
      /*
       * It is possible that the ACPI companion is not yet bound
       * for the root port so look it up manually here.
       */
      if (!adev && !pci_dev_is_added(root))
        adev = acpi_pci_find_companion(&root->dev);
    }

Since we're now setting the ACPI_COMPANION for every pci_dev long
before we get here, I think this could now be simplified to something
like this:

  acpi_pci_bridge_d3(...)
  {
    if (!dev->is_hotplug_bridge)
      return false;

    adev = ACPI_COMPANION(&dev->dev);
    if (adev && acpi_device_power_manageable(adev))
      return true;

    root = pcie_find_root_port(dev);
    if (!root)
      return false;

    adev = ACPI_COMPANION(&root->dev);
    if (!adev)
      return false;

    rc = acpi_dev_get_property(dev, "HotPlugSupportInD3",
                               ACPI_TYPE_INTEGER, &val);
    if (rc < 0)
      return false;

    return val == 1;
  }

Agree, thanks for your suggestion. Yes, it can be simplified too.
Can I do something like this using the unified device property API?

static bool acpi_pci_bridge_d3(struct pci_dev *dev)
{
        struct acpi_device *adev;
        struct pci_dev *root;
        u8 val;

        if (!dev->is_hotplug_bridge)
                return false;

        adev = ACPI_COMPANION(&dev->dev);
        if (adev && acpi_device_power_manageable(adev))
                return true;

        root = pcie_find_root_port(dev);
        if (!root)
                return false;

        if (device_property_read_u8(&root->dev, "HotPlugSupportInD3", &val))
                return false;

        return val == 1;
}

acpi_pci_bridge_d3() was added by 26ad34d510a8 ("PCI / ACPI: Whitelist
D3 for more PCIe hotplug ports") [1], so I cc'd Mika in case he has
any comment.

quoted

      if (adev && acpi_device_power_manageable(adev))

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 379e85037d9b..15a6975d3757 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c

@@ -1789,6 +1789,9 @@ int pci_setup_device(struct pci_dev *dev)
      dev->error_state = pci_channel_io_normal;
      set_pcie_port_type(dev);

+     pci_set_of_node(dev);
+     pci_set_acpi_fwnode(dev);

Is there a reason why you moved pci_set_of_node() from
pci_scan_device() to here?  I think it's a good change; I'm just
curious if you tripped over something that required it.

There is no specific reason and not required but setting both the fwnodes
at the same time improves the code readability and provides consistent
device properties for callers.

The pci_set_of_node() was added to pci_scan_device() by 98d9f30c820d
("pci/of: Match PCI devices to OF nodes dynamically") [2], so I cc'd
Ben just in case there's some reason he didn't put it in
pci_setup_device() in the first place.

Thanks,
Shanker Donthineni