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
