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@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > > ---
> > > 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
> > >
