| Message ID | 20210528071503.1444680-4-chenhuacai@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Loongson-related pci quirks | expand |
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. 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. 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. > 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 >
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. > > 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 > >
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 > > >
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 > > > >
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 {
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. 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(+)