* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
@ 2006-02-24 21:58 ` Jesper Juhl
2006-02-24 23:07 ` Tim Cooijmans
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Jesper Juhl @ 2006-02-24 21:58 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
2006-02-24 21:58 ` Jesper Juhl
@ 2006-02-24 23:07 ` Tim Cooijmans
2006-02-24 23:11 ` Jesper Juhl
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tim Cooijmans @ 2006-02-24 23:07 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
2006-02-24 21:58 ` Jesper Juhl
2006-02-24 23:07 ` Tim Cooijmans
@ 2006-02-24 23:11 ` Jesper Juhl
2006-02-25 13:58 ` Tim Cooijmans
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Jesper Juhl @ 2006-02-24 23:11 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (2 preceding siblings ...)
2006-02-24 23:11 ` Jesper Juhl
@ 2006-02-25 13:58 ` Tim Cooijmans
2006-02-25 14:13 ` Jesper Juhl
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tim Cooijmans @ 2006-02-25 13:58 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (3 preceding siblings ...)
2006-02-25 13:58 ` Tim Cooijmans
@ 2006-02-25 14:13 ` Jesper Juhl
2006-02-25 17:26 ` Tim Cooijmans
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Jesper Juhl @ 2006-02-25 14:13 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (4 preceding siblings ...)
2006-02-25 14:13 ` Jesper Juhl
@ 2006-02-25 17:26 ` Tim Cooijmans
2006-02-25 17:34 ` Jesper Juhl
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tim Cooijmans @ 2006-02-25 17:26 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (5 preceding siblings ...)
2006-02-25 17:26 ` Tim Cooijmans
@ 2006-02-25 17:34 ` Jesper Juhl
2006-02-25 20:42 ` Tim Cooijmans
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Jesper Juhl @ 2006-02-25 17:34 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (6 preceding siblings ...)
2006-02-25 17:34 ` Jesper Juhl
@ 2006-02-25 20:42 ` Tim Cooijmans
2006-02-25 21:03 ` Jesper Juhl
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Tim Cooijmans @ 2006-02-25 20:42 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (7 preceding siblings ...)
2006-02-25 20:42 ` Tim Cooijmans
@ 2006-02-25 21:03 ` Jesper Juhl
2006-02-26 4:50 ` Darren Jenkins\
2006-02-26 19:52 ` Tim Cooijmans
10 siblings, 0 replies; 12+ messages in thread
From: Jesper Juhl @ 2006-02-25 21:03 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (8 preceding siblings ...)
2006-02-25 21:03 ` Jesper Juhl
@ 2006-02-26 4:50 ` Darren Jenkins\
2006-02-26 19:52 ` Tim Cooijmans
10 siblings, 0 replies; 12+ messages in thread
From: Darren Jenkins\ @ 2006-02-26 4:50 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] check request_region return value
2006-02-24 21:49 [KJ] [PATCH] check request_region return value Tim Cooijmans
` (9 preceding siblings ...)
2006-02-26 4:50 ` Darren Jenkins\
@ 2006-02-26 19:52 ` Tim Cooijmans
10 siblings, 0 replies; 12+ messages in thread
From: Tim Cooijmans @ 2006-02-26 19:52 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread