Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support

[David E. Box]

On Thu, 2020-06-25 at 13:30 +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 11:15 PM Bjorn Helgaas <helgaas@xxxxxxxxxx>
> wrote:
> > On Fri, Jun 12, 2020 at 01:48:19PM -0700, David E. Box wrote:
> > > StorageD3Enable is a boolean property that indicates that the
> > > platform
> > > wants to use D3 for PCIe storage drives during suspend-to-idle.
> > > It is a
> > > BIOS work around that is currently in use on shipping systems
> > > like some
> > > Intel Comet Lake platforms. It is meant to change default driver
> > > policy for
> > > suspend that may cause higher power consumption.
> > > 
> > > Add the DSD property for recognition by fwnode calls and provide
> > > an
> > > exported symbol for device drivers to use to read the property as
> > > needed.
> > > 
> > > Link: 
> > > https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
> > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/acpi/property.c |  3 +++
> > >  drivers/pci/pci-acpi.c  | 59
> > > +++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/pci.h     |  2 ++
> > >  3 files changed, 64 insertions(+)
> > > 
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index e601c4511a8b..c2e2ae774a19 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -45,6 +45,9 @@ static const guid_t prp_guids[] = {
> > >       /* Thunderbolt GUID for WAKE_SUPPORTED: 6c501103-c189-4296-
> > > ba72-9bf5a26ebe5d */
> > >       GUID_INIT(0x6c501103, 0xc189, 0x4296,
> > >                 0xba, 0x72, 0x9b, 0xf5, 0xa2, 0x6e, 0xbe, 0x5d),
> > > +     /* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-
> > > 99a5189762d0 */
> > > +     GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
> > > +               0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
> > >  };
> > > 
> > >  /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-
> > > 1319f52a966b */
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index d21969fba6ab..732df524e09c 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -972,6 +972,65 @@ static bool acpi_pci_bridge_d3(struct
> > > pci_dev *dev)
> > >       return val == 1;
> > >  }
> > > 
> > > +/**
> > > + * pci_acpi_storage_d3 - whether root port requests D3 for idle
> > > suspend
> > > + * @pdev: PCI device to check
> > > + *
> > > + * Returns true if the ACPI companion device contains the
> > > "StorageD3Enable"
> > > + * _DSD property and the value is 1. This indicates that the
> > > root port is
> > > + * used by a storage device and the platform is requesting D3
> > > for the
> > > + * device during suspend to idle in order to support platform
> > > pm.
> > > + */
> > > +bool pci_acpi_storage_d3(struct pci_dev *dev)
> > > +{
> > > +     const struct fwnode_handle *fwnode;
> > > +     struct acpi_device *adev;
> > > +     struct pci_dev *root;
> > > +     acpi_handle handle;
> > > +     acpi_status status;
> > > +     bool ret = false;
> > > +     u8 val;
> > > +
> > > +     /*
> > > +      * Look for _DSD property specifying that the storage
> > > device on
> > > +      * the port must use D3 to support deep platform power
> > > savings during
> > > +      * suspend-to-idle
> > > +      */
> > > +     root = pci_find_pcie_root_port(dev);
> > 
> > I think this would need to be updated to apply to v5.8-rc1 after
> > 6ae72bfa656e ("PCI: Unify pcie_find_root_port() and
> > pci_find_pcie_root_port()").
> > 
> > https://git.kernel.org/linus/6ae72bfa656e
> > 
> > > +     if (!root)
> > > +             return false;
> > > +
> > > +     adev = ACPI_COMPANION(&root->dev);
> > > +     if (!adev) {
> > > +             /*
> > > +              * 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);
> > 
> > I see that you copied this "ACPI companion not yet bound" thing
> > from
> > acpi_pci_bridge_d3().  But it's ugly.
> > 
> > Isn't there a way we can bind the ACPI companion during normal PCI
> > enumeration so we don't need this exception case?
> > 
> > I really do not like the idea of putting this code in the PCI core
> > because AFAICT the PCI core can do nothing with this information.
> > 
> > If we could make sure during enumeration that the root port always
> > has
> > an ACPI companion, this code could go to the nvme driver itself.

This is fine with me if it's acceptable with the nvme maintainers.

> >   And
> > we could also clean up the ugliness in acpi_pci_bridge_d3().
> > 
> > Rafael, is that possible?  I don't really know how the companion
> > device gets set.
> 
> That's a bit convoluted.
> 
> device_add() calls device_platform_notify(), before calling
> bus_add_device().
> 
> device_platform_notify() calls acpi_platform_notify() which invokes
> acpi_device_notify() that looks for the companion via
> type->find_companion() which for PCI points to
> acpi_pci_find_companion().  If found, the companion is attached to
> the
> dev structure as a physical_node, via acpi_bind_one().
> 
> So by the time bus_probe_device() runs, the companion should be there
> already - if it is there at all.
> 
> The parent ACPI companion should be present when the child is probing
> too, as per the above.
> 
> > Maybe this is could be done somewhere around pci_device_add()?
> 
> It is done in there.
> 
> It is not necessary to call acpi_pci_find_companion() from
> pci_acpi_storage_d3() as long as that function is required to be
> called by the target device's driver probe or later.

Thanks for the clarification. Since this would get called at driver
probe this bit of code is not needed.

David