From patchwork Tue Dec 16 10:52:41 2008 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: roel kluin X-Patchwork-Id: 14211 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from ozlabs.org (localhost [127.0.0.1]) by ozlabs.org (Postfix) with ESMTP id 4544547517 for ; Tue, 16 Dec 2008 22:02:19 +1100 (EST) X-Original-To: linuxppc-dev@ozlabs.org Delivered-To: linuxppc-dev@ozlabs.org X-Greylist: delayed 488 seconds by postgrey-1.31 at ozlabs; Tue, 16 Dec 2008 22:00:54 EST Received: from mail-ew0-f20.google.com (mail-ew0-f20.google.com [209.85.219.20]) by ozlabs.org (Postfix) with ESMTP id DA156DDF75 for ; Tue, 16 Dec 2008 22:00:54 +1100 (EST) Received: by ewy13 with SMTP id 13so4627957ewy.9 for ; Tue, 16 Dec 2008 03:00:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:cc:subject:references:in-reply-to :content-type:content-transfer-encoding; bh=5i16AYQRDDBZEMq1Czhi3UVjT/OebiJsKMd3kIKSGkw=; b=UEel3kqRnDJGo/w/X6em7OZND3dIGw/dV1WEavqrLBOYYbzhf6lMchRxR8TU5bMg2r 717F+Mp+IW2c3ksPmwbIAoFAyCOneNmsW2G0QJlsoOlGPQhfa772WV6dDJ2TV/3xV2Ub y6UfqlKAH84zDhIdVi5HxtVrV70jFpxuo1/5g= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; b=OFqHZJs+VJgH5IMj3UpAVWiboJX5obySZBgAgs+0w9nZzEh1BOXMHYWBUVSIgTENxF mpvTEc4hLGwdE4bxCNHvmoVn6He+roXHqNwidHZR4rFoRBWdc99DKKqOJsNWmAQ5HVvT Ioy9YI4qUv9STvthhjIS4LYkmsB9OKSAbjHsA= Received: by 10.210.73.12 with SMTP id v12mr9040850eba.107.1229424762873; Tue, 16 Dec 2008 02:52:42 -0800 (PST) Received: from ?192.168.1.148? (d133062.upc-d.chello.nl [213.46.133.62]) by mx.google.com with ESMTPS id z37sm2383510ikz.0.2008.12.16.02.52.41 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 16 Dec 2008 02:52:41 -0800 (PST) Message-ID: <49478879.2030409@gmail.com> Date: Tue, 16 Dec 2008 11:52:41 +0100 From: Roel Kluin User-Agent: Thunderbird 2.0.0.18 (X11/20081105) MIME-Version: 1.0 To: Olof Johansson Subject: Re: [PATCH] [POWERPC] pasemi: ioremap/iounmap balance and failure handling References: <4943E6B5.6010501@gmail.com> <20081213211335.GC17331@lixom.net> In-Reply-To: <20081213211335.GC17331@lixom.net> Cc: linuxppc-dev@ozlabs.org X-BeenThere: linuxppc-dev@ozlabs.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@ozlabs.org Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@ozlabs.org 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 --- 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);