Re: hibernate event order question
From: Rafael J. Wysocki
Date: Sun May 18 2008 - 07:32:10 EST
On Sunday, 18 of May 2008, Tobias Diedrich wrote:
> Rafael J. Wysocki wrote:
> > On Sunday, 18 of May 2008, Tobias Diedrich wrote:
> > > Tobias Diedrich wrote:
> > > > > > Shouldn't there be a 'prepare for poweroff'-callback, which gets
> > > > > > called
> > > > > > before the system is powered off for real?
> > > > >
> > > > > Yes, it should and it's called in recent kernels.
> > > >
> > > > For what values of 'recent'?
> > > > I'm running 2.6.26-rc2 here. :)
> > > > How do I hook into this callback?
> > > > (suspend_late maybe? Going to try that one next...)
> > >
> > > Ok, this patch fixes the 'regression' introduced by the previous
> > > patch (at least for me ;)):
> >
> > Well, what exactly do you do to hibernate the box?
>
> |#!/bin/sh
> |
> |SYSPRINTK=/proc/sys/kernel/printk
> |OLDPRINTK=$(cat $SYSPRINTK)
> |chvt 1
> |# work around forcedeth wake-on-lan bug
> |#rmmod forcedeth
> |echo 9 > $SYSPRINTK
> |# shutdown/reboot/platform
> |echo shutdown > /sys/power/disk
You don't see the devices' suspend callbacks invoked before powering off
because of this. Change that to 'platform' (unless that breaks things).
> |echo disk > /sys/power/state
> |echo "$OLDPRINTK" > $SYSPRINTK
> |# work around forcedeth wake-on-lan bug
> |#modprobe forcedeth
> |#brctl addif br0 eth0
> |#brctl addif br0 eth1
> |#ifconfig eth0 up
> |#ifconfig eth1 up
>
> I now also now why I get the 'swapped mac' problem:
>
> In my case (DEV_HAS_CORRECT_MACADDR unset and
> NVREG_TRANSMITPOLL_MAC_ADDR_REV unset on probe)
> nv_probe() reads the original macaddress in reversed order to
> correct it and sets NVREG_TRANSMITPOLL_MAC_ADDR_REV.
>
> However the suspend/resume path does not touch the mac address at
> all, so after a cold boot and resume from disk the macaddress is
> again in 'BIOS order' and NVREG_TRANSMITPOLL_MAC_ADDR_REV is unset,
> but NVREG_TRANSMITPOLL_MAC_ADDR_REV gets set unconditionally in
> nv_resume(), so the net effect is that the MAC is now reversed.
>
> With the promisc fix this is hidden for my brige setup and only
> noticeable at the next hibernate, where I now have to use a reversed
> MAC to wakeup the device.
>
> The following patch (on top of the other two) fixes this by saving
> and restoring the memory mapped device configuration space in
> suspend/resume. The shutdown hook is still needed since the promisc
> mode seems to be incompatible with wake on lan.
>
> It also reorders the code in suspend/resume to match that in e100/e1000e.
> (configuration space is also saved for devices that are not running)
I'm unable to comment on the device-specific things, but apart from this the
patch looks okay to me except for one thing.
> Index: linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c
> ===================================================================
> --- linux-2.6.26-rc2.forcedwol.orig/drivers/net/forcedeth.c 2008-05-18 11:26:12.000000000 +0200
> +++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c 2008-05-18 13:00:23.000000000 +0200
> @@ -426,6 +426,7 @@
> #define NV_PCI_REGSZ_VER1 0x270
> #define NV_PCI_REGSZ_VER2 0x2d4
> #define NV_PCI_REGSZ_VER3 0x604
> +#define NV_PCI_REGSZ_MAX 0x604
>
> /* various timeout delays: all in usec */
> #define NV_TXRX_RESET_DELAY 4
> @@ -784,6 +785,9 @@
>
> /* flow control */
> u32 pause_flags;
> +
> + /* power saved state */
> + u32 saved_config_space[NV_PCI_REGSZ_MAX/4];
> };
>
> /*
> @@ -5785,19 +5789,24 @@
> {
> struct net_device *dev = pci_get_drvdata(pdev);
> struct fe_priv *np = netdev_priv(dev);
> -
> - if (!netif_running(dev))
> - goto out;
> + u8 __iomem *base = get_hwbase(dev);
> + int i;
>
> netif_device_detach(dev);
>
> - // Gross.
> - nv_close(dev);
> + if (netif_running(dev)) {
> + // Gross.
> + nv_close(dev);
> + }
> +
> + /* save non-pci configuration space */
> + for (i = 0;i <= np->register_size/sizeof(u32); i++)
> + np->saved_config_space[i] = readl(base + i*sizeof(u32));
>
> pci_save_state(pdev);
> pci_enable_wake(pdev, pci_choose_state(pdev, state), np->wolenabled);
> + pci_disable_device(pdev);
> pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -out:
>
> return 0;
> }
> @@ -5805,27 +5814,25 @@
> static int nv_resume(struct pci_dev *pdev)
> {
> struct net_device *dev = pci_get_drvdata(pdev);
> + struct fe_priv *np = netdev_priv(dev);
> u8 __iomem *base = get_hwbase(dev);
> - int rc = 0;
> - u32 txreg;
> -
> - if (!netif_running(dev))
> - goto out;
> -
> - netif_device_attach(dev);
> + int i, rc = 0;
>
> pci_set_power_state(pdev, PCI_D0);
> pci_restore_state(pdev);
> + /* ack any pending wake events, disable PME */
> pci_enable_wake(pdev, PCI_D0, 0);
>
> - /* restore mac address reverse flag */
> - txreg = readl(base + NvRegTransmitPoll);
> - txreg |= NVREG_TRANSMITPOLL_MAC_ADDR_REV;
> - writel(txreg, base + NvRegTransmitPoll);
> + /* restore non-pci configuration space */
> + for (i = 0;i <= np->register_size/sizeof(u32); i++)
> + writel(np->saved_config_space[i], base+i*sizeof(u32));
> +
> + netif_device_attach(dev);
> + if (netif_running(dev)) {
> + rc = nv_open(dev);
> + nv_set_multicast(dev);
> + }
>
> - rc = nv_open(dev);
> - nv_set_multicast(dev);
> -out:
> return rc;
> }
>
> @@ -5833,7 +5840,6 @@
> {
> struct net_device *dev = pci_get_drvdata(pdev);
> struct fe_priv *np = netdev_priv(dev);
> - u8 __iomem *base = get_hwbase(dev);
>
> if (netif_running(dev))
> nv_close(dev);
> @@ -5841,7 +5847,9 @@
> pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
> pci_enable_wake(pdev, PCI_D3cold, np->wolenabled);
> pci_disable_device(pdev);
> - pci_set_power_state(pdev, PCI_D3hot);
> + if (np->wolenabled)
> + pci_set_power_state(pdev, PCI_D3hot);
> + else pci_set_power_state(pdev, PCI_D3cold);
I'm not sure if it's possible to put the device into D3cold without actually
removing power from it.
> }
> #else
> #define nv_suspend NULL
>
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/