Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.orgfor pnp devices with no ctl

From: Linus Torvalds
Date: Thu May 29 2008 - 13:04:32 EST




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.

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.

Linus
--
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/