[PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

Here is some patches do the optimization/fix/cleanup for the function
pci_assign_unassigned_resources().

Wei Yang (4):
  PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy
  PCI: add comment for pbus_size_mem() parameter
  PCI: trivial cleanup in pbus_size_io()
  PCI: fix the io resource alignment calculation in pbus_size_io()

 drivers/pci/setup-bus.c |   56 +++++++++++++++++++++++++++++++++++-----------
 1 files changed, 42 insertions(+), 14 deletions(-)

-- 
1.7.5.4

Re: [PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

On Tue, Jul 02, 2013 at 07:15:41AM +0800, Gavin Shan wrote:
>On Tue, Jul 02, 2013 at 07:10:41AM +0800, Gavin Shan wrote:
>>On Mon, Jul 01, 2013 at 11:10:28PM +0800, Wei Yang wrote:
>>>Here is some patches do the optimization/fix/cleanup for the function
>>>pci_assign_unassigned_resources().
>>>
>>
>>It looks good to me. Please make sure to rebase to upstream or linux-next
>>before sending out :-)
>>
>
>You already posted it to public and just realize it.

Yes :-)

This patch set is based on 3.10-rc2, so not the latest one.

Next time I will rebased the patch on the latest one and then send out.

>
>Thanks,
>Gavin

-- 
Richard Yang
Help you, Help me

Re: [PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

All,

Any comment on these patch set is welcome~

Not sure those are reasonable or not?

On Mon, Jul 01, 2013 at 11:10:28PM +0800, Wei Yang wrote:
>Here is some patches do the optimization/fix/cleanup for the function
>pci_assign_unassigned_resources().
>
>Wei Yang (4):
>  PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy
>  PCI: add comment for pbus_size_mem() parameter
>  PCI: trivial cleanup in pbus_size_io()
>  PCI: fix the io resource alignment calculation in pbus_size_io()
>
> drivers/pci/setup-bus.c |   56 +++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 42 insertions(+), 14 deletions(-)
>
>-- 
>1.7.5.4

-- 
Richard Yang
Help you, Help me

[PATCH 0/4] optimization/fix/cleanup in pci_assign_unassigned_resources

[Wei Yang]

Here is some patches do the optimization/fix/cleanup for the function
pci_assign_unassigned_resources().

Wei Yang (4):
  PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy
  PCI: add comment for pbus_size_mem() parameter
  PCI: trivial cleanup in pbus_size_io()
  PCI: fix the io resource alignment calculation in pbus_size_io()

 drivers/pci/setup-bus.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

-- 
1.7.5.4

[PATCH 1/4] PCI: optimize pci_bus_get_depth() by enumerating on pci bus hierachy

[Wei Yang]

Normally, on one pci bus there would be more devices than pci buses. When
calculating the depth of pci bus, it would be more time efficient by
enumerating through the child buses instead of the child devices.

Also by doing so, the code seems more self explaining. Previously, it go
through the pci devices and check whether a bridge introduce a child bus or
not, which needs more background knowledge to understand it.

This patch caculating the depth by enumerating on pci bus hierachy.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d254e23..6dbe562 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1300,15 +1300,12 @@ static void pci_bus_dump_resources(struct pci_bus *bus)
 static int __init pci_bus_get_depth(struct pci_bus *bus)
 {
 	int depth = 0;
-	struct pci_dev *dev;
+	struct pci_bus *child_bus;
 
-	list_for_each_entry(dev, &bus->devices, bus_list) {
+	list_for_each_entry(child_bus, &bus->children, node){
 		int ret;
-		struct pci_bus *b = dev->subordinate;
-		if (!b)
-			continue;
 
-		ret = pci_bus_get_depth(b);
+		ret = pci_bus_get_depth(child_bus);
 		if (ret + 1 > depth)
 			depth = ret + 1;
 	}
-- 
1.7.5.4

[PATCH 2/4] PCI: add comment for pbus_size_mem() parameter

[Wei Yang]

In the comment of pbus_size_mem(), the comment of two parameters are missed.

This patch fills in the missing description for two parameters of
pbus_size_mem().

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 6dbe562..939ca1b 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -838,6 +838,8 @@ static inline resource_size_t calculate_mem_align(resource_size_t *aligns,
  * pbus_size_mem() - size the memory window of a given bus
  *
  * @bus : the bus
+ * @mask: mask the resource flag, then compare it with type
+ * @type: the type of free resource from bridge
  * @min_size : the minimum memory window that must to be allocated
  * @add_size : additional optional memory window
  * @realloc_head : track the additional memory window on this list
-- 
1.7.5.4

[PATCH 3/4] PCI: trivial cleanup in pbus_size_io()

[Wei Yang]

This patch did several cleanup in pbus_size_io():
1. change the type of size to resource_size_t
2. fix the warning in dev_printk() for the change of type

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 939ca1b..d4f1ad9 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -747,7 +747,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 {
 	struct pci_dev *dev;
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
-	unsigned long size = 0, size0 = 0, size1 = 0;
+	resource_size_t size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
 	resource_size_t min_align, io_align, align;
 
@@ -807,8 +807,9 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		add_to_list(realloc_head, bus->self, b_res, size1-size0,
 			    min_align);
 		dev_printk(KERN_DEBUG, &bus->self->dev, "bridge window "
-				 "%pR to %pR add_size %lx\n", b_res,
-				 &bus->busn_res, size1-size0);
+				 "%pR to %pR add_size %llx\n", b_res,
+				 &bus->busn_res,
+				 (unsigned long long)size1-size0);
 	}
 }
 
-- 
1.7.5.4

[PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
it introduce a new method to calculate the window alignment of P2P bridge.

When the io_window_1k is set,  the calculation for the io resource alignment
is different from the original one. In the original logic before 462d9303,
the alignment is no bigger than 4K even the io_window_1k is set. The logic
introduced in 462d9303 will limit the alignment to 1k in this case.

This patch fix this issue.

Here is an example for this case.

Assume:
        1. pcibios_window_alignment() return 1.
        2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
        3. one of the child device has an IO resource with size of 2K.

Result comparison:

                    Before 462d9303             After 462d9303
    min_align       1k                          1k
                                        |
                             after loop |
                                        V
    min_align       2k                          2k
                                        |
                         check boundary |
                                        V
    min_align       2k                          1k

After applying the change:

Result comparison:

                    with  462d9303             with this patch
    min_align       1k                          1k
    io_align        1k                          4k
                                        |
                             after loop |
                                        V
    min_align       2k                          2k
    io_align        1k                          4k
                                        |
                         check boundary |
                                        V
    min_align       1k                          2k
    io_align        1k                          1k

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d4f1ad9..6c111e9 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
  		return;
 
 	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
+	/* Don't exceed 4KiB for windows requesting 1KiB alignment */
+	if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
+		io_align = PCI_P2P_DEFAULT_IO_ALIGN;
+
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
 
-- 
1.7.5.4

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> it introduce a new method to calculate the window alignment of P2P bridge.
>
> When the io_window_1k is set,  the calculation for the io resource alignment
> is different from the original one. In the original logic before 462d9303,
> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> introduced in 462d9303 will limit the alignment to 1k in this case.
>
> This patch fix this issue.
>
> Here is an example for this case.
>
> Assume:
>         1. pcibios_window_alignment() return 1.
>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>         3. one of the child device has an IO resource with size of 2K.
>
> Result comparison:
>
>                     Before 462d9303             After 462d9303
>     min_align       1k                          1k
>                                         |
>                              after loop |
>                                         V
>     min_align       2k                          2k
>                                         |
>                          check boundary |
>                                         V
>     min_align       2k                          1k
>
> After applying the change:
>
> Result comparison:
>
>                     with  462d9303             with this patch
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                              after loop |
>                                         V
>     min_align       2k                          2k
>     io_align        1k                          4k
>                                         |
>                          check boundary |
>                                         V
>     min_align       1k                          2k
>     io_align        1k                          1k
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>

Gavin, can you ack this, since you wrote the original commit involved?

> ---
>  drivers/pci/setup-bus.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index d4f1ad9..6c111e9 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                 return;
>
>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);
> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */
> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
> +
>         list_for_each_entry(dev, &bus->devices, bus_list) {
>                 int i;
>
> --
> 1.7.5.4
>

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

[+cc Yinghai]

On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> it introduce a new method to calculate the window alignment of P2P bridge.
>
> When the io_window_1k is set,  the calculation for the io resource alignment
> is different from the original one. In the original logic before 462d9303,
> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> introduced in 462d9303 will limit the alignment to 1k in this case.
>
> This patch fix this issue.

Presumably this fixes a bug, but you don't say what it is.  "different
from the original" is not a description of a problem.  You need
something like "with the current code, we allocate the wrong window
size in situation X, or we allocate a window with incorrect alignment
in situation Y, etc."

> Here is an example for this case.
>
> Assume:
>         1. pcibios_window_alignment() return 1.
>         2. window_alignment() return PCI_P2P_DEFAULT_IO_ALIGN_1K.
>         3. one of the child device has an IO resource with size of 2K.
>
> Result comparison:
>
>                     Before 462d9303             After 462d9303
>     min_align       1k                          1k
>                                         |
>                              after loop |
>                                         V
>     min_align       2k                          2k
>                                         |
>                          check boundary |
>                                         V
>     min_align       2k                          1k
>
> After applying the change:
>
> Result comparison:
>
>                     with  462d9303             with this patch
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                              after loop |
>                                         V
>     min_align       2k                          2k
>     io_align        1k                          4k
>                                         |
>                          check boundary |
>                                         V
>     min_align       1k                          2k
>     io_align        1k                          1k
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/setup-bus.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index d4f1ad9..6c111e9 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -755,6 +755,10 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>                 return;
>
>         io_align = min_align = window_alignment(bus, IORESOURCE_IO);

I don't think it makes sense to set "min_align = window_alignment(bus,
IORESOURCE_IO)" before the loop.  "io_align" is a function of the
bridge and is independent of downstream devices and is computed by
window_alignment().  "min_align" is a function of the downstream
devices and bridges and should be zero when entering the loop.

> +       /* Don't exceed 4KiB for windows requesting 1KiB alignment */

This comment doesn't make sense to me.  The code changes io_align from
1024 to 4096, so I don't understand where the "don't exceed 4KiB" part
comes from.  io_align doesn't exceed 4096 anyway, unless
pcibios_window_alignment() gives us a larger alignment, and in that
case, this patch has no effect because io_align is not 1024.

> +       if (bus->self->io_window_1k && io_align == PCI_P2P_DEFAULT_IO_ALIGN_1K)
> +               io_align = PCI_P2P_DEFAULT_IO_ALIGN;
> +

>         list_for_each_entry(dev, &bus->devices, bus_list) {
>                 int i;

This list_for_each_entry() loop computes the alignment required by
downstream devices and bridges.  The result is "min_align".

After the loop we have this (added by Yinghai in fd5913411):

    if (min_align > io_align)
        min_align = io_align;

I don't understand this.  There are three cases:

1) No downstream bridges: min_align <= 256 because I/O BARs are
limited to 256 bytes.
2) All downstream bridges have 1K extension: min_align = 1024.
3) At least one downstream bridge requires 4K alignment: min_align = 4096.

