[KJ] [PATCH] check request_region return value

[KJ] [PATCH] check request_region return value

[Tim Cooijmans]

From: Tim Cooijmans <tim@aapopfiets.nl>

Check request_region return value in arch/powerpc/platform/chrp/setup.c.  
Passed compilation.

Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
---
--- linux-2.6.16-rc4-orig/arch/powerpc/platforms/chrp/setup.c	2006-02-21 
20:18:41.000000000 +0100
+++ linux-2.6.16-rc4/arch/powerpc/platforms/chrp/setup.c	2006-02-24 
18:30:10.000000000 +0100
@@ -471,15 +471,54 @@ chrp_init2(void)
 	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") = NULL) {
+		printk(KERN_WARNING "chrp_init2: could not "
+				    "allocate pic1 region\n");
+		goto out_pic1;
+	}
+	if (request_region(0xa0,0x20,"pic2") = NULL) {
+		printk(KERN_WARNING "chrp_init2: could not "
+				    "allocate pic2 region\n");
+		goto out_pic2;
+	}
+	if (request_region(0x00,0x20,"dma1") = NULL) {
+		printk(KERN_WARNING "chrp_init2: could not "
+				    "allocate dma1 region\n");
+		goto out_dma1;
+	}
+	if (request_region(0x40,0x20,"timer") = NULL) {
+		printk(KERN_WARNING "chrp_init2: could not "
+				    "allocate timer region\n");
+		goto out_timer;
+	}
+	if (request_region(0x80,0x10,"dma page reg") = NULL) {
+		printk(KERN_WARNING "chrp_init2: could not "
+				    "allocate dma page reg region\n");
+		goto out_dma_page_reg;
+	}
+	if (request_region(0xc0,0x20,"dma2") = NULL) {
+		printk(KERN_WARNING "chrp_init2: could not "
+				    "allocate dma2 region\n");
+		goto out_dma2;
+	}
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
+
+	return;
+
+out_dma2:
+	release_region(0x80,0x10);
+out_dma_page_reg:
+	release_region(0x40,0x20);
+out_timer:
+	release_region(0x00,0x20);
+out_dma1:
+	release_region(0xa0,0x20);
+out_pic2:
+	release_region(0x20,0x20);
+out_pic1:
+	return;
 }
 
 void __init chrp_init(void)
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] check request_region return value

[Jesper Juhl]

On 2/24/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> From: Tim Cooijmans <tim@aapopfiets.nl>
>
> Check request_region return value in arch/powerpc/platform/chrp/setup.c.
> Passed compilation.
>

Looks OK to me, but couldn't this be improved by changing the function
to return an int stating if things are OK or not and then having
caller check the return value and act appropriately?


--
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

Re: [KJ] [PATCH] check request_region return value

[Tim Cooijmans]

On Friday 24 February 2006 22:58, Jesper Juhl wrote:
> On 2/24/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> > From: Tim Cooijmans <tim@aapopfiets.nl>
> >
> > Check request_region return value in arch/powerpc/platform/chrp/setup.c.
> > Passed compilation.
>
> Looks OK to me, but couldn't this be improved by changing the function
> to return an int stating if things are OK or not and then having
> caller check the return value and act appropriately?
Yes, I thought about that.  The reason I didn't do this is that chrp_init2 is 
assigned to ppc_md.init.  ppc_md is a struct machdep_calls, which seems to be 
an abstraction of machine-dependent stuff.  Changing the return type is 
probably going to break lots of things.
Anyway, warning when things fail is better than nothing.

Tim
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] check request_region return value

[Jesper Juhl]

On 2/25/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> On Friday 24 February 2006 22:58, Jesper Juhl wrote:
> > On 2/24/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> > > From: Tim Cooijmans <tim@aapopfiets.nl>
> > >
> > > Check request_region return value in arch/powerpc/platform/chrp/setup.c.
> > > Passed compilation.
> >
> > Looks OK to me, but couldn't this be improved by changing the function
> > to return an int stating if things are OK or not and then having
> > caller check the return value and act appropriately?
> Yes, I thought about that.  The reason I didn't do this is that chrp_init2 is
> assigned to ppc_md.init.  ppc_md is a struct machdep_calls, which seems to be
> an abstraction of machine-dependent stuff.  Changing the return type is
> probably going to break lots of things.

Ok, I must admit I didn't look that much into it.


> Anyway, warning when things fail is better than nothing.
>

Definately.


--
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

