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