linux-kernel - Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant

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: <20251022224553.GA1271981@bhelgaas>
Date: Wed, 22 Oct 2025 17:45:53 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Jim Quinlan <james.quinlan@...adcom.com>
Cc: linux-pci@...r.kernel.org, Nicolas Saenz Julienne <nsaenz@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Rob Herring <robh@...nel.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@...ts.infradead.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@...ts.infradead.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: brcmstb: Fix use of incorrect constant

On Fri, Oct 03, 2025 at 01:04:36PM -0400, Jim Quinlan wrote:
> The driver was using the PCIE_LINK_STATE_L1 constant as a field mask for
> setting the private PCI_EXP_LNKCAP register, but this constant is
> Linux-created and has nothing to do with the PCIe spec.  Serendipitously,
> the value of this constant was correct for its usage until after 6.1, when
> its value changed from BIT(1) to BIT(2);
> ...

>  #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY			0x04dc
>  #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_WIDTH_MASK	0x1f0
> -#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK	0xc00

This all tangential questions for possible future changes, not
anything for *this* patch.

We have these in include/uapi/linux/pci_regs.h:

  #define PCI_EXP_LNKCAP          0x0c    /* Link Capabilities */
  #define  PCI_EXP_LNKCAP_MLW     0x000003f0 /* Maximum Link Width */
  #define  PCI_EXP_LNKCAP_ASPMS   0x00000c00 /* ASPM Support */
  #define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */

Since you're using PCI_EXP_LNKCAP_ASPM_L0S below for writes to
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY, I assume
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY is another name for PCI_EXP_LNKCAP?

If so, it looks like we should also use PCI_EXP_LNKCAP_MLW instead of
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_WIDTH_MASK (although the
value is different for some reason; maybe 0x1f0 just reflects the
limits of brcmstb).

It would be nice to have a #define for the base of the PCIe
Capability so we could use that plus PCI_EXP_LNKCAP instead of
PCIE_RC_CFG_PRIV1_LINK_CAPABILITY.

But you did something like that already for PCI_EXP_LNKCTL2 using
BRCM_PCIE_CAP_REGS (0x00ac), which means LNKCTL2 and LNKCAP must be
at:

  LNKCTL2: 0x00dc (0x00ac + 0x30)
  LNKCAP:  0x04dc (0x04d0 + 0x0c)

which doesn't look at all like they would both be in the actual PCIe
Capability format.

>  #define PCIE_RC_CFG_PRIV1_ROOT_CAP			0x4f8
>  #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK	0xf8

>From its usage to "un-advertise L1 substates",
PCIE_RC_CFG_PRIV1_ROOT_CAP looks like it might be related to 
PCI_L1SS_CAP, but 0xf8 doesn't really match up to
PCI_L1SS_CAP_L1_PM_SS (0x10)

If this is really related to PCI_L1SS_CAP, can we use the names from
pci_regs.h somehow?

>  	/* Don't advertise L0s capability if 'aspm-no-l0s' */
> -	aspm_support = PCIE_LINK_STATE_L1;
> -	if (!of_property_read_bool(pcie->np, "aspm-no-l0s"))
> -		aspm_support |= PCIE_LINK_STATE_L0S;
>  	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> -	u32p_replace_bits(&tmp, aspm_support,
> -		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> +	if (of_property_read_bool(pcie->np, "aspm-no-l0s"))
> +		tmp &= ~PCI_EXP_LNKCAP_ASPM_L0S;
>  	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
>  
>  	/* 'tmp' still holds the contents of PRIV1_LINK_CAPABILITY */