[KJ] [PATCH] check request_region return value

[Tim Cooijmans]

From: Tim Cooijmans <tim@aapopfiets.nl>

Check request_region return value in drivers/acpi/motherboard.c.  
acpi_reserve_resources now returns int, 0 being success.  
acpi_motherboard_init propagates that return value.

Passed compilation.

Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
---
--- linux-2.6.16-rc4-orig/drivers/acpi/motherboard.c	2006-02-21 
20:18:41.000000000 +0100
+++ linux-2.6.16-rc4/drivers/acpi/motherboard.c	2006-02-25 14:08:01.000000000 
+0100
@@ -123,42 +123,115 @@ static struct acpi_driver acpi_motherboa
 		},
 };
 
-static void __init acpi_reserve_resources(void)
+static int __init acpi_reserve_resources(void)
 {
 	if (acpi_gbl_FADT->xpm1a_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
-		request_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
-			       acpi_gbl_FADT->pm1_evt_len, "PM1a_EVT_BLK");
+		if (request_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
+				   acpi_gbl_FADT->pm1_evt_len,
+				   "PM1a_EVT_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate PM1a_EVT_BLK region\n");
+			goto out_pm1a_evt_blk;
+		}
 
 	if (acpi_gbl_FADT->xpm1b_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
-		request_region(acpi_gbl_FADT->xpm1b_evt_blk.address,
-			       acpi_gbl_FADT->pm1_evt_len, "PM1b_EVT_BLK");
+		if (request_region(acpi_gbl_FADT->xpm1b_evt_blk.address,
+				   acpi_gbl_FADT->pm1_evt_len,
+				   "PM1b_EVT_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate PM1b_EVT_BLK region\n");
+			goto out_pm1b_evt_blk;
+		}
 
 	if (acpi_gbl_FADT->xpm1a_cnt_blk.address && acpi_gbl_FADT->pm1_cnt_len)
-		request_region(acpi_gbl_FADT->xpm1a_cnt_blk.address,
-			       acpi_gbl_FADT->pm1_cnt_len, "PM1a_CNT_BLK");
+		if (request_region(acpi_gbl_FADT->xpm1a_cnt_blk.address,
+				   acpi_gbl_FADT->pm1_cnt_len,
+				   "PM1a_CNT_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate PM1a_CNT_BLK region\n");
+			goto out_pm1a_cnt_blk;
+		}
 
 	if (acpi_gbl_FADT->xpm1b_cnt_blk.address && acpi_gbl_FADT->pm1_cnt_len)
-		request_region(acpi_gbl_FADT->xpm1b_cnt_blk.address,
-			       acpi_gbl_FADT->pm1_cnt_len, "PM1b_CNT_BLK");
+		if (request_region(acpi_gbl_FADT->xpm1b_cnt_blk.address,
+				   acpi_gbl_FADT->pm1_cnt_len,
+				   "PM1b_CNT_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate PM1b_CNT_BLK region\n");
+			goto out_pm1b_cnt_blk;
+		}
 
 	if (acpi_gbl_FADT->xpm_tmr_blk.address && acpi_gbl_FADT->pm_tm_len = 4)
-		request_region(acpi_gbl_FADT->xpm_tmr_blk.address, 4, "PM_TMR");
+		if (request_region(acpi_gbl_FADT->xpm_tmr_blk.address, 4,
+				   "PM_TMR") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate PM_TMR region\n");
+			goto out_pm_tmr;
+		}
 
 	if (acpi_gbl_FADT->xpm2_cnt_blk.address && acpi_gbl_FADT->pm2_cnt_len)
-		request_region(acpi_gbl_FADT->xpm2_cnt_blk.address,
-			       acpi_gbl_FADT->pm2_cnt_len, "PM2_CNT_BLK");
+		if (request_region(acpi_gbl_FADT->xpm2_cnt_blk.address,
+				   acpi_gbl_FADT->pm2_cnt_len,
+				   "PM2_CNT_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate PM2_CNT_BLK region\n");
+			goto out_pm2_cnt_blk;
+		}
 
 	/* Length of GPE blocks must be a non-negative multiple of 2 */
 
 	if (acpi_gbl_FADT->xgpe0_blk.address && acpi_gbl_FADT->gpe0_blk_len &&
 	    !(acpi_gbl_FADT->gpe0_blk_len & 0x1))
-		request_region(acpi_gbl_FADT->xgpe0_blk.address,
-			       acpi_gbl_FADT->gpe0_blk_len, "GPE0_BLK");
+		if (request_region(acpi_gbl_FADT->xgpe0_blk.address,
+				   acpi_gbl_FADT->gpe0_blk_len,
+				   "GPE0_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate GPE0_BLK region\n");
+			goto out_gpe0_blk;
+		}
 
 	if (acpi_gbl_FADT->xgpe1_blk.address && acpi_gbl_FADT->gpe1_blk_len &&
 	    !(acpi_gbl_FADT->gpe1_blk_len & 0x1))
-		request_region(acpi_gbl_FADT->xgpe1_blk.address,
-			       acpi_gbl_FADT->gpe1_blk_len, "GPE1_BLK");
+		if (request_region(acpi_gbl_FADT->xgpe1_blk.address,
+				   acpi_gbl_FADT->gpe1_blk_len,
+				   "GPE1_BLK") = NULL) {
+			printk(KERN_WARNING "acpi_reserve_resources: could not"
+					    "allocate GPE1_BLK region\n");
+			goto out_gpe1_blk;
+		}
+
+	return 0;
+
+out_gpe1_blk:
+	if (acpi_gbl_FADT->xgpe0_blk.address && acpi_gbl_FADT->gpe0_blk_len &&
+	    !(acpi_gbl_FADT->gpe0_blk_len & 0x1))
+		release_region(acpi_gbl_FADT->xgpe0_blk.address,
+			       acpi_gbl_FADT->gpe0_blk_len);
+out_gpe0_blk:
+	if (acpi_gbl_FADT->xpm2_cnt_blk.address && acpi_gbl_FADT->pm2_cnt_len)
+		release_region(acpi_gbl_FADT->xpm2_cnt_blk.address,
+			       acpi_gbl_FADT->pm2_cnt_len);
+out_pm2_cnt_blk:
+	if (acpi_gbl_FADT->xpm_tmr_blk.address && acpi_gbl_FADT->pm_tm_len = 4)
+		release_region(acpi_gbl_FADT->xpm_tmr_blk.address, 4);
+out_pm_tmr:
+	if (acpi_gbl_FADT->xpm1b_cnt_blk.address && acpi_gbl_FADT->pm1_cnt_len)
+		release_region(acpi_gbl_FADT->xpm1b_cnt_blk.address,
+			       acpi_gbl_FADT->pm1_cnt_len);
+out_pm1b_cnt_blk:
+	if (acpi_gbl_FADT->xpm1a_cnt_blk.address && acpi_gbl_FADT->pm1_cnt_len)
+		release_region(acpi_gbl_FADT->xpm1a_cnt_blk.address,
+			       acpi_gbl_FADT->pm1_cnt_len);
+out_pm1a_cnt_blk:
+	if (acpi_gbl_FADT->xpm1b_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
+		release_region(acpi_gbl_FADT->xpm1b_evt_blk.address,
+			       acpi_gbl_FADT->pm1_evt_len);
+out_pm1b_evt_blk:
+	if (acpi_gbl_FADT->xpm1a_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
+		release_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
+			       acpi_gbl_FADT->pm1_evt_len);
+out_pm1a_evt_blk:
+	return -EBUSY;
 }
 
 static int __init acpi_motherboard_init(void)
@@ -170,7 +243,8 @@ static int __init acpi_motherboard_init(
 	 * This module must run after scan.c
 	 */
 	if (!acpi_disabled)
-		acpi_reserve_resources();
+		return acpi_reserve_resources();
+
 	return 0;
 }
 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] check request_region return value

