lkml.org 
[lkml]   [2009]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] e100: do not go D3 in shutdown unless system is powering off
Date
On Tuesday 21 April 2009, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Apr 20, 2009 at 09:13:59PM +0200, Rafael J. Wysocki wrote:
> > On Monday 20 April 2009, Thadeu Lima de Souza Cascardo wrote:
> > > After experimenting with kexec with the last merges after 2.6.29, I've
> > > had some problems when probing e100. It would not read the eeprom. After
> > > some bisects, I realized this has been like that since forever (at least
> > > 2.6.18). The problem is that shutdown is doing the same thing that
> > > suspend does and puts the device in D3 state. I couldn't find a way to
> > > get the device back to a sane state in the probe function. So, based on
> > > some similar patches from Rafael J. Wysocki for e1000, e1000e and ixgbe,
> > > I wrote this one for e100.
> > >
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> > > ---
> > > drivers/net/e100.c | 27 +++++++++++++++++++++------
> > > 1 files changed, 21 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> > > index c0f8443..3db7b29 100644
> > > --- a/drivers/net/e100.c
> > > +++ b/drivers/net/e100.c
> > > @@ -2728,7 +2728,7 @@ static void __devexit e100_remove(struct pci_dev *pdev)
> > > #define E100_82552_SMARTSPEED 0x14 /* SmartSpeed Ctrl register */
> > > #define E100_82552_REV_ANEG 0x0200 /* Reverse auto-negotiation */
> > > #define E100_82552_ANEG_NOW 0x0400 /* Auto-negotiate now */
> > > -static int e100_suspend(struct pci_dev *pdev, pm_message_t state)
> > > +static int __e100_shutdown(struct pci_dev *pdev, bool *enable_wake)
> > > {
> > > struct net_device *netdev = pci_get_drvdata(pdev);
> > > struct nic *nic = netdev_priv(netdev);
> > > @@ -2749,19 +2749,31 @@ static int e100_suspend(struct pci_dev *pdev, pm_message_t state)
> > > E100_82552_SMARTSPEED, smartspeed |
> > > E100_82552_REV_ANEG | E100_82552_ANEG_NOW);
> > > }
> > > - if (pci_enable_wake(pdev, PCI_D3cold, true))
> > > - pci_enable_wake(pdev, PCI_D3hot, true);
> > > + *enable_wake = true;
> > > } else {
> > > - pci_enable_wake(pdev, PCI_D3hot, false);
> > > + *enable_wake = false;
> > > }
> > >
> > > pci_disable_device(pdev);
> > > - pci_set_power_state(pdev, PCI_D3hot);
> > >
> > > return 0;
> > > }
> > >
> > > +static void __e100_power_off(struct pci_dev *pdev, bool wake)
> > > +{
> > > + pci_enable_wake(pdev, PCI_D3hot, wake);
> > > + pci_set_power_state(pdev, PCI_D3hot);
> > > +}
> > > +
> > > #ifdef CONFIG_PM
> > > +static int e100_suspend(struct pci_dev *pdev, pm_message_t state)
> > > +{
> > > + bool wake;
> > > + int retval = __e100_shutdown(pdev, &wake);
> >
> > I'd call pci_prepare_to_sleep() here if wake is 'true' instead of the
> > __e100_power_off(), because there is a chance the platform will prefer some
> > other power state to put the device into.
> >
> > In fact, looking at the entire driver's code, I think you could just call
> > pci_prepare_to_sleep(pdev) here instead of __e100_power_off(pdev, wake)
> > and discard the value of wake.
> >
>
> If there is no advantage in using pci_enable_wake with false in the case
> the device cannot WOL or ASF, I will just use pci_prepare_to_sleep and
> drop this enable_wake/wake variable in both suspend and shutdown. Any
> reason we should use pci_enable_wake with false?

In principle there is one. Namely, if you call it with 'false', it will call
the platform (eg. ACPI) to disable the wake-up functionality of the device,
which generally may be necessary.

Also, pci_prepare_to_sleep() is really designed for suspend/hibernation,
because it first finds the appropriate state to put the device into and that
depends on the target sleep state of the system.

So, I'd recommend using pci_prepare_to_sleep() in .suspend() and the
pci_enable_wake()/pci_set_power_state() combo in .shutdown() (if the system
is going for power off).

Thanks,
Rafael


\
 
 \ /
  Last update: 2009-04-22 00:21    [from the cache]
©2003-2011 Jasper Spaans