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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 29 May 2008 13:46:13 -0400
From:	Jeff Garzik <jeff@...zik.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
CC:	Alan Cox <alan@...rguk.ukuu.org.uk>, linux-kernel@...r.kernel.org,
	linux-ide@...r.kernel.org
Subject: Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org
 for pnp devices with no ctl

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/