Patchworkβ resources: when allocate_resource() fails, leave resource untouched

login
register
about
Submitter Bjorn Helgaas
Date 2009-11-02 17:45:36
Message ID <20091102174536.12512.56685.stgit@bob.kio>
Download mbox | patch
Permalink /patch/57096/
State Not Applicable
Headers show

Comments

Bjorn Helgaas - 2009-11-02 17:45:36
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 <bjorn.helgaas@hp.com>
---
 kernel/resource.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 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
Jesse Barnes - 2009-11-04 18:20:26
On Mon, 02 Nov 2009 10:45:36 -0700
Bjorn Helgaas <bjorn.helgaas@hp.com> 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 <bjorn.helgaas@hp.com>
> ---
>  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.
Linus Torvalds - 2009-11-04 18:29:54
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 <torvalds@linux-foundation.org>

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
--
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 - 2009-11-04 18:49:13
On Wed, Nov 4, 2009 at 10:29 AM, Linus Torvalds
<torvalds@linux-foundation.org> 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 <torvalds@linux-foundation.org>
>
> 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;
--
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 - 2009-11-04 18:51:12
On Wed, 4 Nov 2009 10:29:54 -0800 (PST)
Linus Torvalds <torvalds@linux-foundation.org> 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 <torvalds@linux-foundation.org>
> 
> 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.

Patch

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;