"io_align" is the minimum alignment enforced by the bridge.  This is
independent of any devices below the bridge, so "io_align >= 1024" in
all cases.

Therefore, the "min_align = io_align" assignment only happens in case
3, when a downstream bridge requires 4K alignment and *this* bridge
supports the 1K extension.  But that seems wrong.  The downstream
bridge's 4K requirement also applies to all bridges all the way
upstream.

It looks to me like this test should instead be:

    if (min_align < io_align)
        min_align = io_align;

> --
> 1.7.5.4
>

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Yinghai Lu]

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

On Mon, Aug 5, 2013 at 10:58 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Yinghai]
>
> After the loop we have this (added by Yinghai in fd5913411):
>
>     if (min_align > io_align)
>         min_align = io_align;
>
> I don't understand this.  There are three cases:
>
> 1) No downstream bridges: min_align <= 256 because I/O BARs are
> limited to 256 bytes.
> 2) All downstream bridges have 1K extension: min_align = 1024.
> 3) At least one downstream bridge requires 4K alignment: min_align = 4096.
>
> "io_align" is the minimum alignment enforced by the bridge.  This is
> independent of any devices below the bridge, so "io_align >= 1024" in
> all cases.
>
> Therefore, the "min_align = io_align" assignment only happens in case
> 3, when a downstream bridge requires 4K alignment and *this* bridge
> supports the 1K extension.  But that seems wrong.  The downstream
> bridge's 4K requirement also applies to all bridges all the way
> upstream.
>
> It looks to me like this test should instead be:
>
>     if (min_align < io_align)
>         min_align = io_align;

before my change, min_align always set to 4096.
in the loop to get min_align, dev resource could have size bigger than
4k, those resource will have SIZEALIGN, so final min_align could be
more than 4096.
so at last we cap it to 4096 again.

But according to your findings: "I/O BARs are limited to 256 bytes"
we should not worry about that.

So we should just drop io_align and checking like attached patch.
(min_align is already set to 1k or 4k before the looping.)

and that should solve Wei Yang's problem.

Thanks

Yinghai

[-- Attachment #2: fix_io_align_checking.patch --]
[-- Type: application/octet-stream, Size: 1031 bytes --]

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 64a7de2..edaa9e8 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -816,12 +816,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
 	unsigned long size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
-	resource_size_t min_align, io_align, align;
+	resource_size_t min_align, align;
 
 	if (!b_res)
  		return;
 
-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
+	min_align = window_alignment(bus, IORESOURCE_IO);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
 
@@ -848,8 +848,8 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		}
 	}
 
-	if (min_align > io_align)
-		min_align = io_align;
+	if (min_align > PCI_P2P_DEFAULT_IO_ALIGN)
+		min_align = PCI_P2P_DEFAULT_IO_ALIGN;
 
 	size0 = calculate_iosize(size, min_size, size1,
 			resource_size(b_res), min_align);

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Mon, Aug 05, 2013 at 12:05:03PM -0700, Yinghai Lu wrote:
> On Mon, Aug 5, 2013 at 10:58 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Yinghai]
> >
> > After the loop we have this (added by Yinghai in fd5913411):
> >
> >     if (min_align > io_align)
> >         min_align = io_align;
> >
> > I don't understand this.  There are three cases:
> >
> > 1) No downstream bridges: min_align <= 256 because I/O BARs are
> > limited to 256 bytes.
> > 2) All downstream bridges have 1K extension: min_align = 1024.
> > 3) At least one downstream bridge requires 4K alignment: min_align = 4096.
> >
> > "io_align" is the minimum alignment enforced by the bridge.  This is
> > independent of any devices below the bridge, so "io_align >= 1024" in
> > all cases.
> >
> > Therefore, the "min_align = io_align" assignment only happens in case
> > 3, when a downstream bridge requires 4K alignment and *this* bridge
> > supports the 1K extension.  But that seems wrong.  The downstream
> > bridge's 4K requirement also applies to all bridges all the way
> > upstream.
> >
> > It looks to me like this test should instead be:
> >
> >     if (min_align < io_align)
> >         min_align = io_align;
> 
> before my change, min_align always set to 4096.
> in the loop to get min_align, dev resource could have size bigger than
> 4k, those resource will have SIZEALIGN, so final min_align could be
> more than 4096.
> so at last we cap it to 4096 again.

