* [KJ] [PATCH] request_region - ppc/platforms/prep_setup.c - Please
@ 2006-02-04 21:10 Ron Gage
2006-02-04 22:26 ` Jesper Juhl
2006-02-04 22:32 ` Jesper Juhl
0 siblings, 2 replies; 3+ messages in thread
From: Ron Gage @ 2006-02-04 21:10 UTC (permalink / raw)
To: kernel-janitors
Is this a good way to handle this problem? Original code was (in essence) 5
request_region calls in a row with no return checking.
This patch adds return value checking to request_region. No other structural
changes included...
diff -ur linux-2.6.15.orig/arch/ppc/platforms/prep_setup.c
linux-2.6.15/arch/ppc/platforms/prep_setup.c
--- linux-2.6.15.orig/arch/ppc/platforms/prep_setup.c 2006-02-04
11:00:29.000000000 -0500
+++ linux-2.6.15/arch/ppc/platforms/prep_setup.c 2006-02-04 16:04:58.000000000
-0500
@@ -1071,16 +1071,43 @@
static int __init
prep_request_io(void)
{
+ struct resource *region;
+ unsigned char ret=0;
+
+ /* if ret = 0, everything worked and we return normally.
+ if ret != 0, then something failed and we return -EBUSY. */
+
if (_machine = _MACH_prep) {
#ifdef CONFIG_NVRAM
- request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ region = request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ if (!region) {
+ printk (KERN_INFO "Could not allocate nvram region");
+ ret = 1;
+ }
#endif
- request_region(0x00,0x20,"dma1");
- request_region(0x40,0x20,"timer");
- request_region(0x80,0x10,"dma page reg");
- request_region(0xc0,0x20,"dma2");
+ region = request_region(0x00,0x20,"dma1");
+ if (!region) {
+ printk (KERN_INFO "Could not allocate dma1 region");
+ ret = 1;
+ }
+ region = request_region(0x40,0x20,"timer");
+ if (!region) {
+ printk (KERN_INFO "Could not allocate timer region");
+ ret = 1;
+ }
+ region = request_region(0x80,0x10,"dma page reg");
+ if (!region) {
+ printk (KERN_INFO "Could not allocate dma page region");
+ ret = 1;
+ }
+ region = request_region(0xc0,0x20,"dma2");
+ if (!region) {
+ printk (KERN_INFO "Could not allocate dma2 region");
+ ret = 1;
+ }
}
-
+ if (ret != 0)
+ return -EBUSY;
return 0;
}
--
Ron Gage
(LPIC1, MCP, A+, Net+)
Westland Michigan
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [KJ] [PATCH] request_region - ppc/platforms/prep_setup.c - Please
2006-02-04 21:10 [KJ] [PATCH] request_region - ppc/platforms/prep_setup.c - Please Ron Gage
@ 2006-02-04 22:26 ` Jesper Juhl
2006-02-04 22:32 ` Jesper Juhl
1 sibling, 0 replies; 3+ messages in thread
From: Jesper Juhl @ 2006-02-04 22:26 UTC (permalink / raw)
To: kernel-janitors
On Saturday 04 February 2006 22:10, Ron Gage wrote:
> Is this a good way to handle this problem? Original code was (in essence) 5
> request_region calls in a row with no return checking.
>
Your patch completely neglects to free the resources already allocated. Thus if
a the driver aborts half way through allocating its regions then the regions it did
successfully allocate will never be freed and another driver needing those
regions will never be able to load.
A better patch is (in my opinion) something like the below.
(also, you forgot to add a Signed-off-by: line to your patch)
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
arch/ppc/platforms/prep_setup.c | 30 ++++++++++++++++++++++++++++--
1 files changed, 28 insertions(+), 2 deletions(-)
--- linux-2.6.16-rc2-git1-orig/arch/ppc/platforms/prep_setup.c 2006-02-04 14:43:58.000000000 +0100
+++ linux-2.6.16-rc2-git1/arch/ppc/platforms/prep_setup.c 2006-02-04 22:39:57.000000000 +0100
@@ -1069,17 +1069,43 @@ prep_map_io(void)
static int __init
prep_request_io(void)
{
+ struct resource *region;
+ int ret = 0;
+
if (_machine = _MACH_prep) {
#ifdef CONFIG_NVRAM
- request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ region = request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ if (!region)
+ goto out_nvram;
#endif
request_region(0x00,0x20,"dma1");
+ if (!region)
+ goto out_dma1;
request_region(0x40,0x20,"timer");
+ if (!region)
+ goto out_timer;
request_region(0x80,0x10,"dma page reg");
+ if (!region)
+ goto out_dma_page;
request_region(0xc0,0x20,"dma2");
+ if (!region)
+ goto out_dma2;
}
- return 0;
+ out:
+ return ret;
+ out_dma2:
+ release_region(0x80,0x10);
+ out_dma_page:
+ release_region(0x40,0x20);
+ out_timer:
+ release_region(0x00,0x20);
+ out_dma1:
+ release_region(PREP_NVRAM_AS0, 0x8);
+ out_nvram:
+ ret = -EBUSY;
+ printk(KERN_WARNING "Unable to allocate required regions\n");
+ goto out;
}
device_initcall(prep_request_io);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [KJ] [PATCH] request_region - ppc/platforms/prep_setup.c - Please
2006-02-04 21:10 [KJ] [PATCH] request_region - ppc/platforms/prep_setup.c - Please Ron Gage
2006-02-04 22:26 ` Jesper Juhl
@ 2006-02-04 22:32 ` Jesper Juhl
1 sibling, 0 replies; 3+ messages in thread
From: Jesper Juhl @ 2006-02-04 22:32 UTC (permalink / raw)
To: kernel-janitors
On Saturday 04 February 2006 23:26, Jesper Juhl wrote:
> On Saturday 04 February 2006 22:10, Ron Gage wrote:
> > Is this a good way to handle this problem? Original code was (in essence) 5
> > request_region calls in a row with no return checking.
> >
> Your patch completely neglects to free the resources already allocated. Thus if
> a the driver aborts half way through allocating its regions then the regions it did
> successfully allocate will never be freed and another driver needing those
> regions will never be able to load.
> A better patch is (in my opinion) something like the below.
>
D'OH, included the wrong file, the below should be better :
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
arch/ppc/platforms/prep_setup.c | 38 ++++++++++++++++++++++++++++++++------
1 files changed, 32 insertions(+), 6 deletions(-)
--- linux-2.6.16-rc2-git1-orig/arch/ppc/platforms/prep_setup.c 2006-02-04 14:43:58.000000000 +0100
+++ linux-2.6.16-rc2-git1/arch/ppc/platforms/prep_setup.c 2006-02-04 23:28:26.000000000 +0100
@@ -1069,17 +1069,43 @@ prep_map_io(void)
static int __init
prep_request_io(void)
{
+ struct resource *region;
+ int ret = 0;
+
if (_machine = _MACH_prep) {
#ifdef CONFIG_NVRAM
- request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ region = request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+ if (!region)
+ goto out_nvram;
#endif
- request_region(0x00,0x20,"dma1");
- request_region(0x40,0x20,"timer");
- request_region(0x80,0x10,"dma page reg");
- request_region(0xc0,0x20,"dma2");
+ region = request_region(0x00,0x20,"dma1");
+ if (!region)
+ goto out_dma1;
+ region = request_region(0x40,0x20,"timer");
+ if (!region)
+ goto out_timer;
+ region = request_region(0x80,0x10,"dma page reg");
+ if (!region)
+ goto out_dma_page;
+ region = request_region(0xc0,0x20,"dma2");
+ if (!region)
+ goto out_dma2;
}
- return 0;
+ out:
+ return ret;
+ out_dma2:
+ release_region(0x80,0x10);
+ out_dma_page:
+ release_region(0x40,0x20);
+ out_timer:
+ release_region(0x00,0x20);
+ out_dma1:
+ release_region(PREP_NVRAM_AS0, 0x8);
+ out_nvram:
+ ret = -EBUSY;
+ printk(KERN_WARNING "Unable to allocate required regions\n");
+ goto out;
}
device_initcall(prep_request_io);
--
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] 3+ messages in thread
end of thread, other threads:[~2006-02-04 22:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-04 21:10 [KJ] [PATCH] request_region - ppc/platforms/prep_setup.c - Please Ron Gage
2006-02-04 22:26 ` Jesper Juhl
2006-02-04 22:32 ` Jesper Juhl
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.