Received: with ECARTIS (v1.0.0; list netdev); Fri, 12 Sep 2003 15:07:18 -0700 (PDT) Received: from www.linux.org.uk (IDENT:93@parcelfarce.linux.theplanet.co.uk [195.92.249.252]) by oss.sgi.com (8.12.9/8.12.5) with SMTP id h8CM7BYa000341 for ; Fri, 12 Sep 2003 15:07:12 -0700 Received: from rdu74-153-143.nc.rr.com ([24.74.153.143]:40360 helo=pobox.com) by www.linux.org.uk with esmtp (Exim 4.22) id 19xw4I-0005xc-5y; Fri, 12 Sep 2003 23:07:06 +0100 Message-ID: <3F62437F.9080202@pobox.com> Date: Fri, 12 Sep 2003 18:06:55 -0400 From: Jeff Garzik Organization: none User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20021213 Debian/1.2.1-2.bunk X-Accept-Language: en MIME-Version: 1.0 To: Stephen Hemminger CC: Jes Sorensen , netdev@oss.sgi.com Subject: Re: [PATCH] Road Runner HIPPI driver (rrunner) References: <20030912144208.2886e2b9.shemminger@osdl.org> In-Reply-To: <20030912144208.2886e2b9.shemminger@osdl.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-archive-position: 5868 X-ecartis-version: Ecartis v1.0.0 Sender: netdev-bounce@oss.sgi.com Errors-to: netdev-bounce@oss.sgi.com X-original-sender: jgarzik@pobox.com Precedence: bulk X-list: netdev Content-Length: 1966 Lines: 70 Stephen Hemminger wrote: > @@ -209,9 +208,9 @@ static int __devinit rr_init_one(struct > > dev->base_addr = 0; > > - ret = register_netdev(dev); > - if (ret) > + if (register_netdev(dev)) > goto out; > + > return 0; > > out: We lose the register_netdev() return value, whereas the driver got it right before. > static void __devexit rr_remove_one (struct pci_dev *pdev) > { > - struct net_device *dev = pci_get_drvdata(pdev); > - struct rr_private *rr = (struct rr_private *)dev->priv; > + struct net_device *dev; > + struct rr_private *rr; > > - if (dev) { > - if (!(readl(&rr->regs->HostCtrl) & NIC_HALTED)){ > - printk(KERN_ERR "%s: trying to unload running NIC\n", > - dev->name); > - writel(HALT_NIC, &rr->regs->HostCtrl); > - } > - > - pci_free_consistent(pdev, EVT_RING_SIZE, rr->evt_ring, > - rr->evt_ring_dma); > - pci_free_consistent(pdev, RX_TOTAL_SIZE, rr->rx_ring, > - rr->rx_ring_dma); > - pci_free_consistent(pdev, TX_TOTAL_SIZE, rr->tx_ring, > - rr->tx_ring_dma); > - unregister_netdev(dev); > - iounmap(rr->regs); > - free_netdev(dev); > - pci_release_regions(pdev); > - pci_disable_device(pdev); > - pci_set_drvdata(pdev, NULL); > + if (!(dev = pci_get_drvdata(pdev))) > + return; this is a fairly ugly construct. It looks better as the driver had it, as an initializer. Don't combine C statements when you don't need to. > @@ -1201,8 +1205,8 @@ static int rr_open(struct net_device *de > readl(®s->HostCtrl); > spin_unlock_irqrestore(&rrpriv->lock, flags); > > - if (request_irq(dev->irq, rr_interrupt, SA_SHIRQ, rrpriv->name, dev)) > - { > + if (request_irq(dev->irq, rr_interrupt, SA_SHIRQ, > + "RoadRunner serial HIPPI", dev)) { > printk(KERN_WARNING "%s: Requested IRQ %d is busy\n", > dev->name, dev->irq); > ecode = -EAGAIN; If Jes doesn't mind, I would prefer that we pass in the interface name (dev->name I assume?) here. Jeff