Message ID | 4DCDD589.8010309@kernel.org (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
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
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
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
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
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
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) {