Re: [E1000-devel] [BUG 2.6.30+] e100 sometimes causes oops during resume

[Karol Lewandowski <karol.k.lewandowski@gmail.com>]

On Tue, Sep 15, 2009 at 03:54:20PM -0700, Graham, David wrote:

> A v2.6.30..v2.6.31 diff shows that this is probably exposed by
> Rafael Wysocki's commit 6905b1f1, which now allows systems with e100
> to sleep. If I understand correctly, it looks like these systems
> simply couldn't sleep before. Is that right Rafael?.

Probably true, but that wasn't the case for my (I guess
ACPI-controlled) system.


> I don't think its likely that the commit is a direct cause of the
> problem, but that the suspend/resume cycle now allows us to see
> another issue.

>From my (very limited) understanding commit message is at least in
conflict with patch body.

Precisely patch was supposed to "Fix this problem by ignoring the
return value of pci_set_power_state() in __e100_power_off()."

That patch is doing something rather different -- it returns 0, yes,
but it also ignores 'wake' bool as set by __e100_shutdown().  That
seems wrong to me.


--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -2895,12 +2895,13 @@ static void __e100_shutdown(struct pci_dev *pdev, bool *enable_wake)
 
 static int __e100_power_off(struct pci_dev *pdev, bool wake)
 {
-       if (wake) {
+       if (wake)
                return pci_prepare_to_sleep(pdev);
-       } else {
-               pci_wake_from_d3(pdev, false);
-               return pci_set_power_state(pdev, PCI_D3hot);
-       }
+
+       pci_wake_from_d3(pdev, false);
+       pci_set_power_state(pdev, PCI_D3hot);
+
+       return 0;
 }


Correct patch would be that (hand-made), right?


+++ b/drivers/net/e100.c
@@ -2895,12 +2895,13 @@ static void __e100_shutdown(struct pci_dev *pdev, bool *enable_wake)
 
 static int __e100_power_off(struct pci_dev *pdev, bool wake)
 {
        if (wake) {
                return pci_prepare_to_sleep(pdev);
        } else {
                pci_wake_from_d3(pdev, false);
-               return pci_set_power_state(pdev, PCI_D3hot);
+               pci_set_power_state(pdev, PCI_D3hot);
        }
+
+       return 0;
 }

I can test, or rather -- start testing this tommorow, if this makes
sense to you.

Thanks.