linux-kernel - Re: [PATCH v2 1/4] PCI: Clear the LBMS bit after a link retrain

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]

Message-ID: <alpine.DEB.2.21.2408121505260.59022@angie.orcam.me.uk>
Date: Mon, 12 Aug 2024 15:21:08 +0100 (BST)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
cc: Bjorn Helgaas <bhelgaas@...gle.com>, 
    Matthew W Carlis <mattc@...estorage.com>, 
    Mika Westerberg <mika.westerberg@...ux.intel.com>, 
    Oliver O'Halloran <oohall@...il.com>, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] PCI: Clear the LBMS bit after a link retrain

On Mon, 12 Aug 2024, Ilpo Järvinen wrote:

> I'm slightly worried this particular change could interfere with the 
> intent of pcie_failed_link_retrain() because LBMS is cleared also in the 
> failure cases.

 I think it doesn't really matter, because in a correctly operating system 
LBMS is not supposed to be set at the point `pcie_failed_link_retrain' is 
called in the first place.  We don't want to respond to LBMS being set 
just as a consequence of writing 1 to the Retrain Link bit, because it is 
always set in this scenario even for open links and we know we've did the 
retraining anyway, so we can communicate it via other means if we need to.

> In the case of your HW, there's retraining loop by HW so LBMS gets set 
> again but if the HW would not retrain in a loop and needs similar gen1 
> bootstrap, it's very non-obvious to me how things will end up interacting 
> with pcie_retrain_link() call from pcie_aspm_configure_common_clock(). 
> That is, this could clear the LBMS indication and another is not going to 
> be asserted (and even in case of with the retraining loop, it would be 
> racy to get LBMS re-asserted soon enough).

 Yes, and it is an intended effect.  We only want to trigger for LBMS set 
by hardware in an attempt to correct unreliable link operation.

> My impression is that things seem to work with the current ordering of the 
> code but it seems quite fragile (however, the callchains are quite 
> complicated to track so I might have missed something). Perhaps that won't 
> matter much in the end with the bandwidth controller coming to rework all 
> this anyway but wanted to note this may have caveats.

 I look forward to the outcome of your effort.  I expect you'll remove 
this part then and handle the clearing of the LBMS bit in the bandwidth 
controller interrupt handler.

  Maciej