Re: [PATCH V2 3/4] PCI: Improve the MRRS quirk for LS7A

[Huacai Chen <chenhuacai@gmail.com>]

Hi, Bjorn,

On Sun, Jun 6, 2021 at 5:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 04, 2021 at 05:43:36PM +0800, Huacai Chen wrote:
> > Hi, Bjorn,
> >
> > On Sat, May 29, 2021 at 4:32 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Fri, May 28, 2021 at 03:15:02PM +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(). So the only possible way is configure
> > > > MRRS of all devices in BIOS, and add a PCI device flag (PCI_DEV_FLAGS_
> > > > NO_INCREASE_MRRS) to stop the increasing MRRS operations.
> > >
> > > It's still not clear what the problem is.
> > >
> > > As far as I can tell from the PCIe spec, it is legal for an OS to
> > > program any value for MRRS, and it is legal for an endpoint to
> > > generate a Read Request with any size up to its MRRS.  If you
> > > disagree, please cite the relevant section in the spec.
> > >
> > > There is no requirement for the OS to limit the MRRS based on a
> > > restriction elsewhere in the system.  There is no mechanism for the OS
> > > to even discover such a restriction.
> > >
> > > Of course, there is also no requirement that the PCIe Completions
> > > carrying the read data be the same size as the MRRS.  If the non-PCIe
> > > part of the system has a restriction on read burst size, that part of
> > > the system can certainly break up the read and respond with several
> > > PCIe Completions.
> > >
> > > If LS7A can't break up read requests, that sounds like a problem in
> > > the LS7A design.  We should have a description of this erratum.  And
> > > we should have some statement about this being fixed in future
> > > designs, because we don't want to have to update the fixup with the
> > > PCI vendor/device IDs every time new versions come out.
> >
> > Thanks for your information, but I think only Mr. Shuai Huang can
> > explain the root cause, too.
> >
> > > I also don't want to rely on some value left in MRRS by BIOS.  If
> > > certain bridges have specific limits on what MRRS can be, the fixup
> > > should have those limits in it.
> >
> > As I know, each port of LS7A has a different maximum MRRS value (Yes,
> > as you said, this is unreasonable in PCIe spec. but it is a fact in
> > LS7A), and also different between hardware revisions. So, the kernel
> > cannot configure it, and relying on BIOS is the only way.
>
> Maybe we should just set MRRS to the minimum (128 bytes) for
> everything on this platform.
>
> The generic MPS/MRRS config is messy enough already, and I'm hesitant
> to add much complication for what seems to be a fairly broken PCIe
> controller.
I have had an offline discussion with Mr. Shuai Huang, and he told me
that the root cause is LS7A doesn't break up large read requests (Yes,
that is a problem in the LS7A design). But I think set MRRS to 128
bytes in the quirk is not enough, because as I said before, some
devices (e.g. Realtek 8169) set a big value in its driver. We cannot
block such operations if we don't touch the PCIe core.

Huacai
>
> > > loongson_mrrs_quirk() doesn't look efficient.  We should not have to
> > > run the fixup for *every* PCI device in the system.  Also, we should
> > > not mark every *device* below an LS7A, because it's not the devices
> > > that are defective.
> > >
> > > If it's the root port or the host bridge that's defective, we should
> > > mark *that*, e.g., something along the lines of how quirk_no_ext_tags()
> > > works.
> > OK, I'll improve my code.
> >
> > Huacai
> > >
> > > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > ---
> > > >  drivers/pci/pci.c    | 5 +++++
> > > >  drivers/pci/quirks.c | 6 ++++++
> > > >  include/linux/pci.h  | 2 ++
> > > >  3 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index b717680377a9..6f0d2f5b6f30 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -5802,6 +5802,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> > > >
> > > >       v = (ffs(rq) - 8) << 12;
> > > >
> > > > +     if (dev->dev_flags & PCI_DEV_FLAGS_NO_INCREASE_MRRS) {
> > > > +             if (rq > pcie_get_readrq(dev))
> > > > +                     return -EINVAL;
> > > > +     }
> > > > +
> > > >       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 66e4bea69431..10b3b2057940 100644
> > > > --- a/drivers/pci/quirks.c
> > > > +++ b/drivers/pci/quirks.c
> > > > @@ -264,6 +264,12 @@ static void loongson_mrrs_quirk(struct pci_dev *dev)
> > > >                * any devices attached under these ports.
> > > >                */
> > > >               if (pci_match_id(bridge_devids, bridge)) {
> > > > +                     dev->dev_flags |= PCI_DEV_FLAGS_NO_INCREASE_MRRS;
> > > > +
> > > > +                     if (pcie_bus_config == PCIE_BUS_DEFAULT ||
> > > > +                         pcie_bus_config == PCIE_BUS_TUNE_OFF)
> > > > +                             break;
> > > > +
> > > >                       if (pcie_get_readrq(dev) > 256) {
> > > >                               pci_info(dev, "limiting MRRS to 256\n");
> > > >                               pcie_set_readrq(dev, 256);
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index c20211e59a57..7fb2072a83b8 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -227,6 +227,8 @@ enum pci_dev_flags {
> > > >       PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
> > > >       /* Don't use Relaxed Ordering for TLPs directed at this device */
> > > >       PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
> > > > +     /* Don't increase BIOS's MRRS configuration */
> > > > +     PCI_DEV_FLAGS_NO_INCREASE_MRRS = (__force pci_dev_flags_t) (1 << 12),
> > > >  };
> > > >
> > > >  enum pci_irq_reroute_variant {
> > > > --
> > > > 2.27.0
> > > >