On Fri, Aug 13, 2021 at 10:35:46PM -0500, Shanker R Donthineni wrote:
> 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:
> >> From: Shanker Donthineni <sdonthineni@xxxxxxxxxx>
> >>
> >> 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 <sdonthineni@xxxxxxxxxx>
> >> Reviewed-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >> ---
> >> 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;
I guess that might be OK.
TBH I don't really like the device_property_read_u8() thing because
(1) we know this is an ACPI property and I don't see a reason to use
an "generic" interface that doesn't buy us anything, and (2) the
connection to the source of the data (a _DSD method) is really, really
hard to find.
Admittedly, it's still pretty hard to connect acpi_dev_get_property()
with "_DSD". The only real clue is the comment about "Look for a
special _DSD property ..."
> 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.
> >
> >> 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.
Sounds good.
Bjorn
