All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
@ 2021-05-27 13:59 Mario Limonciello
  2021-05-27 14:35 ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Mario Limonciello @ 2021-05-27 13:59 UTC (permalink / raw)
  To: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg
  Cc: open list:NVM EXPRESS DRIVER, Mario Limonciello, Prike Liang,
	rrangel, david.e.box, Shyam-sundar S-k, Alexander Deucher

The documentation around the StorageD3Enable property hints that it
should be made on the PCI device.  This is where AMD systems set
the property and it's required for S0i3 support.

So rather than look for nodes of the root port only present on Intel
systems, switch to the companion ACPI device for all systems.
David Box from Intel indicated this should work on Intel as well.

Link: https://lore.kernel.org/linux-nvme/YK6gmAWqaRmvpJXb@google.com/T/#m900552229fa455867ee29c33b854845fce80ba70
Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
Suggested-by: Prike Liang <Prike.Liang@amd.com>
CC: rrangel@chromium.org
CC: david.e.box@linux.intel.com
CC: Shyam-sundar S-k <Shyam-sundar.S-k@amd.com>
CC: Alexander Deucher <Alexander.Deucher@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/nvme/host/pci.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

Changes from v1 to v2:
 * Drop the old PXSX/PEGP logic instead of supplement to it
 * Add references to other discussions on this topic

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a29b170701fc..d4eef8caa4cc 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2832,9 +2832,6 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 {
 	struct acpi_device *adev;
-	struct pci_dev *root;
-	acpi_handle handle;
-	acpi_status status;
 	u8 val;
 
 	/*
@@ -2842,28 +2839,10 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 	 * must use D3 to support deep platform power savings during
 	 * suspend-to-idle.
 	 */
-	root = pcie_find_root_port(dev);
-	if (!root)
-		return false;
 
-	adev = ACPI_COMPANION(&root->dev);
+	adev = ACPI_COMPANION(&dev->dev);
 	if (!adev)
 		return false;
-
-	/*
-	 * The property is defined in the PXSX device for South complex ports
-	 * and in the PEGP device for North complex ports.
-	 */
-	status = acpi_get_handle(adev->handle, "PXSX", &handle);
-	if (ACPI_FAILURE(status)) {
-		status = acpi_get_handle(adev->handle, "PEGP", &handle);
-		if (ACPI_FAILURE(status))
-			return false;
-	}
-
-	if (acpi_bus_get_device(handle, &adev))
-		return false;
-
 	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
 			&val))
 		return false;
