From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e28smtp03.in.ibm.com ([122.248.162.3]:35729 "EHLO e28smtp03.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752372Ab3GJBeu (ORCPT ); Tue, 9 Jul 2013 21:34:50 -0400 Received: from /spool/local by e28smtp03.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 Jul 2013 06:58:21 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id 64A46E0055 for ; Wed, 10 Jul 2013 07:04:31 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6A1Yeb55177500 for ; Wed, 10 Jul 2013 07:04:40 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6A1Yhs2016472 for ; Wed, 10 Jul 2013 11:34:44 +1000 Date: Wed, 10 Jul 2013 09:34:42 +0800 From: Wei Yang To: Bjorn Helgaas Cc: "linux-pci@vger.kernel.org" , Ram Pai , Gavin Shan Subject: Re: [PATCH 4/4] PCI: fix the io resource alignment calculation in pbus_size_io() Message-ID: <20130710013442.GA6671@weiyang.vnet.ibm.com> Reply-To: Wei Yang References: <1372691432-6440-1-git-send-email-weiyang@linux.vnet.ibm.com> <1372691432-6440-5-git-send-email-weiyang@linux.vnet.ibm.com> <20130709032019.GC7094@weiyang.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-pci-owner@vger.kernel.org List-ID: On Tue, Jul 09, 2013 at 11:38:06AM -0600, Bjorn Helgaas wrote: >On Mon, Jul 8, 2013 at 9:20 PM, Wei Yang 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 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 >>>> Reviewed-by: Gavin Shan >>>> Reviewed-by: Ram Pai >>>> --- >>>> 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 -- Richard Yang Help you, Help me