On Tue, Jun 29, 2021 at 11:20:45AM +0800, Huacai Chen wrote:
> 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.
True. The MPS/MRRS algorithm, including pcie_bus_config, is very
complicated and poorly understood, so I'm a little hesitant to
complicate it even more with one-off quirks. I'd really prefer to
push LS7A off into a corner and have the generic MPS/MRRS algorithm
ignore it completely.
But I guess just keeping pcie_set_readrq() from *increasing* MRRS
doesn't add too much complication and should always be safe.
Bjorn
