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
