Re: 2.6.19-rc <-> ThinkPads

[Linus Torvalds <torvalds@osdl.org>]

On Wed, 1 Nov 2006, Michael S. Tsirkin wrote:
> 
> I've been bisecting ACPI/suspend thinkpad issue myself and I seem to get
> eea0e11c1f0d6ef89e64182b2f1223a4ca2b74a2 good,
> cf4c6a2f27f5db810b69dcb1da7f194489e8ff88 bad.

Very interesting..

That commit cf4c6a2f on the face of it looks like an obvious cleanup, but 
looking closer, it actually changes the order of the apic writes in many 
cases (high word first -> low word first).

It also does something else that looks really really wrong: it turns an 
atomic "update ioapic and set irq-info" into two separate events, where 
interrupts can happen in between. Same goes for resume (instead of 
atomically changing all entries with the ioapic lock held, it now does 
them individually, and locks them individually).

I wonder if the order matters more, though. Andi? We _used_ to write the 
high word first, and I think the order matters. The low word contains the 
enable bit, for example, so when enabling an interrupt, you should write 
the low word last, when you disable it you should write the low word 
first.

And that's exactly what we _used_ to do, and it's what that particular 
commit totally and utterly _broke_.

I think that commit should either be reverted, or the code should be fixed 
to do the writes in the proper order. 

I suspect reverting it is the right thing to do - the patch only 
introduces bugs, an doesn't actually _fix_ anything, it just "cleans 
things up".

Andi, you need to be a hell of a lot more careful! Apparently x86-64 is 
also totally broken in this regard, because somebody didn't realize that 
the ordering _matters_.

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/