On Thu, 29 May 2008, Alan Cox wrote:
>
> And this is how a revised one would look.
Ok, this looks nicer, but how about:
> +static u8 ata_sff_irq_status(struct ata_port *ap)
> +{
> + u8 status;
> +
> + if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
> + status = ata_sff_altstatus(ap);
This here is all about not oopsing in ata_sff_altstatus(), ie just knowing
about the bug.
Why not just fix ata_sff_altstatus(), instead of causing these kinds of
problems downstream?
If you don't want to read the status register, fine. But at least do
something like
- make ata_sff_altstatus static, since it's useless and dangerous to call
otherwise (ie make the exported interfaces be the nicer higher-level
ones that are sane)
- and make it just return 0 if the altstatus register doesn't exist.
At that point, both 'ata_sff_irq_status()' and 'ata_sff_sync()' can just
use ata_sff_altstatus() directly, *without* having to check that altstatus
setup by hand, or re-implement the function just because it was buggy and
broken to begin with.
So now ata_sff_irq_status() just becomes
static u8 ata_sff_irq_status(struct ata_port *ap)
{
u8 status;
status = ata_sff_altstatus(ap);
if (status & ATA_BUSY)
return status;
/* Clear INTRQ latch */
status = ata_sff_check_status(ap);
return status;
}
and if there was no altstatus register, everything is fine because
"ata_sff_altstatus()" was safe and returned 0 (and not ATA_BUSY).
Or feel free and make it return ATA_INVALID, which has a value of 0x100,
or something - it still won't be busy, and it will clearly not be a byte
read, so people *can* check for "no altstatus existed" if they want to.
Similarly, 'ata_sff_sync()' just becomes
void ata_sff_sync(struct ata_port *ap)
{
ata_sff_altstatus();
}
and 'ata_sff_pause()' becomes
void ata_sff_pause(struct ata_port *ap)
{
ata_sff_altstatus();
ndelay(400);
}
and again, if there is no altstatus register, that's a low-level driver
issue.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
