All of lore.kernel.org
 help / color / mirror / Atom feed
* [KJ] [PATCH] check request_region() return value in arch/ppc and
@ 2006-02-27 21:29 Tim Cooijmans
  2006-02-28  5:33 ` Tim Cooijmans
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Tim Cooijmans @ 2006-02-27 21:29 UTC (permalink / raw)
  To: kernel-janitors

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

From: Tim Cooijmans <tim@aapopfiets.nl>

Check request_region() return value and warn on failure.  Note that releasing succeeded requests doesn't make much sense, as the calling code doesn't know if anything failed.

The powerpc code has been compile tested.  The ppc code has not been tested, but the changes are very similar.

Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
---
diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c
--- linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c	2006-02-27 20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c	2006-02-27 21:52:27.000000000 +0100
@@ -465,18 +465,26 @@ void __init chrp_init_IRQ(void)
 }
 
 void __init
+chrp_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc)) {
+		printk(KERN_WARNING "CHRP: unable to allocate %s region.\n", desc);
+	}
+}
+
+void __init
 chrp_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	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");
+	chrp_request_region(0x20,0x20,"pic1");
+	chrp_request_region(0xa0,0x20,"pic2");
+	chrp_request_region(0x00,0x20,"dma1");
+	chrp_request_region(0x40,0x20,"timer");
+	chrp_request_region(0x80,0x10,"dma page reg");
+	chrp_request_region(0xc0,0x20,"dma2");
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/pseries/pci.c linux-2.6.16-rc5/arch/powerpc/platforms/pseries/pci.c
--- linux-2.6.16-rc5-orig/arch/powerpc/platforms/pseries/pci.c	2006-02-27 20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/powerpc/platforms/pseries/pci.c	2006-02-27 21:49:54.000000000 +0100
@@ -92,17 +92,25 @@ void __devinit pSeries_irq_bus_setup(str
 	}
 }
 
+void __init
+pSeries_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc)) {
+		printk(KERN_WARNING "pSeries: unable to allocate %s region.\n", desc);
+	}
+}
+
 static void __init pSeries_request_regions(void)
 {
 	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");
+	pSeries_request_region(0x20,0x20,"pic1");
+	pSeries_request_region(0xa0,0x20,"pic2");
+	pSeries_request_region(0x00,0x20,"dma1");
+	pSeries_request_region(0x40,0x20,"timer");
+	pSeries_request_region(0x80,0x10,"dma page reg");
+	pSeries_request_region(0xc0,0x20,"dma2");
 }
 
 void __init pSeries_final_fixup(void)
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/chrp_setup.c linux-2.6.16-rc5/arch/ppc/platforms/chrp_setup.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/chrp_setup.c	2006-02-27 20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/chrp_setup.c	2006-02-27 21:52:29.000000000 +0100
@@ -450,18 +450,26 @@ void __init chrp_init_IRQ(void)
 }
 
 void __init
+chrp_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc)) {
+		printk(KERN_WARNING "CHRP: unable to allocate %s region.\n", desc);
+	}
+}
+
+void __init
 chrp_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	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");
+	chrp_request_region(0x20,0x20,"pic1");
+	chrp_request_region(0xa0,0x20,"pic2");
+	chrp_request_region(0x00,0x20,"dma1");
+	chrp_request_region(0x40,0x20,"timer");
+	chrp_request_region(0x80,0x10,"dma page reg");
+	chrp_request_region(0xc0,0x20,"dma2");
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/mvme5100.c linux-2.6.16-rc5/arch/ppc/platforms/mvme5100.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/mvme5100.c	2006-02-27 20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/mvme5100.c	2006-02-27 21:57:46.000000000 +0100
@@ -189,16 +189,24 @@ mvme5100_setup_arch(void)
 	return;
 }
 
+void __init
+mvme5100_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc)) {
+		printk(KERN_WARNING "mvme5100: unable to allocate %s region.\n", desc);
+	}
+}
+
 static void __init
 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");
+		mvme5100_request_region(0x00,0x20,"dma1");
+		mvme5100_request_region(0x20,0x20,"pic1");
+		mvme5100_request_region(0x40,0x20,"timer");
+		mvme5100_request_region(0x80,0x10,"dma page reg");
+		mvme5100_request_region(0xa0,0x20,"pic2");
+		mvme5100_request_region(0xc0,0x20,"dma2");
 #endif
 	return;
 }
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/pplus.c linux-2.6.16-rc5/arch/ppc/platforms/pplus.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/pplus.c	2006-02-27 20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/pplus.c	2006-02-27 21:59:07.000000000 +0100
@@ -814,17 +814,25 @@ static void __init pplus_map_io(void)
 	io_block_mapping(0xfef80000, 0xfef80000, 0x00080000, _PAGE_IO);
 }
 
+void __init
+pplus_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc)) {
+		printk(KERN_WARNING "pplus: unable to allocate %s region.\n", desc);
+	}
+}
+
 static void __init pplus_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	request_region(PREP_NVRAM_AS0, 0x8, "nvram");
 #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");
+	pplus_request_region(0x20, 0x20, "pic1");
+	pplus_request_region(0xa0, 0x20, "pic2");
+	pplus_request_region(0x00, 0x20, "dma1");
+	pplus_request_region(0x40, 0x20, "timer");
+	pplus_request_region(0x80, 0x10, "dma page reg");
+	pplus_request_region(0xc0, 0x20, "dma2");
 }
 
 /*

[-- 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] 8+ messages in thread

* Re: [KJ] [PATCH] check request_region() return value in arch/ppc and
@ 2006-02-27 21:47 Nishanth Aravamudan
  0 siblings, 0 replies; 8+ messages in thread
From: Nishanth Aravamudan @ 2006-02-27 21:47 UTC (permalink / raw)
  To: kernel-janitors

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

On 27.02.2006 [22:29:51 +0100], Tim Cooijmans wrote:
> From: Tim Cooijmans <tim@aapopfiets.nl>
> 
> Check request_region() return value and warn on failure.  Note that
> releasing succeeded requests doesn't make much sense, as the calling
> code doesn't know if anything failed.
> 
> The powerpc code has been compile tested.  The ppc code has not been
> tested, but the changes are very similar.

Looks good in general, one small nit.

> Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
> ---
> diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c
> --- linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c	2006-02-27 20:36:36.000000000 +0100
> +++ linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c	2006-02-27 21:52:27.000000000 +0100
> @@ -465,18 +465,26 @@ void __init chrp_init_IRQ(void)
>  }
>  
>  void __init
> +chrp_request_region(unsigned long start, unsigned long n, char *desc)
> +{
> +	if (!request_region(start, n, desc)) {
> +		printk(KERN_WARNING "CHRP: unable to allocate %s region.\n", desc);
> +	}

Here and in the other request_region() wrappers, no { } for the one-line
if, please.

Thanks,
Nish

[-- 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] 8+ messages in thread

* [KJ] [PATCH] check request_region() return value in arch/ppc and
  2006-02-27 21:29 Tim Cooijmans
@ 2006-02-28  5:33 ` Tim Cooijmans
  2006-02-28  5:53 ` Tim Cooijmans
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Tim Cooijmans @ 2006-02-28  5:33 UTC (permalink / raw)
  To: kernel-janitors

From: Tim Cooijmans <tim@aapopfiets.nl>

Check request_region() return value and warn on failure.  Note that releasing 
succeeded requests doesn't make much sense, as the calling code doesn't know 
if anything failed.

The powerpc code has been compile tested.  The ppc code has not been tested, 
but the changes are very similar.

Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
---
diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c 
linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c
--- linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c	2006-02-27 
21:52:27.000000000 +0100
@@ -465,18 +465,25 @@ void __init chrp_init_IRQ(void)
 }
 
 void __init
+chrp_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "CHRP: unable to allocate %s region.\n", desc);
+}
+
+void __init
 chrp_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	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");
+	chrp_request_region(0x20,0x20,"pic1");
+	chrp_request_region(0xa0,0x20,"pic2");
+	chrp_request_region(0x00,0x20,"dma1");
+	chrp_request_region(0x40,0x20,"timer");
+	chrp_request_region(0x80,0x10,"dma page reg");
+	chrp_request_region(0xc0,0x20,"dma2");
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/pseries/pci.c 
linux-2.6.16-rc5/arch/powerpc/platforms/pseries/pci.c
--- linux-2.6.16-rc5-orig/arch/powerpc/platforms/pseries/pci.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/powerpc/platforms/pseries/pci.c	2006-02-27 
21:49:54.000000000 +0100
@@ -92,17 +92,24 @@ void __devinit pSeries_irq_bus_setup(str
 	}
 }
 
+void __init
+pSeries_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "pSeries: unable to allocate %s region.\n", desc);
+}
+
 static void __init pSeries_request_regions(void)
 {
 	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");
+	pSeries_request_region(0x20,0x20,"pic1");
+	pSeries_request_region(0xa0,0x20,"pic2");
+	pSeries_request_region(0x00,0x20,"dma1");
+	pSeries_request_region(0x40,0x20,"timer");
+	pSeries_request_region(0x80,0x10,"dma page reg");
+	pSeries_request_region(0xc0,0x20,"dma2");
 }
 
 void __init pSeries_final_fixup(void)
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/chrp_setup.c 
linux-2.6.16-rc5/arch/ppc/platforms/chrp_setup.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/chrp_setup.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/chrp_setup.c	2006-02-27 
21:52:29.000000000 +0100
@@ -450,18 +450,25 @@ void __init chrp_init_IRQ(void)
 }
 
 void __init
+chrp_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "CHRP: unable to allocate %s region.\n", desc);
+}
+
+void __init
 chrp_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	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");
+	chrp_request_region(0x20,0x20,"pic1");
+	chrp_request_region(0xa0,0x20,"pic2");
+	chrp_request_region(0x00,0x20,"dma1");
+	chrp_request_region(0x40,0x20,"timer");
+	chrp_request_region(0x80,0x10,"dma page reg");
+	chrp_request_region(0xc0,0x20,"dma2");
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/mvme5100.c 
linux-2.6.16-rc5/arch/ppc/platforms/mvme5100.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/mvme5100.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/mvme5100.c	2006-02-27 
21:57:46.000000000 +0100
@@ -189,16 +189,23 @@ mvme5100_setup_arch(void)
 	return;
 }
 
+void __init
+mvme5100_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "mvme5100: unable to allocate %s region.\n", desc);
+}
+
 static void __init
 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");
+		mvme5100_request_region(0x00,0x20,"dma1");
+		mvme5100_request_region(0x20,0x20,"pic1");
+		mvme5100_request_region(0x40,0x20,"timer");
+		mvme5100_request_region(0x80,0x10,"dma page reg");
+		mvme5100_request_region(0xa0,0x20,"pic2");
+		mvme5100_request_region(0xc0,0x20,"dma2");
 #endif
 	return;
 }
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/pplus.c 
linux-2.6.16-rc5/arch/ppc/platforms/pplus.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/pplus.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/pplus.c	2006-02-27 21:59:07.000000000 
+0100
@@ -814,17 +814,24 @@ static void __init pplus_map_io(void)
 	io_block_mapping(0xfef80000, 0xfef80000, 0x00080000, _PAGE_IO);
 }
 
+void __init
+pplus_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "pplus: unable to allocate %s region.\n", desc);
+}
+
 static void __init pplus_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	request_region(PREP_NVRAM_AS0, 0x8, "nvram");
 #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");
+	pplus_request_region(0x20, 0x20, "pic1");
+	pplus_request_region(0xa0, 0x20, "pic2");
+	pplus_request_region(0x00, 0x20, "dma1");
+	pplus_request_region(0x40, 0x20, "timer");
+	pplus_request_region(0x80, 0x10, "dma page reg");
+	pplus_request_region(0xc0, 0x20, "dma2");
 }
 
 /*
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [KJ] [PATCH] check request_region() return value in arch/ppc and
  2006-02-27 21:29 Tim Cooijmans
  2006-02-28  5:33 ` Tim Cooijmans
