Hi, Bjorn,
On Tue, Jun 29, 2021 at 6:35 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Mon, Jun 28, 2021 at 06:10:26PM +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 bus flag (i.e.,
> > PCI_BUS_FLAGS_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" (Yes, that is a problem in the LS7A
> > hardware design).
> >
> > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > ---
> > drivers/pci/pci.c | 5 +++++
> > drivers/pci/quirks.c | 41 +++++++++++------------------------------
> > include/linux/pci.h | 1 +
> > 3 files changed, 17 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 8d4ebe095d0c..0f1ff4a6fe44 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5812,6 +5812,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> >
> > v = (ffs(rq) - 8) << 12;
> >
> > + if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_INC_MRRS) {
> > + if (rq > pcie_get_readrq(dev))
> > + return -EINVAL;
>
> I'd prefer to make this simpler, so we just never touch MRRS at all,
> like this:
>
> @@ -5785,6 +5785,9 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> u16 v;
> int ret;
>
> + if (<loongson-quirk>)
> + return -EINVAL;
> +
> if (rq < 128 || rq > 4096 || !is_power_of_2(rq))
> return -EINVAL;
>
> What would that break? It's just harder to analyze the behavior if it
> depends on what the driver is trying to do. AFAIK, devices should
> *work* correctly with any value of MRRS.
This disables both increase and decrease MRRS, and so make
pcie_bus_config completely useless. I think allowing decrease MRRS is
useful in the PEER2PEER case.
>
> > + }
> > +
> > ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> > PCI_EXP_DEVCTL_READRQ, v);
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index dee4798a49fc..4bbdf5a5425f 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -242,37 +242,18 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > DEV_LS7A_LPC, loongson_system_bus_quirk);
> >
> > -static void loongson_mrrs_quirk(struct pci_dev *dev)
> > -{
> > - struct pci_bus *bus = dev->bus;
> > - struct pci_dev *bridge;
> > - static const struct pci_device_id bridge_devids[] = {
> > - { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> > - { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> > - { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> > - { 0, },
> > - };
> > -
> > - /* look for the matching bridge */
> > - while (!pci_is_root_bus(bus)) {
> > - bridge = bus->self;
> > - bus = bus->parent;
> > - /*
> > - * Some Loongson PCIe ports have a h/w limitation of
> > - * 256 bytes maximum read request size. They can't handle
> > - * anything larger than this. So force this limit on
> > - * any devices attached under these ports.
> > - */
> > - 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;
> > - }
> > - }
> > +static void loongson_mrrs_quirk(struct pci_dev *pdev)
> > +{
> > + /*
> > + * Some Loongson PCIe ports have h/w limitations of maximum read
> > + * request size. They can't handle anything larger than this. So
> > + * force this limit on any devices attached under these ports.
> > + */
> > + pdev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_INC_MRRS;
> > }
> > -DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
> > +DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_0, loongson_mrrs_quirk);
> > +DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_1, loongson_mrrs_quirk);
> > +DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_2, loongson_mrrs_quirk);
>
> Thanks for making this quirk Loongson-specific. Can you reverse the
> order of patches 2 and 3, so this fix happens before moving the quirk
> to drivers/pci/quirks.c?
OK, I will reverse them.
>
> > /*
> > * The Mellanox Tavor device gives false positive parity errors. Disable
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 24306504226a..b336239b5282 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -240,6 +240,7 @@ enum pci_bus_flags {
> > PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
> > PCI_BUS_FLAGS_NO_AERSID = (__force pci_bus_flags_t) 4,
> > PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 8,
> > + PCI_BUS_FLAGS_NO_INC_MRRS = (__force pci_bus_flags_t) 16,
>
> This is not a property of the *bus*.
>
> Apparently it's a property of the Root Port or maybe of the Root
> Complex itself. What about RCiePs, which don't have a Root Port above
> them? They still have an MRRS field in their Device Control
> registers. Are there restrictions on how MRRS can be set for an
> RCiEP?
>
> If you need to restrict MRRS for RCiEPs as well as for devices below
> LS7A Root Ports, I think setting a bit in struct pci_host_bridge and
> using pci_find_host_bridge() would work.
OK, I will set a bit in pci_host_bridge.
Huacai
>
> If you don't need to restrict MRRS for RCiEPs (or if there are no
> RCiEPs at all) you could put a bit in the struct pci_dev and use
> pcie_find_root_port(). But this would consume a bit in *every*
> pci_dev on every system, so it's a little more wasteful in that sense.
>
> > };
> >
> > /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
> > --
> > 2.27.0
> >
