Messages in this thread | | | Date | Thu, 29 May 2008 14:13:26 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl |
| |
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
| |