@ 2006-02-28  5:53 ` Tim Cooijmans
  2006-02-28  6:18 ` Tim Cooijmans
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Tim Cooijmans @ 2006-02-28  5:53 UTC (permalink / raw)
  To: kernel-janitors

On Tuesday 28 February 2006 06:33, Tim Cooijmans wrote:
>  static void __init pplus_init2(void)
>  {
>  #ifdef CONFIG_NVRAM
>  	request_region(PREP_NVRAM_AS0, 0x8, "nvram");
>  #endif
Whoops, missed one there.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [KJ] [PATCH] check request_region() return value in arch/ppc and
  2006-02-27 21:29 Tim Cooijmans
  2006-02-28  5:33 ` Tim Cooijmans
  2006-02-28  5:53 ` Tim Cooijmans
@ 2006-02-28  6:18 ` Tim Cooijmans
  2006-02-28 21:48 ` Alexey Dobriyan
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Tim Cooijmans @ 2006-02-28  6:18 UTC (permalink / raw)
  To: kernel-janitors

From: Tim Cooijmans <tim@aapopfiets.nl>

Check request_region() return value and warn on failure.  Note that releasing 
succeeded requests doesn't make much sense, as the calling code doesn't know 
if anything failed.  This is not the case in lopec.c, prep_setup.c and 
sandpoint.c.

The powerpc code has been compile tested.  The ppc code has not been tested, 
but the changes are very similar.

Signed-off-by: Tim Cooijmans <tim@aapopfiets.nl>
---
Note: this patch is quite different from the previous one, it also handles 
request_region() in lopec.c, prep_setup.c and sandpoint.c.  There's only one 
unchecked request_region() left in arch/p{ower,}pc now, in 
arch/ppc/platforms/hdpu.c:hdpu_ide_request_region().  That function is not 
used, so I don't know if the return type can be changed.

diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c 
linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c
--- linux-2.6.16-rc5-orig/arch/powerpc/platforms/chrp/setup.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/powerpc/platforms/chrp/setup.c	2006-02-28 
07:03:41.000000000 +0100
@@ -465,18 +465,25 @@ void __init chrp_init_IRQ(void)
 }
 
 void __init
