Received-SPF: pass (google.com: domain of linux-kernel+bounces-83504-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 27 Feb 2024 16:29:15 +0100
From: Johan Hovold <johan@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Johan Hovold <johan+linaro@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v2 04/12] PCI: qcom: Add support for disabling ASPM L0s
 in devicetree
Message-ID: <Zd3_y_jbY8Ic2F0Y@hovoldconsulting.com>
References: <20240223152124.20042-5-johan+linaro@kernel.org>
 <20240223221000.GA118088@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240223221000.GA118088@bhelgaas>

On Fri, Feb 23, 2024 at 04:10:00PM -0600, Bjorn Helgaas wrote:
> On Fri, Feb 23, 2024 at 04:21:16PM +0100, Johan Hovold wrote:
> > Commit 9f4f3dfad8cf ("PCI: qcom: Enable ASPM for platforms supporting
> > 1.9.0 ops") started enabling ASPM unconditionally when the hardware
> > claims to support it. This triggers Correctable Errors for some PCIe
> > devices on machines like the Lenovo ThinkPad X13s, which could indicate
> > an incomplete driver ASPM implementation or that the hardware does in
> > fact not support L0s.
> 
> Are there any more details about this?  Do the errors occur around
> suspend/resume, a power state transition, or some other event?  Might
> other DWC-based devices be susceptible?  Is there a specific driver
> you suspect might be incomplete?

I see these errors when the devices in question are active as well as
idle (not during suspend/resume). For example, when running iperf3 or
fio to test the wifi and nvme, but I also see this occasionally for a
wifi device which is (supposedly) not active (e.g. a handful errors over
night).

I skimmed Qualcomm's driver and noted that there are some registers
related to ASPM which that driver updates, while the mainline driver
leaves them at their default settings, but I essentially only mentioned
that the ASPM implementation may be incomplete as a theoretical
possibility. The somewhat erratic ASPM behaviour for one of the modems
also suggests that some further tweak/quirk may be needed, and I was
hoping to catch Mani's interest by reporting it.

But based on what I've since heard from Qualcomm, it seems like these
correctable error may be a known issue with the hardware (e.g. seen
also with Windows), which is also why we decided to disable it for all
controllers on these two platforms where I've seen this in v2.
 
> Do you want the DT approach because the problem is believed to be
> platform-specific?  Otherwise, maybe we should consider reverting
> 9f4f3dfad8cf until the problem is understood?

Enabling ASPM gave a very significant improvement in battery life on the
Lenovo ThinkPad X13s, from 10.5 h to 15 h, so reverting is not really an
option there.

And with L0s disabled, the AER error reports about correctable errors
(that prevent enabling the GIC ITS and possibly degrades performance
somewhat) are gone.

I don't know for sure if there are further Qualcomm platform that are
affected by this so I also don't want to use a too big of a hammer. The
devicetree property allows us to disable L0s only after confirming that
it's needed, and we can always extend this to broader classes of device
when/if we learn more.

> Could this be done via a quirk like quirk_disable_aspm_l0s()?  That
> currently uses pci_disable_link_state(), which I don't think is
> completely safe because it leaves the possibility that drivers or
> users could re-enable L0s, e.g., via sysfs.

That was my first approach, thinking that it was the endpoint devices
which did not really support L0s. But initially it seemed like the wifi
controller on the CRD was not affected by this, while the same
controller on the X13s was. That made me conclude that this is not just
a property of the device but (also) of the controller and/or machine.

I then noticed that we already had some controller drivers implementing
'aspm-no-l0s' and decided to go with that.

> This patch is nice because IIUC it directly changes PCI_EXP_LNKCAP,
> which avoids that issue, but quirk_disable_aspm_l0s() could
> conceivably be reimplemented to cache PCI_EXP_LNKCAP in struct pci_dev
> so quirks could override it, as we do with struct pci_dev.devcap.

Johan