[PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[Devendra Naga]

the calls after the pci_enable_device may fail, and will error out with out
disabling it. disable the device at error paths.

Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
---
 drivers/net/ethernet/rdc/r6040.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c
index 4de7364..8f5079a 100644
--- a/drivers/net/ethernet/rdc/r6040.c
+++ b/drivers/net/ethernet/rdc/r6040.c
@@ -1096,20 +1096,20 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 	if (err) {
 		dev_err(&pdev->dev, "32-bit PCI DMA addresses"
 				"not supported by the card\n");
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
 	if (err) {
 		dev_err(&pdev->dev, "32-bit PCI DMA addresses"
 				"not supported by the card\n");
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 
 	/* IO Size check */
 	if (pci_resource_len(pdev, bar) < io_size) {
 		dev_err(&pdev->dev, "Insufficient PCI resources, aborting\n");
 		err = -EIO;
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 
 	pci_set_master(pdev);
@@ -1117,7 +1117,7 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 	dev = alloc_etherdev(sizeof(struct r6040_private));
 	if (!dev) {
 		err = -ENOMEM;
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	lp = netdev_priv(dev);
@@ -1238,6 +1238,8 @@ err_out_free_res:
 	pci_release_regions(pdev);
 err_out_free_dev:
 	free_netdev(dev);
+err_out_disable_dev:
+	pci_disable_device(dev);
 err_out:
 	return err;
 }
-- 
1.7.9.5

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[Florian Fainelli]

On Monday 28 May 2012 17:27:03 Devendra Naga wrote:
> the calls after the pci_enable_device may fail, and will error out with out
> disabling it. disable the device at error paths.

Looks good, thanks Devendra!

> 
> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>

Acked-by: Florian Fainelli <florian@openwrt.org>

> ---
>  drivers/net/ethernet/rdc/r6040.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/rdc/r6040.c 
b/drivers/net/ethernet/rdc/r6040.c
> index 4de7364..8f5079a 100644
> --- a/drivers/net/ethernet/rdc/r6040.c
> +++ b/drivers/net/ethernet/rdc/r6040.c
> @@ -1096,20 +1096,20 @@ static int __devinit r6040_init_one(struct pci_dev 
*pdev,
>  	if (err) {
>  		dev_err(&pdev->dev, "32-bit PCI DMA addresses"
>  				"not supported by the card\n");
> -		goto err_out;
> +		goto err_out_disable_dev;
>  	}
>  	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
>  	if (err) {
>  		dev_err(&pdev->dev, "32-bit PCI DMA addresses"
>  				"not supported by the card\n");
> -		goto err_out;
> +		goto err_out_disable_dev;
>  	}
>  
>  	/* IO Size check */
>  	if (pci_resource_len(pdev, bar) < io_size) {
>  		dev_err(&pdev->dev, "Insufficient PCI resources, aborting\n");
>  		err = -EIO;
> -		goto err_out;
> +		goto err_out_disable_dev;
>  	}
>  
>  	pci_set_master(pdev);
> @@ -1117,7 +1117,7 @@ static int __devinit r6040_init_one(struct pci_dev 
*pdev,
>  	dev = alloc_etherdev(sizeof(struct r6040_private));
>  	if (!dev) {
>  		err = -ENOMEM;
> -		goto err_out;
> +		goto err_out_disable_dev;
>  	}
>  	SET_NETDEV_DEV(dev, &pdev->dev);
>  	lp = netdev_priv(dev);
> @@ -1238,6 +1238,8 @@ err_out_free_res:
>  	pci_release_regions(pdev);
>  err_out_free_dev:
>  	free_netdev(dev);
> +err_out_disable_dev:
> +	pci_disable_device(dev);
>  err_out:
>  	return err;
>  }
> -- 
> 1.7.9.5
> 

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[devendra.aaru]

Hello Florian,

On Tue, May 29, 2012 at 2:50 PM, Florian Fainelli <florian@openwrt.org> wrote:
> On Monday 28 May 2012 17:27:03 Devendra Naga wrote:
>> the calls after the pci_enable_device may fail, and will error out with out
>> disabling it. disable the device at error paths.
>
> Looks good, thanks Devendra!
>
>>
>> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
>
> Acked-by: Florian Fainelli <florian@openwrt.org>
>
>> ---
>>  drivers/net/ethernet/rdc/r6040.c |   10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/rdc/r6040.c
> b/drivers/net/ethernet/rdc/r6040.c
>> index 4de7364..8f5079a 100644
>> --- a/drivers/net/ethernet/rdc/r6040.c
>> +++ b/drivers/net/ethernet/rdc/r6040.c
>> @@ -1096,20 +1096,20 @@ static int __devinit r6040_init_one(struct pci_dev
> *pdev,
>>       if (err) {
>>               dev_err(&pdev->dev, "32-bit PCI DMA addresses"
>>                               "not supported by the card\n");
>> -             goto err_out;
>> +             goto err_out_disable_dev;
>>       }
>>       err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
>>       if (err) {
>>               dev_err(&pdev->dev, "32-bit PCI DMA addresses"
>>                               "not supported by the card\n");
>> -             goto err_out;
>> +             goto err_out_disable_dev;
>>       }
>>
>>       /* IO Size check */
>>       if (pci_resource_len(pdev, bar) < io_size) {
>>               dev_err(&pdev->dev, "Insufficient PCI resources, aborting\n");
>>               err = -EIO;
>> -             goto err_out;
>> +             goto err_out_disable_dev;
>>       }
>>
>>       pci_set_master(pdev);
>> @@ -1117,7 +1117,7 @@ static int __devinit r6040_init_one(struct pci_dev
> *pdev,
>>       dev = alloc_etherdev(sizeof(struct r6040_private));
>>       if (!dev) {
>>               err = -ENOMEM;
>> -             goto err_out;
>> +             goto err_out_disable_dev;
>>       }
>>       SET_NETDEV_DEV(dev, &pdev->dev);
>>       lp = netdev_priv(dev);
>> @@ -1238,6 +1238,8 @@ err_out_free_res:
>>       pci_release_regions(pdev);
>>  err_out_free_dev:
>>       free_netdev(dev);
>> +err_out_disable_dev:
>> +     pci_disable_device(dev);
>>  err_out:
>>       return err;
>>  }
>> --
>> 1.7.9.5
>>

Thanks for the Ack.
I found one more problem. Its when mdiobus_alloc fails in
r6040_init_one, we need to do call to the netif_napi_del and set the
NULL to pci_set_drvdata, at  err_out_unmap.

Thanks,
Devendra.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[Florian Fainelli]

On Tuesday 29 May 2012 15:28:51 devendra.aaru wrote:
> Hello Florian,
> 
> On Tue, May 29, 2012 at 2:50 PM, Florian Fainelli <florian@openwrt.org> 
wrote:

> 
> Thanks for the Ack.
> I found one more problem. Its when mdiobus_alloc fails in
> r6040_init_one, we need to do call to the netif_napi_del and set the
> NULL to pci_set_drvdata, at  err_out_unmap.

Ok, can you please submit a patch to fix this issue as well? Thanks!
--
Florian

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[devendra.aaru]

Hi Florian,

>> On Tue, May 29, 2012 at 2:50 PM, Florian Fainelli <florian@openwrt.org>
> wrote:
>
>>
>> Thanks for the Ack.
>> I found one more problem. Its when mdiobus_alloc fails in
>> r6040_init_one, we need to do call to the netif_napi_del and set the
>> NULL to pci_set_drvdata, at  err_out_unmap.
>
> Ok, can you please submit a patch to fix this issue as well? Thanks!
> --
Ok sure. will be doing it shortly.
> Florian

Thanks,
Devendra.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[David Miller]

From: Florian Fainelli <florian@openwrt.org>
Date: Tue, 29 May 2012 11:20:50 +0200

> On Monday 28 May 2012 17:27:03 Devendra Naga wrote:
>> the calls after the pci_enable_device may fail, and will error out with out
>> disabling it. disable the device at error paths.
> 
> Looks good, thanks Devendra!
> 
>> 
>> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
> 
> Acked-by: Florian Fainelli <florian@openwrt.org>

Applied.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 29 May 2012 17:22:29 -0400 (EDT)

> From: Florian Fainelli <florian@openwrt.org>
> Date: Tue, 29 May 2012 11:20:50 +0200
> 
>> On Monday 28 May 2012 17:27:03 Devendra Naga wrote:
>>> the calls after the pci_enable_device may fail, and will error out with out
>>> disabling it. disable the device at error paths.
>> 
>> Looks good, thanks Devendra!
>> 
>>> 
>>> Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
>> 
>> Acked-by: Florian Fainelli <florian@openwrt.org>
> 
> Applied.

Actually, reverted, you didn't test this patch at all.

You didn't even look at the warnings emitted by the compiler
with your changes installed.

You're passing a network device pointer into the PCI device
disable routine, which takes a PCI device pointer.

I can't express to you how extremely irritating it is when
people submit patches like this.

The bug in question is 1,000 times less harmful than your fix.

[PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[Devendra Naga]

the calls after the pci_enable_device may fail, and will error out with out
disabling it. disable the device at error paths.

Signed-off-by: Devendra Naga <devendra.aaru@gmail.com>
---
 drivers/net/ethernet/rdc/r6040.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/rdc/r6040.c b/drivers/net/ethernet/rdc/r6040.c
index 4de7364..8f5079a 100644
--- a/drivers/net/ethernet/rdc/r6040.c
+++ b/drivers/net/ethernet/rdc/r6040.c
@@ -1096,20 +1096,20 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 	if (err) {
 		dev_err(&pdev->dev, "32-bit PCI DMA addresses"
 				"not supported by the card\n");
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 	err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
 	if (err) {
 		dev_err(&pdev->dev, "32-bit PCI DMA addresses"
 				"not supported by the card\n");
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 
 	/* IO Size check */
 	if (pci_resource_len(pdev, bar) < io_size) {
 		dev_err(&pdev->dev, "Insufficient PCI resources, aborting\n");
 		err = -EIO;
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 
 	pci_set_master(pdev);
@@ -1117,7 +1117,7 @@ static int __devinit r6040_init_one(struct pci_dev *pdev,
 	dev = alloc_etherdev(sizeof(struct r6040_private));
 	if (!dev) {
 		err = -ENOMEM;
-		goto err_out;
+		goto err_out_disable_dev;
 	}
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	lp = netdev_priv(dev);
@@ -1238,6 +1238,8 @@ err_out_free_res:
 	pci_release_regions(pdev);
 err_out_free_dev:
 	free_netdev(dev);
+err_out_disable_dev:
+	pci_disable_device(pdev);
 err_out:
 	return err;
 }
-- 
1.7.9.5

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[David Miller]

Please number your patches so that there is no ambguity as to what
order the patches should be applied.

That is especially important for patches like these two which make
changes in the same areas of code.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[devendra.aaru]

Hi David,

On Wed, May 30, 2012 at 2:58 AM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 29 May 2012 17:22:29 -0400 (EDT)
>
> Actually, reverted, you didn't test this patch at all.
>
> You didn't even look at the warnings emitted by the compiler
> with your changes installed.
>
> You're passing a network device pointer into the PCI device
> disable routine, which takes a PCI device pointer.
>
> I can't express to you how extremely irritating it is when
> people submit patches like this.
>
> The bug in question is 1,000 times less harmful than your fix.

Sorry about that, i agree that i didn't even tested it as i dont have
r6040 network adapter here, but i didn't even compiled it.

i sent the patch again to fix this problem.

Sorry,
Devendra.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[devendra.aaru]

Hello David,

On Wed, May 30, 2012 at 4:48 AM, David Miller <davem@davemloft.net> wrote:
>
> Please number your patches so that there is no ambguity as to what
> order the patches should be applied.
>
> That is especially important for patches like these two which make
> changes in the same areas of code.

OK, so may i put these two changes into one and send it? or you want
it to be two patches?

Thanks,
Devendra.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[David Miller]

From: "devendra.aaru" <devendra.aaru@gmail.com>
Date: Wed, 30 May 2012 04:52:08 +0530

> Hello David,
> 
> On Wed, May 30, 2012 at 4:48 AM, David Miller <davem@davemloft.net> wrote:
>>
>> Please number your patches so that there is no ambguity as to what
>> order the patches should be applied.
>>
>> That is especially important for patches like these two which make
>> changes in the same areas of code.
> 
> OK, so may i put these two changes into one and send it? or you want
> it to be two patches?

It's up to you.

Re: [PATCH] r6040: disable pci device if the subsequent calls (after pci_enable_device) fails

[devendra.aaru]

Hello David,

On Wed, May 30, 2012 at 4:54 AM, David Miller <davem@davemloft.net> wrote:
> From: "devendra.aaru" <devendra.aaru@gmail.com>
> Date: Wed, 30 May 2012 04:52:08 +0530
>
>> Hello David,
>>
>>
>> OK, so may i put these two changes into one and send it? or you want
>> it to be two patches?
>
> It's up to you.

OK, i will be sending two patches shortly(with sequence numbers (git
format-patch -s -n)).

This time i have done compile testing and got no warnings.

Thanks,
Devendra.