Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280

From: Bjorn Helgaas
Date: Mon May 29 2023 - 08:23:27 EST

On Mon, May 29, 2023 at 02:24:53PM +0800, 楊宗翰 wrote:
> Hi Bjorn,
> Thanks for your kind directions.

Your response was a multi-part message, which doesn't work on the
Linux mailing lists. See

> - Subject line in style of the file (use "git log --oneline
> drivers/pci/quirks.c").
> Done, and I resend in topic "[PATCH v1] PCI: Add suspend fixup for SSD
> on sc7280", please review it.

This would actually have been "v2", since you sent v1 previously.

> - Description of incorrect behavior. What does the user see? If
> there's a bug report, include a link to it.
> This issue seems to be discovered in ChromeOS only. SSD will randomly
> crashed at 100~250+ suspend/resume cycle. Phison and Qualcomm
> found that its due to NVMe entering D3cold instead of L1ss.

This kind of information needs to be in the commit log, not just in
the email thread.

It's best if there is a published errata document from Qualcomm that
describes the issue and how software should work around it. Obviously
a URL to that document would be in the commit log.

> - Multi-line code comments in style of the file (look at existing
> comments in the file).
> Done.

Not quite done. Needs to be like this:

* Text ...

Not like this:

/* Text ...

> - Details of "the correct ASPM state". ASPM may be enabled or
> disabled by the user, so you can't assume any particular ASPM
> configuration.
> According to Qualcomm. This issue has been found last year and they have
> attempt to submit some patches to fix the pci suspend behavior.
> (ref:
> series=665060&state=%2A&archive=both).
> But somehow these patches were rejected because of its complexity. And
> we've got advise from Google that it will be more efficient that we
> implement
> a quirks to fix this issue.

Some of this history or at least a pointer to it should be in the
commit log.

> - Details on the Qualcomm sc7280 connection. This quirk would
> affect Phison SSDs on *all* platforms, not just sc7280. I don't
> want to slow down suspend on all platforms just for a sc7280
> issue.
> The DECLARE_PCI_FIXUP_SUSPEND function has already specify the PCI device
> ID. And this SSD will only be used at our Chromebook device only.

It's hard to guarantee that this will only be used in Chromebook, so
this is a little weak. But if it's the best we have, it needs to be
mentioned in the code comment.