Suspend to RAM regression (bisected)

Suspend to RAM regression (bisected)

[Carlos R. Mafra]

Hi,

There is a suspend to RAM regression in the latest Linus tree of
17th of july (5b664cb235e). It was working perfectly with 
2.6.26.

If I suspend to RAM from X with 'echo mem > /sys/power/state' the
screen comes back very dark and I can't read anything.
If I switch to the text console and then go back to X this problem
is fixed and everything is ok.

I bisected this regression down to c0e09200dc0813972442e550a5905a132768e56c
("drm: reorganise drm tree to be more future proof"), and I checked that
reverting this commit (it reverts cleanly) really makes the regression go
away.

The laptop is a Vaio 64bit with Intel GM965/GL960 graphics card.
More information about my laptop can be found here:
http://www.ift.unesp.br/users/crmafra/dmesg-good.txt

Re: Suspend to RAM regression (bisected)

[Linus Torvalds]

On Sun, 20 Jul 2008, Carlos R. Mafra wrote:
> 
> I bisected this regression down to c0e09200dc0813972442e550a5905a132768e56c
> ("drm: reorganise drm tree to be more future proof"), and I checked that
> reverting this commit (it reverts cleanly) really makes the regression go
> away.

Hmm. That did almost only just renames, so maybe it's fixed by the pull I 
just did that added the commit "gpu: re-order GPU subdirectory vs char for 
AGP vs DRM startup".

I _just_ pushed out the end result, so it should be in my git tree in a 
couple of minutes when the mirroring finishes.

		Linus

Re: Suspend to RAM regression (bisected)

[Carlos R. Mafra]

On Sun 20.Jul'08 at 21:21:06 -0700, Linus Torvalds wrote:
> On Sun, 20 Jul 2008, Carlos R. Mafra wrote:
> > 
> > I bisected this regression down to c0e09200dc0813972442e550a5905a132768e56c
> > ("drm: reorganise drm tree to be more future proof"), and I checked that
> > reverting this commit (it reverts cleanly) really makes the regression go
> > away.
> 
> Hmm. That did almost only just renames, so maybe it's fixed by the pull I 
> just did that added the commit "gpu: re-order GPU subdirectory vs char for 
> AGP vs DRM startup".
> 
> I _just_ pushed out the end result, so it should be in my git tree in a 
> couple of minutes when the mirroring finishes.

Ok, I tried your new v2.6.26-5253-g14b395e and unfortunately suspend to
RAM fails completely now. The backlight doesn't turn off and the keyboard
leds keep blinking.

After several boots I bisected this new regression down to 
37437bb2e1ae8af470dfcd5b4ff454110894ccaf ("pkt_sched: Schedule qdiscs 
instead of netdev_queue.") by David Miller. 

I could not check whether reverting this commit on top of current 
kernel fixes the problem because it does not revert cleanly.

But the "good" kernels marked below with the exception of the first one 
(5b664cb23) did not have the original regression from my report of yesterday, 
so they were really good.

This is the bisection log:

# good: [5b664cb235e97afbf34db9c4d77f08ebd725335e] Merge branch 'upstream-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2
git-bisect good 5b664cb235e97afbf34db9c4d77f08ebd725335e
# bad: [14b395e35d1afdd8019d11b92e28041fad591b71] Merge branch 'for-2.6.27' of git://linux-nfs.org/~bfields/linux
git-bisect bad 14b395e35d1afdd8019d11b92e28041fad591b71
# good: [6ba33ac8a47d33d0b5996d24ddf66c457cd7180b] amd8111e: delete non NAPI code from the driver.
git-bisect good 6ba33ac8a47d33d0b5996d24ddf66c457cd7180b
# skip: [db6d8c7a4027b48d797b369a53f8470aaeed7063] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-2.6
git-bisect skip db6d8c7a4027b48d797b369a53f8470aaeed7063
# bad: [3a682fbd732d3d27bec722a923952b0938e8a404] pkt_sched: Fix build with NET_SCHED disabled.
git-bisect bad 3a682fbd732d3d27bec722a923952b0938e8a404
# good: [925068dcdc746236264d1877d3d5df656e87882a] Merge branch 'davem-next' of master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/netdev-2.6
git-bisect good 925068dcdc746236264d1877d3d5df656e87882a
# bad: [17715e62a5e5c7224e5f906a4b8f9e5084100118] pkt_sched: Use per-queue locking in shutdown_scheduler_queue.
git-bisect bad 17715e62a5e5c7224e5f906a4b8f9e5084100118
# good: [70efce27fc3d54271519244dc5e47da4ed711dd4] net/ipv4/tcp.c: Fix use of PULLHUP instead of POLLHUP in comments.
git-bisect good 70efce27fc3d54271519244dc5e47da4ed711dd4
# good: [9a6d276e85aa3d8f308fc5e8de6892daeb60ae5f] core: add stat to track unresolved discards in neighbor cache
git-bisect good 9a6d276e85aa3d8f308fc5e8de6892daeb60ae5f
# good: [51cb6db0f5654f08a4a6bfa3888dc36a51c2df3e] mac80211: Reimplement WME using ->select_queue().
git-bisect good 51cb6db0f5654f08a4a6bfa3888dc36a51c2df3e
# good: [d3b753db7c4f1f37a98b51974d484fda5d86dab5] pkt_sched: Move gso_skb into Qdisc.
git-bisect good d3b753db7c4f1f37a98b51974d484fda5d86dab5
# bad: [37437bb2e1ae8af470dfcd5b4ff454110894ccaf] pkt_sched: Schedule qdiscs instead of netdev_queue.
git-bisect bad 37437bb2e1ae8af470dfcd5b4ff454110894ccaf
# good: [7698b4fcabcd790efc4f226bada1e7b5870653af] pkt_sched: Add and use qdisc_root() and qdisc_root_lock().
git-bisect good 7698b4fcabcd790efc4f226bada1e7b5870653af

Re: Suspend to RAM regression (bisected)

[Linus Torvalds]

On Mon, 21 Jul 2008, Carlos R. Mafra wrote:
> 
> Ok, I tried your new v2.6.26-5253-g14b395e and unfortunately suspend to
> RAM fails completely now. The backlight doesn't turn off and the keyboard
> leds keep blinking.
> 
> After several boots I bisected this new regression down to 
> 37437bb2e1ae8af470dfcd5b4ff454110894ccaf ("pkt_sched: Schedule qdiscs 
> instead of netdev_queue.") by David Miller. 

Ok, I think this is an oops, and since it's bisected down to the same 
commit that some other oopses were bisected down to at boot-time, it's 
probably the same thing: something is calling "netif_wake_queue()" without 
having called "netif_start_queue()".

Or, to be more precise, in the case of suspending, something has probably 
called "dev_deactivate()" because of a link event or something like that, 
which seems to be a total piece-of-sh*t code that sets the qdisc back to 
the "illegal" noop_qdisc (thus causing oopses if some qdisc event 
happens), but does so *before* al the qdisc's have been quiesced (which it 
must do - because otherwise they may keep coming), so the same problem 
that plagued netif_wake_queue() will happen.

I don't really know the code very well (I'm waiting for David to fix up 
the mess), but I can imagine that the appended patch may at least turn the 
dead machine into a single warning and hopefully a working setup. Can you 
please try?

			Linus

