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

[Linus Torvalds]

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-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/