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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux