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

[Lukas Wunner <lukas@wunner.de>]

[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?


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

This is quite subtle so I thought I'd ask.  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