Re: hibernate event order question

Re: hibernate event order question

[Tobias Diedrich]

Rafael J. Wysocki wrote:

> > Now for my question:
> > AFAICS the ordering of events is as follows:
> > 1) User reqests hibernate
> > 2) Tasks are frozen
> > 3) Device suspend callbacks get called
> > 4) Device resume callbacks get called
> > 5) Memory image is written to disk
> 
> 6) Device suspend callbacks get called again to prepare for the system sleep
>    state.

According to my pci postcard this is not the case.
I modified nv_suspend to wiggle port 0x80 with mdelay(100) between
0xa0/0x50 wiggles and mdelay(1000) before returning, so it is easily
countable.
nv_suspend is called only once per ethernet port (step 3 above).
I have not yet tried adding a suspend_late handler.

> > In my case, I think the fix for the first bug (promiscous mode does
> > not get restored on resume) breaks wake-on-lan (since the new value
> > of NvRegPacketFilterFlags may be incompatible with wake-on-lan).

I reconfirmed this.  Without the promiscous mode bugfix,
wake-on-lan (partially) works.  With the promiscous mode bugfix the
machine doesn't wake up.  This is also easily explainable by 6) not
happening. :)

> > 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...)

-- 
Tobias						PGP: http://9ac7e0bc.uguu.de
このメールは十割再利用されたビットで作られています。

Re: hibernate event order question

[Tobias Diedrich]

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 ;)):

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 01:10:18.000000000 +0200
+++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c	2008-05-18 01:11:33.000000000 +0200
@@ -5828,8 +5828,24 @@
 out:
 	return rc;
 }
+
+static void nv_shutdown(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);
+
+	if (netif_running(dev))
+		nv_close(dev);
+
+	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);
+}
 #else
 #define nv_suspend NULL
+#define nv_shutdown NULL
 #define nv_resume NULL
 #endif /* CONFIG_PM */
 
@@ -6000,6 +6016,7 @@
 	.remove		= __devexit_p(nv_remove),
 	.suspend	= nv_suspend,
 	.resume		= nv_resume,
+	.shutdown	= nv_shutdown,
 };
 
 static int __init init_nic(void)

-- 
Tobias						PGP: http://9ac7e0bc.uguu.de
このメールは十割再利用されたビットで作られています。

Re: hibernate event order question

[Rafael J. Wysocki]

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?


> 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 01:10:18.000000000 +0200
> +++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c	2008-05-18 01:11:33.000000000 +0200
> @@ -5828,8 +5828,24 @@
>  out:
>  	return rc;
>  }
> +
> +static void nv_shutdown(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);
> +
> +	if (netif_running(dev))
> +		nv_close(dev);
> +
> +	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);
> +}
>  #else
>  #define nv_suspend NULL
> +#define nv_shutdown NULL
>  #define nv_resume NULL
>  #endif /* CONFIG_PM */
>  
> @@ -6000,6 +6016,7 @@
>  	.remove		= __devexit_p(nv_remove),
>  	.suspend	= nv_suspend,
>  	.resume		= nv_resume,
> +	.shutdown	= nv_shutdown,
>  };
>  
>  static int __init init_nic(void)
> 

Re: hibernate event order question

[Tobias Diedrich]

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?

/usr/local/bin/s2disk:
|#!/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
|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)

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);
 }
 #else
 #define nv_suspend NULL

-- 
Tobias						PGP: http://9ac7e0bc.uguu.de
このメールは十割再利用されたビットで作られています。

Re: hibernate event order question

[Rafael J. Wysocki]

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

Re: hibernate event order question

[Tobias Diedrich]

Rafael J. Wysocki wrote:
> 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).

Hmm, I think someone recommended to use shutdown since that is
supposed to be more reliable.
Anyway, I think the shutdown hook is good anyway since I suspect
that if I do a normal system shutdown with promiscous mode still on,
then wake-on-lan would also not work?
I haven't checked that though.
At least I think the shutdown hook won't hurt. :)

