Proper pci_enable_device() error handling in resume routine

Proper pci_enable_device() error handling in resume routine

[Valerie Henson]

Hello,

I'm trying to properly handle pci_enable_device() errors in the resume
routines of a couple of tulip drivers.  I noticed that several drivers
pay attention to errors from pci_enable_device() in the init routine
but ignore it on resume; other drivers vary wildly.  What's proper
behavior when resuming?  Extant examples:

0. Don't call pci_enable_device() at all (8139too)
1. Ignore the return value (eepro100, many others)
2. Check for failure and bail out, but return success (sungem)

-VAL

Re: Proper pci_enable_device() error handling in resume routine

[Auke Kok]

Valerie Henson wrote:
> I'm trying to properly handle pci_enable_device() errors in the resume
> routines of a couple of tulip drivers.  I noticed that several drivers
> pay attention to errors from pci_enable_device() in the init routine
> but ignore it on resume; other drivers vary wildly.  What's proper
> behavior when resuming?  Extant examples:
> 
> 0. Don't call pci_enable_device() at all (8139too)
> 1. Ignore the return value (eepro100, many others)
> 2. Check for failure and bail out, but return success (sungem)

Digging through e1000 I spot that we even pci_enable_device after a PCI error, 
so it is good practice I think to make sure the device is up. I suppose that 
most people can live without explicit re-enabling the device (most NICs are on 
anyway), but if we do enable it explicitly we should certainly check the result 
code. Interestingly enough we failed to do this in e1000, so I'll submit a 
patch for that later.

Cheers,

Auke

Re: Proper pci_enable_device() error handling in resume routine

[Valerie Henson]

On Mon, Aug 21, 2006 at 11:12:03AM -0700, Auke Kok wrote:
> Valerie Henson wrote:
> >I'm trying to properly handle pci_enable_device() errors in the resume
> >routines of a couple of tulip drivers.  I noticed that several drivers
> >pay attention to errors from pci_enable_device() in the init routine
> >but ignore it on resume; other drivers vary wildly.  What's proper
> >behavior when resuming?  Extant examples:
> >
> >0. Don't call pci_enable_device() at all (8139too)
> >1. Ignore the return value (eepro100, many others)
> >2. Check for failure and bail out, but return success (sungem)
> 
> Digging through e1000 I spot that we even pci_enable_device after a PCI 
> error, so it is good practice I think to make sure the device is up. I 
> suppose that most people can live without explicit re-enabling the device 
> (most NICs are on anyway), but if we do enable it explicitly we should 
> certainly check the result code. Interestingly enough we failed to do this 
> in e1000, so I'll submit a patch for that later.

For people looking for how to implement this in their own driver,
here's how I solved it in tulip (will submit later):

---
 drivers/net/tulip/tulip_core.c  |    5 ++++-
 drivers/net/tulip/winbond-840.c |   10 +++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

--- a/drivers/net/tulip/tulip_core.c
+++ b/drivers/net/tulip/tulip_core.c
@@ -1780,7 +1780,10 @@ static int tulip_resume(struct pci_dev *
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
 
-	pci_enable_device(pdev);
+	if ((retval = pci_enable_device(pdev))) {
+		printk (KERN_ERR "tulip: pci_enable_device failed in resume\n");
+		return retval;
+	}
 
 	if ((retval = request_irq(dev->irq, &tulip_interrupt, IRQF_SHARED, dev->name, dev))) {
 		printk (KERN_ERR "tulip: request_irq failed in resume\n");



-VAL