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    [W:0.034 / U:0.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site