Skip to contentSkip to conversation

pasemi: ioremap/iounmap balance and failure handling

PATCH In review Roel Kluin 3 participants 5 messages Dec 13, 2008 – Jan 7, 2009

Subsystems (2)
Network drivers PowerPC
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 pasemi: ioremap/iounmap balance and failure handling

map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be found.

Signed-off-by Roel Kluin <[email protected]>

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

arch/powerpc/platforms/pasemi/dma_lib.c +12 -1
index 217af32..bdf5440 100644
@@ -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);
author
#1 lore raw
Quoted 2 lines · Roel Kluin

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?

arch/powerpc/include/asm/pasemi_dma.h +3 -0
index 19fd793..7256a2c 100644
@@ -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 */
arch/powerpc/platforms/pasemi/dma_lib.c +18 -1
index 217af32..fc0e1f9 100644
@@ -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);
+}
+
drivers/net/pasemi_mac.c +5 -1
index edc0fd5..0897fa0 100644
@@ -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);
#2 lore raw
In reply to Roel Kluin
Quoted 8 lines · Roel Kluin

map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be found.

Signed-off-by Roel Kluin <[email protected]>

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

Quoted 8 lines

* is it ok to add another BUG() here?

arch/powerpc/platforms/pasemi/dma_lib.c +0 -0
index 217af32..bdf5440 100644
@@ -534,14 +534,21 @@ int pasemi_dma_init(void)
 		err = -ENODEV;

arch/powerpc/platforms/pasemi/dma_lib.c int pasemi_dma_init(void) · line 544

Quoted 10 lines
arch/powerpc/platforms/pasemi/dma_lib.c +7 -0
 		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.

Quoted 22 lines
arch/powerpc/platforms/pasemi/dma_lib.c +5 -1
 	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);
author
#3 lore raw
Quoted 4 lines · Olof Johansson
Quoted 2 lines · Roel Kluin

map_onedev can return NULL, so catch that. Also iounmap if DMA controller can't be found.

Quoted 12 lines

Review

Quoted 8 lines
(quoted diff) +7 -0
+
 	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 <[email protected]>
---
 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(-)
arch/powerpc/include/asm/pasemi_dma.h +3 -0
index 19fd793..7256a2c 100644
@@ -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 */
arch/powerpc/platforms/pasemi/dma_lib.c +14 -1
index 217af32..c97fba9 100644
@@ -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);
+}
+
drivers/net/pasemi_mac.c +5 -1
index edc0fd5..0897fa0 100644
@@ -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);
#4 lore raw
Quoted 5 lines
arch/powerpc/platforms/pasemi/dma_lib.c +2 -0
+
 	iob_regs = map_onedev(iob_pdev, 0);
+	if (iob_regs == NULL)

arch/powerpc/platforms/pasemi/dma_lib.c int pasemi_dma_init(void) · line 548

Quoted 10 lines
arch/powerpc/platforms/pasemi/dma_lib.c +2 -1
+		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.