Re: [PATCH] Road Runner HIPPI driver (rrunner)

[Jeff Garzik <jgarzik@xxxxxxxxx>]

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(&regs->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