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