[PATCH] sk98lin: handle pci_enable_device() return value in skge_resume() properly
Fix missing handling of pci_enable_device() return value in skge_resume()
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/net/sk98lin/skge.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
index 99e9262..e12fb62 100644
--- a/drivers/net/sk98lin/skge.c
+++ b/drivers/net/sk98lin/skge.c
@@ -5070,7 +5070,11 @@ static int skge_resume(struct pci_dev *p
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
- pci_enable_device(pdev);
+ if ((ret = pci_enable_device(pdev))) {
+ printk(KERN_ERR "sk98lin: Cannot enable PCI device during resume\n");
+ unregister_netdev(dev);
+ return ret;
+ }
pci_set_master(pdev);
if (pAC->GIni.GIMacsFound == 2)
ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
--
Jiri Kosina
On Fri, 13 Oct 2006 00:17:50 +0200 (CEST)
Jiri Kosina <[email protected]> wrote:
> [PATCH] sk98lin: handle pci_enable_device() return value in skge_resume() properly
>
> Fix missing handling of pci_enable_device() return value in skge_resume()
>
> Signed-off-by: Jiri Kosina <[email protected]>
>
> ---
>
> drivers/net/sk98lin/skge.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
> index 99e9262..e12fb62 100644
> --- a/drivers/net/sk98lin/skge.c
> +++ b/drivers/net/sk98lin/skge.c
> @@ -5070,7 +5070,11 @@ static int skge_resume(struct pci_dev *p
>
> pci_set_power_state(pdev, PCI_D0);
> pci_restore_state(pdev);
> - pci_enable_device(pdev);
> + if ((ret = pci_enable_device(pdev))) {
> + printk(KERN_ERR "sk98lin: Cannot enable PCI device during resume\n");
> + unregister_netdev(dev);
>
Having the device unregister seems harsh.
Why put condtional on same line?
Why not print device name dev->name.
--
Stephen Hemminger <[email protected]>
On Thu, 12 Oct 2006, Stephen Hemminger wrote:
> > pci_set_power_state(pdev, PCI_D0);
> > pci_restore_state(pdev);
> > - pci_enable_device(pdev);
> > + if ((ret = pci_enable_device(pdev))) {
> > + printk(KERN_ERR "sk98lin: Cannot enable PCI device during resume\n");
> > + unregister_netdev(dev);
> >
> Having the device unregister seems harsh.
What would be the proper way? As the initialization failed, accessing the
device would not make sense any more (therefore I don't think that calling
skge_remove_one() would be OK, as it issues calls to SkEventQueue() and
SkEventDispatcher(), trying to send something to the card).
> Why put condtional on same line?
Pardon me?
> Why not print device name dev->name.
Thanks.
[PATCH] fix sk98lin driver, ignoring return value from pci_enable_device()
add check of return value to _resume() function of sk98lin driver.
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/net/sk98lin/skge.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
index d4913c3..1f03cf8 100644
--- a/drivers/net/sk98lin/skge.c
+++ b/drivers/net/sk98lin/skge.c
@@ -5070,7 +5070,11 @@ static int skge_resume(struct pci_dev *p
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
- pci_enable_device(pdev);
+ if ((ret = pci_enable_device(pdev))) {
+ printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
+ dev->name);
+ return ret;
+ }
pci_set_master(pdev);
if (pAC->GIni.GIMacsFound == 2)
ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
--
Jiri Kosina
On Fri, 13 Oct 2006 00:38:20 +0200 (CEST)
Jiri Kosina <[email protected]> wrote:
> On Thu, 12 Oct 2006, Stephen Hemminger wrote:
>
> > > pci_set_power_state(pdev, PCI_D0);
> > > pci_restore_state(pdev);
> > > - pci_enable_device(pdev);
> > > + if ((ret = pci_enable_device(pdev))) {
> > > + printk(KERN_ERR "sk98lin: Cannot enable PCI device during resume\n");
> > > + unregister_netdev(dev);
> > >
> > Having the device unregister seems harsh.
>
> What would be the proper way? As the initialization failed, accessing the
> device would not make sense any more (therefore I don't think that calling
> skge_remove_one() would be OK, as it issues calls to SkEventQueue() and
> SkEventDispatcher(), trying to send something to the card).
I guess, its just not clear what the state of the machine is anyway
if you can't enable the device something is hosed (or the device was
hot removed).
> > Why put condtional on same line?
>
> Pardon me?
I prefer:
ret = pci_enable_device(pdev);
if (ret) {
>
> > Why not print device name dev->name.
>
> Thanks.
>
> [PATCH] fix sk98lin driver, ignoring return value from pci_enable_device()
>
> add check of return value to _resume() function of sk98lin driver.
>
> Signed-off-by: Jiri Kosina <[email protected]>
>
> ---
>
> drivers/net/sk98lin/skge.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
> index d4913c3..1f03cf8 100644
> --- a/drivers/net/sk98lin/skge.c
> +++ b/drivers/net/sk98lin/skge.c
> @@ -5070,7 +5070,11 @@ static int skge_resume(struct pci_dev *p
>
> pci_set_power_state(pdev, PCI_D0);
> pci_restore_state(pdev);
> - pci_enable_device(pdev);
> + if ((ret = pci_enable_device(pdev))) {
> + printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
> + dev->name);
> + return ret;
> + }
> pci_set_master(pdev);
> if (pAC->GIni.GIMacsFound == 2)
> ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
>
--
Stephen Hemminger <[email protected]>
On Thu, 12 Oct 2006, Stephen Hemminger wrote:
> > > Having the device unregister seems harsh.
> > What would be the proper way? As the initialization failed, accessing
> > the device would not make sense any more (therefore I don't think that
> > calling skge_remove_one() would be OK, as it issues calls to
> > SkEventQueue() and SkEventDispatcher(), trying to send something to
> > the card).
> I guess, its just not clear what the state of the machine is anyway
> if you can't enable the device something is hosed (or the device was
> hot removed).
Well, it depends on definition of 'hot'. What would for example happen in
the case suspend-to-disk -> remove the card when the machine is switched
off -> resume-from-disk? I guess that exactly this pci_enable_device()
will fail, so we definitely have to handle this case, as it can easily
happen.
> > > Why put condtional on same line?
> > Pardon me?
> I prefer:
> ret = pci_enable_device(pdev);
As you wish.
[PATCH] fix sk98lin driver, ignoring return value from pci_enable_device()
add check of return value to _resume() function of sk98lin driver.
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/net/sk98lin/skge.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
index d4913c3..d691811 100644
--- a/drivers/net/sk98lin/skge.c
+++ b/drivers/net/sk98lin/skge.c
@@ -5070,7 +5070,13 @@ static int skge_resume(struct pci_dev *p
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
- pci_enable_device(pdev);
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
+ dev->name);
+ unregister_netdev(dev);
+ return ret;
+ }
pci_set_master(pdev);
if (pAC->GIni.GIMacsFound == 2)
ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
--
Jiri Kosina
On Fri, 13 Oct 2006 00:57:18 +0200 (CEST)
Jiri Kosina <[email protected]> wrote:
> @@ -5070,7 +5070,13 @@ static int skge_resume(struct pci_dev *p
>
> pci_set_power_state(pdev, PCI_D0);
> pci_restore_state(pdev);
> - pci_enable_device(pdev);
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
> + dev->name);
> + unregister_netdev(dev);
This looks rather wrong - skge_exit() will run unregister_netdev() again.
Look a few lines down, to where this function already handles request_irq()
failure, reuse that code path. Hopefully it has been tested..
(Once we have an easy-to-use fault-injection framework we'll be able to
test all these things more easily)
(But it's possible to test them already, with a bit of ad-hoc testing code)
On Thu, 12 Oct 2006, Andrew Morton wrote:
> > pci_set_power_state(pdev, PCI_D0);
> > pci_restore_state(pdev);
> > - pci_enable_device(pdev);
> > + ret = pci_enable_device(pdev);
> > + if (ret) {
> > + printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
> > + dev->name);
> > + unregister_netdev(dev);
> This looks rather wrong - skge_exit() will run unregister_netdev() again.
You are of course right (the problem was also spotted by Russell King).
This I believe is the correct one for the sk98lin case.
[PATCH] fix sk98lin driver, ignoring return value from pci_enable_device()
add check of return value to _resume() function of sk98lin driver.
Signed-off-by: Jiri Kosina <[email protected]>
---
drivers/net/sk98lin/skge.c | 20 +++++++++++++++-----
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
index d4913c3..3a9323d 100644
--- a/drivers/net/sk98lin/skge.c
+++ b/drivers/net/sk98lin/skge.c
@@ -5070,7 +5070,12 @@ static int skge_resume(struct pci_dev *p
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
- pci_enable_device(pdev);
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ printk(KERN_WARNING "sk98lin: unable to enable device %s in resume\n",
+ dev->name);
+ goto out_err;
+ }
pci_set_master(pdev);
if (pAC->GIni.GIMacsFound == 2)
ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
@@ -5078,10 +5083,8 @@ static int skge_resume(struct pci_dev *p
ret = request_irq(dev->irq, SkGeIsrOnePort, IRQF_SHARED, "sk98lin", dev);
if (ret) {
printk(KERN_WARNING "sk98lin: unable to acquire IRQ %d\n", dev->irq);
- pAC->AllocFlag &= ~SK_ALLOC_IRQ;
- dev->irq = 0;
- pci_disable_device(pdev);
- return -EBUSY;
+ ret = -EBUSY;
+ goto out_err;
}
netif_device_attach(dev);
@@ -5098,6 +5101,13 @@ static int skge_resume(struct pci_dev *p
}
return 0;
+out_err:
+ pAC->AllocFlag &= ~SK_ALLOC_IRQ;
+ dev->irq = 0;
+ pci_disable_device(pdev);
+
+ return ret;
+
}
#else
#define skge_suspend NULL
--
Jiri Kosina
This thread looks dead but issue was't fixed.
Jiri Kosina <[email protected]> writes:
>> > - pci_enable_device(pdev);
>> > + ret = pci_enable_device(pdev);
>> > + if (ret) {
>> > + printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
>> > + dev->name);
>> > + unregister_netdev(dev);
>> This looks rather wrong - skge_exit() will run unregister_netdev() again.
>
> You are of course right (the problem was also spotted by Russell King).
> This I believe is the correct one for the sk98lin case.
>
> [PATCH] fix sk98lin driver, ignoring return value from pci_enable_device()
>
> add check of return value to _resume() function of sk98lin driver.
>
> Signed-off-by: Jiri Kosina <[email protected]>
>
> ---
>
> --- a/drivers/net/sk98lin/skge.c
> +++ b/drivers/net/sk98lin/skge.c
> @@ -5070,7 +5070,12 @@ static int skge_resume(struct pci_dev *p
>
> pci_set_power_state(pdev, PCI_D0);
> pci_restore_state(pdev);
> - pci_enable_device(pdev);
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + printk(KERN_WARNING "sk98lin: unable to enable device %s in resume\n",
> + dev->name);
> + goto out_err;
> + }
[snip]
> +out_err:
> + pAC->AllocFlag &= ~SK_ALLOC_IRQ;
> + dev->irq = 0;
> + pci_disable_device(pdev);
<<<<< Ok what happend if we jump here right after pci_disable_device() has
failed, but pci_disable_device() was called anyway, this is wrong and
may be fatal because pdev->enable_cnt may becomes negative.
> +
> + return ret;
> +
> }
> #else
> #define skge_suspend NULL
This is reworked Jiri's patch:
[PATCH] sk98lin: handle pci_enable_device() return value in skge_resume()
Signed-off-by: Monakhov Dmitriy <[email protected]>
---
drivers/net/sk98lin/skge.c | 20 +++++++++++++++-----
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
index e94ab25..eea753a 100644
--- a/drivers/net/sk98lin/skge.c
+++ b/drivers/net/sk98lin/skge.c
@@ -5125,7 +5125,12 @@ static int skge_resume(struct pci_dev *pdev)
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
- pci_enable_device(pdev);
+ ret = pci_enable_device(pdev);
+ if (ret) {
+ printk(KERN_WARNING "sk98lin: unable to enable device %s "
+ "in resume\n", dev->name);
+ goto err_out;
+ }
pci_set_master(pdev);
if (pAC->GIni.GIMacsFound == 2)
ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
@@ -5133,10 +5138,8 @@ static int skge_resume(struct pci_dev *pdev)
ret = request_irq(dev->irq, SkGeIsrOnePort, IRQF_SHARED, "sk98lin", dev);
if (ret) {
printk(KERN_WARNING "sk98lin: unable to acquire IRQ %d\n", dev->irq);
- pAC->AllocFlag &= ~SK_ALLOC_IRQ;
- dev->irq = 0;
- pci_disable_device(pdev);
- return -EBUSY;
+ ret = -EBUSY;
+ goto err_out_disable_pdev;
}
netif_device_attach(dev);
@@ -5153,6 +5156,13 @@ static int skge_resume(struct pci_dev *pdev)
}
return 0;
+
+err_out_disable_pdev:
+ pci_disable_device(pdev);
+err_out:
+ pAC->AllocFlag &= ~SK_ALLOC_IRQ;
+ dev->irq = 0;
+ return ret;
}
#else
#define skge_suspend NULL
--
1.4.4.4
Dmitriy Monakhov <[email protected]> writes:
> This thread looks dead but issue was't fixed.
>
> Jiri Kosina <[email protected]> writes:
>>> > - pci_enable_device(pdev);
>>> > + ret = pci_enable_device(pdev);
>>> > + if (ret) {
>>> > + printk(KERN_ERR "sk98lin: Cannot enable PCI device %s during resume\n",
>>> > + dev->name);
>>> > + unregister_netdev(dev);
>>> This looks rather wrong - skge_exit() will run unregister_netdev() again.
>>
>> You are of course right (the problem was also spotted by Russell King).
>> This I believe is the correct one for the sk98lin case.
>>
>> [PATCH] fix sk98lin driver, ignoring return value from pci_enable_device()
>>
>> add check of return value to _resume() function of sk98lin driver.
>>
>> Signed-off-by: Jiri Kosina <[email protected]>
>>
>> ---
>>
>> --- a/drivers/net/sk98lin/skge.c
>> +++ b/drivers/net/sk98lin/skge.c
>> @@ -5070,7 +5070,12 @@ static int skge_resume(struct pci_dev *p
>>
>> pci_set_power_state(pdev, PCI_D0);
>> pci_restore_state(pdev);
>> - pci_enable_device(pdev);
>> + ret = pci_enable_device(pdev);
>> + if (ret) {
>> + printk(KERN_WARNING "sk98lin: unable to enable device %s in resume\n",
>> + dev->name);
>> + goto out_err;
>> + }
> [snip]
>> +out_err:
>> + pAC->AllocFlag &= ~SK_ALLOC_IRQ;
>> + dev->irq = 0;
>> + pci_disable_device(pdev);
> <<<<< Ok what happend if we jump here right after pci_disable_device() has
OOPs.....Of course i mean pci_enable_device() here^^^^^^^^^^^^^^^^^^^^^
I'm sorry about this. (Option complete_word not always good, some times
brain work required too :) )
So correct comment looks like:
<<<<< Ok what happend if we jump here right after pci_enable_device() has
failed, but pci_disable_device() was called anyway, this is wrong and
may be fatal because pdev->enable_cnt may becomes negative.
>> +
>> + return ret;
>> +
>> }
>> #else
>> #define skge_suspend NULL
> This is reworked Jiri's patch:
>
> [PATCH] sk98lin: handle pci_enable_device() return value in skge_resume()
>
> Signed-off-by: Monakhov Dmitriy <[email protected]>
> ---
> drivers/net/sk98lin/skge.c | 20 +++++++++++++++-----
> 1 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/sk98lin/skge.c b/drivers/net/sk98lin/skge.c
> index e94ab25..eea753a 100644
> --- a/drivers/net/sk98lin/skge.c
> +++ b/drivers/net/sk98lin/skge.c
> @@ -5125,7 +5125,12 @@ static int skge_resume(struct pci_dev *pdev)
>
> pci_set_power_state(pdev, PCI_D0);
> pci_restore_state(pdev);
> - pci_enable_device(pdev);
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + printk(KERN_WARNING "sk98lin: unable to enable device %s "
> + "in resume\n", dev->name);
> + goto err_out;
> + }
> pci_set_master(pdev);
> if (pAC->GIni.GIMacsFound == 2)
> ret = request_irq(dev->irq, SkGeIsr, IRQF_SHARED, "sk98lin", dev);
> @@ -5133,10 +5138,8 @@ static int skge_resume(struct pci_dev *pdev)
> ret = request_irq(dev->irq, SkGeIsrOnePort, IRQF_SHARED, "sk98lin", dev);
> if (ret) {
> printk(KERN_WARNING "sk98lin: unable to acquire IRQ %d\n", dev->irq);
> - pAC->AllocFlag &= ~SK_ALLOC_IRQ;
> - dev->irq = 0;
> - pci_disable_device(pdev);
> - return -EBUSY;
> + ret = -EBUSY;
> + goto err_out_disable_pdev;
> }
>
> netif_device_attach(dev);
> @@ -5153,6 +5156,13 @@ static int skge_resume(struct pci_dev *pdev)
> }
>
> return 0;
> +
> +err_out_disable_pdev:
> + pci_disable_device(pdev);
> +err_out:
> + pAC->AllocFlag &= ~SK_ALLOC_IRQ;
> + dev->irq = 0;
> + return ret;
> }
> #else
> #define skge_suspend NULL
> --
> 1.4.4.4
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/