To make sure I understand this, I think (correct me if I'm wrong):

  - Every device BAR resource will have IORESOURCE_SIZEALIGN set since it
    must be naturally aligned on its size (sec 6.2.5.1 of PCI spec r3.0).

  - Bridge window resources will have IORESOURCE_STARTALIGN set and
    res->start contains the required alignment, i.e., 1MB for MEM windows,
    4K for architected I/O windows, 1K for bridges with 1K extension, or
    larger values for arch-specific requirements.

> But according to your findings: "I/O BARs are limited to 256 bytes"
> we should not worry about that.

This is also per sec 6.2.5.1 of PCI spec r3.0: "Devices that map control
functions into I/O Space must not consume more than 256 bytes per I/O Base
Address register."

> So we should just drop io_align and checking like attached patch.
> (min_align is already set to 1k or 4k before the looping.)
> 
> and that should solve Wei Yang's problem.

[I inlined your patch for commenting.  You're welcome :)]

> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 64a7de2..edaa9e8 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -816,12 +816,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
>  	unsigned long size = 0, size0 = 0, size1 = 0;
>  	resource_size_t children_add_size = 0;
> -	resource_size_t min_align, io_align, align;
> +	resource_size_t min_align, align;
>  
>  	if (!b_res)
>   		return;
>  
> -	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
> +	min_align = window_alignment(bus, IORESOURCE_IO);

I like this change.

>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		int i;
>  
> @@ -848,8 +848,8 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>  		}
>  	}
>  
> -	if (min_align > io_align)
> -		min_align = io_align;
> +	if (min_align > PCI_P2P_DEFAULT_IO_ALIGN)
> +		min_align = PCI_P2P_DEFAULT_IO_ALIGN;

But I still don't understand this one.  As far as I can tell,
"min_align > 4096" can happen only for arch-specific I/O window
requirements.  It can never happen because of a large device I/O BAR.

Let's assume "min_align = window_alignment(bus, IORESOURCE_IO)" above set
min_align to 16KB because that's what pnv_pci_window_alignment() returned.
Why would we cap it to 4KB here?  I think we should set this:

    b_res->start = 16384;
    b_res->end = b_res->start + size0 - 1;
    b_res->flags |= IORESOURCE_STARTALIGN;

to indicate that the bridge needs a 16KB-aligned I/O window.

In what case do we need to cap min_align to 4KB?

>  	size0 = calculate_iosize(size, min_size, size1,
>  			resource_size(b_res), min_align);

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Yinghai Lu]

On Mon, Aug 5, 2013 at 12:51 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> @@ -848,8 +848,8 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>>               }
>>       }
>>
>> -     if (min_align > io_align)
>> -             min_align = io_align;
>> +     if (min_align > PCI_P2P_DEFAULT_IO_ALIGN)
>> +             min_align = PCI_P2P_DEFAULT_IO_ALIGN;
>
> But I still don't understand this one.  As far as I can tell,
> "min_align > 4096" can happen only for arch-specific I/O window
> requirements.  It can never happen because of a large device I/O BAR.
>
> Let's assume "min_align = window_alignment(bus, IORESOURCE_IO)" above set
> min_align to 16KB because that's what pnv_pci_window_alignment() returned.
> Why would we cap it to 4KB here?  I think we should set this:
>
>     b_res->start = 16384;
>     b_res->end = b_res->start + size0 - 1;
>     b_res->flags |= IORESOURCE_STARTALIGN;
>
> to indicate that the bridge needs a 16KB-aligned I/O window.
>
> In what case do we need to cap min_align to 4KB?
>
>>       size0 = calculate_iosize(size, min_size, size1,
>>                       resource_size(b_res), min_align);

then, we should drop that 4k capping.
I was thinking there could be strange or wild res with bigger than 4k.

Thanks

Yinghai

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Mon, Aug 5, 2013 at 2:52 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Mon, Aug 5, 2013 at 12:51 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> @@ -848,8 +848,8 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>>>               }
>>>       }
>>>
>>> -     if (min_align > io_align)
>>> -             min_align = io_align;
>>> +     if (min_align > PCI_P2P_DEFAULT_IO_ALIGN)
>>> +             min_align = PCI_P2P_DEFAULT_IO_ALIGN;
>>
>> But I still don't understand this one.  As far as I can tell,
>> "min_align > 4096" can happen only for arch-specific I/O window
>> requirements.  It can never happen because of a large device I/O BAR.
>>
>> Let's assume "min_align = window_alignment(bus, IORESOURCE_IO)" above set
>> min_align to 16KB because that's what pnv_pci_window_alignment() returned.
>> Why would we cap it to 4KB here?  I think we should set this:
>>
>>     b_res->start = 16384;
>>     b_res->end = b_res->start + size0 - 1;
>>     b_res->flags |= IORESOURCE_STARTALIGN;
>>
>> to indicate that the bridge needs a 16KB-aligned I/O window.
>>
>> In what case do we need to cap min_align to 4KB?
>>
>>>       size0 = calculate_iosize(size, min_size, size1,
>>>                       resource_size(b_res), min_align);
>
> then, we should drop that 4k capping.
> I was thinking there could be strange or wild res with bigger than 4k.

If there *were* an I/O BAR larger than 4KB, how should it be handled?
I don't think capping the alignment to 4KB sounds like the best way.
For example, a 16KB I/O BAR would still need to be aligned on 16KB.

And I think capping to 4KB as you did above will break the powerpc
pcibios_window_alignment() implementation.  For example, if
pcibios_window_alignment() returned 16KB, and we later capped it to
4KB, we're going to allocate space for the bridge window with the
wrong alignment.

Bjorn

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Yinghai Lu]

On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> then, we should drop that 4k capping.
>> I was thinking there could be strange or wild res with bigger than 4k.
>
> If there *were* an I/O BAR larger than 4KB, how should it be handled?
> I don't think capping the alignment to 4KB sounds like the best way.
> For example, a 16KB I/O BAR would still need to be aligned on 16KB.
>
> And I think capping to 4KB as you did above will break the powerpc
> pcibios_window_alignment() implementation.  For example, if
> pcibios_window_alignment() returned 16KB, and we later capped it to
> 4KB, we're going to allocate space for the bridge window with the
> wrong alignment.

Agree.

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Mon, Aug 05, 2013 at 02:09:27PM -0700, Yinghai Lu wrote:
> On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> then, we should drop that 4k capping.
> >> I was thinking there could be strange or wild res with bigger than 4k.
> >
> > If there *were* an I/O BAR larger than 4KB, how should it be handled?
> > I don't think capping the alignment to 4KB sounds like the best way.
> > For example, a 16KB I/O BAR would still need to be aligned on 16KB.
> >
> > And I think capping to 4KB as you did above will break the powerpc
> > pcibios_window_alignment() implementation.  For example, if
> > pcibios_window_alignment() returned 16KB, and we later capped it to
> > 4KB, we're going to allocate space for the bridge window with the
> > wrong alignment.
> 
> Agree.

OK.  Can you guys try this out and see whether it fixes the problem?
I don't know what the actual problem *is*, so I can't tell whether
this is a possible fix.

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d4f1ad9..8333c92 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -749,12 +749,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
 	resource_size_t size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
-	resource_size_t min_align, io_align, align;
+	resource_size_t min_align, align;
 
 	if (!b_res)
  		return;
 
-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
+	min_align = window_alignment(bus, IORESOURCE_IO);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
 
@@ -781,9 +781,6 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		}
 	}
 
