[PATCH 05/10] r6040: call napi_disable when puting down the interface and set lp->dev accordingly.

[PATCH 05/10] r6040: call napi_disable when puting down the interface and set lp->dev accordingly.

[Florian Fainelli]

We did not call napi_disabled when putting down the interface
which should be done. Finally initialize lp->dev when everything
is set.

Signed-off-by: Florian Fainelli <florian.fainelli@telecomint.eu>
---
diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
index 4f2ebb2..5eb057d 100644
--- a/drivers/net/r6040.c
+++ b/drivers/net/r6040.c
@@ -485,6 +485,7 @@ static int r6040_close(struct net_device *dev)
 	del_timer_sync(&lp->timer);
 
 	spin_lock_irq(&lp->lock);
+	napi_disable(&lp->napi);
 	netif_stop_queue(dev);
 	r6040_down(dev);
 	spin_unlock_irq(&lp->lock);
@@ -1080,8 +1081,6 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	lp = netdev_priv(dev);
-	lp->pdev = pdev;
-	lp->dev = dev;
 
 	if (pci_request_regions(pdev, DRV_NAME)) {
 		printk(KERN_ERR DRV_NAME ": Failed to request PCI regions\n");
@@ -1113,6 +1112,7 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 
 	/* Link new device into r6040_root_dev */
 	lp->pdev = pdev;
+	lp->dev = dev;
 
 	/* Init RDC private data */
 	lp->mcr0 = 0x1002;

Re: [PATCH 05/10] r6040: call napi_disable when puting down the interface and set lp->dev accordingly.

[Francois Romieu]

Florian Fainelli <florian.fainelli@telecomint.eu> :
> We did not call napi_disabled when putting down the interface
> which should be done. Finally initialize lp->dev when everything
> is set.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@telecomint.eu>
> ---
> diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
> index 4f2ebb2..5eb057d 100644
> --- a/drivers/net/r6040.c
> +++ b/drivers/net/r6040.c
[...]
> @@ -1080,8 +1081,6 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
>  	}
>  	SET_NETDEV_DEV(dev, &pdev->dev);
>  	lp = netdev_priv(dev);
> -	lp->pdev = pdev;
> -	lp->dev = dev;
>  
>  	if (pci_request_regions(pdev, DRV_NAME)) {
>  		printk(KERN_ERR DRV_NAME ": Failed to request PCI regions\n");

Can you fix the error paths of r6040_init_one as well ?

They badly leak.

-- 
Ueimor

Re: [PATCH 05/10] r6040: call napi_disable when puting down the interface and set lp->dev accordingly.

[Florian Fainelli]

Hi Francois,

Le Wednesday 16 July 2008 23:01:38 Francois Romieu, vous avez écrit :
> Can you fix the error paths of r6040_init_one as well ?

Patch attached below, are you okay with the other patches ?

--
From: Florian Fainelli <florian.fainelli@telecomint.eu>
Subject: [PATCH] r6040: rework init_one error handling

This patch reworks the error handling in r6040_init_one
in order not to leak resources and correcly unmap and release
PCI regions of the MAC. Also prefix printk's with the driver name
for clarity.

Signed-off-by: Florian Fainelli <florian.fainelli@telecomint.eu>
---
diff --git a/drivers/net/r6040.c b/drivers/net/r6040.c
index 1cafa92..6531ff5 100644
--- a/drivers/net/r6040.c
+++ b/drivers/net/r6040.c
@@ -1054,24 +1054,27 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 
 	err = pci_enable_device(pdev);
 	if (err)
-		return err;
+		goto err_out;
 
 	/* this should always be supported */
-	if (pci_set_dma_mask(pdev, DMA_32BIT_MASK)) {
+	err = pci_set_dma_mask(pdev, DMA_32BIT_MASK);
+	if (err) {
 		printk(KERN_ERR DRV_NAME "32-bit PCI DMA addresses"
 				"not supported by the card\n");
-		return -ENODEV;
+		goto err_out;
 	}
-	if (pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK)) {
+	err = pci_set_consistent_dma_mask(pdev, DMA_32BIT_MASK);
+	if (err) {
 		printk(KERN_ERR DRV_NAME "32-bit PCI DMA addresses"
 				"not supported by the card\n");
-		return -ENODEV;
+		goto err_out;
 	}
 
 	/* IO Size check */
 	if (pci_resource_len(pdev, 0) < io_size) {
-		printk(KERN_ERR "Insufficient PCI resources, aborting\n");
-		return -EIO;
+		printk(KERN_ERR DRV_NAME "Insufficient PCI resources, aborting\n");
+		err = -EIO;
+		goto err_out;
 	}
 
 	pioaddr = pci_resource_start(pdev, 0);	/* IO map base address */
@@ -1079,23 +1082,26 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 
 	dev = alloc_etherdev(sizeof(struct r6040_private));
 	if (!dev) {
-		printk(KERN_ERR "Failed to allocate etherdev\n");
-		return -ENOMEM;
+		printk(KERN_ERR DRV_NAME "Failed to allocate etherdev\n");
+		err = -ENOMEM;
+		goto err_out;
 	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	lp = netdev_priv(dev);
 
-	if (pci_request_regions(pdev, DRV_NAME)) {
+	err = pci_request_regions(pdev, DRV_NAME);
+
+	if (err) {
 		printk(KERN_ERR DRV_NAME ": Failed to request PCI regions\n");
-		err = -ENODEV;
-		goto err_out_disable;
+		goto err_out_free_dev;
 	}
 
 	ioaddr = pci_iomap(pdev, bar, io_size);
 	if (!ioaddr) {
 		printk(KERN_ERR "ioremap failed for device %s\n",
 			pci_name(pdev));
-		return -EIO;
+		err = -EIO;
+		goto err_out_free_res;
 	}
 
 	/* Init system & device */
@@ -1147,17 +1153,17 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 	err = register_netdev(dev);
 	if (err) {
 		printk(KERN_ERR DRV_NAME ": Failed to register net device\n");
-		goto err_out_res;
+		goto err_out_unmap;
 	}
 	return 0;
 
-err_out_res:
+err_out_unmap:
+	pci_iounmap(pdev, ioaddr);
+err_out_free_res:
 	pci_release_regions(pdev);
-err_out_disable:
-	pci_disable_device(pdev);
-	pci_set_drvdata(pdev, NULL);
+err_out_free_dev:
 	free_netdev(dev);
-
+err_out:
 	return err;
 }
 

Re: [PATCH 05/10] r6040: call napi_disable when puting down the interface and set lp->dev accordingly.

[Francois Romieu]

Florian Fainelli <florian.fainelli@telecomint.eu> :
[...]
> Patch attached below

Thanks.

> are you okay with the other patches ?

1) The comment in "[PATCH 06/10] r6040: completely rework the RX path"
seems a it misleading to me:
! ...
! We now allocate skbs on the fly in r6040_rx, and we handle allocation
! failure instead of simply dropping the packet. Remove the
! ...

Afaiks netif_receive_skb is skipped as soon as the allocation fails.
The packet is thus still dropped, isn't it ?

2) I am a bit surprized that r6040_descriptor mixes endian tainted
fields (buf, ndesc) with untainted ones (status, len). It the latter
ought to be tainted too, it could be done in the #define lines of
"[PATCH 07/10] r6040: use definitions for magic values in descriptor status"

-- 
Ueimor

Re: [PATCH 05/10] r6040: call napi_disable when puting down the interface and set lp->dev accordingly.

[Jeff Garzik]

Florian Fainelli wrote:
> Hi Francois,
> 
> Le Wednesday 16 July 2008 23:01:38 Francois Romieu, vous avez écrit :
>> Can you fix the error paths of r6040_init_one as well ?
> 
> Patch attached below, are you okay with the other patches ?
> 
> --
> From: Florian Fainelli <florian.fainelli@telecomint.eu>
> Subject: [PATCH] r6040: rework init_one error handling
> 
> This patch reworks the error handling in r6040_init_one
> in order not to leak resources and correcly unmap and release
> PCI regions of the MAC. Also prefix printk's with the driver name
> for clarity.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@telecomint.eu>

applied 5-10, plus this one