[PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
Limonciello, Mario
mario.limonciello at amd.com
Thu May 27 10:03:45 PDT 2021
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 at 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&data=04%7C01%7Cmario.limonciello%40amd.com%7C5480c9a7da154bb519f908d92130b272%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637577315313175138%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=xDn4vxurgMOokOvwLHdqHWa3tlSRUzh5XPBJdBmBUHo%3D&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&data=04%7C01%7Cmario.limonciello%40amd.com%7C5480c9a7da154bb519f908d92130b272%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637577315313175138%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=uEOEgtuxZI3mkSU7G%2BUvZ2d2C18%2BcQu%2B1748kHhSuzU%3D&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)
> --
>
More information about the Linux-nvme
mailing list