[Jesper Juhl]

On 2/25/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> From: Tim Cooijmans <tim@aapopfiets.nl>
>
> Check request_region return value in drivers/acpi/motherboard.c.
> acpi_reserve_resources now returns int, 0 being success.
> acpi_motherboard_init propagates that return value.
>
> Passed compilation.
>

Some comments below.


> Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
> ---
> --- linux-2.6.16-rc4-orig/drivers/acpi/motherboard.c    2006-02-21
> 20:18:41.000000000 +0100
> +++ linux-2.6.16-rc4/drivers/acpi/motherboard.c 2006-02-25 14:08:01.000000000
> +0100
> @@ -123,42 +123,115 @@ static struct acpi_driver acpi_motherboa
>                 },
>  };
>
> -static void __init acpi_reserve_resources(void)
> +static int __init acpi_reserve_resources(void)
>  {
>         if (acpi_gbl_FADT->xpm1a_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
> -               request_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
> -                              acpi_gbl_FADT->pm1_evt_len, "PM1a_EVT_BLK");
> +               if (request_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
> +                                  acpi_gbl_FADT->pm1_evt_len,
> +                                  "PM1a_EVT_BLK") = NULL) {

Why not

+               if (!request_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
+                                  acpi_gbl_FADT->pm1_evt_len,
+                                  "PM1a_EVT_BLK")) {

Shorter and easier to read IMHO.


> +                       printk(KERN_WARNING "acpi_reserve_resources: could not"
> +                                           "allocate PM1a_EVT_BLK region\n");
> +                       goto out_pm1a_evt_blk;
> +               }
>

