From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Cox Subject: Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl Date: Thu, 29 May 2008 19:02:35 +0100 Message-ID: <20080529190235.258d304e@core> References: <20080529161453.24735.805.stgit@core> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: Received: from earthlight.etchedpixels.co.uk ([81.2.110.250]:45453 "EHLO lxorguk.ukuu.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755536AbYE2SRS (ORCPT ); Thu, 29 May 2008 14:17:18 -0400 In-Reply-To: Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Linus Torvalds Cc: linux-kernel@vger.kernel.org, linux-ide@vger.kernel.org, jeff@garzik.org > 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.