linux-kernel - Re: [PATCH v2 1/1] PCI/bwctrl: Replace lbms_count with PCI_LINK_LBMS

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: <aAtgIfG8VG7vLDPN@wunner.de>
Date: Fri, 25 Apr 2025 12:12:49 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>,
	"Maciej W. Rozycki" <macro@...am.me.uk>
Subject: Re: [PATCH v2 1/1] PCI/bwctrl: Replace lbms_count with
 PCI_LINK_LBMS_SEEN flag

On Thu, Apr 24, 2025 at 03:37:38PM +0300, Ilpo Järvinen wrote:
> On Thu, 24 Apr 2025, Lukas Wunner wrote:
> >   The only concern here is whether the cached
> >   link speed is updated.  pcie_bwctrl_change_speed() does call
> >   pcie_update_link_speed() after calling pcie_retrain_link(), so that
> >   looks fine.  But there's a second caller of pcie_retrain_link():
> >   pcie_aspm_configure_common_clock().  It doesn't update the cached
> >   link speed after calling pcie_retrain_link().  Not sure if this can
> >   lead to a change in link speed and therefore the cached link speed
> >   should be updated?  The Target Link Speed isn't changed, but maybe
> >   the link fails to retrain to the same speed for electrical reasons?
> 
> I've never seen that to happen but it would seem odd if that is forbidden 
> (as the alternative is probably that the link remains down).
> 
> Perhaps pcie_reset_lbms() should just call pcie_update_link_speed() as the 
> last step, then the irq handler returning IRQ_NONE doesn't matter.

Why pcie_reset_lbms()?  I was rather thinking that pcie_update_link_speed()
should be called from pcie_retrain_link().  Maybe right after the final
pcie_wait_for_link_status().

That would ensure that the speed is updated in case retraining from
pcie_aspm_configure_common_clock() happens to lead to a lower speed.
And the call to pcie_update_link_speed() from pcie_bwctrl_change_speed()
could then be dropped.

PCIe r6.2 sec 7.5.3.19 says the Target Link Speed "sets an upper limit
on Link operational speed", which implies that the actual negotiated
speed might be lower.


> > - pciehp's remove_board() calls the function after bringing down the slot
> >   to avoid a stale PCI_LINK_LBMS_SEEN flag.  No real harm in clearing the
> >   bit in the register at this point I guess.  But I do wonder, is the link
> >   speed updated somewhere when a new board is added?  The replacement
> >   device may not support the same speeds as the previous device.
> 
> The supported speeds are always recalculated using dev->supported_speeds. 
> A new board implies a new pci_dev structure with newly read supported 
> speeds. Also, bringing the link up with the replacement device will also 
> trigger LBMS so the new Link Speed should be picked up by that.
> 
> Racing LBMS reset from remove_board() with LBMS due to the replacement 
> board shouldn't result in stale Link Speed because of:
> 
> board_added()
>   pciehp_check_link_status()
>     __pcie_update_link_speed()

Good!  That's the information I was looking for.

Thanks,

Lukas