-	if (min_align > io_align)
-		min_align = io_align;
-
 	size0 = calculate_iosize(size, min_size, size1,
 			resource_size(b_res), min_align);
 	if (children_add_size > add_size)

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Mon, Aug 05, 2013 at 04:21:35PM -0600, Bjorn Helgaas wrote:
>On Mon, Aug 05, 2013 at 02:09:27PM -0700, Yinghai Lu wrote:
>> On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >> then, we should drop that 4k capping.
>> >> I was thinking there could be strange or wild res with bigger than 4k.
>> >
>> > If there *were* an I/O BAR larger than 4KB, how should it be handled?
>> > I don't think capping the alignment to 4KB sounds like the best way.
>> > For example, a 16KB I/O BAR would still need to be aligned on 16KB.
>> >
>> > And I think capping to 4KB as you did above will break the powerpc
>> > pcibios_window_alignment() implementation.  For example, if
>> > pcibios_window_alignment() returned 16KB, and we later capped it to
>> > 4KB, we're going to allocate space for the bridge window with the
>> > wrong alignment.
>> 
>> Agree.
>
>OK.  Can you guys try this out and see whether it fixes the problem?
>I don't know what the actual problem *is*, so I can't tell whether
>this is a possible fix.
>

Thanks all for the comments, this makes me re-consider the cases. Let me do a
summary. Maybe I misunderstand the idea, please fix me~

Requirement from PCI Spec
============================================================================
1. I/O BAR for non-bridge PCI devices are limited to 256 bytes
2. Most I/O window for PCI bridge is 4k aligned
3. Some bridge support 1k aligned I/O window

Ancient time -- when 1k aligned I/O window is not there
============================================================================
The alignment is 4k in all cases. (As it is hard coded.)

    For devices under this bridge, I/O BAR is less then 256.
    
    For downstream port, I/O window is 4k aligned, since the IORESOURCE_STARTALIGN
    is set. This means even the downstream port connects other bridge, the
    alignment is still 4k.

Middle Age -- when 1k aligned I/O window is introduced
============================================================================
This introduce two other cases: 
1. All downstream port is 1k aligned
2. One of the downstream port is 4k aligned.

Case 1: the "min_align" should be set to 1k. This could save some I/O resource.
Case 2: the "min_align" should be set to 4k, even itself anounced could support
        1k alignment.
	               ^--- Fix me, if not correct.

The "min_align" could be set to other value? Previously, I thought it could
be, for example 2k. Now I think no, the list_for_each_entry loop will iterate
on two kinds of resources: 1. Device I/O BAR; 2. Bridge I/O window.

Device I/O BAR is less then 256 bytes, it won't contribut to the alignment.
Bridge I/O window will be 1k or 4k aligned.

This means only two possible value for "min_align": 1k or 4k.
	               ^--- Fix me, if not correct.


Back to Yinghai's commit(fd5913411), the "min_align" is set to 1k at the
beginning. During the list_for_each_entry loop, (align > min_align) is true
means align is 4k.

And this (min_align > 4096) will never be true, since at most "min_align" is
4k. So, I think, this comparison could be removed in commit(fd5913411).
	               ^--- Fix me, if not correct.


Present Age -- when architecture require specific alignment for bridge window
============================================================================
This introduce 3 cases:
1. 1k < 4k < arch_align
2. 1k < arch_align < 4k
3. arch_align < 1k < 4k

Case 1: the "min_align" should be arch_align.
Case 2: this is a little complicated. downstream port could be all 1k aligned
        or one of the downstream port is 4k aligned.
	a. all 1k aligned, the "min_align" should be arch_align
	b. one is 4k aligned, the "min_align" should be 4k
Case 3: this is the same as before

The final result of "min_align" in these three cases are all the biggest one
of upstream/downstream/arch alignment. So the algorithm could be changed to
calculate the biggest one of the three.


Personal Conclusion
============================================================================
I think Bjorn's patch works.
Will test on powernv platform and give the result.

Last but not the least, please fix me, if I am not correct. :-)

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Mon, Aug 05, 2013 at 11:58:50AM -0600, Bjorn Helgaas wrote:
>[+cc Yinghai]
>
>On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>> it introduce a new method to calculate the window alignment of P2P bridge.
>>
>> When the io_window_1k is set,  the calculation for the io resource alignment
>> is different from the original one. In the original logic before 462d9303,
>> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>> introduced in 462d9303 will limit the alignment to 1k in this case.
>>
>> This patch fix this issue.
>
>Presumably this fixes a bug, but you don't say what it is.  "different
>from the original" is not a description of a problem.  You need
>something like "with the current code, we allocate the wrong window
>size in situation X, or we allocate a window with incorrect alignment
>in situation Y, etc."
>

With current code, we allocate the wrong window size when upstream bridge
could be 1k aligned and one of the downstream port is 4k aligned.

