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

[Bjorn Helgaas <bhelgaas@google.com>]

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
>