linux-kernel - Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

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: <9460098.jyMkT8TSO0@vostro.rjw.lan>
Date:	Tue, 29 Jan 2013 13:00:21 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Konstantin Khlebnikov <khlebnikov@...nvz.org>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	e1000-devel@...ts.sourceforge.net, linux-pci@...r.kernel.org,
	Bruce Allan <bruce.w.allan@...el.com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH 1/5] e1000e: fix resuming from runtime-suspend

On Tuesday, January 29, 2013 10:32:14 AM Konstantin Khlebnikov wrote:
> Rafael J. Wysocki wrote:
> > On Monday, January 28, 2013 04:05:33 PM Bjorn Helgaas wrote:
> >> [+cc Rafael, author of patch you cited]
> >>
> >> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> >> <khlebnikov@...nvz.org>  wrote:
> >>> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> >>> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35
> >>>
> >>> Signed-off-by: Konstantin Khlebnikov<khlebnikov@...nvz.org>
> >>> Cc: e1000-devel@...ts.sourceforge.net
> >>> Cc: Jeff Kirsher<jeffrey.t.kirsher@...el.com>
> >>> Cc: Bruce Allan<bruce.w.allan@...el.com>
> >>> ---
> >>>   drivers/net/ethernet/intel/e1000e/netdev.c |   13 ++++++++-----
> >>>   1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> index fbf75fd..2853c11 100644
> >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> >>> @@ -5691,14 +5691,17 @@ static int e1000_runtime_suspend(struct device *dev)
> >>>          struct pci_dev *pdev = to_pci_dev(dev);
> >>>          struct net_device *netdev = pci_get_drvdata(pdev);
> >>>          struct e1000_adapter *adapter = netdev_priv(netdev);
> >>> +       int retval;
> >>> +       bool wake;
> >>>
> >>> -       if (e1000e_pm_ready(adapter)) {
> >>> -               bool wake;
> >>> +       if (!e1000e_pm_ready(adapter))
> >>> +               return 0;
> >>>
> >>> -               __e1000_shutdown(pdev,&wake, true);
> >>> -       }
> >>> +       retval = __e1000_shutdown(pdev,&wake, true);
> >>> +       if (!retval)
> >>> +               e1000_power_off(pdev, true, wake);
> >>>
> >>> -       return 0;
> >>> +       return retval;
> >>>   }
> >>>
> >>>   static int e1000_idle(struct device *dev)
> >>>
> >
> > I'd like the changelog to say what the bug is and how it is being fixed in
> > general terms.
> 
> Ok, my bad.
> 
> Problem: ethernet device does not work (no carrier signal).
> Right after boot it goes to runtime-suspend and never wake up.
> 
> Original code (before your commit) calls pci_prepare_to_sleep() and it
> calls pci_enable_wake() and switches device to one of D3 state.
> 
> It seems redundant, because pci_pm_runtime_suspend() do the same thing
> after calling ->runtime_suspend callback. Or rather it did it before commit
> 42eca2302146fed51335b95128e949ee6f54478f ("PCI: Don't touch card regs after
> runtime suspend D3") and third patch aimed fix this damage.
> 
> More over seems like calling pci_enable_wake() from e1000e isn't enough for my case,
> because my enthernet cannot wakeup from runtime-suspend without third patch.
> Seems like it's because pci_enable_wake() and pci_finish_runtime_suspend()
> calls different pratform-pm callbacks -- platform_pci_run_wake() /
> platform_pci_sleep_wake().
> 
> All this looks messy and I don't know how it should work.
> 
> If you prefer to minimize changes -- I can test how it would work without
> first (this) patch. Probably fine.

OK, I need to have a look at that driver, then.  Please give me some time,
though, I'm working on something different at the moment.

And by the way, by sending patches you declare that you know what you're doing.
If you really don't, it's better to just describe the problem. :-)

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/