| Message ID | 1438308908-12259-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Jul 30, 2015 at 7:15 PM, Guenter Roeck <linux@roeck-us.net> wrote: > The PCI subsystem always assumes that I/O is supported on PCIe bridges > and tries to assign an I/O window to each child bus even if that is not > the case. > > This may result in messages such as: > > pcieport 0000:02:00.0: res[7]=[io 0x1000-0x0fff] get_res_add_size add_size 1000 > pcieport 0000:02:00.0: BAR 7: no space for [io size 0x1000] > pcieport 0000:02:00.0: BAR 7: failed to assign [io size 0x1000] > > for each bridge port, even if a bus or its parent does not support I/O in > the first place. > > To avoid this message, check if a bus supports I/O before trying to enable > it. Also check if the root bus has an IO window assigned; if not, it does > not make sense to try to assign one to any of its child busses. > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index cefd636681b6..d9e02ba34035 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -332,6 +332,24 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom) > } > } > > +static int pci_bridge_supports_io(struct pci_dev *bridge) > +{ > + u16 io; > + > + pci_read_config_word(bridge, PCI_IO_BASE, &io); > + if (io) > + return 1; > + > + /* IO_BASE/LIMIT is either hard-wired to zero or programmed to zero */ > + pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); > + pci_read_config_word(bridge, PCI_IO_BASE, &io); > + pci_write_config_word(bridge, PCI_IO_BASE, 0x0); > + if (io) > + return 1; > + > + return 0; > +} > + > static void pci_read_bridge_io(struct pci_bus *child) > { > struct pci_dev *dev = child->self; > @@ -340,6 +358,15 @@ static void pci_read_bridge_io(struct pci_bus *child) > struct pci_bus_region region; > struct resource *res; > > + if (!(child->bus_flags & PCI_BUS_FLAGS_SUPPORTS_IO)) > + return; > + > + if (!pci_bridge_supports_io(dev)) { > + dev_printk(KERN_DEBUG, &dev->dev, " no I/O window\n"); > + child->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO; > + return; > + } > + It only can avoid warning with bridge, and still have warning on devices under the bridge. also would have problem on transparent bridges, like BRIDGE_A ---- BRIDGE_AA----DEVICE_AA | \-- BRIDGE_AB ---DEVICE_AB if only BRIDGE_A and BRIDGE_AA are transparent, and BRIDGE_AB is not. and if pci_bridge_supports_io() return false for BRIDGE_A. We will have all three bridges have PCI_BUS_FLAGS_SUPPORTS_IO cleared. so all will not been sized and will not get assigned io port resource. later DEVICE_AA could root bus io port as parent, and get io resource assigned. but DEVICE_AB will not get assigned. so we should get rid of pci_bridge_supports_io(), and just check root resource IO port. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/05/2015 06:14 PM, Yinghai Lu wrote: > On Thu, Jul 30, 2015 at 7:15 PM, Guenter Roeck <linux@roeck-us.net> wrote: >> The PCI subsystem always assumes that I/O is supported on PCIe bridges >> and tries to assign an I/O window to each child bus even if that is not >> the case. >> >> This may result in messages such as: >> >> pcieport 0000:02:00.0: res[7]=[io 0x1000-0x0fff] get_res_add_size add_size 1000 >> pcieport 0000:02:00.0: BAR 7: no space for [io size 0x1000] >> pcieport 0000:02:00.0: BAR 7: failed to assign [io size 0x1000] >> >> for each bridge port, even if a bus or its parent does not support I/O in >> the first place. >> >> To avoid this message, check if a bus supports I/O before trying to enable >> it. Also check if the root bus has an IO window assigned; if not, it does >> not make sense to try to assign one to any of its child busses. >> >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index cefd636681b6..d9e02ba34035 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -332,6 +332,24 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom) >> } >> } >> >> +static int pci_bridge_supports_io(struct pci_dev *bridge) >> +{ >> + u16 io; >> + >> + pci_read_config_word(bridge, PCI_IO_BASE, &io); >> + if (io) >> + return 1; >> + >> + /* IO_BASE/LIMIT is either hard-wired to zero or programmed to zero */ >> + pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); >> + pci_read_config_word(bridge, PCI_IO_BASE, &io); >> + pci_write_config_word(bridge, PCI_IO_BASE, 0x0); >> + if (io) >> + return 1; >> + >> + return 0; >> +} >> + >> static void pci_read_bridge_io(struct pci_bus *child) >> { >> struct pci_dev *dev = child->self; >> @@ -340,6 +358,15 @@ static void pci_read_bridge_io(struct pci_bus *child) >> struct pci_bus_region region; >> struct resource *res; >> >> + if (!(child->bus_flags & PCI_BUS_FLAGS_SUPPORTS_IO)) >> + return; >> + >> + if (!pci_bridge_supports_io(dev)) { >> + dev_printk(KERN_DEBUG, &dev->dev, " no I/O window\n"); >> + child->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO; >> + return; >> + } >> + > > It only can avoid warning with bridge, and still have warning on > devices under the bridge. > Not really, because children inherit bus_flags. > also would have problem on transparent bridges, like > > BRIDGE_A ---- BRIDGE_AA----DEVICE_AA > | > \-- BRIDGE_AB ---DEVICE_AB > > if only BRIDGE_A and BRIDGE_AA are transparent, and BRIDGE_AB is not. > > and if pci_bridge_supports_io() return false for BRIDGE_A. > > We will have all three bridges have PCI_BUS_FLAGS_SUPPORTS_IO cleared. > so all will not been sized and will not get assigned io port resource. > What is wrong with that ? Doesn't it reflect reality ? > later DEVICE_AA could root bus io port as parent, and get io resource assigned. > but DEVICE_AB will not get assigned. > > so we should get rid of pci_bridge_supports_io(), and just check root > resource IO port. > You lost me here. That would mean that we would not clear the flag and claim that a bridge supports IO even if it doesn't, and all ports and bridges connected to that bridge would try (and fail) to get IO address assignments. That does not make much sense to me. What do you expect to do in this case ? I must admit I have no idea how non-transparent bridges fit into this picture. However, if Bridge A doesn't support IO, I don't see how we can get IO anywhere, transparent or not. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cefd636681b6..d9e02ba34035 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -332,6 +332,24 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom) } } +static int pci_bridge_supports_io(struct pci_dev *bridge) +{ + u16 io; + + pci_read_config_word(bridge, PCI_IO_BASE, &io); + if (io) + return 1; + + /* IO_BASE/LIMIT is either hard-wired to zero or programmed to zero */ + pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); + pci_read_config_word(bridge, PCI_IO_BASE, &io); + pci_write_config_word(bridge, PCI_IO_BASE, 0x0); + if (io) + return 1; + + return 0; +} + static void pci_read_bridge_io(struct pci_bus *child) { struct pci_dev *dev = child->self; @@ -340,6 +358,15 @@ static void pci_read_bridge_io(struct pci_bus *child) struct pci_bus_region region; struct resource *res; + if (!(child->bus_flags & PCI_BUS_FLAGS_SUPPORTS_IO)) + return; + + if (!pci_bridge_supports_io(dev)) { + dev_printk(KERN_DEBUG, &dev->dev, " no I/O window\n"); + child->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO; + return; + } + io_mask = PCI_IO_RANGE_MASK; io_granularity = 0x1000; if (dev->io_window_1k) { @@ -496,6 +523,7 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus *parent) INIT_LIST_HEAD(&b->resources); b->max_bus_speed = PCI_SPEED_UNKNOWN; b->cur_bus_speed = PCI_SPEED_UNKNOWN; + b->bus_flags = PCI_BUS_FLAGS_SUPPORTS_IO; #ifdef CONFIG_PCI_DOMAINS_GENERIC if (parent) b->domain_nr = parent->domain_nr; @@ -1938,6 +1966,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, resource_size_t offset; char bus_addr[64]; char *fmt; + bool has_io = false; b = pci_alloc_bus(NULL); if (!b) @@ -2016,8 +2045,13 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, } else bus_addr[0] = '\0'; dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr); + if (resource_type(res) == IORESOURCE_IO) + has_io = true; } + if (!has_io) + b->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO; + down_write(&pci_bus_sem); list_add_tail(&b->node, &pci_root_buses); up_write(&pci_bus_sem); diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 508cc56130e3..c3fdace30faf 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -744,7 +744,6 @@ int pci_claim_bridge_resource(struct pci_dev *bridge, int i) base/limit registers must be read-only and read as 0. */ static void pci_bridge_check_ranges(struct pci_bus *bus) { - u16 io; u32 pmem; struct pci_dev *bridge = bus->self; struct resource *b_res; @@ -752,13 +751,7 @@ static void pci_bridge_check_ranges(struct pci_bus *bus) b_res = &bridge->resource[PCI_BRIDGE_RESOURCES]; b_res[1].flags |= IORESOURCE_MEM; - pci_read_config_word(bridge, PCI_IO_BASE, &io); - if (!io) { - pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0); - pci_read_config_word(bridge, PCI_IO_BASE, &io); - pci_write_config_word(bridge, PCI_IO_BASE, 0x0); - } - if (io) + if (bus->bus_flags & PCI_BUS_FLAGS_SUPPORTS_IO) b_res[0].flags |= IORESOURCE_IO; /* DECchip 21050 pass 2 errata: the bridge may miss an address diff --git a/include/linux/pci.h b/include/linux/pci.h index 8a0321a8fb59..3bbabf344bfc 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -191,6 +191,7 @@ typedef unsigned short __bitwise pci_bus_flags_t; enum pci_bus_flags { PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1, PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2, + PCI_BUS_FLAGS_SUPPORTS_IO = (__force pci_bus_flags_t) 4, }; /* These values come from the PCI Express Spec */
The PCI subsystem always assumes that I/O is supported on PCIe bridges and tries to assign an I/O window to each child bus even if that is not the case. This may result in messages such as: pcieport 0000:02:00.0: res[7]=[io 0x1000-0x0fff] get_res_add_size add_size 1000 pcieport 0000:02:00.0: BAR 7: no space for [io size 0x1000] pcieport 0000:02:00.0: BAR 7: failed to assign [io size 0x1000] for each bridge port, even if a bus or its parent does not support I/O in the first place. To avoid this message, check if a bus supports I/O before trying to enable it. Also check if the root bus has an IO window assigned; if not, it does not make sense to try to assign one to any of its child busses. Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Yinghai Lu <yinghai@kernel.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v3: Reverse order of new flag, and name it PCI_BUS_FLAGS_SUPPORTS_IO instead of PCI_BUS_FLAGS_NO_IO. Don't use bool in pci_bridge_supports_io. Drop pci_root_has_io_resource(). Instead, determine if the root bus has an io window in pci_create_root_bus(), and clear PCI_BUS_FLAGS_SUPPORTS_IO in its bus flags if it doesn't. v2: Use a new bus flag to indicate if IO is supported on a bus or not. Using IORESOURCE_DISABLED in resource flags turned out to be futile, since the term "!res->flags" is widely used to detect if a resource window is enabled or not, and setting IORESOURCE_DISABLED would affect all this code. This patch depends on 'PCI: move pci_read_bridge_bases to the generic PCI layer' by Lorenzo Pieralisi; without it, pci_read_bridge_io() is not always called. --- drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++++++++ drivers/pci/setup-bus.c | 9 +-------- include/linux/pci.h | 1 + 3 files changed, 36 insertions(+), 8 deletions(-)