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