diff mbox

[-v2] pci: Check bridge resources after resource allocation.

Message ID 4DCDD589.8010309@kernel.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Yinghai Lu May 14, 2011, 1:06 a.m. UTC 
On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> On Thu, 12 May 2011 12:18:43 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>
>>> Linus, I don't have anything else queued up, so you may as well take
>>> this one directly if you want it in 2.6.39.  It's a regression fix, but
>>> resource changes always make me nervous.  Alternately, I could put it
>>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
>>> 2.6.40-rc2 or so...
>>
>> Considering the trouble resource allocation always ends up being, I'd
>> almost prefer that "mark it for stable and put it in the 2.6.40
>> queue".
>>
>> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
> 
> Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
> (at least I don't know of any other reports).
> 

please check this one, it should be safe for 2.6.39 ?

Thanks

Yinghai

[PATCH -v3] PCI: Clear bridge resource flags if requested size is 0

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 caused 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 allocation of essential resources.

After that commit, pci_hotplug_io_size is changed to additional_io_size from
minium size.
So it will not go through resource_size(res) != 0 path,  and will not be reset.

The root cause is: pci_bridge_check_ranges will set RESOURCE_IO flag for pci
bridge, and later if children do not need IO resource. those bridge
resources will not need to be allocated. but flags is still there. that will
confuse the the pci_enable_bridges later.

related code:
| static void assign_requested_resources_sorted(struct resource_list *head,
|                                  struct resource_list_x *fail_head)
| {
|         struct resource *res;
|         struct resource_list *list;
|         int idx;
|
|         for (list = head->next; list; list = list->next) {
|                 res = list->res;
|                 idx = res - &list->dev->resource[0];
|                 if (resource_size(res) && pci_assign_resource(list->dev, idx)) {
| ...
|                         reset_resource(res);
|                 }
|         }
| }

At last, We have to clear the flags in pbus_size_mem/io when requested size == 0 and !add_head.
becasue this case it will not go through adjust_resources_sorted().

just make size1 = size0 when !add_head. it will make flags get cleared.

At the same time when requested size == 0, add_size != 0, will still have in head and add_list.
because we do not clear the flags for it.

after patch, will get right result:
[  621.206655] pci 0000:c0:03.0: PCI bridge to [bus c4-c9]
[  621.206912] pci 0000:c0:03.0:   bridge window [io  disabled]
[  621.226594] pci 0000:c0:03.0:   bridge window [mem 0xf0000000-0xf00fffff]
[  621.226904] pci 0000:c0:03.0:   bridge window [mem 0xfc180000000-0xfc197ffffff 64bit pref]
[  621.247012] pci 0000:c0:03.0: enabling bus mastering
[  621.247275] pci 0000:c0:03.0: setting latency timer to 64
[  621.267656] pcieport 0000:c0:03.0: setting latency timer to 64
[  621.268134] pcieport 0000:c0:03.0: irq 160 for MSI/MSI-X
[  621.286832] pcieport 0000:c0:03.0: Signaling PME through PCIe PME interrupt
[  621.306360] pci 0000:c4:00.0: Signaling PME through PCIe PME interrupt
[  621.306684] pcie_pme 0000:c0:03.0:pcie01: service driver pcie_pme loaded
[  621.326512] aer 0000:c0:03.0:pcie02: service driver aer loaded
[  621.326911] pciehp 0000:c0:03.0:pcie04: Hotplug Controller:

v3: more simple fix. also fix one typo in pbus_size_mem

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
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

Comments

Ram Pai May 16, 2011, 7:59 a.m. UTC | #1 
On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> > On Thu, 12 May 2011 12:18:43 -0700
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > 
> >> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>>
> >>> Linus, I don't have anything else queued up, so you may as well take
> >>> this one directly if you want it in 2.6.39.  It's a regression fix, but
> >>> resource changes always make me nervous.  Alternately, I could put it
> >>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
> >>> 2.6.40-rc2 or so...
> >>
> >> Considering the trouble resource allocation always ends up being, I'd
> >> almost prefer that "mark it for stable and put it in the 2.6.40
> >> queue".
> >>
> >> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
> > 
> > Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
> > (at least I don't know of any other reports).
> > 
> 
> please check this one, it should be safe for 2.6.39 ?

>  	size0 = calculate_iosize(size, min_size, size1,
>  			resource_size(b_res), 4096);
> -	size1 = !add_size? size0:
> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
>  		calculate_iosize(size, min_size+add_size, size1,
>  			resource_size(b_res), 4096);

This solves the problem you encountered.

But, I think, it still does not fix the following scenario:

adjust_resource() failing to allocate additional resource to a hotplug bridge
that has no children. In this case  ->flags of that 'struct resource'
continues to be set even when no resource is allocated to that hot-plug bridge.

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
Yinghai Lu May 16, 2011, 8:55 p.m. UTC | #2 
On 05/16/2011 12:59 AM, Ram Pai wrote:
> On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
>> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
>>> On Thu, 12 May 2011 12:18:43 -0700
>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>
>>>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>>>>
>>>>> Linus, I don't have anything else queued up, so you may as well take
>>>>> this one directly if you want it in 2.6.39.  It's a regression fix, but
>>>>> resource changes always make me nervous.  Alternately, I could put it
>>>>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
>>>>> 2.6.40-rc2 or so...
>>>>
>>>> Considering the trouble resource allocation always ends up being, I'd
>>>> almost prefer that "mark it for stable and put it in the 2.6.40
>>>> queue".
>>>>
>>>> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
>>>
>>> Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
>>> (at least I don't know of any other reports).
>>>
>>
>> please check this one, it should be safe for 2.6.39 ?
> 
>>  	size0 = calculate_iosize(size, min_size, size1,
>>  			resource_size(b_res), 4096);
>> -	size1 = !add_size? size0:
>> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
>>  		calculate_iosize(size, min_size+add_size, size1,
>>  			resource_size(b_res), 4096);
> 
> This solves the problem you encountered.
> 
> But, I think, it still does not fix the following scenario:
> 
> adjust_resource() failing to allocate additional resource to a hotplug bridge
> that has no children. In this case  ->flags of that 'struct resource'
> continues to be set even when no resource is allocated to that hot-plug bridge.
> 
that case: requested_size will be 0, but add_size will not be zero.

res->flags is not cleared in pbus_size_xx, so it will be put into head.
so it will go through first path.
...
                if (!resource_size(res) && add_size) {
                         res->end = res->start + add_size - 1;
                         if(pci_assign_resource(list->dev, idx))
                                reset_resource(res);
                } else if (add_size) {
                        adjust_resource(res, res->start,
                                resource_size(res) + add_size);
                }

and if it fails to get assign, the flags will get clear in reset_resource.

so it should be ok. and testing in one my setup show those flags get clear correctly and does not emit any warning.

Thanks

Yinghai
--
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
Ram Pai May 16, 2011, 10:36 p.m. UTC | #3 
On Mon, May 16, 2011 at 01:55:38PM -0700, Yinghai Lu wrote:
> On 05/16/2011 12:59 AM, Ram Pai wrote:
> > On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
> >> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> >>> On Thu, 12 May 2011 12:18:43 -0700
> >>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >>>
> >>>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >>>>>
> >>>>> Linus, I don't have anything else queued up, so you may as well take
> >>>>> this one directly if you want it in 2.6.39.  It's a regression fix, but
> >>>>> resource changes always make me nervous.  Alternately, I could put it
> >>>>> into 2.6.40 instead, the backport to 2.6.39.x if it survives until
> >>>>> 2.6.40-rc2 or so...
> >>>>
> >>>> Considering the trouble resource allocation always ends up being, I'd
> >>>> almost prefer that "mark it for stable and put it in the 2.6.40
> >>>> queue".
> >>>>
> >>>> Afaik this problem hasn't actually hit any "normal" users, has it? So ...
> >>>
> >>> Sounds good, thanks.  Yeah I don't think it's hit anyone but Yinghai
> >>> (at least I don't know of any other reports).
> >>>
> >>
> >> please check this one, it should be safe for 2.6.39 ?
> > 
> >>  	size0 = calculate_iosize(size, min_size, size1,
> >>  			resource_size(b_res), 4096);
> >> -	size1 = !add_size? size0:
> >> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
> >>  		calculate_iosize(size, min_size+add_size, size1,
> >>  			resource_size(b_res), 4096);
> > 
> > This solves the problem you encountered.
> > 
> > But, I think, it still does not fix the following scenario:
> > 
> > adjust_resource() failing to allocate additional resource to a hotplug bridge
> > that has no children. In this case  ->flags of that 'struct resource'
> > continues to be set even when no resource is allocated to that hot-plug bridge.
> > 
> that case: requested_size will be 0, but add_size will not be zero.
> 
> res->flags is not cleared in pbus_size_xx, so it will be put into head.
> so it will go through first path.
> ...
>                 if (!resource_size(res) && add_size) {
>                          res->end = res->start + add_size - 1;
>                          if(pci_assign_resource(list->dev, idx))
>                                 reset_resource(res);
>                 } else if (add_size) {
>                         adjust_resource(res, res->start,
>                                 resource_size(res) + add_size);
>                 }
> 
> and if it fails to get assign, the flags will get clear in reset_resource.
> 
> so it should be ok. and testing in one my setup show those flags get clear correctly and does not emit any warning.


Ack. You are right.

Linus/Jesse:  can we consider this patch for 2.6.39? It is the simplest fix to the problem.

Reviewed-by: Ram Pai <linuxram@us.ibm.com>

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
Jesse Barnes May 17, 2011, 3:52 a.m. UTC | #4 
On Mon, 16 May 2011 15:36:21 -0700
Ram Pai <linuxram@us.ibm.com> wrote:

> On Mon, May 16, 2011 at 01:55:38PM -0700, Yinghai Lu wrote:
> > On 05/16/2011 12:59 AM, Ram Pai wrote:
> > > On Fri, May 13, 2011 at 06:06:17PM -0700, Yinghai Lu wrote:
> > >> On 05/12/2011 12:34 PM, Jesse Barnes wrote:
> > >>> On Thu, 12 May 2011 12:18:43 -0700
> > >>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >>>
> > >>>> On Thu, May 12, 2011 at 11:37 AM, Jesse Barnes
> > >>>> <jbarnes@virtuousgeek.org> wrote:
> > >>>>>
> > >>>>> Linus, I don't have anything else queued up, so you may as
> > >>>>> well take this one directly if you want it in 2.6.39.  It's a
> > >>>>> regression fix, but resource changes always make me nervous.
> > >>>>> Alternately, I could put it into 2.6.40 instead, the backport
> > >>>>> to 2.6.39.x if it survives until 2.6.40-rc2 or so...
> > >>>>
> > >>>> Considering the trouble resource allocation always ends up
> > >>>> being, I'd almost prefer that "mark it for stable and put it
> > >>>> in the 2.6.40 queue".
> > >>>>
> > >>>> Afaik this problem hasn't actually hit any "normal" users, has
> > >>>> it? So ...
> > >>>
> > >>> Sounds good, thanks.  Yeah I don't think it's hit anyone but
> > >>> Yinghai (at least I don't know of any other reports).
> > >>>
> > >>
> > >> please check this one, it should be safe for 2.6.39 ?
> > > 
> > >>  	size0 = calculate_iosize(size, min_size, size1,
> > >>  			resource_size(b_res), 4096);
> > >> -	size1 = !add_size? size0:
> > >> +	size1 = (!add_head || (add_head && !add_size)) ? size0 :
> > >>  		calculate_iosize(size, min_size+add_size, size1,
> > >>  			resource_size(b_res), 4096);
> > > 
> > > This solves the problem you encountered.
> > > 
> > > But, I think, it still does not fix the following scenario:
> > > 
> > > adjust_resource() failing to allocate additional resource to a
> > > hotplug bridge that has no children. In this case  ->flags of
> > > that 'struct resource' continues to be set even when no resource
> > > is allocated to that hot-plug bridge.
> > > 
> > that case: requested_size will be 0, but add_size will not be zero.
> > 
> > res->flags is not cleared in pbus_size_xx, so it will be put into
> > head. so it will go through first path.
> > ...
> >                 if (!resource_size(res) && add_size) {
> >                          res->end = res->start + add_size - 1;
> >                          if(pci_assign_resource(list->dev, idx))
> >                                 reset_resource(res);
> >                 } else if (add_size) {
> >                         adjust_resource(res, res->start,
> >                                 resource_size(res) + add_size);
> >                 }
> > 
> > and if it fails to get assign, the flags will get clear in
> > reset_resource.
> > 
> > so it should be ok. and testing in one my setup show those flags
> > get clear correctly and does not emit any warning.
> 
> 
> Ack. You are right.
> 
> Linus/Jesse:  can we consider this patch for 2.6.39? It is the
> simplest fix to the problem.
> 
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>

I'll queue it up for 2.6.40 with stable cc'd.

Thanks,
Jesse
--
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
Linus Torvalds May 17, 2011, 5:22 a.m. UTC | #5 
On Mon, May 16, 2011 at 8:52 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> I'll queue it up for 2.6.40 with stable cc'd.

I ended up taking it after all..

                   Linus
--
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 mbox

Patch

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -579,7 +579,7 @@  static void pbus_size_io(struct pci_bus
 	}
 	size0 = calculate_iosize(size, min_size, size1,
 			resource_size(b_res), 4096);
-	size1 = !add_size? size0:
+	size1 = (!add_head || (add_head && !add_size)) ? size0 :
 		calculate_iosize(size, min_size+add_size, size1,
 			resource_size(b_res), 4096);
 	if (!size0 && !size1) {
@@ -677,7 +677,7 @@  static int pbus_size_mem(struct pci_bus
 		align += aligns[order];
 	}
 	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
-	size1 = !add_size ? size :
+	size1 = (!add_head || (add_head && !add_size)) ? size0 :
 		calculate_memsize(size, min_size+add_size, 0,
 				resource_size(b_res), min_align);
 	if (!size0 && !size1) {