* [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 @ 2023-05-25 8:35 Owen Yang 2023-05-25 22:10 ` Bjorn Helgaas 2023-05-29 16:48 ` Manivannan Sadhasivam 0 siblings, 2 replies; 7+ messages in thread From: Owen Yang @ 2023-05-25 8:35 UTC (permalink / raw) To: LKML Cc: Bob Moragues, Abner Yen, Doug Anderson, Matthias Kaehlcke, Stephen Boyd, Harvey, Gavin Lee, Owen Yang, Bjorn Helgaas, linux-pci Implement this workaround until Qualcomm fixed the correct NVMe suspend process. Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com> --- drivers/pci/quirks.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index f4e2a88729fd..b57876dc2624 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) } DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter + * the correct ASPM state properly. Therefore. Implement this workaround + * until Qualcomm fixed the correct NVMe suspend process*/ +static void phison_suspend_fixup(struct pci_dev *pdev) +{ + msleep(30); +} +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5013, phison_suspend_fixup); +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5015, phison_suspend_fixup); + static void rom_bar_overlap_defect(struct pci_dev *dev) { pci_info(dev, "working around ROM BAR overlap defect\n"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 2023-05-25 8:35 [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 Owen Yang @ 2023-05-25 22:10 ` Bjorn Helgaas [not found] ` <CAPao8GJNbXnh1R2-9rueMygyYyy-r3kqvQ55xdN61E7m6_dkdw@mail.gmail.com> 2023-05-29 16:48 ` Manivannan Sadhasivam 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2023-05-25 22:10 UTC (permalink / raw) To: Owen Yang Cc: LKML, Bob Moragues, Abner Yen, Doug Anderson, Matthias Kaehlcke, Stephen Boyd, Harvey, Gavin Lee, Bjorn Helgaas, linux-pci On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote: > Implement this workaround until Qualcomm fixed the > correct NVMe suspend process. Thanks for the patch. Before I can do anything, this needs: - Subject line in style of the file (use "git log --oneline drivers/pci/quirks.c"). - Format commit log correctly (fill 75 columns, no leading spaces). - Description of incorrect behavior. What does the user see? If there's a bug report, include a link to it. - Multi-line code comments in style of the file (look at existing comments in the file). - Details of "the correct ASPM state". ASPM may be enabled or disabled by the user, so you can't assume any particular ASPM configuration. - 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. - Drop the "until Qualcomm fixes NVMe suspend" text. Even if Qualcomm fixes something, we can't just drop this quirk because there will be platforms in the field that don't have the Qualcomm fix. Bjorn > Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com> > --- > > drivers/pci/quirks.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index f4e2a88729fd..b57876dc2624 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > } > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > > +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter > + * the correct ASPM state properly. Therefore. Implement this workaround > + * until Qualcomm fixed the correct NVMe suspend process*/ > +static void phison_suspend_fixup(struct pci_dev *pdev) > +{ > + msleep(30); > +} > +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5013, phison_suspend_fixup); > +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5015, phison_suspend_fixup); > + > static void rom_bar_overlap_defect(struct pci_dev *dev) > { > pci_info(dev, "working around ROM BAR overlap defect\n"); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAPao8GJNbXnh1R2-9rueMygyYyy-r3kqvQ55xdN61E7m6_dkdw@mail.gmail.com>]
* Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 [not found] ` <CAPao8GJNbXnh1R2-9rueMygyYyy-r3kqvQ55xdN61E7m6_dkdw@mail.gmail.com> @ 2023-05-29 12:23 ` Bjorn Helgaas 2023-05-30 21:06 ` Matthias Kaehlcke 1 sibling, 0 replies; 7+ messages in thread From: Bjorn Helgaas @ 2023-05-29 12:23 UTC (permalink / raw) To: 楊宗翰 Cc: LKML, Bob Moragues, Abner Yen, Doug Anderson, Matthias Kaehlcke, Stephen Boyd, Harvey, Gavin Lee, Bjorn Helgaas, linux-pci 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 [not found] ` <CAPao8GJNbXnh1R2-9rueMygyYyy-r3kqvQ55xdN61E7m6_dkdw@mail.gmail.com> 2023-05-29 12:23 ` Bjorn Helgaas @ 2023-05-30 21:06 ` Matthias Kaehlcke 1 sibling, 0 replies; 7+ messages in thread From: Matthias Kaehlcke @ 2023-05-30 21:06 UTC (permalink / raw) To: 楊宗翰 Cc: Bjorn Helgaas, LKML, Bob Moragues, Abner Yen, Doug Anderson, Stephen Boyd, Harvey, Gavin Lee, Bjorn Helgaas, linux-pci On Mon, May 29, 2023 at 02:24:53PM +0800, 楊宗翰 wrote: > Hi Bjorn, > > Thanks for your kind directions. > >  - 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. > >  - Format commit log correctly (fill 75 columns, no leading spaces). > Done. > >  - 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. It should be noted that D3cold (or whatever condition that causes the issue) is not always entered, but only in the failure case (at least that was the case for the Kioxia NVMe, which has a similar issue). >  - Multi-line code comments in style of the file (look at existing >   comments in the file). > Done. > >  - 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. IIRC the primary goal of this series was to be able to turn off the PCI clocks during suspend, to allow the SoC to enter a lower power state. This fixing element for NVMe with the issue described above is the the retry loop of "PCI: qcom: Add retry logic for link to be stable in L1ss" [1]. It is currently unclear why *some* NVMe *sometimes* need a longer time to enter the L1 sub-state. That's something Qualcomm and the vendors of impacted NVMes should figure out. [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1659526134-22978-4-git-send-email-quic_krichai@quicinc.com/ >  - 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. As of now the issue has only been observed on QC SC7280, I don't know if ECS has tried this part on other platforms. The issue could be QC/SC7280-specific or not. > 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 could be used in devices that are produced by other manufacturers. A dedicated Kconfig option for the Phison NVMe could be an option. Or a QC specific #ifdef (ugh ...) with a comment explaining that the issue has been only observed on QC SC7280 *so far*. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 2023-05-25 8:35 [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 Owen Yang 2023-05-25 22:10 ` Bjorn Helgaas @ 2023-05-29 16:48 ` Manivannan Sadhasivam 2023-05-30 21:17 ` Matthias Kaehlcke 1 sibling, 1 reply; 7+ messages in thread From: Manivannan Sadhasivam @ 2023-05-29 16:48 UTC (permalink / raw) To: Owen Yang Cc: LKML, Bob Moragues, Abner Yen, Doug Anderson, Matthias Kaehlcke, Stephen Boyd, Harvey, Gavin Lee, Bjorn Helgaas, linux-pci On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote: > Implement this workaround until Qualcomm fixed the > correct NVMe suspend process. > > Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com> > --- > > drivers/pci/quirks.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index f4e2a88729fd..b57876dc2624 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > } > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > > +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter > + * the correct ASPM state properly. Therefore. Implement this workaround > + * until Qualcomm fixed the correct NVMe suspend process*/ What is there to fix during suspend? Currently, Qcom PCIe driver just votes for low interconnect bandwidth and keeps the resources (clocks, regulators) ON during suspend. So there is no way the device would move to D3Cold. Earlier Qcom reported that during suspend, link down event happens when the resources are turned OFF without waiting for the link to enter L1ss. But as I said above, we are _not_ turning OFF any resources. I believe this patch is addressing an issue that is caused by an out-of-tree patch. - Mani > +static void phison_suspend_fixup(struct pci_dev *pdev) > +{ > + msleep(30); > +} > +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5013, phison_suspend_fixup); > +DECLARE_PCI_FIXUP_SUSPEND(0x1987, 0x5015, phison_suspend_fixup); > + > static void rom_bar_overlap_defect(struct pci_dev *dev) > { > pci_info(dev, "working around ROM BAR overlap defect\n"); > -- > 2.17.1 > -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 2023-05-29 16:48 ` Manivannan Sadhasivam @ 2023-05-30 21:17 ` Matthias Kaehlcke 2023-05-31 6:46 ` Manivannan Sadhasivam 0 siblings, 1 reply; 7+ messages in thread From: Matthias Kaehlcke @ 2023-05-30 21:17 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Owen Yang, LKML, Bob Moragues, Abner Yen, Doug Anderson, Stephen Boyd, Harvey, Gavin Lee, Bjorn Helgaas, linux-pci On Mon, May 29, 2023 at 10:18:56PM +0530, Manivannan Sadhasivam wrote: > On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote: > > Implement this workaround until Qualcomm fixed the > > correct NVMe suspend process. > > > > Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com> > > --- > > > > drivers/pci/quirks.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index f4e2a88729fd..b57876dc2624 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > > } > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > > > > +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter > > + * the correct ASPM state properly. Therefore. Implement this workaround > > + * until Qualcomm fixed the correct NVMe suspend process*/ > > What is there to fix during suspend? Currently, Qcom PCIe driver just votes for > low interconnect bandwidth and keeps the resources (clocks, regulators) ON > during suspend. So there is no way the device would move to D3Cold. > > Earlier Qcom reported that during suspend, link down event happens when the > resources are turned OFF without waiting for the link to enter L1ss. But as I > said above, we are _not_ turning OFF any resources. Right, it makes little sense that the NVMe would move to D3Cold. And why does the issue only reproduces sometimes (with certain NVMes) and not consistently? > I believe this patch is addressing an issue that is caused by an out-of-tree > patch. I think ECS observed this with Chrome OS v5.15 kernel. On the PCI side this kernel only has backported changes from upstream (mostly clean picks), no downstream patches, so it seems unlikely that the issue is caused by a downstream patch. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 2023-05-30 21:17 ` Matthias Kaehlcke @ 2023-05-31 6:46 ` Manivannan Sadhasivam 0 siblings, 0 replies; 7+ messages in thread From: Manivannan Sadhasivam @ 2023-05-31 6:46 UTC (permalink / raw) To: Matthias Kaehlcke Cc: Owen Yang, LKML, Bob Moragues, Abner Yen, Doug Anderson, Stephen Boyd, Harvey, Gavin Lee, Bjorn Helgaas, linux-pci On Tue, May 30, 2023 at 09:17:02PM +0000, Matthias Kaehlcke wrote: > On Mon, May 29, 2023 at 10:18:56PM +0530, Manivannan Sadhasivam wrote: > > On Thu, May 25, 2023 at 04:35:12PM +0800, Owen Yang wrote: > > > Implement this workaround until Qualcomm fixed the > > > correct NVMe suspend process. > > > > > > Signed-off-by: Owen Yang <ecs.taipeikernel@gmail.com> > > > --- > > > > > > drivers/pci/quirks.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > > index f4e2a88729fd..b57876dc2624 100644 > > > --- a/drivers/pci/quirks.c > > > +++ b/drivers/pci/quirks.c > > > @@ -5945,6 +5945,16 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev) > > > } > > > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup); > > > > > > +/* In Qualcomm 7c gen 3 sc7280 platform. Some of the SSD won't enter > > > + * the correct ASPM state properly. Therefore. Implement this workaround > > > + * until Qualcomm fixed the correct NVMe suspend process*/ > > > > What is there to fix during suspend? Currently, Qcom PCIe driver just votes for > > low interconnect bandwidth and keeps the resources (clocks, regulators) ON > > during suspend. So there is no way the device would move to D3Cold. > > > > Earlier Qcom reported that during suspend, link down event happens when the > > resources are turned OFF without waiting for the link to enter L1ss. But as I > > said above, we are _not_ turning OFF any resources. > > Right, it makes little sense that the NVMe would move to D3Cold. And why does > the issue only reproduces sometimes (with certain NVMes) and not consistently? > Honestly, I don't have any idea why it is happening. The link should transition to L1ss during suspend and we keep all resources ON. Did ECS only observe this issue when ASPM is enabled (powersupersave)? If so, then it is a NVMe firmware issue not Qualcomm. > > I believe this patch is addressing an issue that is caused by an out-of-tree > > patch. > > I think ECS observed this with Chrome OS v5.15 kernel. On the PCI side this > kernel only has backported changes from upstream (mostly clean picks), no > downstream patches, so it seems unlikely that the issue is caused by a > downstream patch. Okay, thanks for the clarification. Is it possible to reproduce it on mainline? Just to rule out the upstream vs downstream difference elsewhere. That should also be the case to submit a patch against mainline. - Mani -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-31 6:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-25 8:35 [PATCH v1] drivers: pci: quirks: Add suspend fixup for SSD on sc7280 Owen Yang 2023-05-25 22:10 ` Bjorn Helgaas [not found] ` <CAPao8GJNbXnh1R2-9rueMygyYyy-r3kqvQ55xdN61E7m6_dkdw@mail.gmail.com> 2023-05-29 12:23 ` Bjorn Helgaas 2023-05-30 21:06 ` Matthias Kaehlcke 2023-05-29 16:48 ` Manivannan Sadhasivam 2023-05-30 21:17 ` Matthias Kaehlcke 2023-05-31 6:46 ` Manivannan Sadhasivam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).