Re: [PATCH] pci: Check bridge resources after resource allocation.
From: Yinghai Lu
Date: Fri May 06 2011 - 22:37:32 EST
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@xxxxxxxxxx>
>>>> | 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/