-- 
2.25.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 13:59 [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead Mario Limonciello
@ 2021-05-27 14:35 ` Christoph Hellwig
  2021-05-27 14:44   ` Limonciello, Mario
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2021-05-27 14:35 UTC (permalink / raw)
  To: Mario Limonciello
  Cc: Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg,
	open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, david.e.box,
	Shyam-sundar S-k, Alexander Deucher

Adding Raul who has been asking for something like this as well.

I'd also really like to move nvme_acpi_storage_d3 out of the NVMe
driver.  The Microsoft document that the original document references
makes it very clear that this is not NVMe specific, but also covers
at least AHCI.  On top of that the platform simply can't know what kind
of PCIe device is in any given slot.  Last but not least this will also
allow us to add quirks for devices that fail to properly mark this
misfeature in the ACPI tables.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 14:35 ` Christoph Hellwig
@ 2021-05-27 14:44   ` Limonciello, Mario
  2021-05-27 15:52     ` Bjorn Helgaas
  2021-05-27 15:53     ` Raul Rangel
  0 siblings, 2 replies; 8+ messages in thread
From: Limonciello, Mario @ 2021-05-27 14:44 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Keith Busch, Jens Axboe, Sagi Grimberg,
	open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, david.e.box,
	Shyam-sundar S-k, Alexander Deucher, bhelgaas

On 5/27/2021 09:35, Christoph Hellwig wrote:
> Adding Raul who has been asking for something like this as well.
> 

I thought I already included him on CC.  Sorry if I munged the email or 
something.

> I'd also really like to move nvme_acpi_storage_d3 out of the NVMe
> driver.  The Microsoft document that the original document references
> makes it very clear that this is not NVMe specific, but also covers
> at least AHCI.  On top of that the platform simply can't know what kind
> of PCIe device is in any given slot.  Last but not least this will also
> allow us to add quirks for devices that fail to properly mark this
> misfeature in the ACPI tables.
> 

+Bjorn

Back when this feature was first submitted, that was actually the 
initial way that it was done, but Bjorn had preferred to see it move
into the NVME driver directly:

https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/

Bjorn, are you OK with this coming "back" to PCI based on Christoph's
comments?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 14:44   ` Limonciello, Mario
@ 2021-05-27 15:52     ` Bjorn Helgaas
  2021-05-27 16:58       ` Keith Busch
  2021-05-27 15:53     ` Raul Rangel
  1 sibling, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2021-05-27 15:52 UTC (permalink / raw)
  To: Limonciello, Mario
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box,
	Shyam-sundar S-k, Alexander Deucher

On Thu, May 27, 2021 at 9:44 AM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
> On 5/27/2021 09:35, Christoph Hellwig wrote:
> > Adding Raul who has been asking for something like this as well.
>
> > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe
> > driver.  The Microsoft document that the original document references
> > makes it very clear that this is not NVMe specific, but also covers
> > at least AHCI.  On top of that the platform simply can't know what kind
> > of PCIe device is in any given slot.  Last but not least this will also
> > allow us to add quirks for devices that fail to properly mark this
> > misfeature in the ACPI tables.
>
> +Bjorn
>
> Back when this feature was first submitted, that was actually the
> initial way that it was done, but Bjorn had preferred to see it move
> into the NVME driver directly:
>
> https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/
>
> Bjorn, are you OK with this coming "back" to PCI based on Christoph's
> comments?

My point was that the PCI core can do nothing with this information,
so it doesn't seem like putting it in PCI doesn't really gain
anything.  The Microsoft document you reference ([1]) doesn't mention
PCI or PCIe.  As far as I know, there's no PCI spec that mentions
"StorageD3Enable".

I agree that [1] doesn't seem to be NVMe-specific, since it also
mentions SATA, so it might make sense to look for "StorageD3Enable"
somewhere other than the NVMe driver.  I'm just not convinced the PCI
core is the best place.  I have the impression that it's possible to
have non-PCI SATA or AHCI devices (correct me if I'm wrong), and
"StorageD3Enable" could apply to them as well.

[1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 14:44   ` Limonciello, Mario
  2021-05-27 15:52     ` Bjorn Helgaas
@ 2021-05-27 15:53     ` Raul Rangel
  1 sibling, 0 replies; 8+ messages in thread
From: Raul Rangel @ 2021-05-27 15:53 UTC (permalink / raw)
  To: Limonciello, Mario
  Cc: Christoph Hellwig, Keith Busch, Jens Axboe, Sagi Grimberg,
	open list:NVM EXPRESS DRIVER, Prike Liang, david.e.box,
	Shyam-sundar S-k, Alexander Deucher, bhelgaas

On Thu, May 27, 2021 at 8:44 AM Limonciello, Mario
<mario.limonciello@amd.com> wrote:
>
> On 5/27/2021 09:35, Christoph Hellwig wrote:
> > Adding Raul who has been asking for something like this as well.
> >
>
> I thought I already included him on CC.  Sorry if I munged the email or
> something.

I did get V2. Thanks for adding me. I tested this patch out on an AMD
platform that doesn't use the PXSX name and it works as expected.

Acked-by: Raul E Rangel <rrangel@chromium.org>

>
> > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe
> > driver.  The Microsoft document that the original document references
> > makes it very clear that this is not NVMe specific, but also covers
> > at least AHCI.  On top of that the platform simply can't know what kind
> > of PCIe device is in any given slot.  Last but not least this will also
> > allow us to add quirks for devices that fail to properly mark this
> > misfeature in the ACPI tables.
> >
>
> +Bjorn
>
> Back when this feature was first submitted, that was actually the
> initial way that it was done, but Bjorn had preferred to see it move
> into the NVME driver directly:
>
> https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/
>
> Bjorn, are you OK with this coming "back" to PCI based on Christoph's
> comments?

Just to add another data point, we currently use StorageD3Enabled on
PCI SDHCI controllers:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/mainboard/google/volteer/variants/baseboard/devicetree.cb;l=504

I would be fine doing that refactor in a follow-up patch.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 15:52     ` Bjorn Helgaas
@ 2021-05-27 16:58       ` Keith Busch
  2021-05-27 17:03         ` Limonciello, Mario
  2021-05-27 17:28         ` Christoph Hellwig
  0 siblings, 2 replies; 8+ messages in thread
From: Keith Busch @ 2021-05-27 16:58 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Limonciello, Mario, Christoph Hellwig, Jens Axboe, Sagi Grimberg,
	open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box,
	Shyam-sundar S-k, Alexander Deucher

On Thu, May 27, 2021 at 10:52:43AM -0500, Bjorn Helgaas wrote:
> On Thu, May 27, 2021 at 9:44 AM Limonciello, Mario
> <mario.limonciello@amd.com> wrote:
> > On 5/27/2021 09:35, Christoph Hellwig wrote:
> > > Adding Raul who has been asking for something like this as well.
> >
> > > I'd also really like to move nvme_acpi_storage_d3 out of the NVMe
> > > driver.  The Microsoft document that the original document references
> > > makes it very clear that this is not NVMe specific, but also covers
> > > at least AHCI.  On top of that the platform simply can't know what kind
> > > of PCIe device is in any given slot.  Last but not least this will also
> > > allow us to add quirks for devices that fail to properly mark this
> > > misfeature in the ACPI tables.
> >
> > +Bjorn
> >
> > Back when this feature was first submitted, that was actually the
> > initial way that it was done, but Bjorn had preferred to see it move
> > into the NVME driver directly:
> >
> > https://lore.kernel.org/linux-nvme/20200625173053.GA2694537@bjorn-Precision-5520/
> >
> > Bjorn, are you OK with this coming "back" to PCI based on Christoph's
> > comments?
> 
> My point was that the PCI core can do nothing with this information,
> so it doesn't seem like putting it in PCI doesn't really gain
> anything.  The Microsoft document you reference ([1]) doesn't mention
> PCI or PCIe.  As far as I know, there's no PCI spec that mentions
> "StorageD3Enable".
> 
> I agree that [1] doesn't seem to be NVMe-specific, since it also
> mentions SATA, so it might make sense to look for "StorageD3Enable"
> somewhere other than the NVMe driver.  I'm just not convinced the PCI
> core is the best place.  I have the impression that it's possible to
> have non-PCI SATA or AHCI devices (correct me if I'm wrong), and
> "StorageD3Enable" could apply to them as well.
> 
> [1] https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro

I agree it doesn't appear to belong in PCI either. This should go in
ACPI. Here's my proposal on top of this patch:

---
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index d260bc1f3e6e..ab8a0dfae2df 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1340,4 +1340,25 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 	return 1;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+bool acpi_storage_d3(struct device *dev)
+{
+	struct acpi_device *adev;
+	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.
+	 */
+	adev = ACPI_COMPANION(dev);
+	if (!adev)
+		return false;
+	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
+			&val))
+		return false;
+	return val == 1;
+}
+EXPORT_SYMBOL_GPL(acpi_storage_d3);
+
 #endif /* CONFIG_PM */
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d4eef8caa4cc..8fbc4c87a0d8 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2828,33 +2828,6 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_ACPI
-static bool nvme_acpi_storage_d3(struct pci_dev *dev)
-{
-	struct acpi_device *adev;
-	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.
-	 */
-
-	adev = ACPI_COMPANION(&dev->dev);
-	if (!adev)
-		return false;
-	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
-			&val))
-		return false;
-	return val == 1;
-}
-#else
-static inline bool nvme_acpi_storage_d3(struct pci_dev *dev)
-{
-	return false;
-}
-#endif /* CONFIG_ACPI */
-
 static void nvme_async_probe(void *data, async_cookie_t cookie)
 {
 	struct nvme_dev *dev = data;
@@ -2904,7 +2877,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	quirks |= check_vendor_combination_bug(pdev);
 
-	if (!noacpi && nvme_acpi_storage_d3(pdev)) {
+	if (!noacpi && acpi_storage_d3(&pdev->dev)) {
 		/*
 		 * Some systems use a bios work around to ask for D3 on
 		 * platforms that support kernel managed suspend.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c60745f657e9..dd0dafd21e33 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1004,6 +1004,7 @@ int acpi_dev_resume(struct device *dev);
 int acpi_subsys_runtime_suspend(struct device *dev);
 int acpi_subsys_runtime_resume(struct device *dev);
 int acpi_dev_pm_attach(struct device *dev, bool power_on);
+bool acpi_storage_d3(struct device *dev);
 #else
 static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; }
 static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; }
@@ -1011,6 +1012,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 {
 	return 0;
 }
+static inline bool acpi_storage_d3(struct device *dev)
+{
+	return false;
+}
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
--

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 16:58       ` Keith Busch
@ 2021-05-27 17:03         ` Limonciello, Mario
  2021-05-27 17:28         ` Christoph Hellwig
  1 sibling, 0 replies; 8+ messages in thread
From: Limonciello, Mario @ 2021-05-27 17:03 UTC (permalink / raw)
  To: Keith Busch, Bjorn Helgaas
  Cc: Christoph Hellwig, Jens Axboe, Sagi Grimberg,
	open list:NVM EXPRESS DRIVER, Prike Liang, rrangel, David E. Box,
	Shyam-sundar S-k, Alexander Deucher, Rafael J. Wysocki

On 5/27/2021 11:58, Keith Busch wrote:
> On Thu, May 27, 2021 at 10:52:43AM -0500, Bjorn Helgaas wrote:
>> On Thu, May 27, 2021 at 9:44 AM Limonciello, Mario
>> <mario.limonciello@amd.com> wrote:
>>> On 5/27/2021 09:35, Christoph Hellwig wrote:
>>>> Adding Raul who has been asking for something like this as well.
>>>
>>>> I'd also really like to move nvme_acpi_storage_d3 out of the NVMe
>>>> driver.  The Microsoft document that the original document references
>>>> makes it very clear that this is not NVMe specific, but also covers
>>>> at least AHCI.  On top of that the platform simply can't know what kind
>>>> of PCIe device is in any given slot.  Last but not least this will also
>>>> allow us to add quirks for devices that fail to properly mark this
>>>> misfeature in the ACPI tables.
>>>
>>> +Bjorn
>>>
>>> Back when this feature was first submitted, that was actually the
>>> initial way that it was done, but Bjorn had preferred to see it move
>>> into the NVME driver directly:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-nvme%2F20200625173053.GA2694537%40bjorn-Precision-5520%2F&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C5480c9a7da154bb519f908d92130b272%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637577315313175138%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=xDn4vxurgMOokOvwLHdqHWa3tlSRUzh5XPBJdBmBUHo%3D&amp;reserved=0
>>>
>>> Bjorn, are you OK with this coming "back" to PCI based on Christoph's
>>> comments?
>>
>> My point was that the PCI core can do nothing with this information,
>> so it doesn't seem like putting it in PCI doesn't really gain
>> anything.  The Microsoft document you reference ([1]) doesn't mention
>> PCI or PCIe.  As far as I know, there's no PCI spec that mentions
>> "StorageD3Enable".
>>
>> I agree that [1] doesn't seem to be NVMe-specific, since it also
>> mentions SATA, so it might make sense to look for "StorageD3Enable"
>> somewhere other than the NVMe driver.  I'm just not convinced the PCI
>> core is the best place.  I have the impression that it's possible to
>> have non-PCI SATA or AHCI devices (correct me if I'm wrong), and
>> "StorageD3Enable" could apply to them as well.
>>
>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows-hardware%2Fdesign%2Fcomponent-guidelines%2Fpower-management-for-storage-hardware-devices-intro&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7C5480c9a7da154bb519f908d92130b272%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637577315313175138%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=uEOEgtuxZI3mkSU7G%2BUvZ2d2C18%2BcQu%2B1748kHhSuzU%3D&amp;reserved=0
> 
> I agree it doesn't appear to belong in PCI either. This should go in
> ACPI. Here's my proposal on top of this patch:

This seems fine to me.

+Rafael for comments on this approach.

> 
> ---
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index d260bc1f3e6e..ab8a0dfae2df 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1340,4 +1340,25 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>   	return 1;
>   }
>   EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> +
> +bool acpi_storage_d3(struct device *dev)
> +{
> +	struct acpi_device *adev;
> +	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.
> +	 */
> +	adev = ACPI_COMPANION(dev);
> +	if (!adev)
> +		return false;
> +	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
> +			&val))
> +		return false;
> +	return val == 1;
> +}
> +EXPORT_SYMBOL_GPL(acpi_storage_d3);
> +
>   #endif /* CONFIG_PM */
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d4eef8caa4cc..8fbc4c87a0d8 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2828,33 +2828,6 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
>   	return 0;
>   }
>   
> -#ifdef CONFIG_ACPI
> -static bool nvme_acpi_storage_d3(struct pci_dev *dev)
> -{
> -	struct acpi_device *adev;
> -	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.
> -	 */
> -
> -	adev = ACPI_COMPANION(&dev->dev);
> -	if (!adev)
> -		return false;
> -	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
> -			&val))
> -		return false;
> -	return val == 1;
> -}
> -#else
> -static inline bool nvme_acpi_storage_d3(struct pci_dev *dev)
> -{
> -	return false;
> -}
> -#endif /* CONFIG_ACPI */
> -
>   static void nvme_async_probe(void *data, async_cookie_t cookie)
>   {
>   	struct nvme_dev *dev = data;
> @@ -2904,7 +2877,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   
>   	quirks |= check_vendor_combination_bug(pdev);
>   
> -	if (!noacpi && nvme_acpi_storage_d3(pdev)) {
> +	if (!noacpi && acpi_storage_d3(&pdev->dev)) {
>   		/*
>   		 * Some systems use a bios work around to ask for D3 on
>   		 * platforms that support kernel managed suspend.
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index c60745f657e9..dd0dafd21e33 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1004,6 +1004,7 @@ int acpi_dev_resume(struct device *dev);
>   int acpi_subsys_runtime_suspend(struct device *dev);
>   int acpi_subsys_runtime_resume(struct device *dev);
>   int acpi_dev_pm_attach(struct device *dev, bool power_on);
> +bool acpi_storage_d3(struct device *dev);
>   #else
>   static inline int acpi_subsys_runtime_suspend(struct device *dev) { return 0; }
>   static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; }
> @@ -1011,6 +1012,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
>   {
>   	return 0;
>   }
> +static inline bool acpi_storage_d3(struct device *dev)
> +{
> +	return false;
> +}
>   #endif
>   
>   #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
> --
> 


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
  2021-05-27 16:58       ` Keith Busch
  2021-05-27 17:03         ` Limonciello, Mario
@ 2021-05-27 17:28         ` Christoph Hellwig
  1 sibling, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2021-05-27 17:28 UTC (permalink / raw)
  To: Keith Busch
  Cc: Bjorn Helgaas, Limonciello, Mario, Christoph Hellwig, Jens Axboe,
	Sagi Grimberg, open list:NVM EXPRESS DRIVER, Prike Liang,
	rrangel, David E. Box, Shyam-sundar S-k, Alexander Deucher,
	Rafael J. Wysocki

[adding Rafael]

On Thu, May 27, 2021 at 09:58:44AM -0700, Keith Busch wrote:
>  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> +
> +bool acpi_storage_d3(struct device *dev)

This need a really good kerneldoc comment.  Not like pci_pr3_present
which somehow made it into the PCI core, but has not explanation
whatsoever..

> +{
> +	struct acpi_device *adev;
> +	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.
> +	 */

This is part of what should got into the kerneldoc

> +	adev = ACPI_COMPANION(dev);
> +	if (!adev)
> +		return false;
> +	if (fwnode_property_read_u8(acpi_fwnode_handle(adev), "StorageD3Enable",
> +			&val))
> +		return false;
> +	return val == 1;
> +}

adev could be initialized at declaration time.

Also any chance we could have acpi_fwnode_property_read_* helpers
that include the NULL check and the acpi_fwnode_handle conversion which
seems to be boilerplated all over?

Also I wonder if having this only in ACPI is a good idea.  What when
we get the same thing in OF?  Isn't this something that the PM layer
should tell the device when calling ->suspend?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-05-27 18:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-27 13:59 [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead Mario Limonciello
2021-05-27 14:35 ` Christoph Hellwig
2021-05-27 14:44   ` Limonciello, Mario
2021-05-27 15:52     ` Bjorn Helgaas
2021-05-27 16:58       ` Keith Busch
2021-05-27 17:03         ` Limonciello, Mario
2021-05-27 17:28         ` Christoph Hellwig
2021-05-27 15:53     ` Raul Rangel

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.