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

Linus Torvalds
2008-05-29T21:14:19Z

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