+chrp_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "chrp: unable to allocate %s region.\n", desc);
+}
+
+void __init
 chrp_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	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");
+	chrp_request_region(0x20,0x20,"pic1");
+	chrp_request_region(0xa0,0x20,"pic2");
+	chrp_request_region(0x00,0x20,"dma1");
+	chrp_request_region(0x40,0x20,"timer");
+	chrp_request_region(0x80,0x10,"dma page reg");
+	chrp_request_region(0xc0,0x20,"dma2");
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
diff -uprN linux-2.6.16-rc5-orig/arch/powerpc/platforms/pseries/pci.c 
linux-2.6.16-rc5/arch/powerpc/platforms/pseries/pci.c
--- linux-2.6.16-rc5-orig/arch/powerpc/platforms/pseries/pci.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/powerpc/platforms/pseries/pci.c	2006-02-28 
06:29:47.000000000 +0100
@@ -92,17 +92,24 @@ void __devinit pSeries_irq_bus_setup(str
 	}
 }
 
+void __init
+pSeries_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "pSeries: unable to allocate %s region.\n", desc);
+}
+
 static void __init pSeries_request_regions(void)
 {
 	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");
+	pSeries_request_region(0x20,0x20,"pic1");
+	pSeries_request_region(0xa0,0x20,"pic2");
+	pSeries_request_region(0x00,0x20,"dma1");
+	pSeries_request_region(0x40,0x20,"timer");
+	pSeries_request_region(0x80,0x10,"dma page reg");
+	pSeries_request_region(0xc0,0x20,"dma2");
 }
 
 void __init pSeries_final_fixup(void)
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/chrp_setup.c 
linux-2.6.16-rc5/arch/ppc/platforms/chrp_setup.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/chrp_setup.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/chrp_setup.c	2006-02-28 
07:03:56.000000000 +0100
@@ -450,18 +450,25 @@ void __init chrp_init_IRQ(void)
 }
 
 void __init
