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 19:02:35 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
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

> 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?

Ok the problem is we have three cases to distinguish

- altstatus must be used - in which case we want to blow up anyway if we
touch it (the usual case)
- altstatus should be used if available - shared IRQ check
- altstatus being used for flushing - altstatus irrelevant, it just has
to flush somehow.

So the _maybe naming sucks, but the reasoning I think is sound. The other
way to do it would be to replace it and the bit of irq handler logic with
an ata_sff_busy() that did the status checks correctly for both ctl and
non-ctl capable devices.

> 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 ndelay(400) is correct. The IO-read is Jeff being paranoid and
actually hurts us materially for the usual PIO case (bus PIO not disk
PIO) to the tune of about 1mS a command in many cases, but is needed for
MMIO (which we almost never do for any SFF hardware). That itself is a
different problem that can be fixed later (and not in -rc5). It wants
fixing as its a key reason that old IDE is still faster for PATA.

maybe_altstatus is crap naming but simply making ata_sff_altstatus fake a
reply in arbitary cases risks not catching mistakes and could mean we
don't catch corrupting mistakes which would be very bad indeed.

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