[<prev] [next>] [day] [month] [year] [list]
Message-ID: <ZHSZOKEMNbARQyiG@bhelgaas>
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
Powered by blists - more mailing lists