[RFC PATCH v2] PCI: Only enable IO window if supported

[RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

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>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
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.

With this version of the patch, pci_bridge_check_ranges() still sets
IORESOURCE_IO. Moving this into pci_read_bridge_io() had undesirable
side effects and resulted in missing IO window assignments on one of
the x86 platforms I tested with. I'll have to explore this further.

 drivers/pci/probe.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-bus.c |  9 +--------
 include/linux/pci.h     |  1 +
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636681b6..b21cba7aeb79 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -332,6 +332,35 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 	}
 }
 
+static bool pci_bus_supports_io(struct pci_bus *bus)
+{
+	struct pci_dev *dev = bus->self;
+	u16 io;
+
+	pci_read_config_word(dev, PCI_IO_BASE, &io);
+	if (!io) {
+		pci_write_config_word(dev, PCI_IO_BASE, 0xe0f0);
+		pci_read_config_word(dev, PCI_IO_BASE, &io);
+		pci_write_config_word(dev, PCI_IO_BASE, 0x0);
+	}
+	return !!io;
+}
+
+static bool pci_root_has_io_resource(struct pci_bus *bus)
+{
+	struct resource *res;
+	int i;
+
+	while (bus->parent)
+		bus = bus->parent;
+
+	 pci_bus_for_each_resource(bus, res, i) {
+		if (res && (res->flags & IORESOURCE_IO))
+			return true;
+	 }
+	 return false;
+}
+
 static void pci_read_bridge_io(struct pci_bus *child)
 {
 	struct pci_dev *dev = child->self;
@@ -340,6 +369,23 @@ 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_NO_IO)
+		return;
+
+	if (!pci_bus_supports_io(child)) {
+		dev_printk(KERN_DEBUG, &dev->dev,
+			   "  bus does not support IO\n");
+		child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
+		return;
+	}
+
+	if (!pci_root_has_io_resource(child)) {
+		dev_printk(KERN_DEBUG, &dev->dev,
+			   "  no IO window on root bus\n");
+		child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
+		return;
+	}
+
 	io_mask = PCI_IO_RANGE_MASK;
 	io_granularity = 0x1000;
 	if (dev->io_window_1k) {
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 508cc56130e3..c8c7eecadbfe 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_NO_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..b910ed04aa0b 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_NO_IO    = (__force pci_bus_flags_t) 4,
 };
 
 /* These values come from the PCI Express Spec */
-- 
2.1.0

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

On 07/07/2015 11:11 AM, Guenter Roeck 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.
>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 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.
>
> With this version of the patch, pci_bridge_check_ranges() still sets
> IORESOURCE_IO. Moving this into pci_read_bridge_io() had undesirable
> side effects and resulted in missing IO window assignments on one of
> the x86 platforms I tested with. I'll have to explore this further.
>

Hi Bjorn,

any comments on this patch ? Is the new bus flag acceptable ?

Thanks,
Guenter

