From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>,
Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: "David E. Box" <david.e.box@linux.intel.com>,
shyjumon.n@intel.com, "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Len Brown <lenb@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
Dan Williams <dan.j.williams@intel.com>,
Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
linux-nvme <linux-nvme@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support
Date: Thu, 25 Jun 2020 13:30:53 +0200 [thread overview]
Message-ID: <CAJZ5v0i8dCN=HMFk_+ZX-Wr73P6kdQBtV0i3FtrZrO9cegXsvQ@mail.gmail.com> (raw)
In-Reply-To: <20200624211549.GA2586552@bjorn-Precision-5520>
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()?
next prev parent reply other threads:[~2020-06-25 11:31 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-28 0:32 [PATCH 0/2] Add support for StorageD3Enable _DSD property David E. Box
2020-04-28 0:32 ` [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support David E. Box
2020-05-18 12:34 ` Rafael J. Wysocki
2020-05-19 17:10 ` David E. Box
2020-04-28 0:32 ` [PATCH 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box
2020-04-28 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig
2020-04-28 14:09 ` David E. Box
2020-04-28 14:22 ` Christoph Hellwig
2020-04-28 15:27 ` David E. Box
2020-04-29 5:20 ` Williams, Dan J
2020-04-29 15:10 ` Keith Busch
2020-04-29 16:11 ` David E. Box
2020-05-01 13:12 ` hch
2020-05-01 15:54 ` David E. Box
2020-05-01 13:10 ` hch
2020-05-18 13:51 ` David Woodhouse
2020-05-18 17:20 ` Dan Williams
2020-06-12 20:48 ` [PATCH V2 0/2] nvme: Add support for ACPI StorageD3Enable property David E. Box
2020-06-24 18:55 ` David E. Box
2020-06-24 19:10 ` Dan Williams
2020-06-24 19:39 ` David E. Box
2020-07-02 22:50 ` [PATCH v4] drivers/nvme: " David E. Box
2020-07-06 14:57 ` Rafael J. Wysocki
2020-07-07 21:24 ` David E. Box
2020-07-07 7:09 ` Christoph Hellwig
2020-07-09 18:43 ` [PATCH V5] " David E. Box
2020-07-13 11:12 ` Rafael J. Wysocki
2020-07-16 14:39 ` Christoph Hellwig
2021-05-26 19:25 ` Raul E Rangel
2020-06-12 20:48 ` [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support David E. Box
2020-06-24 21:15 ` Bjorn Helgaas
2020-06-25 11:30 ` Rafael J. Wysocki [this message]
2020-06-25 12:16 ` Mika Westerberg
2020-06-25 17:07 ` David E. Box
2020-06-25 17:30 ` Bjorn Helgaas
2020-06-24 21:37 ` Bjorn Helgaas
2020-06-24 22:09 ` David E. Box
2020-06-12 20:48 ` [PATCH V2 2/2] drivers/nvme: Add support for ACPI StorageD3Enable property David E. Box
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJZ5v0i8dCN=HMFk_+ZX-Wr73P6kdQBtV0i3FtrZrO9cegXsvQ@mail.gmail.com' \
--to=rafael@kernel.org \
--cc=axboe@fb.com \
--cc=bhelgaas@google.com \
--cc=dan.j.williams@intel.com \
--cc=david.e.box@linux.intel.com \
--cc=hch@lst.de \
--cc=helgaas@kernel.org \
--cc=kbusch@kernel.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=linux-pci@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=rjw@rjwysocki.net \
--cc=sagi@grimberg.me \
--cc=shyjumon.n@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).