[PATCH] ide/libata: fix ata_id_is_cfa() (take 2)

[PATCH] ide/libata: fix ata_id_is_cfa() (take 2)

[Sergei Shtylyov]

When checking for the CFA feature set support, ata_id_is_cfa() tests bit 2 in
word 82 of the identify data instead the word 83;  it also checks the ATA/PI
standard support in the word 80 -- which the CompactFlash specification has
as reserved, this having no slightest chance to work on CF drives that don't
have 0x848A in the word 0. Use instead the usual validity check: word 83 bit
14 must be set, bit 15 cleared.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
The patch itself hasn't changed, only the description has.
I'm not sure who should queue this patch... it's against Linus' tree.

 include/linux/ata.h |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/ata.h
===================================================================
--- linux-2.6.orig/include/linux/ata.h
+++ linux-2.6/include/linux/ata.h
@@ -734,9 +734,8 @@ static inline int ata_id_is_cfa(const u1
 	if (id[ATA_ID_CONFIG] == 0x848A)	/* Standard CF */
 		return 1;
 	/* Could be CF hiding as standard ATA */
-	if (ata_id_major_version(id) >= 3 &&
-	    id[ATA_ID_COMMAND_SET_1] != 0xFFFF &&
-	   (id[ATA_ID_COMMAND_SET_1] & (1 << 2)))
+	if ((id[ATA_ID_COMMAND_SET_2] & 0xC000) == 0x4000 &&
+	    (id[ATA_ID_COMMAND_SET_2] & (1 << 2)))
 		return 1;
 	return 0;
 }

Re: [PATCH] ide/libata: fix ata_id_is_cfa() (take 2)

[Alan Cox]

On Tue, 27 Jan 2009 00:09:31 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> When checking for the CFA feature set support, ata_id_is_cfa() tests bit 2 in
> word 82 of the identify data instead the word 83;  it also checks the ATA/PI
> standard support in the word 80 -- which the CompactFlash specification has
> as reserved, this having no slightest chance to work on CF drives that don't
> have 0x848A in the word 0. Use instead the usual validity check: word 83 bit
> 14 must be set, bit 15 cleared.
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

NAK please stop mixing corrections with bugs that have been explained.

You can't exercise logic in the id_is_cfa check that relies upon the
device being CFA, which is what your comment says you are doing.

Re: [PATCH] ide/libata: fix ata_id_is_cfa() (take 2)

[Sergei Shtylyov]

Alan Cox wrote:

>>When checking for the CFA feature set support, ata_id_is_cfa() tests bit 2 in
>>word 82 of the identify data instead the word 83;  it also checks the ATA/PI
>>standard support in the word 80 -- which the CompactFlash specification has
>>as reserved, this having no slightest chance to work on CF drives that don't
>>have 0x848A in the word 0. Use instead the usual validity check: word 83 bit
>>14 must be set, bit 15 cleared.

>>Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

> NAK please stop mixing corrections with bugs that have been explained.

    Huh? You haven't explained anything. Your ATA base explanation is simply 
not valid in the CF world.

> You can't exercise logic in the id_is_cfa check that relies upon the
> device being CFA, which is what your comment says you are doing.

    Otherwise it has no chance to detect a CF drive *following the CF specs* 
and not having 0x848A in word 0. Please suggest a better solution. Perhaps we 
could check for word 80 being 0?

MBR, Sergei