linux-kernel - Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

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: <aEzSilET0wAQ7ozA@geday>
Date: Fri, 13 Jun 2025 22:38:18 -0300
From: Geraldo Nascimento <geraldogabriel@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: linux-rockchip@...ts.infradead.org,
	Shawn Lin <shawn.lin@...k-chips.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Heiko Stuebner <heiko@...ech.de>, Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	linux-phy@...ts.infradead.org, linux-pci@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe
 defines

On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> > -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> > +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
> >  	status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16;
> 
> It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
> I guess this is because rockchip_pcie_write() does 32-bit writes, but
> PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.
> 
> If the hardware supports it, adding rockchip_pcie_readw() and
> rockchip_pcie_writew() for 16-bit accesses would make this read
> better.
> 
> Hopefully the hardware *does* support this (it's required per spec at
> least for config accesses, which would be a different path in the
> hardware).  Doing the 32-bit write of PCI_EXP_LNKCTL above is
> problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
> includes some RW1C bits that may be unintentionally cleared.

Hi Bjorn,

unfortunately Rockchip PCIe IP does not support 16-bit accesses,
I tried and it only rendered the kernel unbootable, which made
people in my house angry since the RK3399 box is my Internet Gateway!

:-)

For thit particular case, it is OK since LABS and LBMS are precisely
the only RW1C bits in LNKSTA as far as I know. But see below.
> >  
> > -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCSR);
> > -	status &= ~PCIE_RC_CONFIG_DCSR_MPS_MASK;
> > -	status |= PCIE_RC_CONFIG_DCSR_MPS_256;
> > -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR);
> > +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
> > +	status &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > +	status |= PCI_EXP_DEVCTL_PAYLOAD_256B;
> > +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
> 
> Similar problem here; PCI_EXP_DEVCTL is only 16 bits, and writing the
> adjacent PCI_EXP_DEVSTA may clear RW1C bits you didn't want to clear.
> 

This is a bit more concerning then above. I'm out of ideas regarding
this particular issue you raised.

Geraldo Nascimento
> Bjorn