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

[Wei Yang <weiyang@linux.vnet.ibm.com>]

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