---
 net/core/dev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 2eed17b..43ab4f5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1325,7 +1325,8 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 
 void __netif_schedule(struct Qdisc *q)
 {
-	BUG_ON(q == &noop_qdisc);
+	if (WARN_ON_ONCE(q == &noop_qdisc))
+		return;
 
 	if (!test_and_set_bit(__QDISC_STATE_SCHED, &q->state)) {
 		struct softnet_data *sd;

Re: Suspend to RAM regression (bisected)

[Carlos R. Mafra]

On Mon 21.Jul'08 at 10:12:30 -0700, Linus Torvalds wrote:
> 
> 
> On Mon, 21 Jul 2008, Carlos R. Mafra wrote:
> > 
> > Ok, I tried your new v2.6.26-5253-g14b395e and unfortunately suspend to
> > RAM fails completely now. The backlight doesn't turn off and the keyboard
> > leds keep blinking.
> > 
> > After several boots I bisected this new regression down to 
> > 37437bb2e1ae8af470dfcd5b4ff454110894ccaf ("pkt_sched: Schedule qdiscs 
> > instead of netdev_queue.") by David Miller. 
> 
> Ok, I think this is an oops, and since it's bisected down to the same 
> commit that some other oopses were bisected down to at boot-time, it's 
> probably the same thing: something is calling "netif_wake_queue()" without 
> having called "netif_start_queue()".
> 
> Or, to be more precise, in the case of suspending, something has probably 
> called "dev_deactivate()" because of a link event or something like that, 
> which seems to be a total piece-of-sh*t code that sets the qdisc back to 
> the "illegal" noop_qdisc (thus causing oopses if some qdisc event 
> happens), but does so *before* al the qdisc's have been quiesced (which it 
> must do - because otherwise they may keep coming), so the same problem 
> that plagued netif_wake_queue() will happen.
> 
> I don't really know the code very well (I'm waiting for David to fix up 
> the mess), but I can imagine that the appended patch may at least turn the 
> dead machine into a single warning and hopefully a working setup. Can you 
> please try?

I tested your patch below and now suspend to RAM is working again, it 
resumed just fine into X. So both regressions are gone now.

Thanks a lot,
Carlos


> 			Linus
> 
> ---
>  net/core/dev.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2eed17b..43ab4f5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1325,7 +1325,8 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  
>  void __netif_schedule(struct Qdisc *q)
>  {
> -	BUG_ON(q == &noop_qdisc);
> +	if (WARN_ON_ONCE(q == &noop_qdisc))
> +		return;
>  
>  	if (!test_and_set_bit(__QDISC_STATE_SCHED, &q->state)) {
>  		struct softnet_data *sd;