[PATCH] libata-sff: Correct use of check_status()

[PATCH] libata-sff: Correct use of check_status()

[Alan Cox]

ata_check_status() does an SFF compliant check
ata_chk_status() does a generic call to ap->ops->check_status (usually
ata_check_status)

libata-sff uses the wrong one. Hardly suprising given the naming here,
which ought to get fixed to ata_sff_check_status() perhaps ?

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --exclude-from /usr/src/exclude --new-file --recursive linux.vanilla-2.6.23-mm1/drivers/ata/libata-sff.c linux-2.6.23-mm1/drivers/ata/libata-sff.c
--- linux.vanilla-2.6.23-mm1/drivers/ata/libata-sff.c	2007-10-15 15:03:26.000000000 +0100
+++ linux-2.6.23-mm1/drivers/ata/libata-sff.c	2007-10-15 15:16:29.000000000 +0100
@@ -156,7 +156,7 @@
 {
 	struct ata_ioports *ioaddr = &ap->ioaddr;
 
-	tf->command = ata_check_status(ap);
+	tf->command = ata_chk_status(ap);
 	tf->feature = ioread8(ioaddr->error_addr);
 	tf->nsect = ioread8(ioaddr->nsect_addr);
 	tf->lbal = ioread8(ioaddr->lbal_addr);

Re: [PATCH] libata-sff: Correct use of check_status()

[Jeff Garzik]

Alan Cox wrote:
> ata_check_status() does an SFF compliant check
> ata_chk_status() does a generic call to ap->ops->check_status (usually
> ata_check_status)
> 
> libata-sff uses the wrong one. Hardly suprising given the naming here,
> which ought to get fixed to ata_sff_check_status() perhaps ?
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> diff -u --exclude-from /usr/src/exclude --new-file --recursive linux.vanilla-2.6.23-mm1/drivers/ata/libata-sff.c linux-2.6.23-mm1/drivers/ata/libata-sff.c
> --- linux.vanilla-2.6.23-mm1/drivers/ata/libata-sff.c	2007-10-15 15:03:26.000000000 +0100
> +++ linux-2.6.23-mm1/drivers/ata/libata-sff.c	2007-10-15 15:16:29.000000000 +0100
> @@ -156,7 +156,7 @@
>  {
>  	struct ata_ioports *ioaddr = &ap->ioaddr;
>  
> -	tf->command = ata_check_status(ap);
> +	tf->command = ata_chk_status(ap);
>  	tf->feature = ioread8(ioaddr->error_addr);
>  	tf->nsect = ioread8(ioaddr->nsect_addr);
>  	tf->lbal = ioread8(ioaddr->lbal_addr);

applied, with a sigh:  it's in SFF, so I saw nothing wrong with 
ata_check_status().  I checked -- no SFF driver overrides it according 
to my audit, therefore the previous version was faster while still correct.

Re: [PATCH] libata-sff: Correct use of check_status()

[Alan Cox]

> > +	tf->command = ata_chk_status(ap);
> >  	tf->feature = ioread8(ioaddr->error_addr);
> >  	tf->nsect = ioread8(ioaddr->nsect_addr);
> >  	tf->lbal = ioread8(ioaddr->lbal_addr);
> 
> applied, with a sigh:  it's in SFF, so I saw nothing wrong with 
> ata_check_status().  I checked -- no SFF driver overrides it according 
> to my audit, therefore the previous version was faster while still correct.

I hit it with the NS87415 where ata_check_status() doesn't do the right
thing. Later on it turned out that there were enough other bugs when
enabling DMA that it needed its own tf read method anyway.

Alternative is to go through and clear up chk_atatus v check_status
(rename one sff_ to be clear), and explicitly document the assumption in
tf_read. That way it'll be obvious to whoever hits it in future and they
can supply their own tf_read method.

Preferences ?

Alan

Re: [PATCH] libata-sff: Correct use of check_status()

[Jeff Garzik]

Alan Cox wrote:
>>> +	tf->command = ata_chk_status(ap);
>>>  	tf->feature = ioread8(ioaddr->error_addr);
>>>  	tf->nsect = ioread8(ioaddr->nsect_addr);
>>>  	tf->lbal = ioread8(ioaddr->lbal_addr);
>> applied, with a sigh:  it's in SFF, so I saw nothing wrong with 
>> ata_check_status().  I checked -- no SFF driver overrides it according 
>> to my audit, therefore the previous version was faster while still correct.
> 
> I hit it with the NS87415 where ata_check_status() doesn't do the right
> thing. Later on it turned out that there were enough other bugs when
> enabling DMA that it needed its own tf read method anyway.
> 
> Alternative is to go through and clear up chk_atatus v check_status
> (rename one sff_ to be clear), and explicitly document the assumption in
> tf_read. That way it'll be obvious to whoever hits it in future and they
> can supply their own tf_read method.
> 
> Preferences ?

That was my general preference -- use your own ->tf_read()

	Jeff