> I'm unable to comment on the device-specific things, but apart from this the
> patch looks okay to me except for one thing.
> > +	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.

Ok, I've cleaned it up a bit and split it up properly (I think):
http://www.tdiedrich.de/~ranma/patches/forcedeth/2.6.26-rc2/
series:
forcedeth.resume.promisc.patch
forcedeth.hibernate.wol.patch
forcedeth.swappedmac.patch
forcedeth.reorder_suspend_resume.patch



forcedeth.resume.promisc.patch:
nv_open() resets multicast settings, call nv_set_multicast(dev) to restore them.
(Maybe this should rather be moved into nv_open())

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 13:52:59.000000000 +0200
+++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c	2008-05-18 13:53:02.000000000 +0200
@@ -5823,6 +5823,7 @@
 	writel(txreg, base + NvRegTransmitPoll);
 
 	rc = nv_open(dev);
+	nv_set_multicast(dev);
 out:
 	return rc;
 }



forcedeth.hibernate.wol.patch:
When hibernating in 'shutdown' mode, after saving the image the suspend hook
is not called again.
However, if the device is in promiscous mode, wake-on-lan will not work.
This adds a shutdown hook to setup wake-on-lan before the final shutdown.

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 13:53:02.000000000 +0200
+++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c	2008-05-18 13:53:06.000000000 +0200
@@ -5827,8 +5827,23 @@
 out:
 	return rc;
 }
+
+static void nv_shutdown(struct pci_dev *pdev)
+{
+	struct net_device *dev = pci_get_drvdata(pdev);
+	struct fe_priv *np = netdev_priv(dev);
+
+	if (netif_running(dev))
+		nv_close(dev);
+
+	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);
+}
 #else
 #define nv_suspend NULL
+#define nv_shutdown NULL
 #define nv_resume NULL
 #endif /* CONFIG_PM */
 
@@ -5999,6 +6014,7 @@
 	.remove		= __devexit_p(nv_remove),
 	.suspend	= nv_suspend,
 	.resume		= nv_resume,
+	.shutdown	= nv_shutdown,
 };
 
 static int __init init_nic(void)



forcedeth.swappedmac.patch:
The memory mapped device configuration space is lost during hibernate.
Save and restore it (fixes 'swapped mac' problem).

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 13:53:06.000000000 +0200
+++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c	2008-05-18 13:53:10.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,6 +5789,8 @@
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct fe_priv *np = netdev_priv(dev);
+	u8 __iomem *base = get_hwbase(dev);
+	int i;
 
 	if (!netif_running(dev))
 		goto out;
@@ -5794,6 +5800,10 @@
 	// 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_set_power_state(pdev, pci_choose_state(pdev, state));
@@ -5804,9 +5814,9 @@
 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;
+	int i, rc = 0;
 
 	if (!netif_running(dev))
 		goto out;
@@ -5817,10 +5827,9 @@
 	pci_restore_state(pdev);
 	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));
 
 	rc = nv_open(dev);
 	nv_set_multicast(dev);



forcedeth.reorder_suspend_resume.patch:
Match the suspend/resume code ordering in e100/e1000e more closely.
For example the configuration space should be saved on suspend even for
devices that are not up.

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 13:53:10.000000000 +0200
+++ linux-2.6.26-rc2.forcedwol/drivers/net/forcedeth.c	2008-05-18 13:54:04.000000000 +0200
@@ -5792,22 +5792,20 @@
 	u8 __iomem *base = get_hwbase(dev);
 	int i;
 
-	if (!netif_running(dev))
-		goto out;
-
+	if (netif_running(dev)) {
+		// Gross.
+		nv_close(dev);
+	}
 	netif_device_detach(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;
 }
 
@@ -5818,22 +5816,20 @@
 	u8 __iomem *base = get_hwbase(dev);
 	int i, rc = 0;
 
