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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
> ---
> 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
>
Hello Florian,
On Tue, May 29, 2012 at 2:50 PM, Florian Fainelli <[email protected]> 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 <[email protected]>
>
> Acked-by: Florian Fainelli <[email protected]>
>
>> ---
>> ?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.
On Tuesday 29 May 2012 15:28:51 devendra.aaru wrote:
> Hello Florian,
>
> On Tue, May 29, 2012 at 2:50 PM, Florian Fainelli <[email protected]>
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
Hi Florian,
>> On Tue, May 29, 2012 at 2:50 PM, Florian Fainelli <[email protected]>
> 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.
From: Florian Fainelli <[email protected]>
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 <[email protected]>
>
> Acked-by: Florian Fainelli <[email protected]>
Applied.
From: David Miller <[email protected]>
Date: Tue, 29 May 2012 17:22:29 -0400 (EDT)
> From: Florian Fainelli <[email protected]>
> 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 <[email protected]>
>>
>> Acked-by: Florian Fainelli <[email protected]>
>
> 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.
Hi David,
On Wed, May 30, 2012 at 2:58 AM, David Miller <[email protected]> wrote:
> From: David Miller <[email protected]>
> 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.