In this case, the "min_align" should be 4k. But the current code set
"min_align" to 1k.

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Tue, Aug 06, 2013 at 11:22:27AM +0800, Gavin Shan wrote:
>On Mon, Aug 05, 2013 at 04:21:35PM -0600, Bjorn Helgaas wrote:
>>On Mon, Aug 05, 2013 at 02:09:27PM -0700, Yinghai Lu wrote:
>>> On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> >> then, we should drop that 4k capping.
>>> >> I was thinking there could be strange or wild res with bigger than 4k.
>>> >
>>> > If there *were* an I/O BAR larger than 4KB, how should it be handled?
>>> > I don't think capping the alignment to 4KB sounds like the best way.
>>> > For example, a 16KB I/O BAR would still need to be aligned on 16KB.
>>> >
>>> > And I think capping to 4KB as you did above will break the powerpc
>>> > pcibios_window_alignment() implementation.  For example, if
>>> > pcibios_window_alignment() returned 16KB, and we later capped it to
>>> > 4KB, we're going to allocate space for the bridge window with the
>>> > wrong alignment.
>>> 
>>> Agree.
>>
>>OK.  Can you guys try this out and see whether it fixes the problem?
>>I don't know what the actual problem *is*, so I can't tell whether
>>this is a possible fix.
>>
>
>The code looks simpler, but it potentially breaks PowerNV platforms.
>Lets have inline description if I understand everything here.
>
>>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>index d4f1ad9..8333c92 100644
>>--- a/drivers/pci/setup-bus.c
>>+++ b/drivers/pci/setup-bus.c
>>@@ -749,12 +749,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>> 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
>> 	resource_size_t size = 0, size0 = 0, size1 = 0;
>> 	resource_size_t children_add_size = 0;
>>-	resource_size_t min_align, io_align, align;
>>+	resource_size_t min_align, align;
>>
>> 	if (!b_res)
>>  		return;
>>
>>-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
>>+	min_align = window_alignment(bus, IORESOURCE_IO);
>> 	list_for_each_entry(dev, &bus->devices, bus_list) {
>> 		int i;
>>
>
>"min_align" here indicates the IO segment size on PowerNV platform.
>On PowerNV platform, the IO range of specific PHB (PCI controller)
>is divided evenly and each piece of that is called IO segment. For
>example, the IO segment size ("min_align") is 64K initially. If one
>of specific IO BAR has size of 96K (it's possible?), "min_align"
>becomes 96K eventually, which isn't IO segment aligned.

I think this caes will not happen. 

As I analysised in previous letter. During the list_for_each_entry loop, the
resources could be in two cased: 1. bridge I/O window(IORESOURCE_STARTALIGN);
2. device I/O BAR(IORESOURCE_SIZEALIGN).

    Bridge I/O window is 64k aligned, as the platform required.
    Device I/O BAR is less than 256 bytes according to the specification.

So the 96k size is not possible. 

Please fix me, if I am not correct.

>
>>@@ -781,9 +781,6 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
>> 		}
>> 	}
>>
>>-	if (min_align > io_align)
>>-		min_align = io_align;
>>-
>> 	size0 = calculate_iosize(size, min_size, size1,
>> 			resource_size(b_res), min_align);
>> 	if (children_add_size > add_size)
>>
>
>Thanks,
>Gavin

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 11:22:27AM +0800, Gavin Shan wrote:
> On Mon, Aug 05, 2013 at 04:21:35PM -0600, Bjorn Helgaas wrote:
> >On Mon, Aug 05, 2013 at 02:09:27PM -0700, Yinghai Lu wrote:
> >> On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> >> then, we should drop that 4k capping.
> >> >> I was thinking there could be strange or wild res with bigger than 4k.
> >> >
> >> > If there *were* an I/O BAR larger than 4KB, how should it be handled?
> >> > I don't think capping the alignment to 4KB sounds like the best way.
> >> > For example, a 16KB I/O BAR would still need to be aligned on 16KB.
> >> >
> >> > And I think capping to 4KB as you did above will break the powerpc
> >> > pcibios_window_alignment() implementation.  For example, if
> >> > pcibios_window_alignment() returned 16KB, and we later capped it to
> >> > 4KB, we're going to allocate space for the bridge window with the
> >> > wrong alignment.
> >> 
> >> Agree.
> >
> >OK.  Can you guys try this out and see whether it fixes the problem?
> >I don't know what the actual problem *is*, so I can't tell whether
> >this is a possible fix.
> >
> 
> The code looks simpler, but it potentially breaks PowerNV platforms.
> Lets have inline description if I understand everything here.
> 
> >diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> >index d4f1ad9..8333c92 100644
> >--- a/drivers/pci/setup-bus.c
> >+++ b/drivers/pci/setup-bus.c
> >@@ -749,12 +749,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> > 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
> > 	resource_size_t size = 0, size0 = 0, size1 = 0;
> > 	resource_size_t children_add_size = 0;
> >-	resource_size_t min_align, io_align, align;
> >+	resource_size_t min_align, align;
> >
> > 	if (!b_res)
> >  		return;
> >
> >-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
> >+	min_align = window_alignment(bus, IORESOURCE_IO);
> > 	list_for_each_entry(dev, &bus->devices, bus_list) {
> > 		int i;
> >
> 
> "min_align" here indicates the IO segment size on PowerNV platform.
> On PowerNV platform, the IO range of specific PHB (PCI controller)
> is divided evenly and each piece of that is called IO segment. For
> example, the IO segment size ("min_align") is 64K initially. If one
> of specific IO BAR has size of 96K (it's possible?), "min_align"
> becomes 96K eventually, which isn't IO segment aligned.

96K is not a valid size for any PCI BAR.  BARs are always a power-of-2
size.  If PowerNV has an I/O segment size of 64K and we have an I/O BAR
of 128K (illegal per spec, but assume we did for the sake of argument),
we would allocate a 128K-aligned area, which does satisfy the PowerNV
alignment restriction.

Bjorn

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 02:15:34PM +0800, Wei Yang wrote:
> On Mon, Aug 05, 2013 at 04:21:35PM -0600, Bjorn Helgaas wrote:
> >On Mon, Aug 05, 2013 at 02:09:27PM -0700, Yinghai Lu wrote:
> >> On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> >> then, we should drop that 4k capping.
> >> >> I was thinking there could be strange or wild res with bigger than 4k.
> >> >
> >> > If there *were* an I/O BAR larger than 4KB, how should it be handled?
> >> > I don't think capping the alignment to 4KB sounds like the best way.
> >> > For example, a 16KB I/O BAR would still need to be aligned on 16KB.
> >> >
> >> > And I think capping to 4KB as you did above will break the powerpc
> >> > pcibios_window_alignment() implementation.  For example, if
> >> > pcibios_window_alignment() returned 16KB, and we later capped it to
> >> > 4KB, we're going to allocate space for the bridge window with the
> >> > wrong alignment.
> >> 
> >> Agree.
> >
> >OK.  Can you guys try this out and see whether it fixes the problem?
> >I don't know what the actual problem *is*, so I can't tell whether
> >this is a possible fix.
> >
> 
> Thanks all for the comments, this makes me re-consider the cases. Let me do a
> summary. Maybe I misunderstand the idea, please fix me~
> 
> Requirement from PCI Spec
> ============================================================================
> 1. I/O BAR for non-bridge PCI devices are limited to 256 bytes
> 2. Most I/O window for PCI bridge is 4k aligned
> 3. Some bridge support 1k aligned I/O window
> 
> Ancient time -- when 1k aligned I/O window is not there
> ============================================================================
> The alignment is 4k in all cases. (As it is hard coded.)
> 
>     For devices under this bridge, I/O BAR is less then 256.
>     
>     For downstream port, I/O window is 4k aligned, since the IORESOURCE_STARTALIGN
>     is set. This means even the downstream port connects other bridge, the
>     alignment is still 4k.

Right.

> Middle Age -- when 1k aligned I/O window is introduced
> ============================================================================
> This introduce two other cases: 
> 1. All downstream port is 1k aligned
> 2. One of the downstream port is 4k aligned.
> 
> Case 1: the "min_align" should be set to 1k. This could save some I/O resource.
> Case 2: the "min_align" should be set to 4k, even itself anounced could support
>         1k alignment.
> 	               ^--- Fix me, if not correct.

Right.

> The "min_align" could be set to other value? Previously, I thought it could
> be, for example 2k. Now I think no, the list_for_each_entry loop will iterate
> on two kinds of resources: 1. Device I/O BAR; 2. Bridge I/O window.
> 
> Device I/O BAR is less then 256 bytes, it won't contribut to the alignment.
> Bridge I/O window will be 1k or 4k aligned.
> 
> This means only two possible value for "min_align": 1k or 4k.
> 	               ^--- Fix me, if not correct.

Right.

> Back to Yinghai's commit(fd5913411), the "min_align" is set to 1k at the
> beginning. During the list_for_each_entry loop, (align > min_align) is true
> means align is 4k.
> 
> And this (min_align > 4096) will never be true, since at most "min_align" is
> 4k. So, I think, this comparison could be removed in commit(fd5913411).
> 	               ^--- Fix me, if not correct.

Right.  I think Yinghai was considering the case of an I/O BAR larger
than 4K.  But that's illegal per spec, and I think we would want to keep
the larger alignment even if it were legal.

> Present Age -- when architecture require specific alignment for bridge window
> ============================================================================
> This introduce 3 cases:
> 1. 1k < 4k < arch_align
> 2. 1k < arch_align < 4k
> 3. arch_align < 1k < 4k
> 
> Case 1: the "min_align" should be arch_align.
> Case 2: this is a little complicated. downstream port could be all 1k aligned
>         or one of the downstream port is 4k aligned.
> 	a. all 1k aligned, the "min_align" should be arch_align
> 	b. one is 4k aligned, the "min_align" should be 4k
> Case 3: this is the same as before
> 
> The final result of "min_align" in these three cases are all the biggest one
> of upstream/downstream/arch alignment. So the algorithm could be changed to
> calculate the biggest one of the three.

Right.

> Personal Conclusion
> ============================================================================
> I think Bjorn's patch works.
> Will test on powernv platform and give the result.

Great, let me know what happens.

Bjorn

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 02:26:24PM +0800, Wei Yang wrote:
> On Tue, Aug 06, 2013 at 11:22:27AM +0800, Gavin Shan wrote:
> >On Mon, Aug 05, 2013 at 04:21:35PM -0600, Bjorn Helgaas wrote:
> >>On Mon, Aug 05, 2013 at 02:09:27PM -0700, Yinghai Lu wrote:
> >>> On Mon, Aug 5, 2013 at 1:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>> >> then, we should drop that 4k capping.
> >>> >> I was thinking there could be strange or wild res with bigger than 4k.
> >>> >
> >>> > If there *were* an I/O BAR larger than 4KB, how should it be handled?
> >>> > I don't think capping the alignment to 4KB sounds like the best way.
> >>> > For example, a 16KB I/O BAR would still need to be aligned on 16KB.
> >>> >
> >>> > And I think capping to 4KB as you did above will break the powerpc
> >>> > pcibios_window_alignment() implementation.  For example, if
> >>> > pcibios_window_alignment() returned 16KB, and we later capped it to
> >>> > 4KB, we're going to allocate space for the bridge window with the
> >>> > wrong alignment.
> >>> 
> >>> Agree.
> >>
> >>OK.  Can you guys try this out and see whether it fixes the problem?
> >>I don't know what the actual problem *is*, so I can't tell whether
> >>this is a possible fix.
> >>
> >
> >The code looks simpler, but it potentially breaks PowerNV platforms.
> >Lets have inline description if I understand everything here.
> >
> >>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> >>index d4f1ad9..8333c92 100644
> >>--- a/drivers/pci/setup-bus.c
> >>+++ b/drivers/pci/setup-bus.c
> >>@@ -749,12 +749,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >> 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
> >> 	resource_size_t size = 0, size0 = 0, size1 = 0;
> >> 	resource_size_t children_add_size = 0;
> >>-	resource_size_t min_align, io_align, align;
> >>+	resource_size_t min_align, align;
> >>
> >> 	if (!b_res)
> >>  		return;
> >>
> >>-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
> >>+	min_align = window_alignment(bus, IORESOURCE_IO);
> >> 	list_for_each_entry(dev, &bus->devices, bus_list) {
> >> 		int i;
> >>
> >
> >"min_align" here indicates the IO segment size on PowerNV platform.
> >On PowerNV platform, the IO range of specific PHB (PCI controller)
> >is divided evenly and each piece of that is called IO segment. For
> >example, the IO segment size ("min_align") is 64K initially. If one
> >of specific IO BAR has size of 96K (it's possible?), "min_align"
> >becomes 96K eventually, which isn't IO segment aligned.
> 
> I think this caes will not happen. 
> 
> As I analysised in previous letter. During the list_for_each_entry loop, the
> resources could be in two cased: 1. bridge I/O window(IORESOURCE_STARTALIGN);
> 2. device I/O BAR(IORESOURCE_SIZEALIGN).
> 
>     Bridge I/O window is 64k aligned, as the platform required.
>     Device I/O BAR is less than 256 bytes according to the specification.
> 
> So the 96k size is not possible. 

It's not possible because of the power-of-2 size requirement.  The spec
does say I/O BARs should be 256 bytes or smaller, but one could imagine
a BAR that violated that requirement.  But the power-of-2 requirement is
more fundamental because of the way BARs are sized (low-order bits are
hardwired to zero, and the number of hardwired bits determines the size)
and PCI really cannot support a non-power-of-2 sized BAR at all.

> Please fix me, if I am not correct.
> 
> >
> >>@@ -781,9 +781,6 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> >> 		}
> >> 	}
> >>
> >>-	if (min_align > io_align)
> >>-		min_align = io_align;
> >>-
> >> 	size0 = calculate_iosize(size, min_size, size1,
> >> 			resource_size(b_res), min_align);
> >> 	if (children_add_size > add_size)
> >>
> >
> >Thanks,
> >Gavin
> 
> -- 
> Richard Yang
> Help you, Help me
> 

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 02:19:24PM +0800, Wei Yang wrote:
> On Mon, Aug 05, 2013 at 11:58:50AM -0600, Bjorn Helgaas wrote:
> >[+cc Yinghai]
> >
> >On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> >> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> >> it introduce a new method to calculate the window alignment of P2P bridge.
> >>
> >> When the io_window_1k is set,  the calculation for the io resource alignment
> >> is different from the original one. In the original logic before 462d9303,
> >> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> >> introduced in 462d9303 will limit the alignment to 1k in this case.
> >>
> >> This patch fix this issue.
> >
> >Presumably this fixes a bug, but you don't say what it is.  "different
> >from the original" is not a description of a problem.  You need
> >something like "with the current code, we allocate the wrong window
> >size in situation X, or we allocate a window with incorrect alignment
> >in situation Y, etc."
> >
> 
> With current code, we allocate the wrong window size when upstream bridge
> could be 1k aligned and one of the downstream port is 4k aligned.
> 
> In this case, the "min_align" should be 4k. But the current code set
> "min_align" to 1k.

I would expect that we allocate a window with incorrect *alignment*,
not incorrect size.  If you include a dmesg log and lspci output and point
out the problem, that will avoid a lot of confusion.

Bjorn

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Tue, Aug 06, 2013 at 07:39:29AM -0600, Bjorn Helgaas wrote:
>> Personal Conclusion
>> ============================================================================
>> I think Bjorn's patch works.
>> Will test on powernv platform and give the result.
>
>Great, let me know what happens.

In both case, with/with out your patch, the assignment result is the same.
Below is the /proc/ioports file.

00010000-0080ffff : /io-hub@3efe00000000/pciex@3efe00080000
  00010000-00010fff : Legacy IO
  00020000-0003ffff : PCI Bus 0000:01
    00020000-0003ffff : PCI Bus 0000:02
      00020000-0002ffff : PCI Bus 0000:40
      00030000-0003ffff : PCI Bus 0000:a0
00820000-0101ffff : /io-hub@3efe00000000/pciex@3efe00090000
  00820000-00820fff : Legacy IO
  00830000-0083ffff : PCI Bus 0001:01
01030000-0182ffff : /io-hub@3efe00000000/pciex@3efe000a0000
  01030000-01030fff : Legacy IO
  01040000-0104ffff : PCI Bus 0002:01
01840000-0203ffff : /io-hub@3efe00000000/pciex@3efe000b0000
  01840000-01840fff : Legacy IO
  01850000-0185ffff : PCI Bus 0003:01
02050000-0284ffff : /io-hub@3efe00000000/pciex@3efe000c0000
  02050000-02050fff : Legacy IO
  02060000-0206ffff : PCI Bus 0004:01
02860000-0305ffff : /io-hub@3efe00000000/pciex@3efe000d0000
  02860000-02860fff : Legacy IO
  02870000-0287ffff : PCI Bus 0005:01

The arch_align is 0x10000. When sizing for Bus 0000:01, its child has a I/O
bridge window with size 0x20000/alignment 0x10000. And the result
[0x20000,0x3ffff] looks like size 0x20000/alignment 0x20000 or size
0x20000/alignment 0x10000. 

But I still think the alignment is 0x10000.

>
>Bjorn
>--
>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

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Tue, Aug 06, 2013 at 07:44:21AM -0600, Bjorn Helgaas wrote:
>On Tue, Aug 06, 2013 at 02:19:24PM +0800, Wei Yang wrote:
>> On Mon, Aug 05, 2013 at 11:58:50AM -0600, Bjorn Helgaas wrote:
>> >[+cc Yinghai]
>> >
>> >On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> >> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>> >> it introduce a new method to calculate the window alignment of P2P bridge.
>> >>
>> >> When the io_window_1k is set,  the calculation for the io resource alignment
>> >> is different from the original one. In the original logic before 462d9303,
>> >> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>> >> introduced in 462d9303 will limit the alignment to 1k in this case.
>> >>
>> >> This patch fix this issue.
>> >
>> >Presumably this fixes a bug, but you don't say what it is.  "different
>> >from the original" is not a description of a problem.  You need
>> >something like "with the current code, we allocate the wrong window
>> >size in situation X, or we allocate a window with incorrect alignment
>> >in situation Y, etc."
>> >
>> 
>> With current code, we allocate the wrong window size when upstream bridge
>> could be 1k aligned and one of the downstream port is 4k aligned.
>> 
>> In this case, the "min_align" should be 4k. But the current code set
>> "min_align" to 1k.
>

Hmm... sorry I should say. 

With current code, we allocate the wrong window size and alignment when upstream 
bridge could be 1k aligned and one of the downstream port is 4k aligned.

In this case, the "min_align" should be 4k. But the current code set
"min_align" to 1k.

>I would expect that we allocate a window with incorrect *alignment*,
>not incorrect size.  If you include a dmesg log and lspci output and point
>out the problem, that will avoid a lot of confusion.

Agree. 

I notice this potential problem during the code reading and found
the io_window_1k is set only for Intel P64H2 bridge. Currently I don't have
this device, not sure how to create this senario.

Sorry for the confusion, I should state the problem more clearly.

>
>Bjorn

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 11:34:10PM +0800, Wei Yang wrote:
> On Tue, Aug 06, 2013 at 07:39:29AM -0600, Bjorn Helgaas wrote:
> >> Personal Conclusion
> >> ============================================================================
> >> I think Bjorn's patch works.
> >> Will test on powernv platform and give the result.
> >
> >Great, let me know what happens.
> 
> In both case, with/with out your patch, the assignment result is the same.
> Below is the /proc/ioports file.
> 
> 00010000-0080ffff : /io-hub@3efe00000000/pciex@3efe00080000
>   00010000-00010fff : Legacy IO
>   00020000-0003ffff : PCI Bus 0000:01
>     00020000-0003ffff : PCI Bus 0000:02
>       00020000-0002ffff : PCI Bus 0000:40
>       00030000-0003ffff : PCI Bus 0000:a0
> 00820000-0101ffff : /io-hub@3efe00000000/pciex@3efe00090000
>   00820000-00820fff : Legacy IO
>   00830000-0083ffff : PCI Bus 0001:01
> 01030000-0182ffff : /io-hub@3efe00000000/pciex@3efe000a0000
>   01030000-01030fff : Legacy IO
>   01040000-0104ffff : PCI Bus 0002:01
> 01840000-0203ffff : /io-hub@3efe00000000/pciex@3efe000b0000
>   01840000-01840fff : Legacy IO
>   01850000-0185ffff : PCI Bus 0003:01
> 02050000-0284ffff : /io-hub@3efe00000000/pciex@3efe000c0000
>   02050000-02050fff : Legacy IO
>   02060000-0206ffff : PCI Bus 0004:01
> 02860000-0305ffff : /io-hub@3efe00000000/pciex@3efe000d0000
>   02860000-02860fff : Legacy IO
>   02870000-0287ffff : PCI Bus 0005:01
> 
> The arch_align is 0x10000. When sizing for Bus 0000:01, its child has a I/O
> bridge window with size 0x20000/alignment 0x10000. And the result
> [0x20000,0x3ffff] looks like size 0x20000/alignment 0x20000 or size
> 0x20000/alignment 0x10000. 

Obviously, an address with alignment of 0x20000 is *also* aligned to
0x10000.

> But I still think the alignment is 0x10000.

Yes, as it should be.  These are all bridge windows, which never have to be
aligned at more than 1K (if supported), 4K, or the arch alignment,
whichever is largest.

I *think* you're saying that the patch works correctly.

Bjorn

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 11:47:42PM +0800, Wei Yang wrote:
> On Tue, Aug 06, 2013 at 07:44:21AM -0600, Bjorn Helgaas wrote:
> >On Tue, Aug 06, 2013 at 02:19:24PM +0800, Wei Yang wrote:
> >> On Mon, Aug 05, 2013 at 11:58:50AM -0600, Bjorn Helgaas wrote:
> >> >[+cc Yinghai]
> >> >
> >> >On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> >> >> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> >> >> it introduce a new method to calculate the window alignment of P2P bridge.
> >> >>
> >> >> When the io_window_1k is set,  the calculation for the io resource alignment
> >> >> is different from the original one. In the original logic before 462d9303,
> >> >> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> >> >> introduced in 462d9303 will limit the alignment to 1k in this case.
> >> >>
> >> >> This patch fix this issue.
> >> >
> >> >Presumably this fixes a bug, but you don't say what it is.  "different
> >> >from the original" is not a description of a problem.  You need
> >> >something like "with the current code, we allocate the wrong window
> >> >size in situation X, or we allocate a window with incorrect alignment
> >> >in situation Y, etc."
> >> >
> >> 
> >> With current code, we allocate the wrong window size when upstream bridge
> >> could be 1k aligned and one of the downstream port is 4k aligned.
> >> 
> >> In this case, the "min_align" should be 4k. But the current code set
> >> "min_align" to 1k.
> >
> 
> Hmm... sorry I should say. 
> 
> With current code, we allocate the wrong window size and alignment when upstream 
> bridge could be 1k aligned and one of the downstream port is 4k aligned.
> 
> In this case, the "min_align" should be 4k. But the current code set
> "min_align" to 1k.

Actually, I think only the alignment is wrong (not the size).  But I do
agree that this looks like a problem in the current code.  I'll write
this up and post the patch soon.

Bjorn

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Bjorn Helgaas]

On Tue, Aug 06, 2013 at 12:01:55PM -0600, Bjorn Helgaas wrote:
> On Tue, Aug 06, 2013 at 11:47:42PM +0800, Wei Yang wrote:
> > On Tue, Aug 06, 2013 at 07:44:21AM -0600, Bjorn Helgaas wrote:
> > >On Tue, Aug 06, 2013 at 02:19:24PM +0800, Wei Yang wrote:
> > >> On Mon, Aug 05, 2013 at 11:58:50AM -0600, Bjorn Helgaas wrote:
> > >> >[+cc Yinghai]
> > >> >
> > >> >On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> > >> >> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
> > >> >> it introduce a new method to calculate the window alignment of P2P bridge.
> > >> >>
> > >> >> When the io_window_1k is set,  the calculation for the io resource alignment
> > >> >> is different from the original one. In the original logic before 462d9303,
> > >> >> the alignment is no bigger than 4K even the io_window_1k is set. The logic
> > >> >> introduced in 462d9303 will limit the alignment to 1k in this case.
> > >> >>
> > >> >> This patch fix this issue.
> > >> >
> > >> >Presumably this fixes a bug, but you don't say what it is.  "different
> > >> >from the original" is not a description of a problem.  You need
> > >> >something like "with the current code, we allocate the wrong window
> > >> >size in situation X, or we allocate a window with incorrect alignment
> > >> >in situation Y, etc."
> > >> >
> > >> 
> > >> With current code, we allocate the wrong window size when upstream bridge
> > >> could be 1k aligned and one of the downstream port is 4k aligned.
> > >> 
> > >> In this case, the "min_align" should be 4k. But the current code set
> > >> "min_align" to 1k.
> > >
> > 
> > Hmm... sorry I should say. 
> > 
> > With current code, we allocate the wrong window size and alignment when upstream 
> > bridge could be 1k aligned and one of the downstream port is 4k aligned.
> > 
> > In this case, the "min_align" should be 4k. But the current code set
> > "min_align" to 1k.
> 
> Actually, I think only the alignment is wrong (not the size).  But I do
> agree that this looks like a problem in the current code.  I'll write
> this up and post the patch soon.

Here's the patch I propose.  The code change is the same as I posted
yesterday; only the changelog is new.  I'll put these in next pending
comments.

Bjorn


commit 2d1d66780ecd12c9518835303f5302fc5262d49b
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Mon Aug 5 16:15:10 2013 -0600

    PCI: Align bridge I/O windows as required by downstream devices & bridges
    
    An upstream bridge's I/O window must be at least as aligned as any
    downstream device or bridge requires.  In particular, if the upstream
    bridge supports 1K alignment but a downstream bridge requires 4K alignment,
    the upstream window must also be 4K aligned.
    
    Therefore, do not reduce the required alignment ("min_align") based on
    the upstream bridge's capabilities.
    
    Reported-by: Wei Yang <weiyang@linux.vnet.ibm.com>
    Suggested-by: Yinghai Lu <yinghai@kernel.org>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d4f1ad9..8333c92 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -749,12 +749,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
 	resource_size_t size = 0, size0 = 0, size1 = 0;
 	resource_size_t children_add_size = 0;
-	resource_size_t min_align, io_align, align;
+	resource_size_t min_align, align;
 
 	if (!b_res)
  		return;
 
-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
+	min_align = window_alignment(bus, IORESOURCE_IO);
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		int i;
 
@@ -781,9 +781,6 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		}
 	}
 
