[PATCH] x86: fix IO APIC resource allocation error message

[PATCH] x86: fix IO APIC resource allocation error message

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] x86: fix IO APIC resource allocation error message

Impact: fix incorrect error message

- IO APIC resource allocation error message contains one too many "be".

- Print the error message iff there are IO APICs in the system.

Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
I've seen this error message for some time on my x86-32 laptop...

 arch/x86/kernel/io_apic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: b/arch/x86/kernel/io_apic.c
===================================================================
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
 	int i;
 	struct resource *r = ioapic_resources;
 
-	if (!r) {
+	if (!r && nr_ioapics > 0) {
 		printk(KERN_ERR
-		       "IO APIC resources could be not be allocated.\n");
+		       "IO APIC resources couldn't be allocated.\n");
 		return -1;
 	}
 

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Ingo Molnar]

* Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] x86: fix IO APIC resource allocation error message
> 
> Impact: fix incorrect error message
> 
> - IO APIC resource allocation error message contains one too many "be".
> 
> - Print the error message iff there are IO APICs in the system.
> 
> Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> I've seen this error message for some time on my x86-32 laptop...
> 
>  arch/x86/kernel/io_apic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: b/arch/x86/kernel/io_apic.c
> ===================================================================
> --- a/arch/x86/kernel/io_apic.c
> +++ b/arch/x86/kernel/io_apic.c
> @@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
>  	int i;
>  	struct resource *r = ioapic_resources;
>  
> -	if (!r) {
> +	if (!r && nr_ioapics > 0) {
>  		printk(KERN_ERR
> -		       "IO APIC resources could be not be allocated.\n");
> +		       "IO APIC resources couldn't be allocated.\n");
>  		return -1;
>  	}

looks good, but there's one weirdness:

so if nr_ioapics == 0 && !r we'll drop into this codepath:

        for (i = 0; i < nr_ioapics; i++) {
                insert_resource(&iomem_resource, r);
                r++;
        }

        return 0;

we survive the loop by luck, and then return 0 - which we'll survive 
too but still it's a bit unexpected and hence fragile.

So i think we should rather add a standalone:

	if (nr_ioapics > 0)
  		printk(KERN_ERR

check to the printk only. That wont affect the remaining code flow.

	Ingo

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Bartlomiej Zolnierkiewicz]

On Friday 20 March 2009, Ingo Molnar wrote:
> 
> * Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] x86: fix IO APIC resource allocation error message
> > 
> > Impact: fix incorrect error message
> > 
> > - IO APIC resource allocation error message contains one too many "be".
> > 
> > - Print the error message iff there are IO APICs in the system.
> > 
> > Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> > I've seen this error message for some time on my x86-32 laptop...
> > 
> >  arch/x86/kernel/io_apic.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Index: b/arch/x86/kernel/io_apic.c
> > ===================================================================
> > --- a/arch/x86/kernel/io_apic.c
> > +++ b/arch/x86/kernel/io_apic.c
> > @@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
> >  	int i;
> >  	struct resource *r = ioapic_resources;
> >  
> > -	if (!r) {
> > +	if (!r && nr_ioapics > 0) {
> >  		printk(KERN_ERR
> > -		       "IO APIC resources could be not be allocated.\n");
> > +		       "IO APIC resources couldn't be allocated.\n");
> >  		return -1;
> >  	}
> 
> looks good, but there's one weirdness:
> 
> so if nr_ioapics == 0 && !r we'll drop into this codepath:
> 
>         for (i = 0; i < nr_ioapics; i++) {
>                 insert_resource(&iomem_resource, r);
>                 r++;
>         }
> 
>         return 0;
> 
> we survive the loop by luck, and then return 0 - which we'll survive 

s/by luck/by design/ -- we want to return 0 and thus have the correct
return value of the initcall

> too but still it's a bit unexpected and hence fragile.

agreed on this one

> So i think we should rather add a standalone:
> 
> 	if (nr_ioapics > 0)
>   		printk(KERN_ERR
> 
> check to the printk only. That wont affect the remaining code flow.

here is the revised version

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] x86: fix IO APIC resource allocation error message (take 2)

Impact: fix incorrect error message

- IO APIC resource allocation error message contains one too many "be".

