On Mon, Aug 17, 2015 at 5:50 PM, Jiang, Dave <dave.jiang@xxxxxxxxx> wrote:
> On Mon, 2015-08-17 at 17:30 -0500, Bjorn Helgaas wrote:
>> [+cc Mike, linux-rdma]
>>
>> On Wed, Jul 29, 2015 at 04:18:54PM -0600, Keith Busch wrote:
>> > From: Dave Jiang <dave.jiang@xxxxxxxxx>
>> >
>> > This is in perperation of un-exporting the pcie_set_mps() function
>> > symbol. A driver should not be changing the MPS as that is the
>> > responsibility of the PCI subsystem.
>>
>> Please explain the implications of removing this code. Does this
>> affect
>> performance of the device? If so, how do we get that performance
>> back?
>
> Honestly I don't know. But at the same time I think the driver
> shouldn't be touching the MPS at all. Shouldn't that be left to the
> PCIe subsystem and rely on the PCIe subsystem to set this to a sane
> value?
Yes, I think in principle the PCI core should own this, but I also
don't want to introduce a performance regression, so I think we need
to understand whether there's a problem, and if there is, fix it.
>> I also cc'd the QIB maintainers for you:
>>
>> QIB DRIVER
>> M: Mike Marciniszyn <infinipath@xxxxxxxxx>
>> L: linux-rdma@xxxxxxxxxxxxxxx
>> F: drivers/infiniband/hw/qib/
>>
>> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>> > ---
>> > drivers/infiniband/hw/qib/qib_pcie.c | 27 +---------------------
>> > -----
>> > 1 file changed, 1 insertion(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/infiniband/hw/qib/qib_pcie.c
>> > b/drivers/infiniband/hw/qib/qib_pcie.c
>> > index 4758a38..b8a2dcd 100644
>> > --- a/drivers/infiniband/hw/qib/qib_pcie.c
>> > +++ b/drivers/infiniband/hw/qib/qib_pcie.c
>> > @@ -557,12 +557,11 @@ static void qib_tune_pcie_coalesce(struct
>> > qib_devdata *dd)
>> > */
>> > static int qib_pcie_caps;
>> > module_param_named(pcie_caps, qib_pcie_caps, int, S_IRUGO);
>> > -MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: Payload (0..3),
>> > ReadReq (4..7)");
>> > +MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: ReadReq (4..7)");
>> >
>> > static void qib_tune_pcie_caps(struct qib_devdata *dd)
>> > {
>> > struct pci_dev *parent;
>> > - u16 rc_mpss, rc_mps, ep_mpss, ep_mps;
>> > u16 rc_mrrs, ep_mrrs, max_mrrs;
>> >
>> > /* Find out supported and configured values for parent
>> > (root) */
>> > @@ -575,30 +574,6 @@ static void qib_tune_pcie_caps(struct
>> > qib_devdata *dd)
>> > if (!pci_is_pcie(parent) || !pci_is_pcie(dd->pcidev))
>> > return;
>> >
>> > - rc_mpss = parent->pcie_mpss;
>> > - rc_mps = ffs(pcie_get_mps(parent)) - 8;
>> > - /* Find out supported and configured values for endpoint
>> > (us) */
>> > - ep_mpss = dd->pcidev->pcie_mpss;
>> > - ep_mps = ffs(pcie_get_mps(dd->pcidev)) - 8;
>> > -
>> > - /* Find max payload supported by root, endpoint */
>> > - if (rc_mpss > ep_mpss)
>> > - rc_mpss = ep_mpss;
>> > -
>> > - /* If Supported greater than limit in module param, limit
>> > it */
>> > - if (rc_mpss > (qib_pcie_caps & 7))
>> > - rc_mpss = qib_pcie_caps & 7;
>> > - /* If less than (allowed, supported), bump root payload */
>> > - if (rc_mpss > rc_mps) {
>> > - rc_mps = rc_mpss;
>> > - pcie_set_mps(parent, 128 << rc_mps);
>> > - }
>> > - /* If less than (allowed, supported), bump endpoint
>> > payload */
>> > - if (rc_mpss > ep_mps) {
>> > - ep_mps = rc_mpss;
>> > - pcie_set_mps(dd->pcidev, 128 << ep_mps);
>> > - }
>> > -
>> > /*
>> > * Now the Read Request size.
>> > * No field for max supported, but PCIe spec limits it to
>> > 4096,
--
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