-	if (min_align > io_align)
-		min_align = io_align;
-
 	size0 = calculate_iosize(size, min_size, size1,
 			resource_size(b_res), min_align);
 	if (children_add_size > add_size)

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Tue, Aug 06, 2013 at 11:58:56AM -0600, Bjorn Helgaas wrote:
>On Tue, Aug 06, 2013 at 11:34:10PM +0800, Wei Yang wrote:
>> On Tue, Aug 06, 2013 at 07:39:29AM -0600, Bjorn Helgaas wrote:
>> >> Personal Conclusion
>> >> ============================================================================
>> >> I think Bjorn's patch works.
>> >> Will test on powernv platform and give the result.
>> >
>> >Great, let me know what happens.
>> 
>> In both case, with/with out your patch, the assignment result is the same.
>> Below is the /proc/ioports file.
>> 
>> 00010000-0080ffff : /io-hub@3efe00000000/pciex@3efe00080000
>>   00010000-00010fff : Legacy IO
>>   00020000-0003ffff : PCI Bus 0000:01
>>     00020000-0003ffff : PCI Bus 0000:02
>>       00020000-0002ffff : PCI Bus 0000:40
>>       00030000-0003ffff : PCI Bus 0000:a0
>> 00820000-0101ffff : /io-hub@3efe00000000/pciex@3efe00090000
>>   00820000-00820fff : Legacy IO
>>   00830000-0083ffff : PCI Bus 0001:01
>> 01030000-0182ffff : /io-hub@3efe00000000/pciex@3efe000a0000
>>   01030000-01030fff : Legacy IO
>>   01040000-0104ffff : PCI Bus 0002:01
>> 01840000-0203ffff : /io-hub@3efe00000000/pciex@3efe000b0000
>>   01840000-01840fff : Legacy IO
>>   01850000-0185ffff : PCI Bus 0003:01
>> 02050000-0284ffff : /io-hub@3efe00000000/pciex@3efe000c0000
>>   02050000-02050fff : Legacy IO
>>   02060000-0206ffff : PCI Bus 0004:01
>> 02860000-0305ffff : /io-hub@3efe00000000/pciex@3efe000d0000
>>   02860000-02860fff : Legacy IO
>>   02870000-0287ffff : PCI Bus 0005:01
>> 
>> The arch_align is 0x10000. When sizing for Bus 0000:01, its child has a I/O
>> bridge window with size 0x20000/alignment 0x10000. And the result
>> [0x20000,0x3ffff] looks like size 0x20000/alignment 0x20000 or size
>> 0x20000/alignment 0x10000. 
>
>Obviously, an address with alignment of 0x20000 is *also* aligned to
>0x10000.
>
>> But I still think the alignment is 0x10000.
>
>Yes, as it should be.  These are all bridge windows, which never have to be
>aligned at more than 1K (if supported), 4K, or the arch alignment,
>whichever is largest.

