* [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling
@ 2008-12-13 16:45 Roel Kluin
2008-12-13 19:15 ` Roel Kluin
2008-12-13 21:13 ` Olof Johansson
0 siblings, 2 replies; 5+ messages in thread
From: Roel Kluin @ 2008-12-13 16:45 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be
found.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
UNTESTED! I am a bit new, so please confirm whether this is correct. especially:
* can we iounmap while init_lock is held?
* is it ok to add another BUG() here?
diff --git a/arch/powerpc/platforms/pasemi/dma_lib.c b/arch/powerpc/platforms/pasemi/dma_lib.c
index 217af32..bdf5440 100644
--- a/arch/powerpc/platforms/pasemi/dma_lib.c
+++ b/arch/powerpc/platforms/pasemi/dma_lib.c
@@ -534,14 +534,21 @@ int pasemi_dma_init(void)
err = -ENODEV;
goto out;
}
+
iob_regs = map_onedev(iob_pdev, 0);
+ if (iob_regs == NULL) {
+ BUG();
+ printk(KERN_WARNING "Can't ioremap I/O Bridge registers\n");
+ err = -ENODEV;
+ goto out;
+ }
dma_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa007, NULL);
if (!dma_pdev) {
BUG();
printk(KERN_WARNING "Can't find DMA controller\n");
err = -ENODEV;
- goto out;
+ goto out_unmap;
}
dma_regs = map_onedev(dma_pdev, 0);
base_hw_irq = virq_to_hw(dma_pdev->irq);
@@ -624,6 +631,10 @@ int pasemi_dma_init(void)
printk(KERN_INFO "PA Semi PWRficient DMA library initialized "
"(%d tx, %d rx channels)\n", num_txch, num_rxch);
+ goto out;
+
+out_unmap:
+ iounmap(iob_regs);
out:
spin_unlock(&init_lock);
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling
2008-12-13 16:45 [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling Roel Kluin
@ 2008-12-13 19:15 ` Roel Kluin
2008-12-13 21:13 ` Olof Johansson
1 sibling, 0 replies; 5+ messages in thread
From: Roel Kluin @ 2008-12-13 19:15 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
Roel Kluin wrote:
> map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be
> found.
I think there may also be a problem with pasemi_mac_init_module(): if
pci_register_driver() fails, then iob_regs won't get iounmapped.
maybe something like the totally untested patch below?
diff --git a/arch/powerpc/include/asm/pasemi_dma.h b/arch/powerpc/include/asm/pasemi_dma.h
index 19fd793..7256a2c 100644
--- a/arch/powerpc/include/asm/pasemi_dma.h
+++ b/arch/powerpc/include/asm/pasemi_dma.h
@@ -535,4 +535,7 @@ extern void pasemi_dma_free_fun(int fun);
/* Initialize the library, must be called before any other functions */
extern int pasemi_dma_init(void);
+/* Clean up the library */
+extern void pasemi_dma_exit(void);
+
#endif /* ASM_PASEMI_DMA_H */
diff --git a/arch/powerpc/platforms/pasemi/dma_lib.c b/arch/powerpc/platforms/pasemi/dma_lib.c
index 217af32..fc0e1f9 100644
--- a/arch/powerpc/platforms/pasemi/dma_lib.c
+++ b/arch/powerpc/platforms/pasemi/dma_lib.c
@@ -534,14 +534,21 @@ int pasemi_dma_init(void)
err = -ENODEV;
goto out;
}
+
iob_regs = map_onedev(iob_pdev, 0);
+ if (iob_regs == NULL) {
+ BUG();
+ printk(KERN_WARNING "Can't ioremap I/O Bridge registers\n");
+ err = -ENODEV;
+ goto out;
+ }
dma_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa007, NULL);
if (!dma_pdev) {
BUG();
printk(KERN_WARNING "Can't find DMA controller\n");
err = -ENODEV;
- goto out;
+ goto out_unmap;
}
dma_regs = map_onedev(dma_pdev, 0);
base_hw_irq = virq_to_hw(dma_pdev->irq);
@@ -624,9 +631,19 @@ int pasemi_dma_init(void)
printk(KERN_INFO "PA Semi PWRficient DMA library initialized "
"(%d tx, %d rx channels)\n", num_txch, num_rxch);
+ goto out;
+
+out_unmap:
+ iounmap(iob_regs);
out:
spin_unlock(&init_lock);
return err;
}
EXPORT_SYMBOL(pasemi_dma_init);
+
+void pasemi_dma_exit(void)
+{
+ iounmap(iob_regs);
+}
+
diff --git a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
index edc0fd5..0897fa0 100644
--- a/drivers/net/pasemi_mac.c
+++ b/drivers/net/pasemi_mac.c
@@ -1919,7 +1919,11 @@ int pasemi_mac_init_module(void)
if (err)
return err;
- return pci_register_driver(&pasemi_mac_driver);
+ err = pci_register_driver(&pasemi_mac_driver);
+ if (err)
+ pasemi_dma_exit();
+
+ return err;
}
module_init(pasemi_mac_init_module);
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling
2008-12-13 16:45 [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling Roel Kluin
2008-12-13 19:15 ` Roel Kluin
@ 2008-12-13 21:13 ` Olof Johansson
2008-12-16 10:52 ` Roel Kluin
1 sibling, 1 reply; 5+ messages in thread
From: Olof Johansson @ 2008-12-13 21:13 UTC (permalink / raw)
To: Roel Kluin; +Cc: linuxppc-dev
On Sat, Dec 13, 2008 at 05:45:41PM +0100, Roel Kluin wrote:
> map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be
> found.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> UNTESTED! I am a bit new, so please confirm whether this is correct. especially:
>
> * can we iounmap while init_lock is held?
Yes, should be OK.
> * is it ok to add another BUG() here?
>
> diff --git a/arch/powerpc/platforms/pasemi/dma_lib.c b/arch/powerpc/platforms/pasemi/dma_lib.c
> index 217af32..bdf5440 100644
> --- a/arch/powerpc/platforms/pasemi/dma_lib.c
> +++ b/arch/powerpc/platforms/pasemi/dma_lib.c
> @@ -534,14 +534,21 @@ int pasemi_dma_init(void)
> err = -ENODEV;
> goto out;
> }
> +
> iob_regs = map_onedev(iob_pdev, 0);
> + if (iob_regs == NULL) {
> + BUG();
> + printk(KERN_WARNING "Can't ioremap I/O Bridge registers\n");
> + err = -ENODEV;
> + goto out;
> + }
I don't see the point of doing BUG() _and_ error recovery. BUG() is
definitely heavy handed. Something like "pasemi_dma_init: Can't..." would
be a good and detailed enough error message.
>
> dma_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa007, NULL);
> if (!dma_pdev) {
> BUG();
> printk(KERN_WARNING "Can't find DMA controller\n");
> err = -ENODEV;
> - goto out;
> + goto out_unmap;
> }
> dma_regs = map_onedev(dma_pdev, 0);
> base_hw_irq = virq_to_hw(dma_pdev->irq);
> @@ -624,6 +631,10 @@ int pasemi_dma_init(void)
>
> printk(KERN_INFO "PA Semi PWRficient DMA library initialized "
> "(%d tx, %d rx channels)\n", num_txch, num_rxch);
> + goto out;
> +
> +out_unmap:
> + iounmap(iob_regs);
>
> out:
> spin_unlock(&init_lock);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling
2008-12-13 21:13 ` Olof Johansson
@ 2008-12-16 10:52 ` Roel Kluin
2009-01-07 0:50 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 5+ messages in thread
From: Roel Kluin @ 2008-12-16 10:52 UTC (permalink / raw)
To: Olof Johansson; +Cc: linuxppc-dev
Olof Johansson wrote:
> On Sat, Dec 13, 2008 at 05:45:41PM +0100, Roel Kluin wrote:
>> map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be
>> found.
>> +
>> iob_regs = map_onedev(iob_pdev, 0);
>> + if (iob_regs == NULL) {
>> + BUG();
>> + printk(KERN_WARNING "Can't ioremap I/O Bridge registers\n");
>> + err = -ENODEV;
>> + goto out;
>> + }
>
> I don't see the point of doing BUG() _and_ error recovery. BUG() is
> definitely heavy handed. Something like "pasemi_dma_init: Can't..." would
> be a good and detailed enough error message.
Thanks for reviewing. This patch should address your comments to patch V1.
As I asked in my V2, I think there may also be a problem with
pasemi_mac_init_module(): if pci_register_driver() fails, then iob_regs won't get
iounmapped. right?
Is it ok to add the function pasemi_dma_exit() for these cleanup purposes?
and is it correct that we don't need to EXPORT_SYMBOL(pasemi_dma_exit)?
---------------------8<------------>8-------------------
map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be
found.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
arch/powerpc/include/asm/pasemi_dma.h | 3 +++
arch/powerpc/platforms/pasemi/dma_lib.c | 15 ++++++++++++++-
drivers/net/pasemi_mac.c | 6 +++++-
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/pasemi_dma.h b/arch/powerpc/include/asm/pasemi_dma.h
index 19fd793..7256a2c 100644
--- a/arch/powerpc/include/asm/pasemi_dma.h
+++ b/arch/powerpc/include/asm/pasemi_dma.h
@@ -535,4 +535,7 @@ extern void pasemi_dma_free_fun(int fun);
/* Initialize the library, must be called before any other functions */
extern int pasemi_dma_init(void);
+/* Clean up the library */
+extern void pasemi_dma_exit(void);
+
#endif /* ASM_PASEMI_DMA_H */
diff --git a/arch/powerpc/platforms/pasemi/dma_lib.c b/arch/powerpc/platforms/pasemi/dma_lib.c
index 217af32..c97fba9 100644
--- a/arch/powerpc/platforms/pasemi/dma_lib.c
+++ b/arch/powerpc/platforms/pasemi/dma_lib.c
@@ -534,14 +534,17 @@ int pasemi_dma_init(void)
err = -ENODEV;
goto out;
}
+
iob_regs = map_onedev(iob_pdev, 0);
+ if (iob_regs == NULL)
+ printk(KERN_WARNING "pasemi_dma_init: Can't ioremap I/O Bridge registers\n");
dma_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa007, NULL);
if (!dma_pdev) {
BUG();
printk(KERN_WARNING "Can't find DMA controller\n");
err = -ENODEV;
- goto out;
+ goto out_unmap;
}
dma_regs = map_onedev(dma_pdev, 0);
base_hw_irq = virq_to_hw(dma_pdev->irq);
@@ -624,9 +627,19 @@ int pasemi_dma_init(void)
printk(KERN_INFO "PA Semi PWRficient DMA library initialized "
"(%d tx, %d rx channels)\n", num_txch, num_rxch);
+ goto out;
+
+out_unmap:
+ iounmap(iob_regs);
out:
spin_unlock(&init_lock);
return err;
}
EXPORT_SYMBOL(pasemi_dma_init);
+
+void pasemi_dma_exit(void)
+{
+ iounmap(iob_regs);
+}
+
diff --git a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
index edc0fd5..0897fa0 100644
--- a/drivers/net/pasemi_mac.c
+++ b/drivers/net/pasemi_mac.c
@@ -1919,7 +1919,11 @@ int pasemi_mac_init_module(void)
if (err)
return err;
- return pci_register_driver(&pasemi_mac_driver);
+ err = pci_register_driver(&pasemi_mac_driver);
+ if (err)
+ pasemi_dma_exit();
+
+ return err;
}
module_init(pasemi_mac_init_module);
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling
2008-12-16 10:52 ` Roel Kluin
@ 2009-01-07 0:50 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2009-01-07 0:50 UTC (permalink / raw)
To: Roel Kluin; +Cc: Olof Johansson, linuxppc-dev
> +
> iob_regs = map_onedev(iob_pdev, 0);
> + if (iob_regs == NULL)
> + printk(KERN_WARNING "pasemi_dma_init: Can't ioremap I/O Bridge registers\n");
>
> dma_pdev = pci_get_device(PCI_VENDOR_ID_PASEMI, 0xa007, NULL);
> if (!dma_pdev) {
> BUG();
> printk(KERN_WARNING "Can't find DMA controller\n");
> err = -ENODEV;
> - goto out;
> + goto out_unmap;
> }
None of that will be reached due to the previous BUG :-)
Cheers,
Ben.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-01-07 0:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-13 16:45 [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling Roel Kluin
2008-12-13 19:15 ` Roel Kluin
2008-12-13 21:13 ` Olof Johansson
2008-12-16 10:52 ` Roel Kluin
2009-01-07 0:50 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).