linux-kernel - Re: [PATCH] PCI: dw-rockchip: Configure max payload size on host init

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: <vtb7vd7xbdoct3k2ck6nqngevq3os7t6iof5vyxnfoutingjpl@juhcjyp6qstp>
Date: Mon, 21 Apr 2025 20:18:31 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Niklas Cassel <cassel@...nel.org>
Cc: Hans Zhang <18255117159@....com>, Shawn Lin <shawn.lin@...k-chips.com>, 
	lpieralisi@...nel.org, kw@...ux.com, bhelgaas@...gle.com, heiko@...ech.de, 
	robh@...nel.org, jingoohan1@...il.com, thomas.richard@...tlin.com, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH] PCI: dw-rockchip: Configure max payload size on host init

On Thu, Apr 17, 2025 at 10:39:49AM +0200, Niklas Cassel wrote:
> On Thu, Apr 17, 2025 at 04:07:51PM +0800, Hans Zhang wrote:
> > On 2025/4/17 15:48, Niklas Cassel wrote:
> > 
> > Hi Niklas and Shawn,
> > 
> > Thank you very much for your discussion and reply.
> > 
> > I tested it on RK3588 and our platform. By setting pci=pcie_bus_safe, the
> > maximum MPS will be automatically matched in the end.
> > 
> > So is my patch no longer needed? For RK3588, does the customer have to
> > configure CONFIG_PCIE_BUS_SAFE or pci=pcie_bus_safe?
> > 
> > Also, for pci-meson.c, can the meson_set_max_payload be deleted?
> 
> I think the only reason why this works is because
> pcie_bus_configure_settings(), in the case of
> pcie_bus_config == PCIE_BUS_SAFE, will walk the bus and set MPS in
> the bridge to the lowest of the downstream devices:
> https://github.com/torvalds/linux/blob/v6.15-rc2/drivers/pci/probe.c#L2994-L2999
> 
> 
> So Hans, if you look at lspci for the other RCs/bridges that don't
> have any downstream devices connected, do they also show DevCtl.MPS 256B
> or do they still show 128B ?
> 
> 
> One could argue that for all policies (execept for maybe PCIE_BUS_TUNE_OFF),
> pcie_bus_configure_settings() should start off by initializing DevCtl.MPS to
> DevCap.MPS (for the bridge itself), and after that pcie_bus_configure_settings()
> can override it depending on policy, e.g. set MPS to 128B in case of
> pcie_bus_config == PCIE_BUS_PEER2PEER, or walk the bus in case of
> pcie_bus_config == PCIE_BUS_SAFE.
> 
> That way, we should be able to remove the setting for pci-meson.c as well.
> 

Sorry for being late to the party!

I agree with you that we should set the bridge's MPS to MPSS by default. But
doing so would have an effect on almost all platforms. Also, I'm not sure if the
BIOS would've configured the bridge's MPS to a sane default (I'm just
speculating here).

So IMO we should limit the change to platforms having controller drivers in the
kernel. This way, we can get rid of the MPS hacks from controller drivers (like
pci-meson) and also make the change less invasive. If required, we can make it
generic for all platforms in the future.

Let me share the diff I have in mind as a reply to hans's proposal.

- Mani

-- 
மணிவண்ணன் சதாசிவம்