From patchwork Sat May 7 01:52:40 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ram Pai X-Patchwork-Id: 763812 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter2.kernel.org (8.14.4/8.14.3) with ESMTP id p471qlB1021174 for ; Sat, 7 May 2011 01:52:47 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753608Ab1EGBwq (ORCPT ); Fri, 6 May 2011 21:52:46 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:56897 "EHLO e3.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752265Ab1EGBwp (ORCPT ); Fri, 6 May 2011 21:52:45 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by e3.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p471VAtI019907; Fri, 6 May 2011 21:31:10 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p471qi4X106924; Fri, 6 May 2011 21:52:44 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p471qi35000594; Fri, 6 May 2011 22:52:44 -0300 Received: from us.ibm.com (sig-9-65-50-73.mts.ibm.com [9.65.50.73]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p471qgeH000522; Fri, 6 May 2011 22:52:42 -0300 Received: by us.ibm.com (sSMTP sendmail emulation); Fri, 06 May 2011 18:52:40 -0700 Date: Fri, 6 May 2011 18:52:40 -0700 From: Ram Pai To: Yinghai Lu Cc: Ram Pai , Jesse Barnes , "linux-kernel@vger.kernel.org" , "linux-pci@vger.kernel.org" Subject: Re: [PATCH] pci: Check bridge resources after resource allocation. Message-ID: <20110507015240.GN16782@ram-laptop> Reply-To: Ram Pai References: <4DC250A6.9040802@kernel.org> <20110506081243.GF16782@ram-laptop> <4DC4586C.6020308@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4DC4586C.6020308@kernel.org> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Sat, 07 May 2011 01:52:47 +0000 (UTC) On Fri, May 06, 2011 at 01:22:04PM -0700, Yinghai Lu wrote: > On 05/06/2011 01:12 AM, Ram Pai wrote: > > On Thu, May 05, 2011 at 12:24:22AM -0700, Yinghai Lu wrote: > >> > >> During pci remove/rescan testing found: > >> > >> [ 541.141614] pci 0000:c0:03.0: PCI bridge to [bus c4-c9] > >> [ 541.141965] pci 0000:c0:03.0: bridge window [io 0x1000-0x0fff] > >> [ 541.159181] pci 0000:c0:03.0: bridge window [mem 0xf0000000-0xf00fffff] > >> [ 541.159540] pci 0000:c0:03.0: bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref] > >> [ 541.179374] pci 0000:c0:03.0: device not available (can't reserve [io 0x1000-0x0fff]) > >> [ 541.199198] pci 0000:c0:03.0: Error enabling bridge (-22), continuing > >> [ 541.199202] pci 0000:c0:03.0: enabling bus mastering > >> [ 541.199209] pci 0000:c0:03.0: setting latency timer to 64 > >> [ 541.199917] pcieport 0000:c0:03.0: device not available (can't reserve [io 0x1000-0x0fff]) > >> [ 541.199963] pcieport: probe of 0000:c0:03.0 failed with error -22 > >> > >> This bug was uncovered by commit > >> | commit c8adf9a3e873eddaaec11ac410a99ef6b9656938 > >> | Author: Ram Pai > >> | Date: Mon Feb 14 17:43:20 2011 -0800 > >> | > >> | PCI: pre-allocate additional resources to devices only after successful allo > >> cation of essential resources. > >> > >> After that commit, pci_hotplug_io_size is changed to additional_io_size from minium size. So it will not get into failed list, and will not be reset there. > >> > >> The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci > >> bridge, and later if children does not need to IO resource. those bridge > >> resources will not need to be allocated. but flags still there. > >> > >> Add pci_bridge_check_resources() to close the loop. > > > > How about resetting the resource in adjust_resources_sorted() if call to > > adjust_resource() fails, and the resource is left with zero size? > > > > Something like this: > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index 8e73abf..77e8454 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -157,8 +157,11 @@ static void adjust_resources_sorted(struct resource_list_x *add_head, > > if(pci_assign_resource(list->dev, idx)) > > reset_resource(res); > > } else if (add_size) { > > - adjust_resource(res, res->start, > > - resource_size(res) + add_size); > > + if (adjust_resource(res, res->start, > > + resource_size(res) + add_size) && > > + !resource_size(res)) { > > + reset_resource(res); > > + } > > } > > out: > > tmp = list; > > > > > > no, adjust_resource will not change resource size to zero. Ok. There are four scenarios that adjust_resources_sorted() gets called, three of which are handled properly and one case is not handled at all. I think you are running against the one that is not handled. So here are the 4 scenarios: a. non-hotplug resource having children resources b. non-hotplug resource without children resources c. hotplug resource with children resources d. hotplug resource without children resources. case (a): resource is in head but not in add_list -- adjust_resource_sorted() has nothing to do. Currently this case is handled properly. case (b): resource is not in head and not in add_list -- adjust_resource_sorted() has nothing to do. Currently this case is handled properly. case (c): resource is in head and in add_list -- adjust_resource_sorted() extends the size of the resource. And if it fails to do so, the failure is ignored. Currently this case is handled properly. case (d): resource is not in head but in add_list -- adjust_resource_sorted() is not handling this case properly. If the reason the resource is not in head is because its allocation failed, then it must be in the failed list. Since it is in the failed list, adjust_resource_sorted() has nothing to do. However if the resource is not in the head because it had no child resources to begin with, then adjust_resource_sorted() has to increase the size of the resource, in order to satisfy any future hotplug requests. If it unable to increase the size then it has to reset the resource, which means reset its flags too. We need code that handles this case, which should solve your problem. Here is the code, I think, should take care of your problem. Let me know if it works. RP --- 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 diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 8e73abf..56ec6f7 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -118,19 +118,18 @@ static inline void reset_resource(struct resource *res) * * @add_head : head of the list tracking requests requiring additional * resources - * @head : head of the list tracking requests with allocated - * resources + * @fail_head: head of the list tracking requests with failed + * resource allocation * * Walk through each element of the add_head and try to procure * additional resources for the element, provided the element - * is in the head list. + * is not in the failed list. */ static void adjust_resources_sorted(struct resource_list_x *add_head, - struct resource_list *head) + struct resource_list_x *fail_head) { struct resource *res; - struct resource_list_x *list, *tmp, *prev; - struct resource_list *hlist; + struct resource_list_x *list, *tmp, *prev, *f_list; resource_size_t add_size; int idx; @@ -141,10 +140,10 @@ static void adjust_resources_sorted(struct resource_list_x *add_head, if (!res->flags) goto out; - /* skip this resource if not found in head list */ - for (hlist = head->next; hlist && hlist->res != res; - hlist = hlist->next); - if (!hlist) { /* just skip */ + /* skip this resource if found in failed list */ + for (f_list = fail_head->next; f_list && f_list->res != res; + f_list = f_list->next); + if (f_list) { prev = list; list = list->next; continue; @@ -212,7 +211,7 @@ static void __assign_resources_sorted(struct resource_list *head, /* Try to satisfy any additional nice-to-have resource requests */ if (add_head) - adjust_resources_sorted(add_head, head); + adjust_resources_sorted(add_head, fail_head); free_list(resource_list, head); }