Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead

["Limonciello, Mario" <mario.limonciello@amd.com>]

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