+chrp_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "chrp: unable to allocate %s region.\n", desc);
+}
+
+void __init
 chrp_init2(void)
 {
 #ifdef CONFIG_NVRAM
 	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");
+	chrp_request_region(0x20,0x20,"pic1");
+	chrp_request_region(0xa0,0x20,"pic2");
+	chrp_request_region(0x00,0x20,"dma1");
+	chrp_request_region(0x40,0x20,"timer");
+	chrp_request_region(0x80,0x10,"dma page reg");
+	chrp_request_region(0xc0,0x20,"dma2");
 
 	if (ppc_md.progress)
 		ppc_md.progress("  Have fun!    ", 0x7777);
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/lopec.c 
linux-2.6.16-rc5/arch/ppc/platforms/lopec.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/lopec.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/lopec.c	2006-02-28 06:47:09.000000000 
+0100
@@ -274,18 +274,33 @@ lopec_init_IRQ(void)
 	i8259_init(0xfef00000, 0);
 }
 
+struct resource * __init
+lopec_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	struct resource *res = request_region(start, n, desc);
+	if (!res)
+		printk(KERN_WARNING "lopec: unable to allocate %s region.\n", desc);
+	return res;
+}
+
 static int __init
 lopec_request_io(void)
 {
 	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 (!lopec_request_region(0x00, 0x20, "dma1"))
+		return -EBUSY;
+	if (!lopec_request_region(0x20, 0x20, "pic1"))
+		return -EBUSY;
+	if (!lopec_request_region(0x40, 0x20, "timer"))
+		return -EBUSY;
+	if (!lopec_request_region(0x80, 0x10, "dma page reg"))
+		return -EBUSY;
+	if (!lopec_request_region(0xa0, 0x20, "pic2"))
+		return -EBUSY;
+	if (!lopec_request_region(0xc0, 0x20, "dma2"))
+		return -EBUSY;
 
 	return 0;
 }
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/mvme5100.c 
linux-2.6.16-rc5/arch/ppc/platforms/mvme5100.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/mvme5100.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/mvme5100.c	2006-02-28 
06:29:47.000000000 +0100
@@ -189,16 +189,23 @@ mvme5100_setup_arch(void)
 	return;
 }
 
