Hi, Bjorn,
On Tue, Jan 31, 2023 at 8:16 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Fri, Jan 06, 2023 at 05:51:42PM +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).
>
> Can you take a look at
> https://bugzilla.kernel.org/show_bug.cgi?id=216884 ?
>
> That claims to be a regression between v6.1 and v6.2-rc2, and WANG
> Xuerui says this patch is the fix (though AFAICT the submitter has not
> verified this yet). If so, we should reference that bug here and try
> to get this in v6.2.
Yes, this patch can fix that issue. But I don't think this is a
regression, vanila 6.1 kernel also has this problem, maybe the
reporter uses a patched 6.1 kernel.
Huacai
>
> See below.
>
> > - if (pci_match_id(bridge_devids, bridge)) {
> > - if (pcie_get_readrq(dev) > 256) {
> > - pci_info(dev, "limiting MRRS to 256\n");
> > - pcie_set_readrq(dev, 256);
> > - }
> > - break;
> > - }
>
> > + if (bridge->no_inc_mrrs) {
> > + if (rq > pcie_get_readrq(dev))
> > + return -EINVAL;
>
> I think the message about limiting MRRS was useful and we should keep
> it.
>
> Bjorn
