Re: [PATCH] PCI: mvebu: Handle changes to the bridge windows while enabled

[Bjorn Helgaas]

On Wed, Jan 11, 2017 at 12:30:55PM -0600, Bjorn Helgaas wrote:
> On Mon, Dec 12, 2016 at 11:30:20AM -0700, Jason Gunthorpe wrote:
> > The PCI core will write to the bridge window config multiple times
> > while they are enabled. This can lead to mbus failures like:
> > 
> >  mvebu_mbus: cannot add window '4:e8', conflicts with another window
> >  mvebu-pcie mbus:pex@e0000000: Could not create MBus window at [mem 0xe0000000-0xe00fffff]: -22
> > 
> > For me this is happening during a hotplug cycle. The PCI core is
> > not changing the values, just writing them twice while active.
> > 
> > The patch addresses the general case of any change to an active window,
> > but not atomically. The code is slightly refactored so io and mem
> > can share more of the window logic.
> 
> Looks good to me, but I'm waiting for an ack from Thomas or Jason (listed
> as maintainers and already cc'd).

Ping, Thomas, Jason C?

> > Signed-off-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/pci/host/pci-mvebu.c | 101 +++++++++++++++++++++++++------------------
> >  1 file changed, 60 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> > index 307f81d6b479af..af724731b22f53 100644
> > --- a/drivers/pci/host/pci-mvebu.c
> > +++ b/drivers/pci/host/pci-mvebu.c
> > @@ -133,6 +133,12 @@ struct mvebu_pcie {
> >  	int nports;
> >  };
> >  
> > +struct mvebu_pcie_window {
> > +	phys_addr_t base;
> > +	phys_addr_t remap;
> > +	size_t size;
> > +};
> > +
> >  /* Structure representing one PCIe interface */
> >  struct mvebu_pcie_port {
> >  	char *name;
> > @@ -150,10 +156,8 @@ struct mvebu_pcie_port {
> >  	struct mvebu_sw_pci_bridge bridge;
> >  	struct device_node *dn;
> >  	struct mvebu_pcie *pcie;
> > -	phys_addr_t memwin_base;
> > -	size_t memwin_size;
> > -	phys_addr_t iowin_base;
> > -	size_t iowin_size;
> > +	struct mvebu_pcie_window memwin;
> > +	struct mvebu_pcie_window iowin;
> >  	u32 saved_pcie_stat;
> >  };
> >  
> > @@ -379,23 +383,45 @@ static void mvebu_pcie_add_windows(struct mvebu_pcie_port *port,
> >  	}
> >  }
> >  
> > +static void mvebu_pcie_set_window(struct mvebu_pcie_port *port,
> > +				  unsigned int target, unsigned int attribute,
> > +				  const struct mvebu_pcie_window *desired,
> > +				  struct mvebu_pcie_window *cur)
> > +{
> > +	if (desired->base == cur->base && desired->remap == cur->remap &&
> > +	    desired->size == cur->size)
> > +		return;
> > +
> > +	if (cur->size != 0) {
> > +		mvebu_pcie_del_windows(port, cur->base, cur->size);
> > +		cur->size = 0;
> > +		cur->base = 0;
> > +
> > +		/*
> > +		 * If something tries to change the window while it is enabled
> > +		 * the change will not be done atomically. That would be
> > +		 * difficult to do in the general case.
> > +		 */
> > +	}
> > +
> > +	if (desired->size == 0)
> > +		return;
> > +
> > +	mvebu_pcie_add_windows(port, target, attribute, desired->base,
> > +			       desired->size, desired->remap);
> > +	*cur = *desired;
> > +}
> > +
> >  static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
> >  {
> > -	phys_addr_t iobase;
> > +	struct mvebu_pcie_window desired = {};
> >  
> >  	/* Are the new iobase/iolimit values invalid? */
> >  	if (port->bridge.iolimit < port->bridge.iobase ||
> >  	    port->bridge.iolimitupper < port->bridge.iobaseupper ||
> >  	    !(port->bridge.command & PCI_COMMAND_IO)) {
> > -
> > -		/* If a window was configured, remove it */
> > -		if (port->iowin_base) {
> > -			mvebu_pcie_del_windows(port, port->iowin_base,
> > -					       port->iowin_size);
> > -			port->iowin_base = 0;
> > -			port->iowin_size = 0;
> > -		}
> > -
> > +		mvebu_pcie_set_window(port, port->io_target, port->io_attr,
> > +				      &desired, &port->iowin);
> >  		return;
> >  	}
> >  
> > @@ -412,32 +438,27 @@ static void mvebu_pcie_handle_iobase_change(struct mvebu_pcie_port *port)
> >  	 * specifications. iobase is the bus address, port->iowin_base
> >  	 * is the CPU address.
> >  	 */
> > -	iobase = ((port->bridge.iobase & 0xF0) << 8) |
> > -		(port->bridge.iobaseupper << 16);
> > -	port->iowin_base = port->pcie->io.start + iobase;
> > -	port->iowin_size = ((0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
> > -			    (port->bridge.iolimitupper << 16)) -
> > -			    iobase) + 1;
> > -
> > -	mvebu_pcie_add_windows(port, port->io_target, port->io_attr,
> > -			       port->iowin_base, port->iowin_size,
> > -			       iobase);
> > +	desired.remap = ((port->bridge.iobase & 0xF0) << 8) |
> > +			(port->bridge.iobaseupper << 16);
> > +	desired.base = port->pcie->io.start + desired.remap;
> > +	desired.size = ((0xFFF | ((port->bridge.iolimit & 0xF0) << 8) |
> > +			 (port->bridge.iolimitupper << 16)) -
> > +			desired.remap) +
> > +		       1;
> > +
> > +	mvebu_pcie_set_window(port, port->io_target, port->io_attr, &desired,
> > +			      &port->iowin);
> >  }
> >  
> >  static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
> >  {
> > +	struct mvebu_pcie_window desired = {.remap = MVEBU_MBUS_NO_REMAP};
> > +
> >  	/* Are the new membase/memlimit values invalid? */
> >  	if (port->bridge.memlimit < port->bridge.membase ||
> >  	    !(port->bridge.command & PCI_COMMAND_MEMORY)) {
> > -
> > -		/* If a window was configured, remove it */
> > -		if (port->memwin_base) {
> > -			mvebu_pcie_del_windows(port, port->memwin_base,
> > -					       port->memwin_size);
> > -			port->memwin_base = 0;
> > -			port->memwin_size = 0;
> > -		}
> > -
> > +		mvebu_pcie_set_window(port, port->mem_target, port->mem_attr,
> > +				      &desired, &port->memwin);
> >  		return;
> >  	}
> >  
> > @@ -447,14 +468,12 @@ static void mvebu_pcie_handle_membase_change(struct mvebu_pcie_port *port)
> >  	 * window to setup, according to the PCI-to-PCI bridge
> >  	 * specifications.
> >  	 */
> > -	port->memwin_base  = ((port->bridge.membase & 0xFFF0) << 16);
> > -	port->memwin_size  =
> > -		(((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
> > -		port->memwin_base + 1;
> > -
> > -	mvebu_pcie_add_windows(port, port->mem_target, port->mem_attr,
> > -			       port->memwin_base, port->memwin_size,
> > -			       MVEBU_MBUS_NO_REMAP);
> > +	desired.base = ((port->bridge.membase & 0xFFF0) << 16);
> > +	desired.size = (((port->bridge.memlimit & 0xFFF0) << 16) | 0xFFFFF) -
> > +		       desired.base + 1;
> > +
> > +	mvebu_pcie_set_window(port, port->mem_target, port->mem_attr, &desired,
> > +			      &port->memwin);
> >  }
> >  
> >  /*
> > -- 
> > 2.7.4
> > --
> > 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
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel