Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 May 2023 07:23:20 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     =?utf-8?B?5qWK5a6X57+w?= <ecs.taipeikernel@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Bob Moragues <moragues@google.com>,
        Abner Yen <abner.yen@ecs.com.tw>,
        Doug Anderson <dianders@chromium.org>,
        Matthias Kaehlcke <mka@google.com>,
        Stephen Boyd <swboyd@chromium.org>, Harvey <hunge@google.com>,
        Gavin Lee <gavin.lee@ecs.com.tw>,
        Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org
Subject: Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on
 sc7280
Message-ID: <ZHSZOKEMNbARQyiG@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAPao8GJNbXnh1R2-9rueMygyYyy-r3kqvQ55xdN61E7m6_dkdw@mail.gmail.com>
Precedence: bulk

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