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

login
register
mail settings
Submitter Wei Yang
Date July 1, 2013, 3:10 p.m.
Message ID <1372691432-6440-5-git-send-email-weiyang@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/256158/
State Changes Requested
Headers show

Comments

Wei Yang - July 1, 2013, 3:10 p.m.
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.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
---
 drivers/pci/setup-bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Bjorn Helgaas - July 8, 2013, 9:15 p.m.
On Mon, Jul 1, 2013 at 9:10 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.
>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> Reviewed-by: Ram Pai <linuxram@us.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 bd0ce39d..5c60ca0 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;

Please explain why we need this change, with some actual values that
show the problem.  We need to know what the problem is, not merely
that the code behaves differently than it did before 462d9303.

It appears to me that this change will break the ability to use 1K
windows.  For example, assume a bridge that supports 1K windows.
Assume we're using the default pcibios_window_alignment().  Currently
window_alignment() on the secondary bus returns
PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.

With your change, I think io_align will be bumped back up to 4K in
this case, so we'll lose the ability to allocate a 1K window.

>         list_for_each_entry(dev, &bus->devices, bus_list) {
>                 int i;
>
> --
> 1.7.5.4
>
> --
> 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
--
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
Wei Yang - July 9, 2013, 3:20 a.m.
On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>On Mon, Jul 1, 2013 at 9:10 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.
>>
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>> Reviewed-by: Ram Pai <linuxram@us.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 bd0ce39d..5c60ca0 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;
>
>Please explain why we need this change, with some actual values that
>show the problem.  We need to know what the problem is, not merely
>that the code behaves differently than it did before 462d9303.

Yep, sorry for not listing the exact problem value.

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

In this case, with 462d9303 the min_align will be set back to 1k even one of
the child require 2k alignment.

>
>It appears to me that this change will break the ability to use 1K
>windows.  For example, assume a bridge that supports 1K windows.
>Assume we're using the default pcibios_window_alignment().  Currently
>window_alignment() on the secondary bus returns
>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>
>With your change, I think io_align will be bumped back up to 4K in
>this case, so we'll lose the ability to allocate a 1K window.

After applying the change:

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: 

                    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

With this patch, in the same case as above, the min_align is 2k after
calculation.

In my mind, the min_align is the lower bound, io_align is the upper bound. The
final result of min_align should be in this range.

Is my understanding correct? or I missed something important?

>
>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>                 int i;
>>
>> --
>> 1.7.5.4
>>
>> --
>> 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
>--
>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
Bjorn Helgaas - July 9, 2013, 5:38 p.m.
On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>>On Mon, Jul 1, 2013 at 9:10 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.
>>>
>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> Reviewed-by: Ram Pai <linuxram@us.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 bd0ce39d..5c60ca0 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;
>>
>>Please explain why we need this change, with some actual values that
>>show the problem.  We need to know what the problem is, not merely
>>that the code behaves differently than it did before 462d9303.
>
> Yep, sorry for not listing the exact problem value.
>
> 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
>
> In this case, with 462d9303 the min_align will be set back to 1k even one of
> the child require 2k alignment.
>
>>
>>It appears to me that this change will break the ability to use 1K
>>windows.  For example, assume a bridge that supports 1K windows.
>>Assume we're using the default pcibios_window_alignment().  Currently
>>window_alignment() on the secondary bus returns
>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>>
>>With your change, I think io_align will be bumped back up to 4K in
>>this case, so we'll lose the ability to allocate a 1K window.
>
> After applying the change:
>
> 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.

What happens if no child has an I/O resource larger than 1K?  Can we
allocate a 1K window with 1K alignment in that case?

> 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
>
> With this patch, in the same case as above, the min_align is 2k after
> calculation.
>
> In my mind, the min_align is the lower bound, io_align is the upper bound. The
> final result of min_align should be in this range.
>
> Is my understanding correct? or I missed something important?
>
>>
>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>>                 int i;
>>>
>>> --
>>> 1.7.5.4
>>>
>>> --
>>> 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
>>--
>>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
>
--
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
Wei Yang - July 10, 2013, 1:34 a.m.
On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote:
>On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>>>On Mon, Jul 1, 2013 at 9:10 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.
>>>>
>>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>> Reviewed-by: Ram Pai <linuxram@us.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 bd0ce39d..5c60ca0 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;
>>>
>>>Please explain why we need this change, with some actual values that
>>>show the problem.  We need to know what the problem is, not merely
>>>that the code behaves differently than it did before 462d9303.
>>
>> Yep, sorry for not listing the exact problem value.
>>
>> 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
>>
>> In this case, with 462d9303 the min_align will be set back to 1k even one of
>> the child require 2k alignment.
>>
>>>
>>>It appears to me that this change will break the ability to use 1K
>>>windows.  For example, assume a bridge that supports 1K windows.
>>>Assume we're using the default pcibios_window_alignment().  Currently
>>>window_alignment() on the secondary bus returns
>>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>>>
>>>With your change, I think io_align will be bumped back up to 4K in
>>>this case, so we'll lose the ability to allocate a 1K window.
>>
>> After applying the change:
>>
>> 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.
>
>What happens if no child has an I/O resource larger than 1K?  Can we
>allocate a 1K window with 1K alignment in that case?
>

Yes, it could. The result comparison would look like this.
Since no child has an I/O resource larger than 1k, the min_align will remain
1k after loop. And because io_align(4K) is larger than min_align(1k), the
final min_align would be 1k.

In this case, the code from commit 462d9303 and my patch both works.

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

>> 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                          4k
>>
>> With this patch, in the same case as above, the min_align is 2k after
>> calculation.
>>
>> In my mind, the min_align is the lower bound, io_align is the upper bound. The
>> final result of min_align should be in this range.
>>
>> Is my understanding correct? or I missed something important?
>>
>>>
>>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>>>                 int i;
>>>>
>>>> --
>>>> 1.7.5.4
>>>>
>>>> --
>>>> 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
>>>--
>>>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
>>
>--
>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
Wei Yang - July 19, 2013, 3:10 a.m.
On Wed, Jul 10, 2013 at 09:34:42AM +0800, Wei Yang wrote:
>On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote:
>>On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
>>>>On Mon, Jul 1, 2013 at 9:10 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.
>>>>>
>>>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>>>> Reviewed-by: Ram Pai <linuxram@us.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 bd0ce39d..5c60ca0 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;
>>>>
>>>>Please explain why we need this change, with some actual values that
>>>>show the problem.  We need to know what the problem is, not merely
>>>>that the code behaves differently than it did before 462d9303.
>>>
>>> Yep, sorry for not listing the exact problem value.
>>>
>>> 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
>>>
>>> In this case, with 462d9303 the min_align will be set back to 1k even one of
>>> the child require 2k alignment.
>>>
>>>>
>>>>It appears to me that this change will break the ability to use 1K
>>>>windows.  For example, assume a bridge that supports 1K windows.
>>>>Assume we're using the default pcibios_window_alignment().  Currently
>>>>window_alignment() on the secondary bus returns
>>>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
>>>>
>>>>With your change, I think io_align will be bumped back up to 4K in
>>>>this case, so we'll lose the ability to allocate a 1K window.
>>>
>>> After applying the change:
>>>
>>> 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.
>>
>>What happens if no child has an I/O resource larger than 1K?  Can we
>>allocate a 1K window with 1K alignment in that case?
>>
>
>Yes, it could. The result comparison would look like this.
>Since no child has an I/O resource larger than 1k, the min_align will remain
>1k after loop. And because io_align(4K) is larger than min_align(1k), the
>final min_align would be 1k.
>
>In this case, the code from commit 462d9303 and my patch both works.
>
> Result comparison:
>                     with  462d9303             with this patch
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                              after loop |
>                                         V
>     min_align       1k                          1k
>     io_align        1k                          4k
>                                         |
>                          check boundary |
>                                         V
>     min_align       1k                          1k
>     io_align        1k                          4k
>

Bjorn,

Sorry for distubing you again.

Is my analysis correct or I still miss some point?

>>> 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                          4k
>>>
>>> With this patch, in the same case as above, the min_align is 2k after
>>> calculation.
>>>
>>> In my mind, the min_align is the lower bound, io_align is the upper bound. The
>>> final result of min_align should be in this range.
>>>
>>> Is my understanding correct? or I missed something important?
>>>
>>>>
>>>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
>>>>>                 int i;
>>>>>
>>>>> --
>>>>> 1.7.5.4
>>>>>
>>>>> --
>>>>> 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
>>>>--
>>>>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
>>>
>>--
>>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
Bjorn Helgaas - July 25, 2013, 9:22 p.m.
On Wed, Jul 10, 2013 at 09:34:42AM +0800, Wei Yang wrote:
> On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote:
> >On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> >> On Mon, Jul 08, 2013 at 03:15:13PM -0600, Bjorn Helgaas wrote:
> >>>On Mon, Jul 1, 2013 at 9:10 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.
> >>>>
> >>>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> >>>> Reviewed-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> >>>> Reviewed-by: Ram Pai <linuxram@us.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 bd0ce39d..5c60ca0 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;
> >>>
> >>>Please explain why we need this change, with some actual values that
> >>>show the problem.  We need to know what the problem is, not merely
> >>>that the code behaves differently than it did before 462d9303.
> >>
> >> Yep, sorry for not listing the exact problem value.
> >>
> >> 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
> >>
> >> In this case, with 462d9303 the min_align will be set back to 1k even one of
> >> the child require 2k alignment.
> >>
> >>>
> >>>It appears to me that this change will break the ability to use 1K
> >>>windows.  For example, assume a bridge that supports 1K windows.
> >>>Assume we're using the default pcibios_window_alignment().  Currently
> >>>window_alignment() on the secondary bus returns
> >>>PCI_P2P_DEFAULT_IO_ALIGN_1K (0x400, which is 1K), so io_align = 0x400.
> >>>
> >>>With your change, I think io_align will be bumped back up to 4K in
> >>>this case, so we'll lose the ability to allocate a 1K window.
> >>
> >> After applying the change:
> >>
> >> 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.
> >
> >What happens if no child has an I/O resource larger than 1K?  Can we
> >allocate a 1K window with 1K alignment in that case?
> >
> 
> Yes, it could. The result comparison would look like this.
> Since no child has an I/O resource larger than 1k, the min_align will remain
> 1k after loop. And because io_align(4K) is larger than min_align(1k), the
> final min_align would be 1k.
> 
> In this case, the code from commit 462d9303 and my patch both works.
> 
>  Result comparison:
>                      with  462d9303             with this patch
>      min_align       1k                          1k
>      io_align        1k                          4k
>                                          |
>                               after loop |
>                                          V
>      min_align       1k                          1k
>      io_align        1k                          4k
>                                          |
>                           check boundary |
>                                          V
>      min_align       1k                          1k
>      io_align        1k                          4k
> 
> >> 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                          4k
> >>
> >> With this patch, in the same case as above, the min_align is 2k after
> >> calculation.
> >>
> >> In my mind, the min_align is the lower bound, io_align is the upper bound. The
> >> final result of min_align should be in this range.
> >>
> >> Is my understanding correct? or I missed something important?

Since Gavin has reviewed this, I'm OK with it.  If you resend the series
with the updated changelogs and so on, I'll apply it.

Bjorn

> >>
> >>>
> >>>>         list_for_each_entry(dev, &bus->devices, bus_list) {
> >>>>                 int i;
> >>>>
> >>>> --
> >>>> 1.7.5.4
> >>>>
> >>>> --
> >>>> 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
> >>>--
> >>>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
> >>
> >--
> >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
> 
--
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

Patch

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index bd0ce39d..5c60ca0 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;