[+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@xxxxxx/
[3] https://lore.kernel.org/all/m3r1f08p83.fsf@xxxxxxxxxxx/
