* [KJ] [RFC] request_region() error handling fixes
@ 2006-09-27 19:59 Badari Pulavarty
2006-09-27 20:48 ` Nishanth Aravamudan
` (9 more replies)
0 siblings, 10 replies; 11+ messages in thread
From: Badari Pulavarty @ 2006-09-27 19:59 UTC (permalink / raw)
To: kernel-janitors
Hi,
I started working on request_region() error handling fixes.
Before I got too far, want to make sure its following approach
is acceptable. (not happy with too many "goto" statements).
Is there a better way to do this ?
Please comment.
Thanks,
Badari
Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com>
arch/powerpc/platforms/chrp/setup.c | 42 +++++++++++++++++++++++++-----
arch/powerpc/platforms/pseries/pci.c | 41 +++++++++++++++++++++++++----
arch/ppc/platforms/hdpu.c | 4 ++
arch/ppc/platforms/lopec.c | 43 +++++++++++++++++++++++++-----
arch/ppc/platforms/mvme5100.c | 41 +++++++++++++++++++++++++----
arch/ppc/platforms/pplus.c | 49 ++++++++++++++++++++++++++++++-----
6 files changed, 187 insertions(+), 33 deletions(-)
Index: linux-2.6.18/arch/powerpc/platforms/chrp/setup.c
=================================--- linux-2.6.18.orig/arch/powerpc/platforms/chrp/setup.c 2006-09-27 09:17:23.000000000 -0700
+++ linux-2.6.18/arch/powerpc/platforms/chrp/setup.c 2006-09-27 13:20:17.000000000 -0700
@@ -508,12 +508,30 @@
chrp_nvram_init();
#endif
- request_region(0x20,0x20,"pic1");
- request_region(0xa0,0x20,"pic2");
- request_region(0x00,0x20,"dma1");
- request_region(0x40,0x20,"timer");
- request_region(0x80,0x10,"dma page reg");
- request_region(0xc0,0x20,"dma2");
+ if (!request_region(0x20,0x20,"pic1")) {
+ printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
+ return;
+ }
+ if (!request_region(0xa0,0x20,"pic2")) {
+ printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
+ goto out5;
+ }
+ if (!request_region(0x00,0x20,"dma1")) {
+ printk(KERN_WARNING"chrp_init: dma1 request region failed\n");
+ goto out4;
+ }
+ if (!request_region(0x40,0x20,"timer")) {
+ printk(KERN_WARNING"chrp_init: timer1 request region failed\n");
+ goto out3;
+ }
+ if (!request_region(0x80,0x10,"dma page reg")) {
+ printk(KERN_WARNING"chrp_init: dma page request region failed\n");
+ goto out2;
+ }
+ if (!request_region(0xc0,0x20,"dma2")) {
+ printk(KERN_WARNING"chrp_init: dma2 request region failed\n");
+ goto out1;
+ }
/* Get the event scan rate for the rtas so we know how
* often it expects a heartbeat. -- Cort
@@ -551,6 +569,18 @@
if (ppc_md.progress)
ppc_md.progress(" Have fun! ", 0x7777);
+ return;
+out1:
+ release_region(0x80, 0x10);
+out2:
+ release_region(0x40, 0x20);
+out3:
+ release_region(0x00, 0x20);
+out4:
+ release_region(0xa0, 0x20);
+out5:
+ release_region(0x20, 0x20);
+ return;
}
static int __init chrp_probe(void)
Index: linux-2.6.18/arch/ppc/platforms/lopec.c
=================================--- linux-2.6.18.orig/arch/ppc/platforms/lopec.c 2006-09-19 20:42:06.000000000 -0700
+++ linux-2.6.18/arch/ppc/platforms/lopec.c 2006-09-27 09:35:40.000000000 -0700
@@ -277,14 +277,43 @@
outb(0x00, 0x4d0);
outb(0xc0, 0x4d1);
- request_region(0x00, 0x20, "dma1");
- request_region(0x20, 0x20, "pic1");
- request_region(0x40, 0x20, "timer");
- request_region(0x80, 0x10, "dma page reg");
- request_region(0xa0, 0x20, "pic2");
- request_region(0xc0, 0x20, "dma2");
-
+ if (!request_region(0x00, 0x20, "dma1")) {
+ printk(KERN_WARNING"lopec: dma1 request region failed\n");
+ goto out6;
+ }
+ if (!request_region(0x20, 0x20, "pic1")) {
+ printk(KERN_WARNING"lopec: pic1 request region failed\n");
+ goto out5;
+ }
+ if (!request_region(0x40, 0x20, "timer")) {
+ printk(KERN_WARNING"lopec: timer request region failed\n");
+ goto out4;
+ }
+ if (!request_region(0x80, 0x10, "dma page reg")) {
+ printk(KERN_WARNING"lopec: dma page request region failed\n");
+ goto out3;
+ }
+ if (!request_region(0xa0, 0x20, "pic2")) {
+ printk(KERN_WARNING"lopec: pic2 request region failed\n");
+ goto out2;
+ }
+ if (!request_region(0xc0, 0x20, "dma2")) {
+ printk(KERN_WARNING"lopec: dma2 request region failed\n");
+ goto out1;
+ }
return 0;
+out1:
+ release_region(0xa0, 0x20);
+out2:
+ release_region(0x80, 0x10);
+out3:
+ release_region(0x40, 0x20);
+out4:
+ release_region(0x20, 0x20);
+out5:
+ release_region(0x00, 0x20);
+out6:
+ return -EIO;
}
device_initcall(lopec_request_io);
Index: linux-2.6.18/arch/powerpc/platforms/pseries/pci.c
=================================--- linux-2.6.18.orig/arch/powerpc/platforms/pseries/pci.c 2006-09-19 20:42:06.000000000 -0700
+++ linux-2.6.18/arch/powerpc/platforms/pseries/pci.c 2006-09-27 09:44:02.000000000 -0700
@@ -95,12 +95,41 @@
if (!isa_io_base)
return;
- request_region(0x20,0x20,"pic1");
- request_region(0xa0,0x20,"pic2");
- request_region(0x00,0x20,"dma1");
- request_region(0x40,0x20,"timer");
- request_region(0x80,0x10,"dma page reg");
- request_region(0xc0,0x20,"dma2");
+ if (!request_region(0x20,0x20,"pic1")) {
+ printk(KERN_WARNING"pSeries pic1 request region failed\n");
+ return;
+ }
+ if (!request_region(0xa0,0x20,"pic2")) {
+ printk(KERN_WARNING"pSeries pic2 request region failed\n");
+ goto out5;
+ }
+ if (!request_region(0x00,0x20,"dma1")) {
+ printk(KERN_WARNING"pSeries dma1 request region failed\n");
+ goto out4;
+ }
+ if (!request_region(0x40,0x20,"timer")) {
+ printk(KERN_WARNING"pSeries timer request region failed\n");
+ goto out3;
+ }
+ if (!request_region(0x80,0x10,"dma page reg")) {
+ printk(KERN_WARNING"pSeries dma page request region failed\n");
+ goto out2;
+ }
+ if (!request_region(0xc0,0x20,"dma2")) {
+ printk(KERN_WARNING"pSeries dma2 request region failed\n");
+ goto out1;
+ }
+ return;
+out1:
+ release_region(0x80, 0x10);
+out2:
+ release_region(0x40, 0x20);
+out3:
+ release_region(0x00, 0x20);
+out4:
+ release_region(0xa0, 0x20);
+out5:
+ release_region(0x20, 0x20);
}
void __init pSeries_final_fixup(void)
Index: linux-2.6.18/arch/ppc/platforms/hdpu.c
=================================--- linux-2.6.18.orig/arch/ppc/platforms/hdpu.c 2006-09-19 20:42:06.000000000 -0700
+++ linux-2.6.18/arch/ppc/platforms/hdpu.c 2006-09-27 09:47:25.000000000 -0700
@@ -608,7 +608,9 @@
static void
hdpu_ide_request_region(ide_ioreg_t from, unsigned int extent, const char *name)
{
- request_region(from, extent, name);
+ if (!request_region(from, extent, name)) {
+ printk("hdpe ide request region failed\n");
+ }
return;
}
Index: linux-2.6.18/arch/ppc/platforms/mvme5100.c
=================================--- linux-2.6.18.orig/arch/ppc/platforms/mvme5100.c 2006-09-19 20:42:06.000000000 -0700
+++ linux-2.6.18/arch/ppc/platforms/mvme5100.c 2006-09-27 09:51:37.000000000 -0700
@@ -190,12 +190,41 @@
mvme5100_init2(void)
{
#ifdef CONFIG_MVME5100_IPMC761_PRESENT
- request_region(0x00,0x20,"dma1");
- request_region(0x20,0x20,"pic1");
- request_region(0x40,0x20,"timer");
- request_region(0x80,0x10,"dma page reg");
- request_region(0xa0,0x20,"pic2");
- request_region(0xc0,0x20,"dma2");
+ if (!request_region(0x00, 0x20, "dma1")) {
+ printk(KERN_WARNING"mvme5100: dma1 request region failed\n");
+ return;
+ }
+ if (!request_region(0x20, 0x20, "pic1")) {
+ printk(KERN_WARNING"mvme5100: pic1 request region failed\n");
+ goto out5;
+ }
+ if (!request_region(0x40, 0x20, "timer")) {
+ printk(KERN_WARNING"mvme5100: timer request region failed\n");
+ goto out4;
+ }
+ if (!request_region(0x80, 0x10, "dma page reg")) {
+ printk(KERN_WARNING"mvme5100: dma page request region failed\n");
+ goto out3;
+ }
+ if (!request_region(0xa0, 0x20, "pic2")) {
+ printk(KERN_WARNING"mvme5100: pic2 request region failed\n");
+ goto out2;
+ }
+ if (!request_region(0xc0, 0x20, "dma2")) {
+ printk(KERN_WARNING"mvme5100: dma2 request region failed\n");
+ goto out1;
+ }
+ return 0;
+out1:
+ release_region(0xa0, 0x20);
+out2:
+ release_region(0x80, 0x10);
+out3:
+ release_region(0x40, 0x20);
+out4:
+ release_region(0x20, 0x20);
+out5:
+ release_region(0x00, 0x20);
#endif
return;
}
Index: linux-2.6.18/arch/ppc/platforms/pplus.c
=================================--- linux-2.6.18.orig/arch/ppc/platforms/pplus.c 2006-09-19 20:42:06.000000000 -0700
+++ linux-2.6.18/arch/ppc/platforms/pplus.c 2006-09-27 10:02:48.000000000 -0700
@@ -814,14 +814,49 @@
static void __init pplus_init2(void)
{
#ifdef CONFIG_NVRAM
- request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ if (request_region(PREP_NVRAM_AS0, 0x8, "nvram")) {
+ printk(KERN_WARNING"pplus_init nvram request region failed\n");
+ return;
+ }
#endif
- request_region(0x20, 0x20, "pic1");
- request_region(0xa0, 0x20, "pic2");
- request_region(0x00, 0x20, "dma1");
- request_region(0x40, 0x20, "timer");
- request_region(0x80, 0x10, "dma page reg");
- request_region(0xc0, 0x20, "dma2");
+ if (!request_region(0x20,0x20,"pic1")) {
+ printk(KERN_WARNING"pplus_init pic1 request region failed\n");
+#ifdef CONFIG_NVRAM
+ release_region(PREP_NVRAM_AS0);
+#endif
+ return;
+ }
+ if (!request_region(0xa0,0x20,"pic2")) {
+ printk(KERN_WARNING"pplus_init pic2 request region failed\n");
+ goto out5;
+ }
+ if (!request_region(0x00,0x20,"dma1")) {
+ printk(KERN_WARNING"pplus_init dma1 request region failed\n");
+ goto out4;
+ }
+ if (!request_region(0x40,0x20,"timer")) {
+ printk(KERN_WARNING"pplus_init timer request region failed\n");
+ goto out3;
+ }
+ if (!request_region(0x80,0x10,"dma page reg")) {
+ printk(KERN_WARNING"pplus_init dma page request region failed\n");
+ goto out2;
+ }
+ if (!request_region(0xc0,0x20,"dma2")) {
+ printk(KERN_WARNING"pplus_init dma2 request region failed\n");
+ goto out1;
+ }
+ return;
+out1:
+ release_region(0x80, 0x10);
+out2:
+ release_region(0x40, 0x20);
+out3:
+ release_region(0x00, 0x20);
+out4:
+ release_region(0xa0, 0x20);
+out5:
+ release_region(0x20, 0x20);
}
/*
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
@ 2006-09-27 20:48 ` Nishanth Aravamudan
2006-09-27 21:04 ` Badari Pulavarty
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Nishanth Aravamudan @ 2006-09-27 20:48 UTC (permalink / raw)
To: kernel-janitors
On 27.09.2006 [12:59:54 -0700], Badari Pulavarty wrote:
> Hi,
>
> I started working on request_region() error handling fixes.
> Before I got too far, want to make sure its following approach
> is acceptable. (not happy with too many "goto" statements).
> Is there a better way to do this ?
I don't think there is any better way to do multiple stacked failures
than many gotos. At least, if the goal is to avoid code duplication
(which I would say it is).
> Please comment.
Looks good, except one nit and one error below.
<snip>
> =================================> --- linux-2.6.18.orig/arch/powerpc/platforms/chrp/setup.c 2006-09-27 09:17:23.000000000 -0700
> +++ linux-2.6.18/arch/powerpc/platforms/chrp/setup.c 2006-09-27 13:20:17.000000000 -0700
<snip>
> + if (!request_region(0x20,0x20,"pic1")) {
> + printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
nit: usually a space is placed between the KERN_ and the string to be
output. Same goes for all the printk()s added.
<snip>
> Index: linux-2.6.18/arch/ppc/platforms/pplus.c
> =================================> --- linux-2.6.18.orig/arch/ppc/platforms/pplus.c 2006-09-19 20:42:06.000000000 -0700
> +++ linux-2.6.18/arch/ppc/platforms/pplus.c 2006-09-27 10:02:48.000000000 -0700
> @@ -814,14 +814,49 @@
> static void __init pplus_init2(void)
> {
> #ifdef CONFIG_NVRAM
> - request_region(PREP_NVRAM_AS0, 0x8, "nvram");
> + if (request_region(PREP_NVRAM_AS0, 0x8, "nvram")) {
This should be a negated test like all the rest, right?
Thanks,
Nish
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
2006-09-27 20:48 ` Nishanth Aravamudan
@ 2006-09-27 21:04 ` Badari Pulavarty
2006-09-27 21:22 ` Nishanth Aravamudan
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Badari Pulavarty @ 2006-09-27 21:04 UTC (permalink / raw)
To: kernel-janitors
On Wed, 2006-09-27 at 13:48 -0700, Nishanth Aravamudan wrote:
> On 27.09.2006 [12:59:54 -0700], Badari Pulavarty wrote:
> > Hi,
> >
> > I started working on request_region() error handling fixes.
> > Before I got too far, want to make sure its following approach
> > is acceptable. (not happy with too many "goto" statements).
> > Is there a better way to do this ?
>
> I don't think there is any better way to do multiple stacked failures
> than many gotos. At least, if the goal is to avoid code duplication
> (which I would say it is).
Okay. Do you think I should add unlikely() for these to get better
code ?
> > Please comment.
>
> Looks good, except one nit and one error below.
>
> <snip>
>
> > =================================> > --- linux-2.6.18.orig/arch/powerpc/platforms/chrp/setup.c 2006-09-27 09:17:23.000000000 -0700
> > +++ linux-2.6.18/arch/powerpc/platforms/chrp/setup.c 2006-09-27 13:20:17.000000000 -0700
>
> <snip>
>
> > + if (!request_region(0x20,0x20,"pic1")) {
> > + printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
>
> nit: usually a space is placed between the KERN_ and the string to be
> output. Same goes for all the printk()s added.
Sure. Will do. I copied it from another place :(
>
> <snip>
>
> > Index: linux-2.6.18/arch/ppc/platforms/pplus.c
> > =================================> > --- linux-2.6.18.orig/arch/ppc/platforms/pplus.c 2006-09-19 20:42:06.000000000 -0700
> > +++ linux-2.6.18/arch/ppc/platforms/pplus.c 2006-09-27 10:02:48.000000000 -0700
> > @@ -814,14 +814,49 @@
> > static void __init pplus_init2(void)
> > {
> > #ifdef CONFIG_NVRAM
> > - request_region(PREP_NVRAM_AS0, 0x8, "nvram");
> > + if (request_region(PREP_NVRAM_AS0, 0x8, "nvram")) {
>
> This should be a negated test like all the rest, right?
Yes. My fault.
Thanks,
Badari
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
2006-09-27 20:48 ` Nishanth Aravamudan
2006-09-27 21:04 ` Badari Pulavarty
@ 2006-09-27 21:22 ` Nishanth Aravamudan
2006-09-27 22:31 ` Jesper Juhl
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Nishanth Aravamudan @ 2006-09-27 21:22 UTC (permalink / raw)
To: kernel-janitors
On 27.09.2006 [14:04:25 -0700], Badari Pulavarty wrote:
> On Wed, 2006-09-27 at 13:48 -0700, Nishanth Aravamudan wrote:
> > On 27.09.2006 [12:59:54 -0700], Badari Pulavarty wrote:
> > > Hi,
> > >
> > > I started working on request_region() error handling fixes.
> > > Before I got too far, want to make sure its following approach
> > > is acceptable. (not happy with too many "goto" statements).
> > > Is there a better way to do this ?
> >
> > I don't think there is any better way to do multiple stacked failures
> > than many gotos. At least, if the goal is to avoid code duplication
> > (which I would say it is).
>
> Okay. Do you think I should add unlikely() for these to get better
> code ?
iirc, the LKML consensus on unlikely() was only add it if it truly is
very unlikely and actually makes a significant difference. Most of what
you're patching is init code, which will only be run once, it seems. I
don't think unlikely() is necessary (unless you can show a significant
performance difference, I guess).
Thanks,
Nish
--
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (2 preceding siblings ...)
2006-09-27 21:22 ` Nishanth Aravamudan
@ 2006-09-27 22:31 ` Jesper Juhl
2006-09-27 22:45 ` Dan Carpenter
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jesper Juhl @ 2006-09-27 22:31 UTC (permalink / raw)
To: kernel-janitors
On 27/09/06, Badari Pulavarty <pbadari@us.ibm.com> wrote:
> Hi,
>
> I started working on request_region() error handling fixes.
> Before I got too far, want to make sure its following approach
> is acceptable. (not happy with too many "goto" statements).
> Is there a better way to do this ?
>
> Please comment.
>
[snip]
If you don't like all these goto's :
> + if (!request_region(0x20,0x20,"pic1")) {
> + printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
> + return;
> + }
> + if (!request_region(0xa0,0x20,"pic2")) {
> + printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
> + goto out5;
> + }
> + if (!request_region(0x00,0x20,"dma1")) {
> + printk(KERN_WARNING"chrp_init: dma1 request region failed\n");
> + goto out4;
> + }
> + if (!request_region(0x40,0x20,"timer")) {
> + printk(KERN_WARNING"chrp_init: timer1 request region failed\n");
> + goto out3;
> + }
> + if (!request_region(0x80,0x10,"dma page reg")) {
> + printk(KERN_WARNING"chrp_init: dma page request region failed\n");
> + goto out2;
> + }
> + if (!request_region(0xc0,0x20,"dma2")) {
> + printk(KERN_WARNING"chrp_init: dma2 request region failed\n");
> + goto out1;
> + }
>
> /* Get the event scan rate for the rtas so we know how
> * often it expects a heartbeat. -- Cort
> @@ -551,6 +569,18 @@
>
> if (ppc_md.progress)
> ppc_md.progress(" Have fun! ", 0x7777);
> + return;
> +out1:
> + release_region(0x80, 0x10);
> +out2:
> + release_region(0x40, 0x20);
> +out3:
> + release_region(0x00, 0x20);
> +out4:
> + release_region(0xa0, 0x20);
> +out5:
> + release_region(0x20, 0x20);
> + return;
> }
>
then I see something like the following as possible alternatives,
although I think the goto one is actually the best :
1) Yes, this is ugly as hell and will possibly generate warnings at
runtime about freeing non existing resources, but it would work...
Not really an option...
int status = 0;
if (!request_region(0x20,0x20,"pic1")) {
printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
return;
}
if (!request_region(0xa0,0x20,"pic2")) {
printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
status++;
}
if (!request_region(0x00,0x20,"dma1")) {
printk(KERN_WARNING"chrp_init: dma1 request region failed\n");
status++;
}
if (!request_region(0x40,0x20,"timer")) {
printk(KERN_WARNING"chrp_init: timer1 request region failed\n");
status++;
}
if (!request_region(0x80,0x10,"dma page reg")) {
printk(KERN_WARNING"chrp_init: dma page request region
failed\n");
status++;
}
if (!request_region(0xc0,0x20,"dma2")) {
printk(KERN_WARNING"chrp_init: dma2 request region failed\n");
status++;
}
if (status) {
/* one or more request_region() failed, release all */
release_region(0x80, 0x10);
release_region(0x40, 0x20);
release_region(0x00, 0x20);
release_region(0xa0, 0x20);
release_region(0x20, 0x20);
return;
}
2) While this is an alternative, it duplicates a lot of code and is
really, really ugly - actually it makes my eyes hurt...
if (request_region(0x20,0x20,"pic1")) {
if (request_region(0xa0,0x20,"pic2")) {
if (request_region(0x00,0x20,"dma1")) {
if (request_region(0x40,0x20,"timer")) {
if (request_region(0x80,0x10,"dma page reg")) {
if (!request_region(0xc0,0x20,"dma2")) {
printk(KERN_WARNING"chrp_init: dma2 request
region failed\n");
release_region(0x80, 0x10);
release_region(0x40, 0x20);
release_region(0x00, 0x20);
release_region(0xa0, 0x20);
release_region(0x20, 0x20);
return;
}
} else {
printk(KERN_WARNING"chrp_init: dma page request
region failed\n");
release_region(0x40, 0x20);
release_region(0x00, 0x20);
release_region(0xa0, 0x20);
release_region(0x20, 0x20);
return;
}
} else {
printk(KERN_WARNING"chrp_init: timer1 request
region failed\n");
release_region(0x00, 0x20);
release_region(0xa0, 0x20);
release_region(0x20, 0x20);
return;
}
} else {
printk(KERN_WARNING"chrp_init: dma1 request region
failed\n");
release_region(0xa0, 0x20);
release_region(0x20, 0x20);
return;
}
} else {
printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
release_region(0x20, 0x20);
return;
}
} else {
printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
return;
}
So I'd say your version with the goto's (which is similar to what's
used elsewhere in the kernel) is the right way to go...
Ohh btw, shouldn't there be a single space before your label names? Like
out4:
instead of
out4:
I seem to remember something about label names shouldn't start at column 0.
Also, in a line like this :
if (!request_region(0x20,0x20,"pic1")) {
I think the consensus is that it should be spaced like this instead
(space after the commas):
if (!request_region(0x20, 0x20, "pic1")) {
--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (3 preceding siblings ...)
2006-09-27 22:31 ` Jesper Juhl
@ 2006-09-27 22:45 ` Dan Carpenter
2006-09-27 22:58 ` Jesper Juhl
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2006-09-27 22:45 UTC (permalink / raw)
To: kernel-janitors
On 9/27/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On 27/09/06, Badari Pulavarty <pbadari@us.ibm.com> wrote:
> > Hi,
> >
> > I started working on request_region() error handling fixes.
> > Before I got too far, want to make sure its following approach
> > is acceptable. (not happy with too many "goto" statements).
> > Is there a better way to do this ?
> >
> > Please comment.
> >
> [snip]
>
> If you don't like all these goto's :
>
> > + if (!request_region(0x20,0x20,"pic1")) {
> > + printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
> > + return;
> > + }
> > + if (!request_region(0xa0,0x20,"pic2")) {
> > + printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
> > + goto out5;
> > + }
> > + if (!request_region(0x00,0x20,"dma1")) {
> > + printk(KERN_WARNING"chrp_init: dma1 request region failed\n");
> > + goto out4;
> > + }
> > + if (!request_region(0x40,0x20,"timer")) {
> > + printk(KERN_WARNING"chrp_init: timer1 request region failed\n");
> > + goto out3;
> > + }
> > + if (!request_region(0x80,0x10,"dma page reg")) {
> > + printk(KERN_WARNING"chrp_init: dma page request region failed\n");
> > + goto out2;
> > + }
> > + if (!request_region(0xc0,0x20,"dma2")) {
> > + printk(KERN_WARNING"chrp_init: dma2 request region failed\n");
> > + goto out1;
> > + }
I think that if any of those particular request_regions()s fail your
system is unbootable. Things like "timer1" seem pretty essential.
The other solution is to just put a comment at the top that those
always succeed so they don't need to be checked.
regards,
dan carpenter
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (4 preceding siblings ...)
2006-09-27 22:45 ` Dan Carpenter
@ 2006-09-27 22:58 ` Jesper Juhl
2006-09-27 23:15 ` Badari Pulavarty
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Jesper Juhl @ 2006-09-27 22:58 UTC (permalink / raw)
To: kernel-janitors
On 28/09/06, Dan Carpenter <error27@gmail.com> wrote:
> On 9/27/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > On 27/09/06, Badari Pulavarty <pbadari@us.ibm.com> wrote:
> > > Hi,
> > >
> > > I started working on request_region() error handling fixes.
> > > Before I got too far, want to make sure its following approach
> > > is acceptable. (not happy with too many "goto" statements).
> > > Is there a better way to do this ?
> > >
> > > Please comment.
> > >
> > [snip]
> >
> > If you don't like all these goto's :
> >
> > > + if (!request_region(0x20,0x20,"pic1")) {
> > > + printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
> > > + return;
> > > + }
> > > + if (!request_region(0xa0,0x20,"pic2")) {
> > > + printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
> > > + goto out5;
> > > + }
> > > + if (!request_region(0x00,0x20,"dma1")) {
> > > + printk(KERN_WARNING"chrp_init: dma1 request region failed\n");
> > > + goto out4;
> > > + }
> > > + if (!request_region(0x40,0x20,"timer")) {
> > > + printk(KERN_WARNING"chrp_init: timer1 request region failed\n");
> > > + goto out3;
> > > + }
> > > + if (!request_region(0x80,0x10,"dma page reg")) {
> > > + printk(KERN_WARNING"chrp_init: dma page request region failed\n");
> > > + goto out2;
> > > + }
> > > + if (!request_region(0xc0,0x20,"dma2")) {
> > > + printk(KERN_WARNING"chrp_init: dma2 request region failed\n");
> > > + goto out1;
> > > + }
>
> I think that if any of those particular request_regions()s fail your
> system is unbootable. Things like "timer1" seem pretty essential.
> The other solution is to just put a comment at the top that those
> always succeed so they don't need to be checked.
>
I don't think that's a good solution.
In some cases the regions may be essential and if one fails we've
lost, but in other cases the box may be slightly different and
actually be able to survive without. With the vast proliferation of
different hardware out there can you really say for sure if some of
these regions will always be needed in every possible case?
In my opinion it's better to check them, do the right thing and
release if one request fails - then, if the box can't survive without,
checking has cost us nothing, it still won't work with or without the
check. But, if the box is able to survice without the regions, then
we've dont the right thing and stuff works. Better safe than sorry.
--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (5 preceding siblings ...)
2006-09-27 22:58 ` Jesper Juhl
@ 2006-09-27 23:15 ` Badari Pulavarty
2006-09-28 4:34 ` Amol Lad
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Badari Pulavarty @ 2006-09-27 23:15 UTC (permalink / raw)
To: kernel-janitors
On Thu, 2006-09-28 at 00:31 +0200, Jesper Juhl wrote:
> On 27/09/06, Badari Pulavarty <pbadari@us.ibm.com> wrote:
> > Hi,
> >
> > I started working on request_region() error handling fixes.
> > Before I got too far, want to make sure its following approach
> > is acceptable. (not happy with too many "goto" statements).
> > Is there a better way to do this ?
> >
> > Please comment.
> >
> [snip]
>
> If you don't like all these goto's :
>
..
>
>
> So I'd say your version with the goto's (which is similar to what's
> used elsewhere in the kernel) is the right way to go...
Yes. I considered few other options (and infact, coded to see how
they look), but settled on "goto".
> Ohh btw, shouldn't there be a single space before your label names? Like
>
> out4:
>
> instead of
>
> out4:
hmm. I haven't seen many of those (I thought, they were typos).
>
> I seem to remember something about label names shouldn't start at column 0.
>
>
> Also, in a line like this :
>
> if (!request_region(0x20,0x20,"pic1")) {
>
> I think the consensus is that it should be spaced like this instead
> (space after the commas):
>
> if (!request_region(0x20, 0x20, "pic1")) {
>
Sure. I will fix those too, I was trying to keep as much code as
"common".
Thanks for your suggestions.
Thanks,
Badari
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (6 preceding siblings ...)
2006-09-27 23:15 ` Badari Pulavarty
@ 2006-09-28 4:34 ` Amol Lad
2006-09-28 15:04 ` Badari Pulavarty
2006-09-28 16:42 ` Domen Puncer
9 siblings, 0 replies; 11+ messages in thread
From: Amol Lad @ 2006-09-28 4:34 UTC (permalink / raw)
To: kernel-janitors
Also, when you progress on this you will find that there are lots of
places where the fix is required. So take care of below:
1. Split a big patch in multiple patches. Read
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
2. If you are confident that your patch is fine then also CC subsystem
maintainer & subsystem mailing list (if it's there). If you are
confused, then CC lkml
Best of luck !
On Wed, 2006-09-27 at 16:15 -0700, Badari Pulavarty wrote:
> On Thu, 2006-09-28 at 00:31 +0200, Jesper Juhl wrote:
> > On 27/09/06, Badari Pulavarty <pbadari@us.ibm.com> wrote:
> > > Hi,
> > >
> > > I started working on request_region() error handling fixes.
> > > Before I got too far, want to make sure its following approach
> > > is acceptable. (not happy with too many "goto" statements).
> > > Is there a better way to do this ?
> > >
> > > Please comment.
> > >
> > [snip]
> >
> > If you don't like all these goto's :
> >
> ..
> >
> >
> > So I'd say your version with the goto's (which is similar to what's
> > used elsewhere in the kernel) is the right way to go...
>
> Yes. I considered few other options (and infact, coded to see how
> they look), but settled on "goto".
>
> > Ohh btw, shouldn't there be a single space before your label names? Like
> >
> > out4:
> >
> > instead of
> >
> > out4:
>
> hmm. I haven't seen many of those (I thought, they were typos).
>
> >
> > I seem to remember something about label names shouldn't start at column 0.
> >
> >
> > Also, in a line like this :
> >
> > if (!request_region(0x20,0x20,"pic1")) {
> >
> > I think the consensus is that it should be spaced like this instead
> > (space after the commas):
> >
> > if (!request_region(0x20, 0x20, "pic1")) {
> >
>
> Sure. I will fix those too, I was trying to keep as much code as
> "common".
>
> Thanks for your suggestions.
>
> Thanks,
> Badari
>
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
>
>
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (7 preceding siblings ...)
2006-09-28 4:34 ` Amol Lad
@ 2006-09-28 15:04 ` Badari Pulavarty
2006-09-28 16:42 ` Domen Puncer
9 siblings, 0 replies; 11+ messages in thread
From: Badari Pulavarty @ 2006-09-28 15:04 UTC (permalink / raw)
To: kernel-janitors
> > > If you don't like all these goto's :
> > >
> > > > + if (!request_region(0x20,0x20,"pic1")) {
> > > > + printk(KERN_WARNING"chrp_init: pic1 request region failed\n");
> > > > + return;
> > > > + }
> > > > + if (!request_region(0xa0,0x20,"pic2")) {
> > > > + printk(KERN_WARNING"chrp_init: pic2 request region failed\n");
> > > > + goto out5;
> > > > + }
> > > > + if (!request_region(0x00,0x20,"dma1")) {
> > > > + printk(KERN_WARNING"chrp_init: dma1 request region failed\n");
> > > > + goto out4;
> > > > + }
> > > > + if (!request_region(0x40,0x20,"timer")) {
> > > > + printk(KERN_WARNING"chrp_init: timer1 request region failed\n");
> > > > + goto out3;
> > > > + }
> > > > + if (!request_region(0x80,0x10,"dma page reg")) {
> > > > + printk(KERN_WARNING"chrp_init: dma page request region failed\n");
> > > > + goto out2;
> > > > + }
> > > > + if (!request_region(0xc0,0x20,"dma2")) {
> > > > + printk(KERN_WARNING"chrp_init: dma2 request region failed\n");
> > > > + goto out1;
> > > > + }
> >
I see lot of places where we try acquire multiple regions, so all these
places need to handle errors and release already acquired ones properly.
I am just wondering, if we are better off introducing/requesting a new
interface like request_regions() and release_regions() which iterates
over a vector of requests and handles them properly ?
Thanks,
Badari
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [KJ] [RFC] request_region() error handling fixes
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
` (8 preceding siblings ...)
2006-09-28 15:04 ` Badari Pulavarty
@ 2006-09-28 16:42 ` Domen Puncer
9 siblings, 0 replies; 11+ messages in thread
From: Domen Puncer @ 2006-09-28 16:42 UTC (permalink / raw)
To: kernel-janitors
On 27/09/06 16:15 -0700, Badari Pulavarty wrote:
> On Thu, 2006-09-28 at 00:31 +0200, Jesper Juhl wrote:
> Yes. I considered few other options (and infact, coded to see how
> they look), but settled on "goto".
>
> > Ohh btw, shouldn't there be a single space before your label names? Like
> >
> > out4:
> >
> > instead of
> >
> > out4:
>
> hmm. I haven't seen many of those (I thought, they were typos).
They are intentional, because otherwise diff -p prints them.
And please don't use labels with numbers. I think there was a
patch posted here a week or two ago that had to rename those
numbers, because an error condition could happen in between.
Domen
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2006-09-28 16:42 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-27 19:59 [KJ] [RFC] request_region() error handling fixes Badari Pulavarty
2006-09-27 20:48 ` Nishanth Aravamudan
2006-09-27 21:04 ` Badari Pulavarty
2006-09-27 21:22 ` Nishanth Aravamudan
2006-09-27 22:31 ` Jesper Juhl
2006-09-27 22:45 ` Dan Carpenter
2006-09-27 22:58 ` Jesper Juhl
2006-09-27 23:15 ` Badari Pulavarty
2006-09-28 4:34 ` Amol Lad
2006-09-28 15:04 ` Badari Pulavarty
2006-09-28 16:42 ` Domen Puncer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.