>   drivers/pci/probe.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/pci/setup-bus.c |  9 +--------
>   include/linux/pci.h     |  1 +
>   3 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636681b6..b21cba7aeb79 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -332,6 +332,35 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
>   	}
>   }
>
> +static bool pci_bus_supports_io(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev = bus->self;
> +	u16 io;
> +
> +	pci_read_config_word(dev, PCI_IO_BASE, &io);
> +	if (!io) {
> +		pci_write_config_word(dev, PCI_IO_BASE, 0xe0f0);
> +		pci_read_config_word(dev, PCI_IO_BASE, &io);
> +		pci_write_config_word(dev, PCI_IO_BASE, 0x0);
> +	}
> +	return !!io;
> +}
> +
> +static bool pci_root_has_io_resource(struct pci_bus *bus)
> +{
> +	struct resource *res;
> +	int i;
> +
> +	while (bus->parent)
> +		bus = bus->parent;
> +
> +	 pci_bus_for_each_resource(bus, res, i) {
> +		if (res && (res->flags & IORESOURCE_IO))
> +			return true;
> +	 }
> +	 return false;
> +}
> +
>   static void pci_read_bridge_io(struct pci_bus *child)
>   {
>   	struct pci_dev *dev = child->self;
> @@ -340,6 +369,23 @@ 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_NO_IO)
> +		return;
> +
> +	if (!pci_bus_supports_io(child)) {
> +		dev_printk(KERN_DEBUG, &dev->dev,
> +			   "  bus does not support IO\n");
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
> +		return;
> +	}
> +
> +	if (!pci_root_has_io_resource(child)) {
> +		dev_printk(KERN_DEBUG, &dev->dev,
> +			   "  no IO window on root bus\n");
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
> +		return;
> +	}
> +
>   	io_mask = PCI_IO_RANGE_MASK;
>   	io_granularity = 0x1000;
>   	if (dev->io_window_1k) {
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 508cc56130e3..c8c7eecadbfe 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_NO_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..b910ed04aa0b 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_NO_IO    = (__force pci_bus_flags_t) 4,
>   };
>
>   /* These values come from the PCI Express Spec */
>

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Bjorn Helgaas]

On Tue, Jul 07, 2015 at 11:11:20AM -0700, Guenter Roeck 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.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Applied to pci/resource for v4.3, thanks!

> ---
> 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.
> 
> With this version of the patch, pci_bridge_check_ranges() still sets
> IORESOURCE_IO. Moving this into pci_read_bridge_io() had undesirable
> side effects and resulted in missing IO window assignments on one of
> the x86 platforms I tested with. I'll have to explore this further.
> 
>  drivers/pci/probe.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/setup-bus.c |  9 +--------
>  include/linux/pci.h     |  1 +
>  3 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636681b6..b21cba7aeb79 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -332,6 +332,35 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
>  	}
>  }
>  
> +static bool pci_bus_supports_io(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev = bus->self;
> +	u16 io;
> +
> +	pci_read_config_word(dev, PCI_IO_BASE, &io);
> +	if (!io) {
> +		pci_write_config_word(dev, PCI_IO_BASE, 0xe0f0);
> +		pci_read_config_word(dev, PCI_IO_BASE, &io);
> +		pci_write_config_word(dev, PCI_IO_BASE, 0x0);
> +	}
> +	return !!io;
> +}
> +
> +static bool pci_root_has_io_resource(struct pci_bus *bus)
> +{
> +	struct resource *res;
> +	int i;
> +
> +	while (bus->parent)
> +		bus = bus->parent;
> +
> +	 pci_bus_for_each_resource(bus, res, i) {
> +		if (res && (res->flags & IORESOURCE_IO))
> +			return true;
> +	 }
> +	 return false;
> +}
> +
>  static void pci_read_bridge_io(struct pci_bus *child)
>  {
>  	struct pci_dev *dev = child->self;
> @@ -340,6 +369,23 @@ 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_NO_IO)
> +		return;
> +
> +	if (!pci_bus_supports_io(child)) {
> +		dev_printk(KERN_DEBUG, &dev->dev,
> +			   "  bus does not support IO\n");
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
> +		return;
> +	}
> +
> +	if (!pci_root_has_io_resource(child)) {
> +		dev_printk(KERN_DEBUG, &dev->dev,
> +			   "  no IO window on root bus\n");
> +		child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
> +		return;
> +	}
> +
>  	io_mask = PCI_IO_RANGE_MASK;
>  	io_granularity = 0x1000;
>  	if (dev->io_window_1k) {
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 508cc56130e3..c8c7eecadbfe 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_NO_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..b910ed04aa0b 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_NO_IO    = (__force pci_bus_flags_t) 4,
>  };
>  
>  /* These values come from the PCI Express Spec */
> -- 
> 2.1.0
> 

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Yinghai Lu]

