Hi, Bjorn,
On Sun, Jun 6, 2021 at 5:34 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@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.
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@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
> > > >