+void __init
+mvme5100_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "mvme5100: unable to allocate %s region.\n", desc);
+}
+
 static void __init
 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");
+		mvme5100_request_region(0x00,0x20,"dma1");
+		mvme5100_request_region(0x20,0x20,"pic1");
+		mvme5100_request_region(0x40,0x20,"timer");
+		mvme5100_request_region(0x80,0x10,"dma page reg");
+		mvme5100_request_region(0xa0,0x20,"pic2");
+		mvme5100_request_region(0xc0,0x20,"dma2");
 #endif
 	return;
 }
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/pplus.c 
linux-2.6.16-rc5/arch/ppc/platforms/pplus.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/pplus.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/pplus.c	2006-02-28 06:56:32.000000000 
+0100
@@ -814,17 +814,24 @@ static void __init pplus_map_io(void)
 	io_block_mapping(0xfef80000, 0xfef80000, 0x00080000, _PAGE_IO);
 }
 
+void __init
+pplus_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	if (!request_region(start, n, desc))
+		printk(KERN_WARNING "pplus: unable to allocate %s region.\n", desc);
+}
+
 static void __init pplus_init2(void)
 {
 #ifdef CONFIG_NVRAM
-	request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+	pplus_request_region(PREP_NVRAM_AS0, 0x8, "nvram");
 #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");
+	pplus_request_region(0x20, 0x20, "pic1");
+	pplus_request_region(0xa0, 0x20, "pic2");
+	pplus_request_region(0x00, 0x20, "dma1");
+	pplus_request_region(0x40, 0x20, "timer");
+	pplus_request_region(0x80, 0x10, "dma page reg");
+	pplus_request_region(0xc0, 0x20, "dma2");
 }
 
 /*
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/prep_setup.c 
linux-2.6.16-rc5/arch/ppc/platforms/prep_setup.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/prep_setup.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/prep_setup.c	2006-02-28 
06:49:43.000000000 +0100
@@ -1066,17 +1066,31 @@ prep_map_io(void)
 	io_block_mapping(0xf0000000, PREP_ISA_MEM_BASE, 0x08000000, _PAGE_IO);
 }
 
+struct resource * __init
+prep_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	struct resource *res = request_region(start, n, desc);
+	if (!res)
+		printk(KERN_WARNING "prep: unable to allocate %s region.\n", desc);
+	return res;
+}
+
 static int __init
 prep_request_io(void)
 {
 	if (_machine = _MACH_prep) {
 #ifdef CONFIG_NVRAM
-		request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+		if (!prep_request_region(PREP_NVRAM_AS0, 0x8, "nvram"))
+			return -EBUSY;
 #endif
-		request_region(0x00,0x20,"dma1");
-		request_region(0x40,0x20,"timer");
-		request_region(0x80,0x10,"dma page reg");
-		request_region(0xc0,0x20,"dma2");
+		if (!prep_request_region(0x00,0x20,"dma1"))
+			return -EBUSY;
+		if (!prep_request_region(0x40,0x20,"timer"))
+			return -EBUSY;
+		if (!prep_request_region(0x80,0x10,"dma page reg"))
+			return -EBUSY;
+		if (!prep_request_region(0xc0,0x20,"dma2"))
+			return -EBUSY;
 	}
 
 	return 0;
diff -uprN linux-2.6.16-rc5-orig/arch/ppc/platforms/sandpoint.c 
linux-2.6.16-rc5/arch/ppc/platforms/sandpoint.c
--- linux-2.6.16-rc5-orig/arch/ppc/platforms/sandpoint.c	2006-02-27 
20:36:36.000000000 +0100
+++ linux-2.6.16-rc5/arch/ppc/platforms/sandpoint.c	2006-02-28 
06:51:53.000000000 +0100
@@ -460,15 +460,30 @@ sandpoint_setup_natl_87308(void)
 
 arch_initcall(sandpoint_setup_natl_87308);
 
+struct resource * __init
+sandpoint_request_region(unsigned long start, unsigned long n, char *desc)
+{
+	struct resource *res = request_region(start, n, desc);
+	if (!res)
+		printk(KERN_WARNING "sandpoint: unable to allocate %s region.\n", desc);
+	return res;
+}
+
 static int __init
 sandpoint_request_io(void)
 {
-	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 (!sandpoint_request_region(0x00, 0x20, "dma1"))
+		return -EBUSY;
+	if (!sandpoint_request_region(0x20, 0x20, "pic1"))
+		return -EBUSY;
+	if (!sandpoint_request_region(0x40, 0x20, "timer"))
+		return -EBUSY;
+	if (!sandpoint_request_region(0x80, 0x10, "dma page reg"))
+		return -EBUSY;
+	if (!sandpoint_request_region(0xa0, 0x20, "pic2"))
+		return -EBUSY;
+	if (!sandpoint_request_region(0xc0, 0x20, "dma2"))
+		return -EBUSY;
 
 	return 0;
 }
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [KJ] [PATCH] check request_region() return value in arch/ppc and
  2006-02-27 21:29 Tim Cooijmans
                   ` (2 preceding siblings ...)
  2006-02-28  6:18 ` Tim Cooijmans
@ 2006-02-28 21:48 ` Alexey Dobriyan
  2006-02-28 22:00 ` Jesper Juhl
  2006-03-01  5:46 ` Tim Cooijmans
  5 siblings, 0 replies; 8+ messages in thread
From: Alexey Dobriyan @ 2006-02-28 21:48 UTC (permalink / raw)
  To: kernel-janitors

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

On Tue, Feb 28, 2006 at 07:18:58AM +0100, Tim Cooijmans wrote:
>  void __init
> +chrp_request_region(unsigned long start, unsigned long n, char *desc)
> +{
> +	if (!request_region(start, n, desc))
> +		printk(KERN_WARNING "chrp: unable to allocate %s region.\n", desc);
> +}
> +
> +void __init
>  chrp_init2(void)
>  {
>  #ifdef CONFIG_NVRAM
>  	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");
> +	chrp_request_region(0x20,0x20,"pic1");
> +	chrp_request_region(0xa0,0x20,"pic2");
> +	chrp_request_region(0x00,0x20,"dma1");
> +	chrp_request_region(0x40,0x20,"timer");
> +	chrp_request_region(0x80,0x10,"dma page reg");
> +	chrp_request_region(0xc0,0x20,"dma2");

OK, you've followed this route. One question they'll ask me is

	Can those calls ever fail?

I mean, you're deeply in arch-specific setup code. And request_region()
parameters are hardwired. And numbers are common among other subarchs in
this patch.


[-- 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] 8+ messages in thread

* Re: [KJ] [PATCH] check request_region() return value in arch/ppc and
  2006-02-27 21:29 Tim Cooijmans
                   ` (3 preceding siblings ...)
  2006-02-28 21:48 ` Alexey Dobriyan
@ 2006-02-28 22:00 ` Jesper Juhl
  2006-03-01  5:46 ` Tim Cooijmans
  5 siblings, 0 replies; 8+ messages in thread
From: Jesper Juhl @ 2006-02-28 22:00 UTC (permalink / raw)
  To: kernel-janitors

On 2/28/06, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Tue, Feb 28, 2006 at 07:18:58AM +0100, Tim Cooijmans wrote:
> >  void __init
> > +chrp_request_region(unsigned long start, unsigned long n, char *desc)
> > +{
> > +     if (!request_region(start, n, desc))
> > +             printk(KERN_WARNING "chrp: unable to allocate %s region.\n", desc);
> > +}
> > +
> > +void __init
> >  chrp_init2(void)
> >  {
> >  #ifdef CONFIG_NVRAM
> >       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");
> > +     chrp_request_region(0x20,0x20,"pic1");
> > +     chrp_request_region(0xa0,0x20,"pic2");
> > +     chrp_request_region(0x00,0x20,"dma1");
> > +     chrp_request_region(0x40,0x20,"timer");
> > +     chrp_request_region(0x80,0x10,"dma page reg");
> > +     chrp_request_region(0xc0,0x20,"dma2");
>
> OK, you've followed this route. One question they'll ask me is
>
>         Can those calls ever fail?
>
> I mean, you're deeply in arch-specific setup code. And request_region()
> parameters are hardwired. And numbers are common among other subarchs in
> this patch.
>
>

I believe the comments Paul Mackerras gave after he NACK'ed a similar
patch of mine in -mm explains this very well.  The thread is on the
mm-commits list, but I've quoted all of it below for your convenience
(the interresting bits are at the end) :


---< cut here >---


The patch titled

    request_region fixes for ppc prep_request_io

has been added to the -mm tree.  Its filename is

    request_region-fixes-for-ppc-prep_request_io.patch

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this


From: Jesper Juhl <jesper.juhl@gmail.com>

request_region fixes for ppc prep_request_io.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 arch/ppc/platforms/prep_setup.c |   51 +++++++++++++++++++++++++-----
 1 files changed, 44 insertions(+), 7 deletions(-)

diff -puN arch/ppc/platforms/prep_setup.c~request_region-fixes-for-ppc-prep_request_io
arch/ppc/platforms/prep_setup.c
--- 25/arch/ppc/platforms/prep_setup.c~request_region-fixes-for-ppc-prep_request_io
    Fri Feb 24 15:10:27 2006
+++ 25-akpm/arch/ppc/platforms/prep_setup.c     Fri Feb 24 15:10:27 2006
@@ -1064,20 +1064,57 @@ prep_map_io(void)
       io_block_mapping(0xf0000000, PREP_ISA_MEM_BASE, 0x08000000, _PAGE_IO);
 }

+static inline int
+prep_request_region(unsigned long start, unsigned long n, const char *name)
+{
+       struct resource *region;
+
+       region = request_region(start, n, name);
+       if (!region) {
+               printk(KERN_WARNING "prep_request_io: could not allocate "
+                                   "%s region\n", name);
+               return 0;
+       }
+       return 1;
+}
+
 static int __init
 prep_request_io(void)
 {
-       if (_machine = _MACH_prep) {
+       int tmp;
+
+       if (_machine != _MACH_prep)
+               return 0;
+
 #ifdef CONFIG_NVRAM
-               request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+       tmp = prep_request_region(PREP_NVRAM_AS0, 0x8, "nvram");
+       if (!tmp)
+               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");
-       }
+       tmp = prep_request_region(0x00,0x20,"dma1");
+       if (!tmp)
+               goto out_dma1;
+       tmp = prep_request_region(0x40,0x20,"timer");
+       if (!tmp)
+               goto out_timer;
+       tmp = prep_request_region(0x80,0x10,"dma page reg");
+       if (!tmp)
+               goto out_dma_page;
+       tmp = prep_request_region(0xc0,0x20,"dma2");
+       if (!tmp)
+               goto out_dma2;

       return 0;
+ 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:
+       return -EBUSY;
 }

 device_initcall(prep_request_io);
_

Patches currently in -mm which might be from jesper.juhl@gmail.com are

git-net.patch
reduce-nr-of-ptr-derefs-in-fs-jffs2-summaryc.patch
isdn-fix-copy_to_user-unused-result-warning-in-isdn_ppp.patch
docs-update-missing-files-and-descriptions-for-filesystems-00-index.patch
debug-shared-irqs.patch
decrease-number-of-pointer-derefs-in-jsm_ttyc.patch
sound-remove-unneeded-kmalloc-return-value-casts.patch
dont-check-vfree-argument-for-null-in-vx_pcm.patch
dont-check-vfree-arg-for-null-in-usbaudio.patch
small-whitespace-cleanup-for-qlogic-driver.patch
dont-null-check-vfree-argument-in-pdaudiocf_pcm.patch
request_region-fixes-for-ppc-prep_request_io.patch
vfree-null-check-fixup-for-sb_card.patch
maestro3-vfree-null-check-fixup.patch
ne2000-kconfig-help-entry-improvement.patch
no-need-to-check-vfree-arg-for-null-in-oss-sequencer.patch
vfree-does-its-own-null-check-no-need-to-be-explicit-in-oss-msndc.patch
fix-signed-vs-unsigned-in-nmi-watchdog.patch
const-static-vs-static-const-in-nfs4.patch
trivial-typos-in-documentation-cputopologytxt.patch


Reply	Reply to all	Forward	Invite akpm@osdl.org to Gmail
	
		
		
Paul Mackerras 	
to akpm, me, benh, mm-commits
	 More options	  Feb 26 (2 days ago)
akpm@osdl.org writes:

> The patch titled
>
>      request_region fixes for ppc prep_request_io
>
> has been added to the -mm tree.  Its filename is
>
>      request_region-fixes-for-ppc-prep_request_io.patch
>
> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> out what to do about this

Nack.  Printing a message if one of the request_regions fails is OK,
but why release all the previous regions, and why not go on to request
the rest?

> +     tmp = prep_request_region(0x00,0x20,"dma1");
> +     if (!tmp)
> +             goto out_dma1;

Use of the tmp variable is unnecessary and verbose.

Paul.

Reply	Reply to all	Forward	Invite Paul to Gmail
	
		
		
Jesper Juhl 	
to Paul, akpm, benh, mm-commits
	 More options	  Feb 26 (2 days ago)
On 2/26/06, Paul Mackerras <paulus@samba.org> wrote:
> akpm@osdl.org writes:
>
> > The patch titled
> >
> >      request_region fixes for ppc prep_request_io
> >
> > has been added to the -mm tree.  Its filename is
> >
> >      request_region-fixes-for-ppc-prep_request_io.patch
> >
> > See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> > out what to do about this
>
> Nack.

Thanks. I appreciate you taking the time to review the patch.

> Printing a message if one of the request_regions fails is OK,

OK.

> but why release all the previous regions, and why not go on to request
> the rest?

Probably because I misunderstood the code.

As I read it, all regions must be requested successfully or none at
all, and in that case (one or more failing) we should report an error
back.

But from your comment it is clear that that is a misunderstanding.
Would you mind enlightening me on the purpose of the code and what
will happen if one or more of the request_regions fail but we just
continue regardless ?

>
> > +     tmp = prep_request_region(0x00,0x20,"dma1");
> > +     if (!tmp)
> > +             goto out_dma1;
>
> Use of the tmp variable is unnecessary and verbose.
>
True.

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

Reply	Reply to all	Forward
	
		
		
Paul Mackerras 	
to me, akpm, benh, mm-commits
	 More options	  7:10 am (15 hours ago)
Jesper Juhl writes:

> > but why release all the previous regions, and why not go on to request
> > the rest?
>
> Probably because I misunderstood the code.
>
> As I read it, all regions must be requested successfully or none at
> all, and in that case (one or more failing) we should report an error
> back.

Well, returning an error code from an initcall function doesn't
actually do anything.

In the case of this code, it isn't that there is some driver which can
refuse to load if an error is reported, since these are essential
platform functions that the I/O port ranges are being reserved for
(DMA controller, timer, nvram).  The code that uses these devices is
elsewhere and doesn't change its behaviour if the request_region calls
fail.

The main reasons for doing the request_region calls is so that the
user sees the ranges marked as reserved in /proc/ioports, and to stop
any other driver from requesting those regions.

> But from your comment it is clear that that is a misunderstanding.
> Would you mind enlightening me on the purpose of the code and what
> will happen if one or more of the request_regions fail but we just
> continue regardless ?

That's a good question.  The only reasons I can think of for the
request_region calls failing are that there is some kernel bug causing
the request to fail even though nothing else has done an overlapping
request, or that some buggy driver has requested an overlapping
region.  In the first case, the code that uses the DMA controller,
timer or nvram should still work fine.

In the second case, it depends on whether the buggy driver is also
poking I/O ports in the region(s) it has requested.  (The driver must
be buggy, because a PReP platform would never have an actual device
with I/O port regions that overlapped these system devices.)  If the
driver has just requested the wrong ports but is doing I/O to the
correct ports, then things will still work.  If the driver is doing
I/O to the wrong ports then anything can happen.

There is an argument that if there is the possibility that some driver
is doing I/O to the wrong ports, we should just panic to avoid the
possibility of data corruption.  On the other hand there is the
argument that we should try to continue running if possible.

Actual PReP machines are pretty old by now, since it is an old
specification that has been superseded.  There are possibly embedded
PPC boards designed to the PReP spec still being made, though.  Given
that, I tend to lean towards the "continue running" side of the
argument.  (Also, continuing to run will mean that the user can look
in /proc/ioports and see what has done the interfering request.)

Paul.




--
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] 8+ messages in thread

* Re: [KJ] [PATCH] check request_region() return value in arch/ppc and
  2006-02-27 21:29 Tim Cooijmans
                   ` (4 preceding siblings ...)
  2006-02-28 22:00 ` Jesper Juhl
@ 2006-03-01  5:46 ` Tim Cooijmans
  5 siblings, 0 replies; 8+ messages in thread
From: Tim Cooijmans @ 2006-03-01  5:46 UTC (permalink / raw)
  To: kernel-janitors

On Tuesday 28 February 2006 22:48, Alexey Dobriyan wrote:
> OK, you've followed this route. One question they'll ask me is
>
> 	Can those calls ever fail?
>
> I mean, you're deeply in arch-specific setup code. And request_region()
> parameters are hardwired. And numbers are common among other subarchs in
> this patch.
That's a good question.  Jesper's e-mail explains it well, too.  So, no 
request_region checking in /arch?  Should it only be done in drivers?

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2006-03-01  5:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-27 21:47 [KJ] [PATCH] check request_region() return value in arch/ppc and Nishanth Aravamudan
  -- strict thread matches above, loose matches on Subject: below --
2006-02-27 21:29 Tim Cooijmans
2006-02-28  5:33 ` Tim Cooijmans
2006-02-28  5:53 ` Tim Cooijmans
2006-02-28  6:18 ` Tim Cooijmans
2006-02-28 21:48 ` Alexey Dobriyan
2006-02-28 22:00 ` Jesper Juhl
2006-03-01  5:46 ` Tim Cooijmans

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.