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: <5ca0b4a2-ec3a-4fa5-a691-7e3bab88890a@broadcom.com>
Date: Tue, 28 Oct 2025 16:01:50 -0400
From: James Quinlan <james.quinlan@...adcom.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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 10/22/25 18:45, Bjorn Helgaas wrote:
> 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?
yes and no, see below.
>
> 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.

Hi Bjorn,

I believe this has come up before.  Our "priv" registers should be exact 
mirrors of standard PCI config space registers to allow us to set RO 
fields.  Unfortunately, sometimes the "priv" fields do not exactly match 
the standard version.

As you have noticed, our field for MAX_LINK_WIDTH_MASK is 0x1f0 instead 
of 0x3f0.  The reason for this is not necessarily a BRCM limitation; our 
priv register has a CPM field mask of 0x200 (!) instead of where you 
would expect: 0x40000.  Further, everything in this priv register is 
different from its counterpart from bit 18 and higher.

> 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

Just like the fields of our "priv" registers may be non-standard, so may 
be the addresses of our "priv" registers.  The first few registers are 
in the same order as EXP_CAP, but then the rest are not, so using 
standard offsets does not work.

I don't know why HW did these things.  I can add a comment to the code 
like "priv register addresses and fields may not correspond to standard 
PCI registers".

Regards,

Jim Quinlan

Broadcom STB/CM

>  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 */