On Wed, Jul 29, 2015 at 9:09 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Jul 07, 2015 at 11:11:20AM -0700, Guenter Roeck 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]
>>
>
> Applied to pci/resource for v4.3, thanks!
>> @@ -340,6 +369,23 @@ 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_NO_IO)
>> +             return;
>> +
>> +     if (!pci_bus_supports_io(child)) {
>> +             dev_printk(KERN_DEBUG, &dev->dev,
>> +                        "  bus does not support IO\n");
>> +             child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
>> +             return;
>> +     }
>> +
>> +     if (!pci_root_has_io_resource(child)) {
>> +             dev_printk(KERN_DEBUG, &dev->dev,
>> +                        "  no IO window on root bus\n");
>> +             child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
>> +             return;
>> +     }
>> +
>>       io_mask = PCI_IO_RANGE_MASK;
>>       io_granularity = 0x1000;
>>       if (dev->io_window_1k) {

Hi Bjorn,

Looks like you flip the PCI_BUS_FLAGS_NO_IO to
PCI_BUS_FLAGS_SUPPORT_IO, and now have

@@ -340,6 +373,21 @@ 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");
+               return;
+       }
+
+       if (!pci_root_has_io_resource(child)) {
+               dev_printk(KERN_DEBUG, &dev->dev, "  no I/O resource
on root bus\n");
+               return;
+       }
+
+       child->bus_flags |= PCI_BUS_FLAGS_SUPPORTS_IO;
+

so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.

Yinghai

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

On 07/29/2015 12:30 PM, Yinghai Lu wrote:
> On Wed, Jul 29, 2015 at 9:09 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Tue, Jul 07, 2015 at 11:11:20AM -0700, Guenter Roeck 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]
>>>
>>
>> Applied to pci/resource for v4.3, thanks!
>>> @@ -340,6 +369,23 @@ 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_NO_IO)
>>> +             return;
>>> +
>>> +     if (!pci_bus_supports_io(child)) {
>>> +             dev_printk(KERN_DEBUG, &dev->dev,
>>> +                        "  bus does not support IO\n");
>>> +             child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
>>> +             return;
>>> +     }
>>> +
>>> +     if (!pci_root_has_io_resource(child)) {
>>> +             dev_printk(KERN_DEBUG, &dev->dev,
>>> +                        "  no IO window on root bus\n");
>>> +             child->bus_flags |= PCI_BUS_FLAGS_NO_IO;
>>> +             return;
>>> +     }
>>> +
>>>        io_mask = PCI_IO_RANGE_MASK;
>>>        io_granularity = 0x1000;
>>>        if (dev->io_window_1k) {
>
> Hi Bjorn,
>
> Looks like you flip the PCI_BUS_FLAGS_NO_IO to
> PCI_BUS_FLAGS_SUPPORT_IO, and now have
>
> @@ -340,6 +373,21 @@ 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");
> +               return;
> +       }
> +
> +       if (!pci_root_has_io_resource(child)) {
> +               dev_printk(KERN_DEBUG, &dev->dev, "  no I/O resource
> on root bus\n");
> +               return;
> +       }
> +
> +       child->bus_flags |= PCI_BUS_FLAGS_SUPPORTS_IO;
> +
>
> so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.
>

Hi Yinghai,

excellent catch. Unfortunately, I don't know how to make it
work with the reversed flag. The idea here was that the flag
propagates from parent to child. This makes sense for an
"it doesn't work" flag to be inherited from the child,
but not for an "it works" flag.

Guenter

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Yinghai Lu]

On Wed, Jul 29, 2015 at 12:46 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/29/2015 12:30 PM, Yinghai Lu wrote:
>
>>
>> so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.
>>
>
> excellent catch. Unfortunately, I don't know how to make it
> work with the reversed flag. The idea here was that the flag
> propagates from parent to child. This makes sense for an
> "it doesn't work" flag to be inherited from the child,
> but not for an "it works" flag.
>

