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] [day] [month] [year] [list]

Message-ID: <aAnOOj91-N6rwt2x@wunner.de>
Date: Thu, 24 Apr 2025 07:38:02 +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 Wed, Apr 23, 2025 at 02:37:11PM +0300, Ilpo Järvinen wrote:
> On Wed, 23 Apr 2025, Lukas Wunner wrote:
> > On Tue, Apr 22, 2025 at 02:55:47PM +0300, Ilpo Järvinen wrote:
> > > +void pcie_reset_lbms(struct pci_dev *port)
> > >  {
> > > -	struct pcie_bwctrl_data *data;
> > > -
> > > -	guard(rwsem_read)(&pcie_bwctrl_lbms_rwsem);
> > > -	data = port->link_bwctrl;
> > > -	if (data)
> > > -		atomic_set(&data->lbms_count, 0);
> > > -	else
> > > -		pcie_capability_write_word(port, PCI_EXP_LNKSTA,
> > > -					   PCI_EXP_LNKSTA_LBMS);
> > > +	clear_bit(PCI_LINK_LBMS_SEEN, &port->priv_flags);
> > > +	pcie_capability_write_word(port, PCI_EXP_LNKSTA, PCI_EXP_LNKSTA_LBMS);
> > >  }
> > 
> > Hm, previously the LBMS bit was only cleared in the Link Status register
> > if the bandwith controller hadn't probed yet.  Now it's cleared
> > unconditionally.  I'm wondering if this changes the logic somehow?
> 
> Hmm, that's a good question and I hadn't thought all the implications.
> I suppose leaving if (!port->link_bwctrl) there would retain the existing 
> behavior better allowing bwctrl to pick the link speed changes more 
> reliably.

I think the only potential issue with clearing the LBMS bit in the register
is that the bandwidth controller's irq handler won't see the bit and may
return with IRQ_NONE.

However, looking at the callers of pcie_reset_lbms(), that doesn't seem
to be a real issue.  There are only two of them:

- pcie_retrain_link() calls the function after the link was retrained.
  I guess the LBMS bit in the register may be set as a side-effect of
  the link retraining?  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?

- 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.


> Given this flag is only for the purposes of the quirk, it seems very much 
> out of proportions.

Yes, let's try to minimize the amount of locking, flags and code to support
the quirk.  Keep it as simple as possible.  So in that sense, the solution
you've chosen is probably fine.


> > >  static bool pcie_lbms_seen(struct pci_dev *dev, u16 lnksta)
> > >  {
> > > -	unsigned long count;
> > > -	int ret;
> > > -
> > > -	ret = pcie_lbms_count(dev, &count);
> > > -	if (ret < 0)
> > > -		return lnksta & PCI_EXP_LNKSTA_LBMS;
> > > +	if (test_bit(PCI_LINK_LBMS_SEEN, &dev->priv_flags))
> > > +		return true;
> > >  
> > > -	return count > 0;
> > > +	return lnksta & PCI_EXP_LNKSTA_LBMS;
> > >  }
> > 
> > Another small logic change here:  Previously pcie_lbms_count()
> > returned a negative value if the bandwidth controller hadn't
> > probed yet or wasn't compiled into the kernel.
> > Only in those two cases was the LBMS flag in the lnksta variable 
> > returned.
> > 
> > Now the LBMS flag is also returned if the bandwidth controller
> > is compiled into the kernel and has probed, but its irq handler
> > hasn't recorded a seen LBMS bit yet.
> > 
> > I'm guessing this can happen if the quirk races with the irq
> > handler and wins the race, so this safety net is needed?
> 
> The main reason why this check is here is for the boot when bwctrl is not 
> yet probed when the quirk runs. But the check just seems harmless, or 
> even somewhat useful, in the case when bwctrl has already probed. LBMS 
> being asserted should result in PCI_LINK_LBMS_SEEN even if the irq 
> handler has not yet done its job to transfer it into priv_flags.

Okay I'm convinced that the logic change in pcie_lbms_seen() is fine.

Thanks,

Lukas