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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Mon, 29 May 2023 07:23:20 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     楊宗翰 <ecs.taipeikernel@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Bob Moragues <moragues@...gle.com>,
        Abner Yen <abner.yen@....com.tw>,
        Doug Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...gle.com>,
        Stephen Boyd <swboyd@...omium.org>, Harvey <hunge@...gle.com>,
        Gavin Lee <gavin.lee@....com.tw>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org
Subject: Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on
 sc7280

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