I add some printk which shows the alignment is 0x10000 instead of 0x20000.

>
>I *think* you're saying that the patch works correctly.

Yes.

>
>Bjorn

-- 
Richard Yang
Help you, Help me

Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io()

[Wei Yang]

On Tue, Aug 06, 2013 at 02:56:46PM -0600, Bjorn Helgaas wrote:
>On Tue, Aug 06, 2013 at 12:01:55PM -0600, Bjorn Helgaas wrote:
>> On Tue, Aug 06, 2013 at 11:47:42PM +0800, Wei Yang wrote:
>> > On Tue, Aug 06, 2013 at 07:44:21AM -0600, Bjorn Helgaas wrote:
>> > >On Tue, Aug 06, 2013 at 02:19:24PM +0800, Wei Yang wrote:
>> > >> On Mon, Aug 05, 2013 at 11:58:50AM -0600, Bjorn Helgaas wrote:
>> > >> >[+cc Yinghai]
>> > >> >
>> > >> >On Fri, Aug 2, 2013 at 3:31 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> > >> >> In commit 462d9303 ("PCI: Align P2P windows using pcibios_window_alignment()"),
>> > >> >> it introduce a new method to calculate the window alignment of P2P bridge.
>> > >> >>
>> > >> >> When the io_window_1k is set,  the calculation for the io resource alignment
>> > >> >> is different from the original one. In the original logic before 462d9303,
>> > >> >> the alignment is no bigger than 4K even the io_window_1k is set. The logic
>> > >> >> introduced in 462d9303 will limit the alignment to 1k in this case.
>> > >> >>
>> > >> >> This patch fix this issue.
>> > >> >
>> > >> >Presumably this fixes a bug, but you don't say what it is.  "different
>> > >> >from the original" is not a description of a problem.  You need
>> > >> >something like "with the current code, we allocate the wrong window
>> > >> >size in situation X, or we allocate a window with incorrect alignment
>> > >> >in situation Y, etc."
>> > >> >
>> > >> 
>> > >> With current code, we allocate the wrong window size when upstream bridge
>> > >> could be 1k aligned and one of the downstream port is 4k aligned.
>> > >> 
>> > >> In this case, the "min_align" should be 4k. But the current code set
>> > >> "min_align" to 1k.
>> > >
>> > 
>> > Hmm... sorry I should say. 
>> > 
>> > With current code, we allocate the wrong window size and alignment when upstream 
>> > bridge could be 1k aligned and one of the downstream port is 4k aligned.
>> > 
>> > In this case, the "min_align" should be 4k. But the current code set
>> > "min_align" to 1k.
>> 
>> Actually, I think only the alignment is wrong (not the size).  But I do
>> agree that this looks like a problem in the current code.  I'll write
>> this up and post the patch soon.
>
>Here's the patch I propose.  The code change is the same as I posted
>yesterday; only the changelog is new.  I'll put these in next pending
>comments.
>
>Bjorn
>
>
>commit 2d1d66780ecd12c9518835303f5302fc5262d49b
>Author: Bjorn Helgaas <bhelgaas@google.com>
>Date:   Mon Aug 5 16:15:10 2013 -0600
>
>    PCI: Align bridge I/O windows as required by downstream devices & bridges
>    
>    An upstream bridge's I/O window must be at least as aligned as any
>    downstream device or bridge requires.  In particular, if the upstream
>    bridge supports 1K alignment but a downstream bridge requires 4K alignment,
>    the upstream window must also be 4K aligned.
>    
>    Therefore, do not reduce the required alignment ("min_align") based on
>    the upstream bridge's capabilities.
>    
>    Reported-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>    Suggested-by: Yinghai Lu <yinghai@kernel.org>
>    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>index d4f1ad9..8333c92 100644
>--- a/drivers/pci/setup-bus.c
>+++ b/drivers/pci/setup-bus.c
>@@ -749,12 +749,12 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> 	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
> 	resource_size_t size = 0, size0 = 0, size1 = 0;
> 	resource_size_t children_add_size = 0;
>-	resource_size_t min_align, io_align, align;
>+	resource_size_t min_align, align;
>
> 	if (!b_res)
>  		return;
>
>-	io_align = min_align = window_alignment(bus, IORESOURCE_IO);
>+	min_align = window_alignment(bus, IORESOURCE_IO);
> 	list_for_each_entry(dev, &bus->devices, bus_list) {
> 		int i;
>
>@@ -781,9 +781,6 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
> 		}
> 	}
>
>-	if (min_align > io_align)
>-		min_align = io_align;
>-
> 	size0 = calculate_iosize(size, min_size, size1,
> 			resource_size(b_res), min_align);
> 	if (children_add_size > add_size)

This looks good to me.

-- 
Richard Yang
Help you, Help me