If you want to print a warning for each case, why not make a simple
wrapper function that calls request_region and prints the warning
instead of having a ton of printk()s ?


> +out_gpe1_blk:
> +       if (acpi_gbl_FADT->xgpe0_blk.address && acpi_gbl_FADT->gpe0_blk_len &&
> +           !(acpi_gbl_FADT->gpe0_blk_len & 0x1))
> +               release_region(acpi_gbl_FADT->xgpe0_blk.address,
> +                              acpi_gbl_FADT->gpe0_blk_len);
> +out_gpe0_blk:
> +       if (acpi_gbl_FADT->xpm2_cnt_blk.address && acpi_gbl_FADT->pm2_cnt_len)
> +               release_region(acpi_gbl_FADT->xpm2_cnt_blk.address,
> +                              acpi_gbl_FADT->pm2_cnt_len);
> +out_pm2_cnt_blk:
> +       if (acpi_gbl_FADT->xpm_tmr_blk.address && acpi_gbl_FADT->pm_tm_len = 4)
> +               release_region(acpi_gbl_FADT->xpm_tmr_blk.address, 4);
> +out_pm_tmr:
> +       if (acpi_gbl_FADT->xpm1b_cnt_blk.address && acpi_gbl_FADT->pm1_cnt_len)
> +               release_region(acpi_gbl_FADT->xpm1b_cnt_blk.address,
> +                              acpi_gbl_FADT->pm1_cnt_len);
> +out_pm1b_cnt_blk:
> +       if (acpi_gbl_FADT->xpm1a_cnt_blk.address && acpi_gbl_FADT->pm1_cnt_len)
> +               release_region(acpi_gbl_FADT->xpm1a_cnt_blk.address,
> +                              acpi_gbl_FADT->pm1_cnt_len);
> +out_pm1a_cnt_blk:
> +       if (acpi_gbl_FADT->xpm1b_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
> +               release_region(acpi_gbl_FADT->xpm1b_evt_blk.address,
> +                              acpi_gbl_FADT->pm1_evt_len);
> +out_pm1b_evt_blk:
> +       if (acpi_gbl_FADT->xpm1a_evt_blk.address && acpi_gbl_FADT->pm1_evt_len)
> +               release_region(acpi_gbl_FADT->xpm1a_evt_blk.address,
> +                              acpi_gbl_FADT->pm1_evt_len);
> +out_pm1a_evt_blk:
> +       return -EBUSY;
>  }
>

Why all these if's in the error path? Since you get to any of these
labels in the first place you already know what request_region()s
succeeded, so what's the point of checking all these variables before
calling release_region()?


--
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

Re: [KJ] [PATCH] check request_region return value

[Tim Cooijmans]

