[V2,1/2] PCI: Add ACPI StorageD3Enable _DSD support
diff mbox series

Message ID 20200612204820.20111-2-david.e.box@linux.intel.com
State Not Applicable
Delegated to: Bjorn Helgaas
Headers show
Series
  • [V2,1/2] PCI: Add ACPI StorageD3Enable _DSD support
Related show

Commit Message

David E. Box June 12, 2020, 8:48 p.m. UTC
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@linux.intel.com>
---
 drivers/acpi/property.c |  3 +++
 drivers/pci/pci-acpi.c  | 59 +++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 64 insertions(+)

Comments

Bjorn Helgaas June 24, 2020, 9:15 p.m. UTC | #1
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@linux.intel.com>
> ---
>  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.  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.  Maybe this is could be done somewhere around
pci_device_add()?

> +	}
> +
> +	if (!adev)
> +		return false;
> +
> +	status = acpi_get_handle(adev->handle, "PXSX", &handle);
> +	if (ACPI_FAILURE(status))
> +		return false;
> +
> +	adev = acpi_bus_get_acpi_device(handle);
> +	if (!adev)
> +		return false;
> +
> +	fwnode = acpi_fwnode_handle(adev);
> +	if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val))
> +		ret = (val == 1);
> +
> +	acpi_bus_put_acpi_device(adev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pci_acpi_storage_d3);
> +
>  static bool acpi_pci_power_manageable(struct pci_dev *dev)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 83ce1cdf5676..396fcb269a60 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2318,10 +2318,12 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus);
>  void
>  pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *));
>  bool pci_pr3_present(struct pci_dev *pdev);
> +bool pci_acpi_storage_d3(struct pci_dev *dev);
>  #else
>  static inline struct irq_domain *
>  pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; }
>  static inline bool pci_pr3_present(struct pci_dev *pdev) { return false; }
> +static inline bool pci_acpi_storage_d3(struct pci_dev *dev) { return false; }
>  #endif
>  
>  #ifdef CONFIG_EEH
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-nvme mailing list
> linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
Bjorn Helgaas June 24, 2020, 9:37 p.m. UTC | #2
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. 

Is this something that should apply to plug-in drives, or does this
only apply to soldered-in things?

> It is a
> BIOS work around that is currently in use on shipping systems like some
> Intel Comet Lake platforms. 

What is this BIOS work around?  Is there a defect here that's being
worked around?  What's the defect?

> It is meant to change default driver policy for
> suspend that may cause higher power consumption.

I guess this means that by changing the driver policy from the
default, we can save some power?

> 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

There is surprisingly little information in this intro.  The whole
paragraph under "Modern Standby Power Management" is duplicated
immediately below in "D3 Support".  Maybe that's a copyediting error
that displaced useful information.

It says "drivers should go to the deepest appropriate state" so
"function drivers don't have to manage implementation details".  No
doubt "drivers" and "function drivers" is a meaningful distinction to
Windows cognoscenti, but it's not to me.

It talks about "enabling D3" without specifying D3hot or D3cold.

