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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090916014448.GA1070@bizet.domek.prywatny>
Date:	Wed, 16 Sep 2009 03:44:48 +0200
From:	Karol Lewandowski <karol.k.lewandowski@...il.com>
To:	"Graham, David" <david.graham@...el.com>
Cc:	Karol Lewandowski <karol.k.lewandowski@...il.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"e1000-devel@...ts.sourceforge.net" 
	<e1000-devel@...ts.sourceforge.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [E1000-devel] [BUG 2.6.30+] e100 sometimes causes oops during
	resume

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html