linux-kernel - Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

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: <20250507163620.53v5djmhj3ywrge2@pali>
Date: Wed, 7 May 2025 18:36:20 +0200
From: Pali Rohár <pali@...nel.org>
To: Hans Zhang <18255117159@....com>
Cc: lpieralisi@...nel.org, kw@...ux.com, bhelgaas@...gle.com,
	heiko@...ech.de, manivannan.sadhasivam@...aro.org,
	yue.wang@...ogic.com, neil.armstrong@...aro.org, robh@...nel.org,
	jingoohan1@...il.com, khilman@...libre.com, jbrunet@...libre.com,
	martin.blumenstingl@...glemail.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
> On 2025/5/7 23:03, Hans Zhang wrote:
> > On 2025/5/7 01:41, Pali Rohár wrote:
> > > On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
> > > > The Aardvark PCIe controller enforces a fixed 512B payload size via
> > > > PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
> > > > core negotiations.
> > > > 
> > > > Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
> > > > PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
> > > > during device initialization, leveraging root port configurations and
> > > > device-specific capabilities.
> > > > 
> > > > Aligning Aardvark with the unified MPS framework ensures consistency,
> > > > avoids artificial constraints, and allows the hardware to operate at
> > > > its maximum supported payload size while adhering to PCIe
> > > > specifications.
> > > > 
> > > > Signed-off-by: Hans Zhang <18255117159@....com>
> > > > ---
> > > >   drivers/pci/controller/pci-aardvark.c | 2 --
> > > >   1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index a29796cce420..d8852892994a 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
> > > > advk_pcie *pcie)
> > > >       reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > >       reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
> > > >       reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
> > > > -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > > >       reg &= ~PCI_EXP_DEVCTL_READRQ;
> > > > -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
> > > >       reg |= PCI_EXP_DEVCTL_READRQ_512B;
> > > >       advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > Please do not remove this code. It is required part of the
> > > initialization of the aardvark PCI controller at the specific phase,
> > > as defined in the Armada 3700 Functional Specification.
> > > 
> > > There were reported more issues with those Armada PCIe controllers for
> > > which were already sent patches to mailing list in last 5 years. But
> > > unfortunately not all fixes were taken / applied yet.
> > 
> > Hi Pali,
> > 
> > I replied to you in version v2.
> > 
> > Is the maximum MPS supported by Armada 3700 512 bytes?

IIRC yes, 512-byte mode is supported. And I think in past I was testing
some PCIe endpoint card which required 512-byte long payload and did not
worked in 256-byte long mode (not sure if the card was not able to split
transaction or something other was broken, it is quite longer time).

> > What are the default values of DevCap.MPS and DevCtl.MPS?

Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
type?

Aardvark controller does not have the real HW PCI-to-PCI bridge device.
There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
create fake kernel PCI device in the hierarchy to make kernel and
userspace happy. Yes, this is deviation from the PCIe standard but well,
buggy HW is also HW.

And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.

> > Because the default value of DevCtl.MPS is not 512 bytes, it needs to be
> > configured here, right?
> > 
> > If it's my guess, RK3588 also has the same requirements as you, just
> > like the first patch I submitted.
> > 
> > Please take a look at the communication history:
> > https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/
> And this:
> https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-2-18255117159@163.com/

These changes are referring the to root ports PCI devices, which are not
applicable for aardvark PCIe controller.

> > 
> > Please test it using patch 1/3 of this series. If there are any
> > problems, please let me know.
> > 
> > 
> > Best regards,
> > Hans
> 

Sorry, but I stopped doing any testing of the aardvark driver with the
mainline kernel after PCI maintainers stopped taking fixes for the
driver and stopped responding.

I'm not going to debug same issues again, which I have analyzed,
prepared fixes, sent patches and see no progress there.

Seems that there is a status quo, and I'm not going to change it.