It talks about "D3 support for storage devices."  All PCI devices are
required to support both D3hot and D3cold, so this must be talking
about some other sort of support; I suppose maybe it's a hint about
whether a driver should *use* D3hot (or D3cold, I can't tell).

It says nothing about where to look for the _DSD: on a Root Port or on
the NVMe endpoint.

Bjorn
David E. Box June 24, 2020, 10:09 p.m. UTC | #3
On Wed, 2020-06-24 at 16:37 -0500, Bjorn Helgaas 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. 
> 
> Is this something that should apply to plug-in drives, or does this
> only apply to soldered-in things?
> 
> > It is a
> > BIOS work around that is currently in use on shipping systems like
> > some
> > Intel Comet Lake platforms. 
> 
> What is this BIOS work around?  Is there a defect here that's being
> worked around?  What's the defect?

> 
> > It is meant to change default driver policy for
> > suspend that may cause higher power consumption.
> 
> I guess this means that by changing the driver policy from the
> default, we can save some power?

Yes. Maybe 'work around' was a poor choice of words. 'Getting around
default driver policy' is the issue. There is no hardware defect. One
of the uses of the suspend-to-idle flow is to support compliance with
increasingly tighter energy regulations. One of the ways to do this on
desktop systems is to power off the ATX power supply during s2idle and
use the 5V standby rail for self refresh and other low power needs. But
the platforms that support this can't shutdown the PS unless PCI ports
are placed in D3. On Linux this won't happen with NVMe drives because
the default driver policy is to use ASPM (NVMe APST) during s2idle.
Windows has a related concern. So to 'get around' the driver choosing a
policy that will result in higher power consumption, they implemented
this _DSD to inform the OS of its preference for D3 on the PCI port.

David
Rafael J. Wysocki June 25, 2020, 11:30 a.m. UTC | #4
On Wed, Jun 24, 2020 at 11:15 PM Bjorn Helgaas <helgaas@kernel.org> 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@linux.intel.com>
> > ---
> >  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.  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.

Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it
is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(),
and that gets called from pci_device_add() *before* calling
device_add().

Mika, is that why acpi_pci_find_companion() gets callled from
acpi_pci_bridge_d3()?
Mika Westerberg June 25, 2020, 12:16 p.m. UTC | #5
On Thu, Jun 25, 2020 at 01:30:53PM +0200, Rafael J. Wysocki wrote:
> 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.
> 
> Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it
> is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(),
> and that gets called from pci_device_add() *before* calling
> device_add().
> 
> Mika, is that why acpi_pci_find_companion() gets callled from
> acpi_pci_bridge_d3()?

Yes, that's correct.
David E. Box June 25, 2020, 5:07 p.m. UTC | #6
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@kernel.org>
> 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@linux.intel.com>
> > > ---
> > >  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
Bjorn Helgaas June 25, 2020, 5:30 p.m. UTC | #7
On Thu, Jun 25, 2020 at 01:30:53PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 11:15 PM Bjorn Helgaas <helgaas@kernel.org> 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.

> > > +/**
> > > + * 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.  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.

OK, great.  IIUC, that means this function doesn't need to be in
drivers/pci and it could be moved to the NVMe code.

> Ths acpi_pci_bridge_d3() case is different, though, AFAICS, because it
> is invoked in the pci_pm_init() path, via pci_bridge_d3_possible(),
> and that gets called from pci_device_add() *before* calling
> device_add().
> 
> Mika, is that why acpi_pci_find_companion() gets called from
> acpi_pci_bridge_d3()?

Is pdev->bridge_d3 really needed before pci_device_add()?  It would be
really nice if there were a way to get rid of that manual lookup of
the companion device in acpi_pci_bridge_d3().

Bjorn

Patch
diff mbox series

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);
+	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);
+	}
+
+	if (!adev)
+		return false;
+
+	status = acpi_get_handle(adev->handle, "PXSX", &handle);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	adev = acpi_bus_get_acpi_device(handle);
+	if (!adev)
+		return false;
+
+	fwnode = acpi_fwnode_handle(adev);
+	if (!fwnode_property_read_u8(fwnode, "StorageD3Enable", &val))
+		ret = (val == 1);
+
+	acpi_bus_put_acpi_device(adev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_acpi_storage_d3);
+
 static bool acpi_pci_power_manageable(struct pci_dev *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&dev->dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 83ce1cdf5676..396fcb269a60 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2318,10 +2318,12 @@  struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus);
 void
 pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *));
 bool pci_pr3_present(struct pci_dev *pdev);
+bool pci_acpi_storage_d3(struct pci_dev *dev);
 #else
 static inline struct irq_domain *
 pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; }
 static inline bool pci_pr3_present(struct pci_dev *pdev) { return false; }
+static inline bool pci_acpi_storage_d3(struct pci_dev *dev) { return false; }
 #endif
 
 #ifdef CONFIG_EEH