also would be better if we can add has_ioport in hostbridge instead.
like has_mem64 in https://patchwork.ozlabs.org/patch/500926/
and use

to_pci_host_bridge(bus->bridge)->has_ioport

to replace pci_root_has_io_resource()

Thanks

Yinghai

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

On 07/29/2015 12:53 PM, Yinghai Lu wrote:
> On Wed, Jul 29, 2015 at 12:46 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 07/29/2015 12:30 PM, Yinghai Lu wrote:
>>
>>>
>>> so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.
>>>
>>
>> excellent catch. Unfortunately, I don't know how to make it
>> work with the reversed flag. The idea here was that the flag
>> propagates from parent to child. This makes sense for an
>> "it doesn't work" flag to be inherited from the child,
>> but not for an "it works" flag.
>>
>
> also would be better if we can add has_ioport in hostbridge instead.
> like has_mem64 in https://patchwork.ozlabs.org/patch/500926/
> and use
>
> to_pci_host_bridge(bus->bridge)->has_ioport
>
> to replace pci_root_has_io_resource()
>

Sure, that would make sense.

Bjorn, how do you want to handle the flag problem ?
Do you have an idea on how to make it work with
the reversed definition ?

Thanks,
Guenter

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On Wed, Jul 29, 2015 at 1:02 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> Do you have an idea on how to make it work with
> the reversed definition ?

attached one should address the problem.

index ddda0916..e8def25 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -378,16 +378,16 @@ static void pci_read_bridge_io(struct pci_bus *child)

     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;
     }

     if (!pci_root_has_io_resource(child)) {
         dev_printk(KERN_DEBUG, &dev->dev, "  no I/O resource on root bus\n");
+        child->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO;
         return;
     }

