Re: [PATCH V13 4/6] PCI: loongson: Improve the MRRS quirk for LS7A

From: Bjorn Helgaas <helgaas@kernel.org>
To: Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc: "Huacai Chen" <chenhuacai@loongson.cn>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	linux-pci <linux-pci@vger.kernel.org>,
	"Xuefeng Li" <lixuefeng@loongson.cn>,
	"Huacai Chen" <chenhuacai@gmail.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Kishon Vijay Abraham I" <kishon@ti.com>,
	"Krzysztof Hałasa" <khalasa@piap.pl>
Subject: Re: [PATCH V13 4/6] PCI: loongson: Improve the MRRS quirk for LS7A
Date: Thu, 2 Jun 2022 11:20:39 -0500	[thread overview]
Message-ID: <20220602162039.GA20136@bhelgaas> (raw)
In-Reply-To: <06d1f3d1-2864-458a-a1f0-ed3047b1cddf@www.fastmail.com>

[+cc Jingoo, Gustavo, Kishon, Krzysztof]

On Wed, Jun 01, 2022 at 12:59:50PM +0100, Jiaxun Yang wrote:
> 在2022年6月1日六月 上午3:22，Bjorn Helgaas写道：
> > On Sat, Apr 30, 2022 at 04:48:44PM +0800, Huacai Chen wrote:
> >> In new revision of LS7A, some PCIe ports support larger value than 256,
> >> but their maximum supported MRRS values are not detectable. Moreover,
> >> the current loongson_mrrs_quirk() cannot avoid devices increasing its
> >> MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> >> will actually set a big value in its driver. So the only possible way
> >> is configure MRRS of all devices in BIOS, and add a pci host bridge bit
> >> flag (i.e., no_inc_mrrs) to stop the increasing MRRS operations.
> >> 
> >> However, according to PCIe Spec, it is legal for an OS to program any
> >> value for MRRS, and it is also legal for an endpoint to generate a Read
> >> Request with any size up to its MRRS. As the hardware engineers say, the
> >> root cause here is LS7A doesn't break up large read requests. In detail,
> >> LS7A PCIe port reports CA (Completer Abort) if it receives a Memory Read
> >> request with a size that's "too big" ("too big" means larger than the
> >> PCIe ports can handle, which means 256 for some ports and 4096 for the
> >> others, and of course this is a problem in the LS7A's hardware design).
> >
> > This seems essentially similar to ks_pcie_quirk() [1].  Why are they
> > different, and why do you need no_inc_mrrs, when keystone doesn't?
> >
> > Or *does* keystone need it and we just haven't figured that out yet?
> > Are all callers of pcie_set_readrq() vulnerable to issues there?
> 
> Yes actually keystone may need to set this flag as well.
> 
> I think Huacai missed a point in his commit message about why he removed
> the process of walking through the bus and set proper MRRS. That’s
> because Loongson’s firmware will set proper MRRS and the only thing
> that Kernel needs to do is leave it as is. no_inc_mrrs is introduced for
> this purpose.

I'd really like to have a single implementation of whatever quirk
works around this.  I don't think we should have multiple copies just
because we assume some firmware takes care of part of this for us.

> In keystone’s case it’s likely that their firmware won’t do such thing, so
> their workaround shouldn’t be removed.
> And  no_inc_mrrs should be set for them to prevent device drivers modifying
> MRRS afterwards.

I have the vague impression that this issue is related to an arm64 AXI
bus property [2] or maybe a DesignWare controller property [3], so
this might affect several PCIe controller drivers.

> > Whatever we do should be as uniform as possible across host
> > controllers.
> >
> > [1] 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-keystone.c?id=v5.18#n528

[2] https://lore.kernel.org/all/20211126083119.16570-4-kishon@ti.com/
[3] https://lore.kernel.org/all/m3r1f08p83.fsf@t19.piap.pl/