On Saturday 25 February 2006 15:13, Jesper Juhl wrote:
> > +out_gpe1_blk:
> > +       if (acpi_gbl_FADT->xgpe0_blk.address &&
> > acpi_gbl_FADT->gpe0_blk_len && +           !(acpi_gbl_FADT->gpe0_blk_len
> > & 0x1))
> > +               release_region(acpi_gbl_FADT->xgpe0_blk.address,
> > +                              acpi_gbl_FADT->gpe0_blk_len);
> > +out_gpe0_blk:
> > +       if (acpi_gbl_FADT->xpm2_cnt_blk.address &&
> > acpi_gbl_FADT->pm2_cnt_len) +              
> > release_region(acpi_gbl_FADT->xpm2_cnt_blk.address, +                    
> >          acpi_gbl_FADT->pm2_cnt_len);
> > +out_pm2_cnt_blk:
> > +       if (acpi_gbl_FADT->xpm_tmr_blk.address &&
> > acpi_gbl_FADT->pm_tm_len = 4) +              
> > release_region(acpi_gbl_FADT->xpm_tmr_blk.address, 4); +out_pm_tmr:
> > +       if (acpi_gbl_FADT->xpm1b_cnt_blk.address &&
> > acpi_gbl_FADT->pm1_cnt_len) +              
> > release_region(acpi_gbl_FADT->xpm1b_cnt_blk.address, +                   
> >           acpi_gbl_FADT->pm1_cnt_len);
> > +out_pm1b_cnt_blk:
> > +       if (acpi_gbl_FADT->xpm1a_cnt_blk.address &&
> > acpi_gbl_FADT->pm1_cnt_len) +              
> > release_region(acpi_gbl_FADT->xpm1a_cnt_blk.address, +                   
> >           acpi_gbl_FADT->pm1_cnt_len);
> > +out_pm1a_cnt_blk:
> > +       if (acpi_gbl_FADT->xpm1b_evt_blk.address &&
> > acpi_gbl_FADT->pm1_evt_len) +              
> > release_region(acpi_gbl_FADT->xpm1b_evt_blk.address, +                   
> >           acpi_gbl_FADT->pm1_evt_len);
> > +out_pm1b_evt_blk:
> > +       if (acpi_gbl_FADT->xpm1a_evt_blk.address &&
> > acpi_gbl_FADT->pm1_evt_len) +              
> > release_region(acpi_gbl_FADT->xpm1a_evt_blk.address, +                   
> >           acpi_gbl_FADT->pm1_evt_len);
> > +out_pm1a_evt_blk:
> > +       return -EBUSY;
> >  }
>
> Why all these if's in the error path? Since you get to any of these
> labels in the first place you already know what request_region()s
> succeeded, so what's the point of checking all these variables before
> calling release_region()?
Not all request_region()s are tried.  These checks are done before requesting 
them as well, to determine which ones should be requested.  If I don't do 
these checks here, some release_region() calls will try to release regions 
that were never requested.

I'll look into your other comments.

Thanks,
Tim
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] check request_region return value

[Jesper Juhl]

On 2/25/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> On Saturday 25 February 2006 15:13, Jesper Juhl wrote:
> > > +out_gpe1_blk:
> > > +       if (acpi_gbl_FADT->xgpe0_blk.address &&
> > > acpi_gbl_FADT->gpe0_blk_len && +           !(acpi_gbl_FADT->gpe0_blk_len
> > > & 0x1))
> > > +               release_region(acpi_gbl_FADT->xgpe0_blk.address,
> > > +                              acpi_gbl_FADT->gpe0_blk_len);
> > > +out_gpe0_blk:
> > > +       if (acpi_gbl_FADT->xpm2_cnt_blk.address &&
> > > acpi_gbl_FADT->pm2_cnt_len) +
> > > release_region(acpi_gbl_FADT->xpm2_cnt_blk.address, +
> > >          acpi_gbl_FADT->pm2_cnt_len);
> > > +out_pm2_cnt_blk:
> > > +       if (acpi_gbl_FADT->xpm_tmr_blk.address &&
> > > acpi_gbl_FADT->pm_tm_len = 4) +
> > > release_region(acpi_gbl_FADT->xpm_tmr_blk.address, 4); +out_pm_tmr:
> > > +       if (acpi_gbl_FADT->xpm1b_cnt_blk.address &&
> > > acpi_gbl_FADT->pm1_cnt_len) +
> > > release_region(acpi_gbl_FADT->xpm1b_cnt_blk.address, +
> > >           acpi_gbl_FADT->pm1_cnt_len);
> > > +out_pm1b_cnt_blk:
> > > +       if (acpi_gbl_FADT->xpm1a_cnt_blk.address &&
> > > acpi_gbl_FADT->pm1_cnt_len) +
> > > release_region(acpi_gbl_FADT->xpm1a_cnt_blk.address, +
> > >           acpi_gbl_FADT->pm1_cnt_len);
> > > +out_pm1a_cnt_blk:
> > > +       if (acpi_gbl_FADT->xpm1b_evt_blk.address &&
> > > acpi_gbl_FADT->pm1_evt_len) +
> > > release_region(acpi_gbl_FADT->xpm1b_evt_blk.address, +
> > >           acpi_gbl_FADT->pm1_evt_len);
> > > +out_pm1b_evt_blk:
> > > +       if (acpi_gbl_FADT->xpm1a_evt_blk.address &&
> > > acpi_gbl_FADT->pm1_evt_len) +
> > > release_region(acpi_gbl_FADT->xpm1a_evt_blk.address, +
> > >           acpi_gbl_FADT->pm1_evt_len);
> > > +out_pm1a_evt_blk:
> > > +       return -EBUSY;
> > >  }
> >
> > Why all these if's in the error path? Since you get to any of these
> > labels in the first place you already know what request_region()s
> > succeeded, so what's the point of checking all these variables before
> > calling release_region()?
> Not all request_region()s are tried.  These checks are done before requesting
> them as well, to determine which ones should be requested.  If I don't do
> these checks here, some release_region() calls will try to release regions
> that were never requested.
>