-    child->bus_flags |= PCI_BUS_FLAGS_SUPPORTS_IO;
-
     io_mask = PCI_IO_RANGE_MASK;
     io_granularity = 0x1000;
     if (dev->io_window_1k) {
@@ -544,6 +544,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;

[-- Attachment #2: supports_io.patch --]
[-- Type: text/x-patch, Size: 1025 bytes --]

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ddda0916..e8def25 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -378,16 +378,16 @@ static void pci_read_bridge_io(struct pci_bus *child)
 
 	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;
 	}
 
 	if (!pci_root_has_io_resource(child)) {
 		dev_printk(KERN_DEBUG, &dev->dev, "  no I/O resource on root bus\n");
+		child->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO;
 		return;
 	}
 
-	child->bus_flags |= PCI_BUS_FLAGS_SUPPORTS_IO;
-
 	io_mask = PCI_IO_RANGE_MASK;
 	io_granularity = 0x1000;
 	if (dev->io_window_1k) {
@@ -544,6 +544,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;

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Bjorn Helgaas]

On Wed, Jul 29, 2015 at 01:02:25PM -0700, Guenter Roeck wrote:
> On 07/29/2015 12:53 PM, Yinghai Lu wrote:
> >On Wed, Jul 29, 2015 at 12:46 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >>On 07/29/2015 12:30 PM, Yinghai Lu wrote:
> >>
> >>>
> >>>so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.
> >>>
> >>
> >>excellent catch. Unfortunately, I don't know how to make it
> >>work with the reversed flag. The idea here was that the flag
> >>propagates from parent to child. This makes sense for an
> >>"it doesn't work" flag to be inherited from the child,
> >>but not for an "it works" flag.
> >>
> >
> >also would be better if we can add has_ioport in hostbridge instead.
> >like has_mem64 in https://patchwork.ozlabs.org/patch/500926/
> >and use
> >
> >to_pci_host_bridge(bus->bridge)->has_ioport
> >
> >to replace pci_root_has_io_resource()
> >
> 
> Sure, that would make sense.
> 
> Bjorn, how do you want to handle the flag problem ?
> Do you have an idea on how to make it work with
> the reversed definition ?

I'll wait for the revised patch.  Sorry for screwing this up.

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

On Wed, Jul 29, 2015 at 1:26 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Jul 29, 2015 at 01:02:25PM -0700, Guenter Roeck wrote:
>> Bjorn, how do you want to handle the flag problem ?
>> Do you have an idea on how to make it work with
>> the reversed definition ?
>
> I'll wait for the revised patch.  Sorry for screwing this up.

Please check attached updated version.

Yinghai

[-- Attachment #2: commit-4b59da5_x.patch --]
[-- Type: text/x-patch, Size: 5312 bytes --]

commit 4b59da521f7e0aedff75c2aa90b6a653727cdf7f
Author: Guenter Roeck <linux@roeck-us.net>
Date:   Tue Jul 7 11:11:20 2015 -0700

    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.
    
    [bhelgaas: reverse sense of new pci_bus_flags_t value]
    [yinghai: simplify root bus flag check, fix flags initial setting, change to bool]
    Signed-off-by: Guenter Roeck <linux@roeck-us.net>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    CC: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

---
 drivers/pci/probe.c     |   51 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-bus.c |   11 ++--------
 include/linux/pci.h     |    1 
 3 files changed, 55 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -332,6 +332,32 @@ static void pci_read_bases(struct pci_de
 	}
 }
 
+static bool pci_bridge_supports_io(struct pci_dev *bridge)
+{
+	u16 io;
+
+	pci_read_config_word(bridge, PCI_IO_BASE, &io);
+	if (io)
+		return true;
+
+	/* 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 true;
+
+	return false;
+}
+
+static bool pci_root_has_io_resource(struct pci_bus *bus)
+{
+	while (bus->parent)
+		bus = bus->parent;
+
+	return !!(bus->bus_flags & PCI_BUS_FLAGS_SUPPORTS_IO);
+}
+
 static void pci_read_bridge_io(struct pci_bus *child)
 {
 	struct pci_dev *dev = child->self;
@@ -340,6 +366,21 @@ static void pci_read_bridge_io(struct pc
 	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;
+	}
+
+	if (!pci_root_has_io_resource(child)) {
+		dev_printk(KERN_DEBUG, &dev->dev, "  no I/O resource on root bus\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 +537,7 @@ static struct pci_bus *pci_alloc_bus(str
 	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;
@@ -2058,6 +2100,15 @@ int pci_bus_insert_busn_res(struct pci_b
 		res->flags |= IORESOURCE_PCI_FIXED;
 	}
 
+	b->bus_flags &= ~PCI_BUS_FLAGS_SUPPORTS_IO;
+	resource_list_for_each_entry(window, &bridge->windows) {
+		res = window->res;
+		if (resource_type(res) == IORESOURCE_IO) {
+			b->bus_flags |= PCI_BUS_FLAGS_SUPPORTS_IO;
+			break;
+		}
+	}
+
 	conflict = request_resource_conflict(parent_res, res);
 
 	if (conflict)
Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -744,7 +744,6 @@ int pci_claim_bridge_resource(struct pci
    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,14 +751,10 @@ static void pci_bridge_check_ranges(stru
 	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;
+	else
+		b_res[0].flags &= ~IORESOURCE_IO;
 
 	/*  DECchip 21050 pass 2 errata: the bridge may miss an address
 	    disconnect boundary by one PCI data phase.
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -193,6 +193,7 @@ typedef unsigned short __bitwise pci_bus
 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 */

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

On 07/29/2015 08:59 PM, Yinghai Lu wrote:
> On Wed, Jul 29, 2015 at 1:26 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Jul 29, 2015 at 01:02:25PM -0700, Guenter Roeck wrote:
>>> Bjorn, how do you want to handle the flag problem ?
>>> Do you have an idea on how to make it work with
>>> the reversed definition ?
>>
>> I'll wait for the revised patch.  Sorry for screwing this up.
>
> Please check attached updated version.
>
> Yinghai
>

My version is slightly different, but this one should work as well.

The subject line got lost somehow.

Guenter

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Bjorn Helgaas]

On Wed, Jul 29, 2015 at 11:17 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/29/2015 08:59 PM, Yinghai Lu wrote:
>>
>> On Wed, Jul 29, 2015 at 1:26 PM, Bjorn Helgaas <bhelgaas@google.com>
>> wrote:
>>>
>>> On Wed, Jul 29, 2015 at 01:02:25PM -0700, Guenter Roeck wrote:
>>>>
>>>> Bjorn, how do you want to handle the flag problem ?
>>>> Do you have an idea on how to make it work with
>>>> the reversed definition ?
>>>
>>>
>>> I'll wait for the revised patch.  Sorry for screwing this up.
>>
>>
>> Please check attached updated version.
>>
>> Yinghai
>>
>
> My version is slightly different, but this one should work as well.
>
> The subject line got lost somehow.

Guenter, would you mind just sending me a fresh version of your patch
with whatever Yinghai tweaks you want to keep?

It's a PITA for me to deal with attachments.

Bjorn

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

On Thu, Jul 30, 2015 at 11:18:40AM -0500, Bjorn Helgaas wrote:
> >
> > My version is slightly different, but this one should work as well.
> >
> > The subject line got lost somehow.
> 
> Guenter, would you mind just sending me a fresh version of your patch
> with whatever Yinghai tweaks you want to keep?
> 
Sure, as soon as I completed testing. I am currently having trouble
with our infrastructure [again :-(], so it may take a couple of days.

> It's a PITA for me to deal with attachments.
> 
Same here.

Guenter

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Guenter Roeck]

On 07/29/2015 12:53 PM, Yinghai Lu wrote:
> On Wed, Jul 29, 2015 at 12:46 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 07/29/2015 12:30 PM, Yinghai Lu wrote:
>>
>>>
>>> so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.
>>>
>>
>> excellent catch. Unfortunately, I don't know how to make it
>> work with the reversed flag. The idea here was that the flag
>> propagates from parent to child. This makes sense for an
>> "it doesn't work" flag to be inherited from the child,
>> but not for an "it works" flag.
>>
>
> also would be better if we can add has_ioport in hostbridge instead.
> like has_mem64 in https://patchwork.ozlabs.org/patch/500926/
> and use
>
> to_pci_host_bridge(bus->bridge)->has_ioport
>
> to replace pci_root_has_io_resource()
>

Is there a function to get the root bus ? Otherwise I still need to
find the root bus first.

Guenter

Re: [RFC PATCH v2] PCI: Only enable IO window if supported

[Yinghai Lu]

On Wed, Jul 29, 2015 at 1:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/29/2015 12:53 PM, Yinghai Lu wrote:
>>
>> On Wed, Jul 29, 2015 at 12:46 PM, Guenter Roeck <linux@roeck-us.net>
>> wrote:
>>>
>>> On 07/29/2015 12:30 PM, Yinghai Lu wrote:
>>>
>>>>
>>>> so PCI_BUS_FLAGS_SUPPORTS_IO will never get set.
>>>>
>>>
>>> excellent catch. Unfortunately, I don't know how to make it
>>> work with the reversed flag. The idea here was that the flag
>>> propagates from parent to child. This makes sense for an
>>> "it doesn't work" flag to be inherited from the child,
>>> but not for an "it works" flag.
>>>
>>
>> also would be better if we can add has_ioport in hostbridge instead.
>> like has_mem64 in https://patchwork.ozlabs.org/patch/500926/
>> and use
>>
>> to_pci_host_bridge(bus->bridge)->has_ioport
>>
>> to replace pci_root_has_io_resource()
>>
>
> Is there a function to get the root bus ? Otherwise I still need to
> find the root bus first.
host-bridge and rootbus is one to one mapping.

drivers/pci/host-bridge.c has related function.


Yinghai