- Print the error message iff there are IO APICs in the system.

Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
I've seen this error message for some time on my x86-32 laptop...

 arch/x86/kernel/io_apic.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: b/arch/x86/kernel/io_apic.c
===================================================================
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -4151,9 +4151,12 @@ static int __init ioapic_insert_resource
 	struct resource *r = ioapic_resources;
 
 	if (!r) {
-		printk(KERN_ERR
-		       "IO APIC resources could be not be allocated.\n");
-		return -1;
+		if (nr_ioapics > 0) {
+			printk(KERN_ERR
+				"IO APIC resources couldn't be allocated.\n");
+			return -1;
+		}
+		return 0;
 	}
 
 	for (i = 0; i < nr_ioapics; i++) {

[tip:x86/apic] x86: fix IO APIC resource allocation error message

[Bartlomiej Zolnierkiewicz]

Commit-ID:  04c93ce4991fce731dab346d03964504339347db
Gitweb:     http://git.kernel.org/tip/04c93ce4991fce731dab346d03964504339347db
Author:     Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
AuthorDate: Fri, 20 Mar 2009 21:02:55 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 20 Mar 2009 21:02:55 +0100

x86: fix IO APIC resource allocation error message

Impact: fix incorrect error message

- IO APIC resource allocation error message contains one too many "be".

- Print the error message iff there are IO APICs in the system.

I've seen this error message for some time on my x86-32 laptop...

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
LKML-Reference: <200903202100.30789.bzolnier@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/apic/io_apic.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 42cdc78..d882c03 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -4130,9 +4130,12 @@ static int __init ioapic_insert_resources(void)
 	struct resource *r = ioapic_resources;
 
 	if (!r) {
-		printk(KERN_ERR
-		       "IO APIC resources could be not be allocated.\n");
-		return -1;
+		if (nr_ioapics > 0) {
+			printk(KERN_ERR
+				"IO APIC resources couldn't be allocated.\n");
+			return -1;
+		}
+		return 0;
 	}
 
 	for (i = 0; i < nr_ioapics; i++) {

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Cyrill Gorcunov]

[Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 08:12:41PM +0100]
| From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
| Subject: [PATCH] x86: fix IO APIC resource allocation error message
| 
| Impact: fix incorrect error message
| 
| - IO APIC resource allocation error message contains one too many "be".
| 
| - Print the error message iff there are IO APICs in the system.
| 
| Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
| Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
| ---
| I've seen this error message for some time on my x86-32 laptop...
| 
|  arch/x86/kernel/io_apic.c |    4 ++--
|  1 file changed, 2 insertions(+), 2 deletions(-)
| 
| Index: b/arch/x86/kernel/io_apic.c
| ===================================================================
| --- a/arch/x86/kernel/io_apic.c
| +++ b/arch/x86/kernel/io_apic.c
| @@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
|  	int i;
|  	struct resource *r = ioapic_resources;
|  
| -	if (!r) {
| +	if (!r && nr_ioapics > 0) {
|  		printk(KERN_ERR
| -		       "IO APIC resources could be not be allocated.\n");
| +		       "IO APIC resources couldn't be allocated.\n");
|  		return -1;
|  	}
|  
| 

Hi Bartlomiej,

until I miss something I guess you could even make it simplier :)
Something like

---
static int __init ioapic_insert_resources(void)
{
	struct resource *r = ioapic_resources;
	int err;
	int i;

	for (i = 0; i < nr_ioapics; i++) {
		err = insert_resource(&iomem_resource, r);
		if (err) {
			pr_err("IO APIC resources could not be allocated.\n");
			return err;
		}
		r++;
	}

	return 0;
}
---

Now we would have 'err' here and get out only on conflicting resource.
Did I miss something?

	- Cyrill -

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Bartlomiej Zolnierkiewicz]

On Friday 20 March 2009, Cyrill Gorcunov wrote:
> [Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 08:12:41PM +0100]
> | From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> | Subject: [PATCH] x86: fix IO APIC resource allocation error message
> | 
> | Impact: fix incorrect error message
> | 
> | - IO APIC resource allocation error message contains one too many "be".
> | 
> | - Print the error message iff there are IO APICs in the system.
> | 
> | Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
> | Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> | ---
> | I've seen this error message for some time on my x86-32 laptop...
> | 
> |  arch/x86/kernel/io_apic.c |    4 ++--
> |  1 file changed, 2 insertions(+), 2 deletions(-)
> | 
> | Index: b/arch/x86/kernel/io_apic.c
> | ===================================================================
> | --- a/arch/x86/kernel/io_apic.c
> | +++ b/arch/x86/kernel/io_apic.c
> | @@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
> |  	int i;
> |  	struct resource *r = ioapic_resources;
> |  
> | -	if (!r) {
> | +	if (!r && nr_ioapics > 0) {
> |  		printk(KERN_ERR
> | -		       "IO APIC resources could be not be allocated.\n");
> | +		       "IO APIC resources couldn't be allocated.\n");
> |  		return -1;
> |  	}
> |  
> | 
> 
> Hi Bartlomiej,
> 
> until I miss something I guess you could even make it simplier :)
> Something like
> 
> ---
> static int __init ioapic_insert_resources(void)
> {
> 	struct resource *r = ioapic_resources;
> 	int err;
> 	int i;
> 
> 	for (i = 0; i < nr_ioapics; i++) {
> 		err = insert_resource(&iomem_resource, r);
> 		if (err) {
> 			pr_err("IO APIC resources could not be allocated.\n");
> 			return err;
> 		}
> 		r++;
> 	}
> 
> 	return 0;
> }
> ---
> 
> Now we would have 'err' here and get out only on conflicting resource.
> Did I miss something?

nr_ioapics > 0 && r == NULL ?

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Cyrill Gorcunov]

[Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 09:02:28PM +0100]
| On Friday 20 March 2009, Cyrill Gorcunov wrote:
| > [Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 08:12:41PM +0100]
| > | From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
| > | Subject: [PATCH] x86: fix IO APIC resource allocation error message
| > | 
| > | Impact: fix incorrect error message
| > | 
| > | - IO APIC resource allocation error message contains one too many "be".
| > | 
| > | - Print the error message iff there are IO APICs in the system.
| > | 
| > | Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
| > | Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
| > | ---
| > | I've seen this error message for some time on my x86-32 laptop...
| > | 
| > |  arch/x86/kernel/io_apic.c |    4 ++--
| > |  1 file changed, 2 insertions(+), 2 deletions(-)
| > | 
| > | Index: b/arch/x86/kernel/io_apic.c
| > | ===================================================================
| > | --- a/arch/x86/kernel/io_apic.c
| > | +++ b/arch/x86/kernel/io_apic.c
| > | @@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
| > |  	int i;
| > |  	struct resource *r = ioapic_resources;
| > |  
| > | -	if (!r) {
| > | +	if (!r && nr_ioapics > 0) {
| > |  		printk(KERN_ERR
| > | -		       "IO APIC resources could be not be allocated.\n");
| > | +		       "IO APIC resources couldn't be allocated.\n");
| > |  		return -1;
| > |  	}
| > |  
| > | 
| > 
| > Hi Bartlomiej,
| > 
| > until I miss something I guess you could even make it simplier :)
| > Something like
| > 
| > ---
| > static int __init ioapic_insert_resources(void)
| > {
| > 	struct resource *r = ioapic_resources;
| > 	int err;
| > 	int i;
| > 
| > 	for (i = 0; i < nr_ioapics; i++) {
| > 		err = insert_resource(&iomem_resource, r);
| > 		if (err) {
| > 			pr_err("IO APIC resources could not be allocated.\n");
| > 			return err;
| > 		}
| > 		r++;
| > 	}
| > 
| > 	return 0;
| > }
| > ---
| > 
| > Now we would have 'err' here and get out only on conflicting resource.
| > Did I miss something?
| 
| nr_ioapics > 0 && r == NULL ?
| 

This case happens when alloc_bootmem fails but we already panic'ed!

Here is what I mean

>From ioapic_setup_resources()

	if (nr_ioapics <= 0)
		return NULL;

	mem = alloc_bootmem(n); <- we panic here anyway
	...
	ioapic_resources = res;

	- Cyrill -

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Bartlomiej Zolnierkiewicz]