Ahh yes, looking a bit more closely at the code I see that's true.
Hmm, there's got to be a cleaner way, the result of your patch is a
pretty ugly error path...


--
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

Re: [KJ] [PATCH] check request_region return value

[Tim Cooijmans]

On Saturday 25 February 2006 18:34, Jesper Juhl wrote:
> Ahh yes, looking a bit more closely at the code I see that's true.
> Hmm, there's got to be a cleaner way, the result of your patch is a
> pretty ugly error path...
Pretty much the only alternative I can think of is something like this: 

if (condition && request_region(x)) {
        if (condition && request_region(y)) {
                /* ... */
        } else {
                release_region(x);
        }
}

In this case, it will become a nine-level deep nest, and it's not much 
different from my original version.

Having done some research, I don't think the checks are necessary.  
request_region() (actually __request_resource()) does some checks which I'm 
pretty sure will catch any 0 passed to it.  The GPE odd/even checks will 
still have to be done, though.

I'll make a patch without most of the checks, unless anyone objects to 
removing them.

Tim
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] check request_region return value

[Jesper Juhl]

On 2/25/06, Tim Cooijmans <tim@aapopfiets.nl> wrote:
> On Saturday 25 February 2006 18:34, Jesper Juhl wrote:
> > Ahh yes, looking a bit more closely at the code I see that's true.
> > Hmm, there's got to be a cleaner way, the result of your patch is a
> > pretty ugly error path...
> Pretty much the only alternative I can think of is something like this:
>
> if (condition && request_region(x)) {
>         if (condition && request_region(y)) {
>                 /* ... */
>         } else {
>                 release_region(x);
>         }
> }
>
> In this case, it will become a nine-level deep nest, and it's not much
> different from my original version.
>
> Having done some research, I don't think the checks are necessary.
> request_region() (actually __request_resource()) does some checks which I'm
> pretty sure will catch any 0 passed to it.  The GPE odd/even checks will
> still have to be done, though.
>
> I'll make a patch without most of the checks, unless anyone objects to
> removing them.
>

I just did some research myself and looked at the code currently in
2.6.16-rc4-mm2 , and the code seems to have changed quite a bit, so
perhaps you would do well to work against that instead.


--
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

Re: [KJ] [PATCH] check request_region return value

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On Fri, 2006-02-24 at 22:49 +0100, Tim Cooijmans wrote:
> From: Tim Cooijmans <tim@aapopfiets.nl>
> 
> Check request_region return value in arch/powerpc/platform/chrp/setup.c.  
> Passed compilation.
> 
> Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
> ---
> +
> +out_dma2:
> +	release_region(0x80,0x10);
> +out_dma_page_reg:
> +	release_region(0x40,0x20);
> +out_timer:
> +	release_region(0x00,0x20);
> +out_dma1:
> +	release_region(0xa0,0x20);
> +out_pic2:
> +	release_region(0x20,0x20);
> +out_pic1:
> +	return;
>  }
>  

This might sound a bit stupid, but why are you doing release_region
here ? As you can't warn the upstream code, it will presumably try to
use the regions anyway. Wouldn't it be better to just to warn on a
failure, and keep going as normal? leaving all the regions (that were
acquired) mapped, keeping breakage/(usage of unmapped regions) to a
minimum?

Darren Jenkins


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] check request_region return value

[Tim Cooijmans]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Sunday 26 February 2006 05:50, Darren Jenkins\ wrote:
> This might sound a bit stupid, but why are you doing release_region
> here ? As you can't warn the upstream code, it will presumably try to
> use the regions anyway. Wouldn't it be better to just to warn on a
> failure, and keep going as normal? leaving all the regions (that were
> acquired) mapped, keeping breakage/(usage of unmapped regions) to a
> minimum?
Good point.  I'll revise my patch and resubmit.

Tim

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors