From: Bjorn Helgaas <helgaas@kernel.org> To: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>, "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 12:30:53 -0500 [thread overview] Message-ID: <20200625173053.GA2694537@bjorn-Precision-5520> (raw) In-Reply-To: <CAJZ5v0i8dCN=HMFk_+ZX-Wr73P6kdQBtV0i3FtrZrO9cegXsvQ@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Jens Axboe <axboe@fb.com>, Sagi Grimberg <sagi@grimberg.me>, ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Linux PCI <linux-pci@vger.kernel.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-nvme <linux-nvme@lists.infradead.org>, shyjumon.n@intel.com, Keith Busch <kbusch@kernel.org>, "David E. Box" <david.e.box@linux.intel.com>, Bjorn Helgaas <bhelgaas@google.com>, Dan Williams <dan.j.williams@intel.com>, Mika Westerberg <mika.westerberg@linux.intel.com>, Christoph Hellwig <hch@lst.de>, Len Brown <lenb@kernel.org> Subject: Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support Date: Thu, 25 Jun 2020 12:30:53 -0500 [thread overview] Message-ID: <20200625173053.GA2694537@bjorn-Precision-5520> (raw) In-Reply-To: <CAJZ5v0i8dCN=HMFk_+ZX-Wr73P6kdQBtV0i3FtrZrO9cegXsvQ@mail.gmail.com> 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 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-06-25 17:31 UTC|newest] Thread overview: 78+ 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 ` David E. Box 2020-04-28 0:32 ` [PATCH 1/2] pci: Add ACPI StorageD3Enable _DSD support David E. Box 2020-04-28 0:32 ` David E. Box 2020-05-18 12:34 ` Rafael J. Wysocki 2020-05-18 12:34 ` Rafael J. Wysocki 2020-05-19 17:10 ` David E. Box 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 0:32 ` David E. Box 2020-04-28 5:13 ` [PATCH 0/2] Add support for StorageD3Enable _DSD property Christoph Hellwig 2020-04-28 5:13 ` Christoph Hellwig 2020-04-28 14:09 ` David E. Box 2020-04-28 14:09 ` David E. Box 2020-04-28 14:22 ` Christoph Hellwig 2020-04-28 14:22 ` Christoph Hellwig 2020-04-28 15:27 ` David E. Box 2020-04-28 15:27 ` David E. Box 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 5:20 ` Williams, Dan J 2020-04-29 15:10 ` Keith Busch 2020-04-29 15:10 ` Keith Busch 2020-04-29 16:11 ` David E. Box 2020-04-29 16:11 ` David E. Box 2020-05-01 13:12 ` hch 2020-05-01 13:12 ` hch 2020-05-01 15:54 ` David E. Box 2020-05-01 15:54 ` David E. Box 2020-05-01 13:10 ` hch 2020-05-01 13:10 ` hch 2020-05-18 13:51 ` David Woodhouse 2020-05-18 13:51 ` David Woodhouse 2020-05-18 17:20 ` Dan Williams 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-12 20:48 ` David E. Box 2020-06-24 18:55 ` David E. Box 2020-06-24 18:55 ` David E. Box 2020-06-24 19:10 ` Dan Williams 2020-06-24 19:10 ` Dan Williams 2020-06-24 19:39 ` David E. Box 2020-06-24 19:39 ` David E. Box 2020-07-02 21:04 ` [PATCH v3] drivers/nvme: " David E. Box 2020-07-02 22:50 ` [PATCH v4] " David E. Box 2020-07-02 22:50 ` David E. Box 2020-07-03 7:18 ` kernel test robot 2020-07-03 7:18 ` kernel test robot 2020-07-06 14:57 ` Rafael J. Wysocki 2020-07-06 14:57 ` Rafael J. Wysocki 2020-07-07 21:24 ` David E. Box 2020-07-07 21:24 ` David E. Box 2020-07-07 7:09 ` Christoph Hellwig 2020-07-07 7:09 ` Christoph Hellwig 2020-07-09 18:43 ` [PATCH V5] " David E. Box 2020-07-09 18:43 ` David E. Box 2020-07-13 11:12 ` Rafael J. Wysocki 2020-07-13 11:12 ` Rafael J. Wysocki 2020-07-16 14:39 ` Christoph Hellwig 2020-07-16 14:39 ` Christoph Hellwig 2021-05-26 19:25 ` Raul E Rangel 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-12 20:48 ` David E. Box 2020-06-24 21:15 ` Bjorn Helgaas 2020-06-24 21:15 ` Bjorn Helgaas 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 11:30 ` Rafael J. Wysocki 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 12:16 ` Mika Westerberg 2020-06-25 17:07 ` David E. Box 2020-06-25 17:30 ` Bjorn Helgaas [this message] 2020-06-25 17:30 ` Bjorn Helgaas 2020-06-24 21:37 ` Bjorn Helgaas 2020-06-24 21:37 ` Bjorn Helgaas 2020-06-24 22:09 ` David E. Box 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 2020-06-12 20:48 ` 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=20200625173053.GA2694537@bjorn-Precision-5520 \ --to=helgaas@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=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=rafael@kernel.org \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.