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: <aAi734h55l7g6eXH@wunner.de>
Date: Wed, 23 Apr 2025 12:07:27 +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,
	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

[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@...ner.de>

Thanks,

Lukas