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

From: Christoph Hellwig <hch@lst.de>
To: Keith Busch <kbusch@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	"Limonciello, Mario" <mario.limonciello@amd.com>,
	Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@fb.com>,
	Sagi Grimberg <sagi@grimberg.me>,
	"open list:NVM EXPRESS DRIVER" <linux-nvme@lists.infradead.org>,
	Prike Liang <Prike.Liang@amd.com>,
	rrangel@chromium.org,
	"David E. Box" <david.e.box@linux.intel.com>,
	Shyam-sundar S-k <Shyam-sundar.S-k@amd.com>,
	Alexander Deucher <Alexander.Deucher@amd.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Subject: Re: [PATCH v2] nvme: Look for StorageD3Enable on companion ACPI device instead
Date: Thu, 27 May 2021 19:28:58 +0200	[thread overview]
Message-ID: <20210527172857.GA7426@lst.de> (raw)
In-Reply-To: <20210527165844.GC3706388@dhcp-10-100-145-180.wdc.com>

[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