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 10:04:10 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
cc:	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org,
	jeff@...zik.org
Subject: Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org
 for pnp devices with no ctl

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