On Mon, Sep 5, 2011 at 10:48 AM, Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 2011-09-02 at 08:45 -0500, Jon Mason wrote: >> Modifying the Maximum Read Request Size to 0 (value of 128Bytes) has >> massive negative ramifications on some devices. Without knowing which >> devices have this issue, do not modify from the default value when >> walking the PCI-E bus. > > BTW. I recently ran into an MRRS related problem (on a system that > doesn't have any of our recent patches yet). > > This does have an impact however if we use the algorithm I suggested for > setting MPS, which is to allow parents to have a larger MPS than > children in order to avoid clamping an entire hierarchy when it contains > one child device that has a small MPS. > > When doing that, it is critical that the MRRS be set to be lower or > equal to the MPS of the device. Because the parent bridge (and thus the > host bridge) will potentially have an MPS larger than the requesting > function, the bridge -will- honor a large read request with a packet > potential as large as it's own MPS, and thus potentially larger that > what the function can cope with. > > This isn't a problem if the MPS are clamped to the smallest common > denominator of the entire hierarchy. > > So it depends really what algorithm has been chosen for the > configuration of the MPS. Yes, that is why I added the MRRS setting code originally. Since the MRRS setting is almost universally larger than the MPS and some devices are freaking when their MRRS is changed, we must either use the "safe" option by default and not touch the MRRS or allow the "performance" option to have this hole. I suppose a third option is to create a list of faulty devices that cannot handle MRRS modification, but that seems rather ugly. I'm open to any option, but there seems to be a large number of issues with the MRRS modification code and we need to get a fix into 3.1. Thanks, Jon > > Cheers, > Ben. > >> Tested-by: Stephen M. Cameron <scameron@xxxxxxxxxxxxxxxxxx> >> Reported-and-tested-by: Eric Dumazet <eric.dumazet@xxxxxxxxx> >> Reported-and-tested-by: Niels Ole Salscheider <niels_ole@xxxxxxxxxxxxxxxxxxxxx> >> References: https://bugzilla.kernel.org/show_bug.cgi?id=42162 >> Signed-off-by: Jon Mason <mason@xxxxxxxx> >> --- >> drivers/pci/probe.c | 36 ------------------------------------ >> 1 files changed, 0 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 8473727..d896c5e 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1394,41 +1394,6 @@ static void pcie_write_mps(struct pci_dev *dev, int mps) >> dev_err(&dev->dev, "Failed attempting to set the MPS\n"); >> } >> >> -static void pcie_write_mrrs(struct pci_dev *dev, int mps) >> -{ >> - int rc, mrrs; >> - >> - if (pcie_bus_config == PCIE_BUS_PERFORMANCE) { >> - int dev_mpss = 128 << dev->pcie_mpss; >> - >> - /* For Max performance, the MRRS must be set to the largest >> - * supported value. However, it cannot be configured larger >> - * than the MPS the device or the bus can support. This assumes >> - * that the largest MRRS available on the device cannot be >> - * smaller than the device MPSS. >> - */ >> - mrrs = mps < dev_mpss ? mps : dev_mpss; >> - } else >> - /* In the "safe" case, configure the MRRS for fairness on the >> - * bus by making all devices have the same size >> - */ >> - mrrs = mps; >> - >> - >> - /* MRRS is a R/W register. Invalid values can be written, but a >> - * subsiquent read will verify if the value is acceptable or not. >> - * If the MRRS value provided is not acceptable (e.g., too large), >> - * shrink the value until it is acceptable to the HW. >> - */ >> - while (mrrs != pcie_get_readrq(dev) && mrrs >= 128) { >> - rc = pcie_set_readrq(dev, mrrs); >> - if (rc) >> - dev_err(&dev->dev, "Failed attempting to set the MRRS\n"); >> - >> - mrrs /= 2; >> - } >> -} >> - >> static int pcie_bus_configure_set(struct pci_dev *dev, void *data) >> { >> int mps = 128 << *(u8 *)data; >> @@ -1440,7 +1405,6 @@ static int pcie_bus_configure_set(struct pci_dev *dev, void *data) >> pcie_get_mps(dev), 128<<dev->pcie_mpss, pcie_get_readrq(dev)); >> >> pcie_write_mps(dev, mps); >> - pcie_write_mrrs(dev, mps); >> >> dev_info(&dev->dev, "Dev MPS %d MPSS %d MRRS %d\n", >> pcie_get_mps(dev), 128<<dev->pcie_mpss, pcie_get_readrq(dev)); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
