lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 15 Feb 2024 11:21:45 +0100
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Krzysztof WilczyƄski <kw@...ux.com>,
 Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
 Philipp Zabel <p.zabel@...gutronix.de>, linux-arm-msm@...r.kernel.org,
 linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
 Johan Hovold <johan+linaro@...nel.org>
Subject: Re: [PATCH v2 2/3] PCI: qcom: Read back PARF_LTSSM register

On 14.02.2024 23:28, Bjorn Helgaas wrote:
> On Wed, Feb 14, 2024 at 10:35:16PM +0100, Konrad Dybcio wrote:
>> On 12.02.2024 22:17, Bjorn Helgaas wrote:
>>> Maybe include the reason in the subject?  "Read back" is literally
>>> what the diff says.
>>>
>>> On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote:
>>>> To ensure write completion, read the PARF_LTSSM register after setting
>>>> the LTSSM enable bit before polling for "link up".
>>>
>>> The write will obviously complete *some* time; I assume the point is
>>> that it's important for it to complete before some other event, and it
>>> would be nice to know why that's important.
>>
>> Right, that's very much meaningful on non-total-store-ordering
>> architectures, like arm64, where the CPU receives a store instruction,
>> but that does not necessarily impact the memory/MMIO state immediately.
> 
> I was hinting that maybe we could say what the other event is, or what
> problem this solves?  E.g., maybe it's as simple as "there's no point
> in polling for link up until after the PARF_LTSSM store completes."
> 
> But while the read of PARF_LTSSM might reduce the number of "is the
> link up" polls, it probably wouldn't speed anything up otherwise, so I
> suspect there's an actual functional reason for this patch, and that's
> what I'm getting at.

So, the register containing the "enable switch" (PARF_LTSSM) can (due
to the armv8 memory model) be "written" but not "change the value of
memory/mmio from the perspective of other (non-CPU) memory-readers
(such as the MMIO-mapped PCI controller itself)".

In that case, the CPU will happily continue calling qcom_pcie_link_up()
in a loop, waiting for the PCIe controller to bring the link up, however
the PCIE controller may have never received the PARF_LTSSM "enable link"
write by the time we decide to time out on checking the link status.

It may also never happen for you, but that's exactly like a classic race
condition, where it may simply not manifest due to the code around the
problematic lines hiding it. It may also only manifest on certain CPU
cores that try to be smarter than you and keep reordering/delaying
instructions if they don't seem immediately necessary.

Konrad

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