Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Jun 2020 16:37:57 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     "David E. Box" <david.e.box@linux.intel.com>
Cc:     shyjumon.n@intel.com, rjw@rjwysocki.net, lenb@kernel.org,
        bhelgaas@google.com, dan.j.williams@intel.com, kbusch@kernel.org,
        axboe@fb.com, hch@lst.de, sagi@grimberg.me,
        linux-acpi@vger.kernel.org, linux-nvme@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH V2 1/2] PCI: Add ACPI StorageD3Enable _DSD support
Message-ID: <20200624213757.GA2591059@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200612204820.20111-2-david.e.box@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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. 

Is this something that should apply to plug-in drives, or does this
only apply to soldered-in things?

> It is a
> BIOS work around that is currently in use on shipping systems like some
> Intel Comet Lake platforms. 

What is this BIOS work around?  Is there a defect here that's being
worked around?  What's the defect?

> It is meant to change default driver policy for
> suspend that may cause higher power consumption.

I guess this means that by changing the driver policy from the
default, we can save some power?

> Add the DSD property for recognition by fwnode calls and provide an
> exported symbol for device drivers to use to read the property as needed.
> 
> Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro

There is surprisingly little information in this intro.  The whole
paragraph under "Modern Standby Power Management" is duplicated
immediately below in "D3 Support".  Maybe that's a copyediting error
that displaced useful information.

It says "drivers should go to the deepest appropriate state" so
"function drivers don't have to manage implementation details".  No
doubt "drivers" and "function drivers" is a meaningful distinction to
Windows cognoscenti, but it's not to me.

It talks about "enabling D3" without specifying D3hot or D3cold.

It talks about "D3 support for storage devices."  All PCI devices are
required to support both D3hot and D3cold, so this must be talking
about some other sort of support; I suppose maybe it's a hint about
whether a driver should *use* D3hot (or D3cold, I can't tell).

It says nothing about where to look for the _DSD: on a Root Port or on
the NVMe endpoint.

Bjorn