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

[Alan Cox]

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