Re: [PATCH] nvme/pci: default to simple suspend

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

Hi Christoph,

On Tue, Feb 01, 2022 at 08:50:06AM -0800, Keith Busch wrote:
> There is no complete set of attributes a driver can check to know if
> nvme power management is the correct thing to do in a runtime suspend
> situation. Every attempt so far to optimize idle power consumption and
> resume latency for a particular platform just leads to regressions
> elsewhere.
> 
> Default to the simple shutdown since it is the historically safest
> option, and provide a user parameter to override it if the user knows
> it's safe to use for their platform.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215467
> Cc: Rafael Wysocki <rafael.j.wysocki@intel.com>
> Cc: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

I was looking into the nvme simple shutdown path for the Qualcomm platforms and
it looks to me that this patch from Keith is the way to go. I do agree with your
resistance on adding the platform level support to the nvme driver, but there is
no good way of working around it in the platform level.

PCI core only accepts the quirks for the host devices that could be passed onto
the PCI device drivers like this one. In this case, this is not a quirk but
actually an aggressive power saving feature (atleast on the Qcom platforms).
Moreover, adding a flag to the PCI bus will make it applicable to all the
child devices of the RC/bridge and that would be wrong.

In our case, the same power saving feature is not applicable to all PCI devices
like WLAN for an example.

Then if we try to add the platform specific support in nvme driver using the
of_machine_is_compatible() API, it will get piled up over the time. So just
using the simple shutdown as the default suspend choice and using the module
parameter to override it makes much sense to me.

Please let me know what you think.

Thanks,
Mani

> ---
>  drivers/nvme/host/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d8585df2c2fd..7e25cdef09a2 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -103,6 +103,10 @@ static bool noacpi;
>  module_param(noacpi, bool, 0444);
>  MODULE_PARM_DESC(noacpi, "disable acpi bios quirks");
>  
> +static bool pwr_mgmt;
> +module_param(pwr_mgmt, bool, 0444);
> +MODULE_PARM_DESC(pwr_mgmt, "use nvme power management for runtime suspend");
> +
>  struct nvme_dev;
>  struct nvme_queue;
>  
> @@ -3094,7 +3098,8 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		dev_info(&pdev->dev,
>  			 "platform quirk: setting simple suspend\n");
>  		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
> -	}
> +	} else if (!pwr_mgmt)
> +		quirks |= NVME_QUIRK_SIMPLE_SUSPEND;
>  
>  	/*
>  	 * Double check that our mempool alloc size will cover the biggest
> -- 
> 2.25.4