-	if (!netif_running(dev))
-		goto out;
-
-	netif_device_attach(dev);
-
 	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 non-pci configuration space */
 	for (i = 0;i <= np->register_size/sizeof(u32); i++)
 		writel(np->saved_config_space[i], base+i*sizeof(u32));
 
-	rc = nv_open(dev);
-	nv_set_multicast(dev);
-out:
+	netif_device_attach(dev);
+	if (netif_running(dev)) {
+		rc = nv_open(dev);
+		nv_set_multicast(dev);
+	}
 	return rc;
 }
 

-- 
Tobias						PGP: http://9ac7e0bc.uguu.de
このメールは十割再利用されたビットで作られています。

Re: hibernate event order question

[Rafael J. Wysocki]

On Sunday, 18 of May 2008, Tobias Diedrich wrote:
> Rafael J. Wysocki wrote:
> > 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).
> 
> Hmm, I think someone recommended to use shutdown since that is
> supposed to be more reliable.
> Anyway, I think the shutdown hook is good anyway since I suspect
> that if I do a normal system shutdown with promiscous mode still on,
> then wake-on-lan would also not work?
> I haven't checked that though.
> At least I think the shutdown hook won't hurt. :)

No, it won't.

> > I'm unable to comment on the device-specific things, but apart from this the
> > patch looks okay to me except for one thing.
> > > +	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.
> 
> Ok, I've cleaned it up a bit and split it up properly (I think):
> http://www.tdiedrich.de/~ranma/patches/forcedeth/2.6.26-rc2/
> series:

Can you send those patches as a series of messages, please?

> forcedeth.resume.promisc.patch
> forcedeth.hibernate.wol.patch
> forcedeth.swappedmac.patch
> forcedeth.reorder_suspend_resume.patch

Thanks,
Rafael

hibernate event order question

[Tobias Diedrich]

Hello,

for some time now I had the problem that after a suspend to disk
network connectivitiy was down and also wake-on-lan would not work.

First a bit of information, for the real question please see (*)
below.

Now I just found the ethtool -d (register dump) option
and decided I want to try to debug this long-standing bug:
http://bugzilla.kernel.org/show_bug.cgi?id=8381

I already found out why network connectivity is down after resume:

The interfaces eth0 and eth1 are bridged into br0.
The bridging code sets the interfaces into promiscous mode, but this
is not properly restored after resume.
This bug is easily fixable by calling nv_set_multicast(dev) either
from within nv_open() or after nv_open() in nv_resume().

The second problem (does not wol after s2disk) is still unsolved
though.  However I have found that wake-on-lan works fine if I
"rmmod forcedeth" before suspending.  This is even a usable
workaround for me, since the bridge device stays configured and I
just have to reload forcedeth and readd the interfaces to the bridge
after resume.

(*)
Now for my question:
AFAICS the ordering of events is as follows:
1) User reqests hibernate
2) Tasks are frozen
3) Device suspend callbacks get called
4) Device resume callbacks get called
5) Memory image is written to disk

Now, the problem I see with this is that while step 3) prepares the
device for suspend (and wake-on-lan), step 4) may undo some of this
preparation.

In my case, I think the fix for the first bug (promiscous mode does
not get restored on resume) breaks wake-on-lan (since the new value
of NvRegPacketFilterFlags may be incompatible with wake-on-lan).

Shouldn't there be a 'prepare for poweroff'-callback, which gets called
before the system is powered off for real?

--- linux-2.6.26-rc2/drivers/net/forcedeth.c	2008-05-17 14:22:06.000000000 +0200
+++ linux-2.6.26-rc2/drivers/net/forcedeth.c	2008-05-17 19:48:56.000000000 +0200
@@ -5823,6 +5823,7 @@
 	writel(txreg, base + NvRegTransmitPoll);
 
 	rc = nv_open(dev);
+	nv_set_multicast(dev);
 out:
 	return rc;
 }

-- 
Tobias						PGP: http://9ac7e0bc.uguu.de
このメールは十割再利用されたビットで作られています。