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

[Bjorn Helgaas]

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 http://vger.kernel.org/majordomo-info.html

>   - 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.
> https://partnerissuetracker.corp.google.com/issues/275663637

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:https://patchwork.kernel.org/project/linux-arm-msm/list/?
> 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.

Bjorn