Re: [PATCH v2 1/1] PCI/bwctrl: Replace lbms_count with PCI_LINK_LBMS_SEEN flag

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3582 bytes --]

On Wed, 23 Apr 2025, Lukas Wunner wrote:

> [cc += Maciej, start of thread is here:
> https://lore.kernel.org/r/20250422115548.1483-1-ilpo.jarvinen@linux.intel.com/
> ]
> 
> 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.

However, I'm not entirely sure if the old code was a good idea either as 
it assumed the irq handler had read LBMS by the time lbms_count is reset.
Solving that would seemingly require locking to not race with remove, 
which just got removed (LOL) :-(.

Given this flag is only for the purposes of the quirk, it seems very much 
out of proportions. The quirk seeing extra LBMS doesn't seem to have a big 
practical impact. At worst case, the link speed becomes gen1 if the quirk 
fails to restore the original link speed for some reason (which, IIRC, it 
didn't yet attempt do when the original LBMS reset code was added).

So I'd prefer going with the if (!port->link_bwctrl) solution.

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

One cannot disable bwctrl, it always comes on with PCIe.

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

> This is quite subtle so I thought I'd ask.

It's good that you asked! :-)

> The patch otherwise
> LGTM, so assuming the two subtle logic changes above are intentional
> and can be explained, this is
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> 
> Thanks,
> 
> Lukas
> 

-- 
 i.