Proper pci_enable_device() error handling in resume routine

DISCUSSION Valerie Henson 2 participants 3 messages Aug 20, 2006 – Aug 21, 2006

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Valerie Henson

author

2006-08-20 05:29 UTC #0 lore raw

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

Auke Kok

2006-08-21 18:13 UTC #1 lore raw

Quoted 9 lines · Valerie Henson

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

Valerie Henson

author

2006-08-21 23:12 UTC #2 lore raw

Quoted 11 lines · Auke Kok

Quoted 9 lines · Valerie Henson
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)

Quoted 6 lines

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(-)

drivers/net/tulip/tulip_core.c +4 -1

@@ -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