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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

> > 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
> > >