2009-11-02 17:45:34

by Bjorn Helgaas

[permalink] [raw]
Subject: [PATCH] resources: when allocate_resource() fails, leave resource untouched

When "allocate_resource(root, new, size, ...)" fails, we currently
clobber "new". This is inconvenient for the caller, who might care
about the original contents of the resource.

For example, when pci_bus_alloc_resource() fails, the "can't allocate
mem resource %pR" message from pci_assign_resources() currently contains
junk for the resource start/end.

This patch delays the "new" update until we're about to return success.

Signed-off-by: Bjorn Helgaas <[email protected]>
---
kernel/resource.c | 26 ++++++++++++++------------
1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index fb11a58..dc15686 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -308,35 +308,37 @@ static int find_resource(struct resource *root, struct resource *new,
void *alignf_data)
{
struct resource *this = root->child;
+ resource_size_t start, end;

- new->start = root->start;
+ start = root->start;
/*
* Skip past an allocated resource that starts at 0, since the assignment
* of this->start - 1 to new->end below would cause an underflow.
*/
if (this && this->start == 0) {
- new->start = this->end + 1;
+ start = this->end + 1;
this = this->sibling;
}
for(;;) {
if (this)
- new->end = this->start - 1;
+ end = this->start - 1;
else
- new->end = root->end;
- if (new->start < min)
- new->start = min;
- if (new->end > max)
- new->end = max;
- new->start = ALIGN(new->start, align);
+ end = root->end;
+ if (start < min)
+ start = min;
+ if (end > max)
+ end = max;
+ start = ALIGN(start, align);
if (alignf)
alignf(alignf_data, new, size, align);
- if (new->start < new->end && new->end - new->start >= size - 1) {
- new->end = new->start + size - 1;
+ if (start < end && end - start >= size - 1) {
+ new->start = start;
+ new->end = start + size - 1;
return 0;
}
if (!this)
break;
- new->start = this->end + 1;
+ start = this->end + 1;
this = this->sibling;
}
return -EBUSY;


2009-11-04 18:20:28

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] resources: when allocate_resource() fails, leave resource untouched

On Mon, 02 Nov 2009 10:45:36 -0700
Bjorn Helgaas <[email protected]> wrote:

> When "allocate_resource(root, new, size, ...)" fails, we currently
> clobber "new". This is inconvenient for the caller, who might care
> about the original contents of the resource.
>
> For example, when pci_bus_alloc_resource() fails, the "can't allocate
> mem resource %pR" message from pci_assign_resources() currently
> contains junk for the resource start/end.
>
> This patch delays the "new" update until we're about to return
> success.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> kernel/resource.c | 26 ++++++++++++++------------
> 1 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index fb11a58..dc15686 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -308,35 +308,37 @@ static int find_resource(struct resource *root,
> struct resource *new, void *alignf_data)
> {
> struct resource *this = root->child;
> + resource_size_t start, end;
>
> - new->start = root->start;
> + start = root->start;
> /*
> * Skip past an allocated resource that starts at 0, since
> the assignment
> * of this->start - 1 to new->end below would cause an
> underflow. */
> if (this && this->start == 0) {
> - new->start = this->end + 1;
> + start = this->end + 1;
> this = this->sibling;
> }
> for(;;) {
> if (this)
> - new->end = this->start - 1;
> + end = this->start - 1;
> else
> - new->end = root->end;
> - if (new->start < min)
> - new->start = min;
> - if (new->end > max)
> - new->end = max;
> - new->start = ALIGN(new->start, align);
> + end = root->end;
> + if (start < min)
> + start = min;
> + if (end > max)
> + end = max;
> + start = ALIGN(start, align);
> if (alignf)
> alignf(alignf_data, new, size, align);
> - if (new->start < new->end && new->end - new->start
> >= size - 1) {
> - new->end = new->start + size - 1;
> + if (start < end && end - start >= size - 1) {
> + new->start = start;
> + new->end = start + size - 1;
> return 0;
> }
> if (!this)
> break;
> - new->start = this->end + 1;
> + start = this->end + 1;
> this = this->sibling;
> }
> return -EBUSY;

Seems like a reasonable change to me. Linus is usually the gatekeeper
for resource.c.

--
Jesse Barnes, Intel Open Source Technology Center

2009-11-04 18:30:26

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH] resources: when allocate_resource() fails, leave resource untouched



On Wed, 4 Nov 2009, Jesse Barnes wrote:
>
> Seems like a reasonable change to me. Linus is usually the gatekeeper
> for resource.c.

I'm certainly ok with this one. I wouldn't be surprised if it even allows
for better code generation, apart from leaving resources untouched when
allocation fails. So:

Acked-by: Linus Torvalds <[email protected]>

It was apparently sent to Andrew, but I can certainly take it. Or you can
take it into the PCI tree. Or we can leave it in -mm. Anything goes. Just
tell me.

Linus

2009-11-04 18:49:11

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH] resources: when allocate_resource() fails, leave resource untouched

On Wed, Nov 4, 2009 at 10:29 AM, Linus Torvalds
<[email protected]> wrote:
>
>
> On Wed, 4 Nov 2009, Jesse Barnes wrote:
>>
>> Seems like a reasonable change to me. ?Linus is usually the gatekeeper
>> for resource.c.
>
> I'm certainly ok with this one. I wouldn't be surprised if it even allows
> for better code generation, apart from leaving resources untouched when
> allocation fails. So:
>
> ? ? ? ?Acked-by: Linus Torvalds <[email protected]>
>
> It was apparently sent to Andrew, but I can certainly take it. Or you can
> take it into the PCI tree. Or we can leave it in -mm. Anything goes. Just
> tell me.
>

looks should be put into pci tree.

two of patches that is releasing not big enough range from leaf bridge
could be user.

don't need the "save and restore" trick anymore.

YH

@@ -57,10 +100,23 @@ static void pbus_assign_resources_sorted
for (list = head.next; list;) {
res = list->res;
idx = res - &list->dev->resource[0];
+ /* save the size at first */
+ size = resource_size(res);
if (pci_assign_resource(list->dev, idx)) {
- res->start = 0;
- res->end = 0;
- res->flags = 0;
+ if (fail_head && !list->dev->subordinate &&
+ !pci_is_root_bus(list->dev->bus)) {
+ /*
+ * device need to keep flags and size
+ * for second try
+ */
+ res->start = 0;
+ res->end = size - 1;
+ add_to_failed_list(fail_head, list->dev, res);
+ } else {
+ res->start = 0;
+ res->end = 0;
+ res->flags = 0;
+ }
}
tmp = list;
list = list->next;

2009-11-04 18:51:11

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH] resources: when allocate_resource() fails, leave resource untouched

On Wed, 4 Nov 2009 10:29:54 -0800 (PST)
Linus Torvalds <[email protected]> wrote:

>
>
> On Wed, 4 Nov 2009, Jesse Barnes wrote:
> >
> > Seems like a reasonable change to me. Linus is usually the
> > gatekeeper for resource.c.
>
> I'm certainly ok with this one. I wouldn't be surprised if it even
> allows for better code generation, apart from leaving resources
> untouched when allocation fails. So:
>
> Acked-by: Linus Torvalds <[email protected]>
>
> It was apparently sent to Andrew, but I can certainly take it. Or you
> can take it into the PCI tree. Or we can leave it in -mm. Anything
> goes. Just tell me.

Ok, I'll pick it up. Thanks.

--
Jesse Barnes, Intel Open Source Technology Center