| Message ID | 20110507015240.GN16782@ram-laptop (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 05/06/2011 06:52 PM, Ram Pai wrote: > 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 <linuxram@us.ibm.com> >>>> | 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. > > > 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; No, should not change to fail_head here. if one resource is in fail_head, that means that it is get reset already. so !res->flags will be meet and get out ... Thanks Yinghai > > - /* 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); > } > > > 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); }