Re: [PATCH v15 7/9] PCI: Setup ACPI fwnode early and at the same time with OF

[Bjorn Helgaas]

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