Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.orgfor pnp devices with no ctl
From: Jeff Garzik
Date: Thu May 29 2008 - 13:46:33 EST
Linus Torvalds wrote:
On Thu, 29 May 2008, Alan Cox wrote:
(Jeff would you please take a look at this: Its #4 or #5 top OOPS on Arjan's
oops tracker, and it generally causes the boot to fail. First sent 20th May)
Quite frankly, if I was Jeff, I'd have refused to apply this patch as "too
damn ugly to live".
Why the *hell* doesn't it just fix "ata_sff_altstatus()" instead? Why does
it introduce a ludicrously named stupid "maybe" version of it that doesn't
oops?
In other words: in *any* case where the old "ata_sff_altstatus()" function
worked correctly, the new "ata_sff_maybe_altstatus()" function does THE
EXACT SAME THING. And in any case where the old "ata_sff_altstatus()"
function oopsed, the new "maybe" version at least is _better_.
In other words: there is absolutely no excuse for keeping the old (and
known-to-be-broken) "ata_sff_altstatus()" function at all. It should be
removed, not left around with an alternate function that works.
That's my general feeling on the issue. It was ugly and seemed to
needlessly avoid the existing one, which we would probably have to
bugfix later on...
I also think your "ata_sff_sync()" thing is buggy. It has a "ndelay(400)"
that is almost certainly buggy (it's the one that is already in
ata_sff_pause()).
It may be that you meant to make it an "else if" case, ie if there was no
IO-read, then you do a ndelay(400) as a last desperate case, but that's
not how your ata_sdd_sync() is actually written.
The double-ndelay is definitely wrong, but we do need one. Technically
it should -only- be a 400ns delay, but we also have a register read in
there to make sure any posted writes are flushed.
Jeff
--
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/