linux-kernel - Re: [PATCH v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+-6iNwY-Fc-nfawc_EtDRBvYht_491v80THW=4F-iY7Nqa81w@mail.gmail.com>
Date: Thu, 6 Feb 2025 13:27:44 -0500
From: Jim 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>, 
	Cyril Brulebois <kibi@...ian.org>, Stanimir Varbanov <svarbanov@...e.de>, 
	bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com, 
	Florian Fainelli <florian.fainelli@...adcom.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.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 v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Wed, Feb 05, 2025 at 02:12:01PM -0500, Jim Quinlan wrote:
> > Make changes to the code that limits the PCIe max speed.
> >
> > (1) Do the changes before link-up, not after.  We do not want
> >     to temporarily rise to a higher speed than desired.
>
> This is a functional change that should be split into its own patch.
> That will also make it obvious that this is not simple refactoring as
> the subject line advertises.
ack
>
> > (2) Use constants from pci_reg.h when possible
> > (3) Use uXX_replace_bits(...) for setting a register field.
>
> > (4) Use the internal link capabilities register for writing
> >     the max speed, not the official config space register
> >     where the speed field is RO.  Updating this field is
> >     not necessary to limit the speed so this mistake was
> >     harmless.
>
> Also a bug fix (though harmless in this case) that deserves to be
> split out so the distinction between the internal and the architected
> paths to the register is highlighted and may help prevent the same
> mistake in the future.
ack
>
> > Signed-off-by: Jim Quinlan <james.quinlan@...adcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 546056f7f0d3..f8fc3d620ee2 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -47,6 +47,7 @@
> >
> >  #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY                    0x04dc
> >  #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
> > +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK       0xf
>
> If the format of this internal register is different, of course we
> need a new #define for this.  But if this is just a different path to
> LNKCAP, and both paths read the same bits in the same format, I don't
> see the point of a new #define.
ack
>
> >  #define PCIE_RC_CFG_PRIV1_ROOT_CAP                   0x4f8
> >  #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK   0xf8
> > @@ -413,12 +414,12 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
> >  static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
> >  {
> >       u16 lnkctl2 = readw(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
> > -     u32 lnkcap = readl(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> > +     u32 lnkcap = readl(pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > -     lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
> > -     writel(lnkcap, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> > +     u32p_replace_bits(&lnkcap, gen, PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK);
> > +     writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > -     lnkctl2 = (lnkctl2 & ~0xf) | gen;
> > +     u16p_replace_bits(&lnkctl2, gen, PCI_EXP_LNKCTL2_TLS);
>
>
> OK.  I am not really a fan of the uXX_replace_bits() thing because
> it's not widely used (I found 341 instances tree-wide, compared to
> 14000+ for FIELD_PREP()), grep can't find the definition easily, and
> stylistically it doesn't fit with GENMASK(), FIELD_PREP(), etc.
>
> But it's already used throughout brcmstb.c, so we should be
> consistent.

And here I thought that uXX_replace_bits()  was the up-and-coming
solution to be used :-)

Regards,
Jim Quinlan
Broadcom STB/CM

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4210 bytes)