On Friday 20 March 2009, Cyrill Gorcunov wrote:
> [Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 09:02:28PM +0100]
> | On Friday 20 March 2009, Cyrill Gorcunov wrote:
> | > [Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 08:12:41PM +0100]
> | > | From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> | > | Subject: [PATCH] x86: fix IO APIC resource allocation error message
> | > | 
> | > | Impact: fix incorrect error message
> | > | 
> | > | - IO APIC resource allocation error message contains one too many "be".
> | > | 
> | > | - Print the error message iff there are IO APICs in the system.
> | > | 
> | > | Cc: Alan Bartlett <ajb.stxsl@googlemail.com>
> | > | Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> | > | ---
> | > | I've seen this error message for some time on my x86-32 laptop...
> | > | 
> | > |  arch/x86/kernel/io_apic.c |    4 ++--
> | > |  1 file changed, 2 insertions(+), 2 deletions(-)
> | > | 
> | > | Index: b/arch/x86/kernel/io_apic.c
> | > | ===================================================================
> | > | --- a/arch/x86/kernel/io_apic.c
> | > | +++ b/arch/x86/kernel/io_apic.c
> | > | @@ -4150,9 +4150,9 @@ static int __init ioapic_insert_resource
> | > |  	int i;
> | > |  	struct resource *r = ioapic_resources;
> | > |  
> | > | -	if (!r) {
> | > | +	if (!r && nr_ioapics > 0) {
> | > |  		printk(KERN_ERR
> | > | -		       "IO APIC resources could be not be allocated.\n");
> | > | +		       "IO APIC resources couldn't be allocated.\n");
> | > |  		return -1;
> | > |  	}
> | > |  
> | > | 
> | > 
> | > Hi Bartlomiej,
> | > 
> | > until I miss something I guess you could even make it simplier :)
> | > Something like
> | > 
> | > ---
> | > static int __init ioapic_insert_resources(void)
> | > {
> | > 	struct resource *r = ioapic_resources;
> | > 	int err;
> | > 	int i;
> | > 
> | > 	for (i = 0; i < nr_ioapics; i++) {
> | > 		err = insert_resource(&iomem_resource, r);
> | > 		if (err) {
> | > 			pr_err("IO APIC resources could not be allocated.\n");
> | > 			return err;
> | > 		}
> | > 		r++;
> | > 	}
> | > 
> | > 	return 0;
> | > }
> | > ---
> | > 
> | > Now we would have 'err' here and get out only on conflicting resource.
> | > Did I miss something?
> | 
> | nr_ioapics > 0 && r == NULL ?
> | 
> 
> This case happens when alloc_bootmem fails but we already panic'ed!
> 
> Here is what I mean
> 
> From ioapic_setup_resources()
> 
> 	if (nr_ioapics <= 0)
> 		return NULL;
> 
> 	mem = alloc_bootmem(n); <- we panic here anyway

Seems like the following check is superfluous then:

        if (mem != NULL) {

> 	...
> 	ioapic_resources = res;

In either case I don't think we that failing all resource insertions
(for all IO APICs) if only one has failed is a desirable behavior...

Thanks,
Bart

Re: [PATCH] x86: fix IO APIC resource allocation error message

[Cyrill Gorcunov]

[Bartlomiej Zolnierkiewicz - Fri, Mar 20, 2009 at 09:27:14PM +0100]
...
| > | > Now we would have 'err' here and get out only on conflicting resource.
| > | > Did I miss something?
| > | 
| > | nr_ioapics > 0 && r == NULL ?
| > | 
| > 
| > This case happens when alloc_bootmem fails but we already panic'ed!
| > 
| > Here is what I mean
| > 
| > From ioapic_setup_resources()
| > 
| > 	if (nr_ioapics <= 0)
| > 		return NULL;
| > 
| > 	mem = alloc_bootmem(n); <- we panic here anyway
| 
| Seems like the following check is superfluous then:
| 
|         if (mem != NULL) {

Yes I believe

| 
| > 	...
| > 	ioapic_resources = res;
| 
| In either case I don't think we that failing all resource insertions
| (for all IO APICs) if only one has failed is a desirable behavior...

Indeed, which is even better

| 
| Thanks,
| Bart
| 

Your patch is already merged, so all is fine -- sorry for noise.

	- Cyrill -