Re: [PATCH REPOST] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.

[Bjorn Helgaas]

On Mon, May 02, 2016 at 11:53:22AM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 21, 2016 at 10:42:34AM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 21, 2016 at 10:39:52AM +0100, Krzysztof Halasa wrote:
> > > I think this bug needs to be fixed, this way or another.
> > > 
> > > The platform in question is Cavium CNS3xxx, ARMv6.
> > > 
> > > A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid
> > > potential stack overflow") converted an explicit setting of
> > > PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA
> > > read request) to:
> > > +    pcie_bus_config = PCIE_BUS_PEER2PEER;
> > 
> > Any thoughts on this, Arnd?  I can (and will) look at this, but it's
> > a complicated area and it will take me quite a while to dig into it.
> 
> What should we do here?  Should we revert 498a92d42596?  It appeared in
> v4.4, so this has been broken for a while.
> 
> It looks like 498a92d42596 merely fixed a warning, at the expense of
> breaking DMA on Cavium.  Reverting it would bring the warning back, but
> that's better than broken DMA.

I propose that we revert 498a92d42596 ("ARM: cns3xxx: pci: avoid
potential stack overflow").  I haven't had a chance to look into the
problem with it yet.

> > > with the following commentary:
> > >     "The second part is how the driver sets up the Max_Read_Request_Size
> > >     value for the first device/function on bus 1, i.e. the device
> > >     plugged directly into the PCIe root port.
> > >     For all I can tell, this is in fact incomplete, as it does not
> > >     perform the same setting on devices attached to a PCIe switch,
> > >     or multi-function devices.
> > >     The solution for this part fortunately is even easier: if we
> > >     just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER,
> > >     all PCIe devices in the system are limited to 128 byte MPS, which
> > >     in turn limits the MRRS to 128 bytes for all devices, and we
> > >     no longer even need to touch any devices."
> > > 
> > > The problem is the MRRS setting is never written to the hardware.
> > > I propose the following, though I'm not sure if we can do this safely,
> > > especially given the comments in probe.c. OTOH, this change may be
> > > required in other (all?) cases when the user requests
> > > PCIE_BUS_PEER2PEER.
> > > 
> > > On this Laguna GW-2388 the following patch fixes BM DMA with:
> > > 0000:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01)
> > > 0000:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge
> > > 0000:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers)
> > > 0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
> > > 	     the second lane from the CPU)
> > > 
> > > pci 0000:00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128
> > > pci 0000:01:00.0: Max Payload Size set to  128/ 512 (was  128), Max Read Rq  128
> > > pci 0001:00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128
> > > 
> > > Signed-off-by: Krzysztof HaÅasa <khalasa@xxxxxxx>
> > > Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 6d7ab9b..91713b6 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)
> > >  	/* In the "safe" case, do not configure the MRRS.  There appear to be
> > >  	 * issues with setting MRRS to 0 on a number of devices.
> > >  	 */
> > > -	if (pcie_bus_config != PCIE_BUS_PERFORMANCE)
> > > +	if (pcie_bus_config != PCIE_BUS_PERFORMANCE &&
> > > +	    pcie_bus_config != PCIE_BUS_PEER2PEER)
> > >  		return;
> > >  
> > >  	/* For Max performance, the MRRS must be set to the largest supported
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 2771625..6f5088a 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -756,7 +756,7 @@ enum pcie_bus_config_types {
> > >  	PCIE_BUS_DEFAULT,	/* ensure MPS matches upstream bridge */
> > >  	PCIE_BUS_SAFE,		/* use largest MPS boot-time devices support */
> > >  	PCIE_BUS_PERFORMANCE,	/* use MPS and MRRS for best performance */
> > > -	PCIE_BUS_PEER2PEER,	/* set MPS = 128 for all devices */
> > > +	PCIE_BUS_PEER2PEER,	/* set MPS and MRSS to 128 for all devices */
> > >  };
> > >  
> > >  extern enum pcie_bus_config_types pcie_bus_config;
> > > --
> > > 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
> > --
> > 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
> --
> 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