Re: 2.6.19-rc <-> ThinkPads

Linus Torvalds
2006-11-01T06:20:47Z

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