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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>>>> Reviewed-by: Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx>
>>>>> Reviewed-by: Ram Pai <linuxram@xxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxxxxxxx
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>--
>>>>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>>>>the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> 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@xxxxxxxxxxxxxxx
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>--
>Richard Yang
>Help you, Help me
--
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
