[PATCH] ide/libata: fix ata_id_is_cfa()

[PATCH] ide/libata: fix ata_id_is_cfa()

[Sergei Shtylyov]

When checking for 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 revision
in the  word 80 instead of usual validity check for the words 82-83 -- word 83
bit 14 set, bit 15 cleared.

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

---
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()

[Alan Cox]

On Fri, 23 Jan 2009 17:15:37 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> When checking for 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 revision
> in the  word 80 instead of usual validity check for the words 82-83 -- word 83
> bit 14 set, bit 15 cleared.

The word 82 validity bit is not sufficient as that bit is itself not
defined in ATA < 3. Otherwise the change looks correct.

Alan

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>When checking for 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 revision
>>in the  word 80 instead of usual validity check for the words 82-83 -- word 83
>>bit 14 set, bit 15 cleared.

> The word 82 validity bit is not sufficient as that bit is itself not

    There's no word 82 validity bits, its validity bits are in word 83.

> defined in ATA < 3. Otherwise the change looks correct.

    Well, then we need to fix every case of using *only* the validity bits in 
ata.h. Go ahead. ;-)

> Alan

WBR, Sergei

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

[Alan Cox]

> > The word 82 validity bit is not sufficient as that bit is itself not
> 
>     There's no word 82 validity bits, its validity bits are in word 83.

Word 83 sorry - I seem to have 82 and 83 bit flipped in my brain, not
good for ATA work ;)
> 
> > defined in ATA < 3. Otherwise the change looks correct.
> 
>     Well, then we need to fix every case of using *only* the validity bits in 
> ata.h. 

No reason to go removing ones that are correct

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>>The word 82 validity bit is not sufficient as that bit is itself not

>>    There's no word 82 validity bits, its validity bits are in word 83.

> Word 83 sorry - I seem to have 82 and 83 bit flipped in my brain, not
> good for ATA work ;)
> 
>>>defined in ATA < 3. Otherwise the change looks correct.

>>    Well, then we need to fix every case of using *only* the validity bits in 
>>ata.h. 

> No reason to go removing ones that are correct

    You've just effectively claimed them to be incorrect with your claim what 
validity bits are not sufficient and ATA revision must be checked. Check the 
source please -- I wouldn't have dropped the revision check if the rest of the 
inlines that check word 82/82 were using it.

MBR, Sergei

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

[Alan Cox]

> > No reason to go removing ones that are correct
> 
>     You've just effectively claimed them to be incorrect with your claim what 
> validity bits are not sufficient and ATA revision must be checked. Check the 

Nope. Please go re-read what I wrote.

> source please -- I wouldn't have dropped the revision check if the rest of the 
> inlines that check word 82/82 were using it.

In other words, as I said, you removed a correct check rather than
correcting others which may not be correct.

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>>No reason to go removing ones that are correct

>>    You've just effectively claimed them to be incorrect with your claim what 
>>validity bits are not sufficient and ATA revision must be checked. Check the 

> Nope. Please go re-read what I wrote.

    Then I don't understand why you wrote it -- I didn't urge you to fix the 
correct inlines.

>>source please -- I wouldn't have dropped the revision check if the rest of the 
>>inlines that check word 82/82 were using it.

> In other words, as I said, you removed a correct check rather than
> correcting others which may not be correct.

    Yes. But I have no time to fix everything for everybody.

MBR, Sergei

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

[Mark Lord]

Sergei Shtylyov wrote:
..
>> In other words, as I said, you removed a correct check rather than
>> correcting others which may not be correct.
> 
>    Yes. But I have no time to fix everything for everybody.
..

No issue with that, is there?
Just keep the check in the code you're touching, that's all.

Cheers

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

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

> Sergei Shtylyov wrote:

>>> In other words, as I said, you removed a correct check rather than
>>> correcting others which may not be correct.

    I'm now seeing that I misunderstood Alan. I got somewhat angered by 
linux-usb mails right before that.

>>    Yes. But I have no time to fix everything for everybody.

> ..

> No issue with that, is there?

    We'll see... :-)

> Just keep the check in the code you're touching, that's all.

    Sure, I'll repost the patch.

> Cheers

MBR, Sergei

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

[Sergei Shtylyov]

Alan Cox wrote:

>>When checking for 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 revision
>>in the  word 80 instead of usual validity check for the words 82-83 -- word 83
>>bit 14 set, bit 15 cleared.

> The word 82 validity bit is not sufficient as that bit is itself not
> defined in ATA < 3. Otherwise the change looks correct.

    Besides, I find the necessity of ATA revison check somewhat arguable. 
Words 64-127 were reserved since ATA-1 which meant that they shall be set to 0.

> Alan

MBR, Sergei

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

[Alan Cox]

On Fri, 23 Jan 2009 20:27:31 +0300
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Alan Cox wrote:
> 
> >>When checking for 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 revision
> >>in the  word 80 instead of usual validity check for the words 82-83 -- word 83
> >>bit 14 set, bit 15 cleared.
> 
> > The word 82 validity bit is not sufficient as that bit is itself not
> > defined in ATA < 3. Otherwise the change looks correct.
> 
>     Besides, I find the necessity of ATA revison check somewhat arguable. 
> Words 64-127 were reserved since ATA-1 which meant that they shall be set to 0.

One reason for the 0xC000/0x4000 check is that they were not - various
very old devices like to return 0xFFFF for all the spare bits as do a
fair number of EIDE devices.

Yes it's probably paranoia but this is ATA ;)

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

[Sergei Shtylyov]

Alan Cox wrote:

>>>>When checking for 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 revision
>>>>in the  word 80 instead of usual validity check for the words 82-83 -- word 83
>>>>bit 14 set, bit 15 cleared.

>>>The word 82 validity bit is not sufficient as that bit is itself not
>>>defined in ATA < 3. Otherwise the change looks correct.

>>    Besides, I find the necessity of ATA revison check somewhat arguable. 
>>Words 64-127 were reserved since ATA-1 which meant that they shall be set to 0.

> One reason for the 0xC000/0x4000 check is that they were not - various
> very old devices like to return 0xFFFF for all the spare bits as do a
> fair number of EIDE devices.

    Yes, but bit 14/15 check should cover such case without the revision check.

> Yes it's probably paranoia but this is ATA ;)

    So, we want ot stay paranoid WRT ata_id_is_cf()? We then have to be 
paranoid consitently. :-)

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:
> When checking for 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 revision
> in the  word 80 instead of usual validity check for the words 82-83 -- word 83
> bit 14 set, bit 15 cleared.
>
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>
> ---
> 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;
>  }

   Jeff, Bart, I've changed my mind about recasting this patch as Alan 
has requested, as I do consider it correct now. The version check should 
be unnecessary since I'm adding the validity bits check (hte fact that 
ata_id_major_complex() is quite cumbesome way of checking the minumum 
revision # -- this can be done with a single comparison instead of the 
bit scan). Note that words 64-127 were marked reserved, and must be set 
to 0 since ATA-1. Have your say please -- do I still need to recast the 
patch?

MBR, Sergei

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

[Alan Cox]

> bit scan). Note that words 64-127 were marked reserved, and must be set 
> to 0 since ATA-1. Have your say please -- do I still need to recast the 
> patch?

"Must be" but were not.

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

[Alan Cox]

>    Jeff, Bart, I've changed my mind about recasting this patch as Alan 
> has requested, as I do consider it correct now. The version check should 
> be unnecessary since I'm adding the validity bits check (hte fact that 
> ata_id_major_complex() is quite cumbesome way of checking the minumum 
> revision # -- this can be done with a single comparison instead of the 
> bit scan)

If you want to just do a specific check yes. I agree there it would make
some sense to have a ata_has_version(ATA_VER_BITFOO); that was a single
inline & check for ATA3+

Alan

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

[Bartlomiej Zolnierkiewicz]

On Sunday 25 January 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> > When checking for 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 revision
> > in the  word 80 instead of usual validity check for the words 82-83 -- word 83
> > bit 14 set, bit 15 cleared.
> >
> > Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> >
> > ---
> > 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;
> >  }
> 
>    Jeff, Bart, I've changed my mind about recasting this patch as Alan 
> has requested, as I do consider it correct now. The version check should 
> be unnecessary since I'm adding the validity bits check (hte fact that 
> ata_id_major_complex() is quite cumbesome way of checking the minumum 
> revision # -- this can be done with a single comparison instead of the 
> bit scan). Note that words 64-127 were marked reserved, and must be set 
> to 0 since ATA-1. Have your say please -- do I still need to recast the 
> patch?

I'm fine with the current version -- validity checking should take care of
handling reserved words (even if they are set to 0xff) and we can always add
more detailed checks later if necessary (which is very unlikely)...

Thanks,
Bart

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>> bit scan). Note that words 64-127 were marked reserved, and must be set 
>> to 0 since ATA-1. Have your say please -- do I still need to recast the 
>> patch?
>>     
>
> "Must be" but were not.
>   

  Let's see what ATA-3 rev. 7b has to say on this matter:

7.7.38 Words 82-83: Command sets supported

Words 82 and 83 indicate command sets supported. The values 0000h and 
FFFFh in these words indicate
that command set support is not indicated. Bits 4 through 15 of Word 82 
are reserved. Bits 0 through 13 of
Word 83 are reserved. Bit 14 of Word 83 shall be set to one. Bit 15 of 
Word 83 shall be cleared to zero.

   ATA/PI-4 rev. 18 further clarifies:

8.12.45 Words 82-84: Features/command sets supported

Words 82, 83, and 84 shall indicate features/command sets supported. The 
value 0000h or FFFFh was
placed in each of these words by devices prior to ATA-3 and shall be 
interpreted by the host as meaning
that features/command sets supported are not indicated. Bits 1 through 
13 of word 83 and bits 0 through 13
of word 84 are reserved. Bit 14 of word 83 and word 84 shall be set to 
one and bit 15 of word 83 and word
84 shall be cleared to zero to provide indication that the 
features/command sets supported words are valid.

   Since checking bits 14-15 filters out both 0 and 0xFFFF, I don't see 
why we have to check for ATA revision prior to that -- unless we're 
trying to guard against pre ATA-3 values other than 0 or 0xFFFF.

MBR, Sergei

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

[Alan Cox]

> why we have to check for ATA revision prior to that -- unless we're 
> trying to guard against pre ATA-3 values other than 0 or 0xFFFF.

Which we are. At the point we first use this function we are poking
blindly at a device we have not done any other analysis of.

Alan

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

[Sergei Shtylyov]

Alan Cox wrote:

>>why we have to check for ATA revision prior to that -- unless we're 
>>trying to guard against pre ATA-3 values other than 0 or 0xFFFF.

> Which we are.

    On what grounds? Note that ATA-3 only names 0 and 0xFFFF as inappropriate 
values.

> At the point we first use this function we are poking
> blindly at a device we have not done any other analysis of.

    Do you mean ata_dev_readid()? I'm seeing no issue with checking for ATA 
version there directly if you insist it should be done.

> Alan

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

>> When checking for 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 
>> revision
>> in the  word 80 instead of usual validity check for the words 82-83 -- 
>> word 83
>> bit 14 set, bit 15 cleared.

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

>> ---
>> 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;
>>  }

>   Jeff, Bart, I've changed my mind about recasting this patch as Alan 
> has requested, as I do consider it correct now. The version check should 
> be unnecessary since I'm adding the validity bits check

    ... replacing 0xFFFF check (with zero value got sorted out by testing bit 
2) with it to be precise.

> (hte fact that 
> ata_id_major_complex() is quite cumbesome way of checking the minumum 
> revision # -- this can be done with a single comparison instead of the 
> bit scan).

    ... "added to that" I meant to type.

MBR, Sergei

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

[Alan Cox]

On Mon, 26 Jan 2009 21:11:33 +0300
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Alan Cox wrote:
> 
> >>why we have to check for ATA revision prior to that -- unless we're 
> >>trying to guard against pre ATA-3 values other than 0 or 0xFFFF.
> 
> > Which we are.
> 
>     On what grounds? Note that ATA-3 only names 0 and 0xFFFF as inappropriate 
> values.

On the grounds that there are devices that predate ATA-3 and that we
should be robust.

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

[Alan Cox]

This is what I would favour:

libata: Fix CFA detection & improve version inlines

From: Alan Cox <alan@redhat.com>

Sergei Shtylyov noticed thae ata_id_is_cfa checked the wrong word for the
secondary CFA test and posted a patch set to improve this, but which
removed the version checks.

This patch keeps the version checks but incorporates the other suggestions
Sergei made including a better ata version check for the usual case where
we want to know "is version >= x" rather than "what version do you
support".

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
---

 drivers/ata/libata-core.c |    2 +-
 include/linux/ata.h       |   22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)


diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 88c2428..c17b62a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2212,7 +2212,7 @@ retry:
 		 * Note that ATA4 says lba is mandatory so the second check
 		 * shoud never trigger.
 		 */
-		if (ata_id_major_version(id) < 4 || !ata_id_has_lba(id)) {
+		if (!ata_id_has_version(id, 4) || !ata_id_has_lba(id)) {
 			err_mask = ata_dev_init_params(dev, id[3], id[6]);
 			if (err_mask) {
 				rc = -EIO;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index a53318b..e35d8de 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -675,6 +675,13 @@ static inline unsigned int ata_id_major_version(const u16 *id)
 	return mver;
 }
 
+static inline int ata_id_has_version(const u16 *id, int v)
+{
+	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
+		return 0;
+	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;
+}
+ 
 static inline int ata_id_is_sata(const u16 *id)
 {
 	/*
@@ -691,7 +698,7 @@ static inline int ata_id_is_sata(const u16 *id)
 static inline int ata_id_has_tpm(const u16 *id)
 {
 	/* The TPM bits are only valid on ATA8 */
-	if (ata_id_major_version(id) < 8)
+	if (!ata_id_has_version(id, 8))
 		return 0;
 	if ((id[48] & 0xC000) != 0x4000)
 		return 0;
@@ -701,7 +708,7 @@ static inline int ata_id_has_tpm(const u16 *id)
 static inline int ata_id_has_dword_io(const u16 *id)
 {
 	/* ATA 8 reuses this flag for "trusted" computing */
-	if (ata_id_major_version(id) > 7)
+	if (ata_id_has_version(id, 8))
 		return 0;
 	if (id[ATA_ID_DWORD_IO] & (1 << 0))
 		return 1;
@@ -710,7 +717,7 @@ static inline int ata_id_has_dword_io(const u16 *id)
 
 static inline int ata_id_has_unload(const u16 *id)
 {
-	if (ata_id_major_version(id) >= 7 &&
+	if (ata_id_has_version(id, 7) &&
 	    (id[ATA_ID_CFSSE] & 0xC000) == 0x4000 &&
 	    id[ATA_ID_CFSSE] & (1 << 13))
 		return 1;
@@ -734,15 +741,16 @@ static inline int ata_id_is_cfa(const u16 *id)
 	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 (ata_id_has_version(id, 3) &&
+	   (id[ATA_ID_COMMAND_SET_2] & 0xC000) == 0x4000 &&
+	   (id[ATA_ID_COMMAND_SET_2] & (1 << 2)))
 		return 1;
 	return 0;
 }
 
 static inline int ata_id_is_ssd(const u16 *id)
 {
+	/* FIXME: Check validity of version */
 	return id[ATA_ID_ROT_SPEED] == 0x01;
 }
 
@@ -779,7 +787,7 @@ static inline int atapi_command_packet_set(const u16 *dev_id)
 
 static inline int atapi_id_dmadir(const u16 *dev_id)
 {
-	return ata_id_major_version(dev_id) >= 7 && (dev_id[62] & 0x8000);
+	return ata_id_has_version(dev_id, 7) && (dev_id[62] & 0x8000);
 }
 
 /*

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

[Sergei Shtylyov]

Alan Cox wrote:

>>>>why we have to check for ATA revision prior to that -- unless we're 
>>>>trying to guard against pre ATA-3 values other than 0 or 0xFFFF.

>>>Which we are.

>>    On what grounds? Note that ATA-3 only names 0 and 0xFFFF as inappropriate 
>>values.

> On the grounds that there are devices that predate ATA-3 and that we
> should be robust.

    ATA/PI-4 states they had 0 or 0xFFFF there. Again, we weren't robust (by 
your definition) in other cases of using words 82-83 -- have it caused issues?
    Another argument about robustness: CF specifies word 80 as *reserved* but 
does specifies words 82-84 since at least 2.1. What to do about that?

MBR, Sergei

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

[Sergei Shtylyov]

Alan Cox wrote:

> This is what I would favour:

> libata: Fix CFA detection & improve version inlines

> From: Alan Cox <alan@redhat.com>

> Sergei Shtylyov noticed thae ata_id_is_cfa checked the wrong word for the
> secondary CFA test and posted a patch set to improve this, but which
> removed the version checks.

    And did that completely correctly.

> This patch keeps the version checks but incorporates the other suggestions
> Sergei made including a better ata version check for the usual case where
> we want to know "is version >= x" rather than "what version do you
> support".

    Improvements should be mixed with fixes.

> Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>

    NAK.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 88c2428..c17b62a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index a53318b..e35d8de 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
[...]
> @@ -734,15 +741,16 @@ static inline int ata_id_is_cfa(const u16 *id)
>  	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 (ata_id_has_version(id, 3) &&

    Refer to the CF specs (google for cfspc<x_><y>.pdf) as to why it's wrong.

WBR, Sergei

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

[Sergei Shtylyov]

Alan Cox wrote:
> This is what I would favour:

> libata: Fix CFA detection & improve version inlines

> From: Alan Cox <alan@redhat.com>

> Sergei Shtylyov noticed thae ata_id_is_cfa checked the wrong word for the
> secondary CFA test and posted a patch set to improve this, but which
> removed the version checks.

> This patch keeps the version checks but incorporates the other suggestions
> Sergei made including a better ata version check for the usual case where
> we want to know "is version >= x" rather than "what version do you
> support".

> Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>

> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index a53318b..e35d8de 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -675,6 +675,13 @@ static inline unsigned int ata_id_major_version(const u16 *id)
>  	return mver;
>  }
>  
> +static inline int ata_id_has_version(const u16 *id, int v)
> +{
> +	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
> +		return 0;
> +	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;

    Refer to afa_dev_cf_sata() on how it's done in really optimal way.

WBR, Sergei

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>Hello, I wrote:

>>>When checking for 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 revision
>>>in the  word 80 instead of usual validity check for the words 82-83 -- word 83
>>>bit 14 set, bit 15 cleared.

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

>>>---
>>>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;
>>> }

>>   Jeff, Bart, I've changed my mind about recasting this patch as Alan 
>>has requested, as I do consider it correct now. The version check should 
>>be unnecessary since I'm adding the validity bits check (hte fact that 
>>ata_id_major_complex() is quite cumbesome way of checking the minumum 
>>revision # -- this can be done with a single comparison instead of the 
>>bit scan). Note that words 64-127 were marked reserved, and must be set 
>>to 0 since ATA-1. Have your say please -- do I still need to recast the 
>>patch?

> I'm fine with the current version -- validity checking should take care of
> handling reserved words (even if they are set to 0xff) and we can always add
> more detailed checks later if necessary (which is very unlikely)...

    The only improvement it needs is referring to the CF specs on why the 
version check wasn't really ever correct.

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

>> This is what I would favour:

>> libata: Fix CFA detection & improve version inlines

>> From: Alan Cox <alan@redhat.com>

>> Sergei Shtylyov noticed thae ata_id_is_cfa checked the wrong word for the
>> secondary CFA test and posted a patch set to improve this, but which
>> removed the version checks.

>    And did that completely correctly.

>> This patch keeps the version checks but incorporates the other 
>> suggestions
>> Sergei made including a better ata version check for the usual case where
>> we want to know "is version >= x" rather than "what version do you
>> support".

>    Improvements should be mixed with fixes.

    Should not be, I meant to type.

WBR, Sergei

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

[Alan Cox]

> > +	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
> > +		return 0;
> > +	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;
> 
>     Refer to afa_dev_cf_sata() on how it's done in really optimal way.

To what ? - there is no ata or afa_dev_cf_sata ?

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

[Alan Cox]

On Mon, 26 Jan 2009 22:28:51 +0300
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote
> > secondary CFA test and posted a patch set to improve this, but which
> > removed the version checks.
> 
>     And did that completely correctly.

In your opinion but as I've pointed out that seems incorrect so simply
repeating ad-nauseum that your patch is correct isn't helpful.


> > This patch keeps the version checks but incorporates the other suggestions
> > Sergei made including a better ata version check for the usual case where
> > we want to know "is version >= x" rather than "what version do you
> > support".
> 
>     Improvements should be mixed with fixes.

They were.

Which is better than correcting things while intentionally adding flaws
by removing sanity checks.

>  Refer to the CF specs (google for cfspc<x_><y>.pdf) as to why it's wrong.

I've read the CF specs back to 1.1, and the ATA specs back to ATA-1.
Prior to ATA-3 you can't rely on the test in question.

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

[Alan Cox]

> >    Improvements should be mixed with fixes.
> 
>     Should not be, I meant to type.

In this case it makes complete sense to do the work as one. I can
pointlessly split it into two patches so you can generate twice as many
complaints if it amuses you.

Alan

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

[Alan Cox]

> >    Improvements should be mixed with fixes.
> 
>     Should not be, I meant to type.

You neglected to reply to the rest of the mail btw. If you are going to
point people at CF specs without explanation as an response to something
then perhaps you should bother to explain why you think that magically
proves a point we disagree over

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

[Sergei Shtylyov]

Alan Cox wrote:

>>>+	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
>>>+		return 0;
>>>+	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;

>>    Refer to afa_dev_cf_sata() on how it's done in really optimal way.

> To what ? - there is no ata or afa_dev_cf_sata ?

    Very funny. Meant to be ata_dev_is_sata(), of course.

MBR, Sergei

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

[Alan Cox]

On Mon, 26 Jan 2009 22:45:00 +0300
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Alan Cox wrote:
> 
> >>>+	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
> >>>+		return 0;
> >>>+	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;
> 
> >>    Refer to afa_dev_cf_sata() on how it's done in really optimal way.
> 
> > To what ? - there is no ata or afa_dev_cf_sata ?
> 
>     Very funny. Meant to be ata_dev_is_sata(), of course.

We don't have one of those either - do you mean ata_id_is_sata ? If so
then yes that looks like it might be slightly cleaner although its
probably one instruction difference from the .s files.

I'll redo it that way

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

[Sergei Shtylyov]

Alan Cox wrote:

>>>   Improvements should be mixed with fixes.

>>    Should not be, I meant to type.

> You neglected to reply to the rest of the mail btw. If you are going to
> point people at CF specs without explanation as an response to something
> then perhaps you should bother to explain why you think that magically
> proves a point we disagree over

    I have explained everything in the prior mail. I hadn't expect you to 
start the patch fencing.

WBR, Sergei

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

[Alan Cox]

>     I have explained everything in the prior mail. I hadn't expect you to 
> start the patch fencing.

I don't see an explanation just points I queried. I shall draw the obvious
conclusion from the fact you don't feel like providing one

The logic is this

ATA-3 or higher - that word has a defined meaning
ATA < 3 that word should be 0x0000
pre ATA (EIDE) or head up backside implementations that would will be
anything but usually 0x0000 or 0xFFFF

We cannot test for ATA < 3 because there is no version bit for it (and
in fact identify is optional which I need to fix at some point by
permitting geometry passing so that support is at parity with old IDE)

Therefore we want to check

	CFA signature -> CFA (good for CFA 1.1 and later devices using it)
	ATA >= 3 claimed - word is trustable bit is 0 or means CFA


Yes the implementation is paranoid, but having done ten years working for
a distro dealing with PC hardware in volume day in and day out I've yet
to regret being paranoid. Assuming every piece of hardware sucks, nobody
ever read the standard and every BIOS table is wrong is a staple part of
writing a robust OS for the PC platform.

Alan

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

[Sergei Shtylyov]

Alan Cox wrote:

>>>>>+	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
>>>>>+		return 0;
>>>>>+	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;

>>>>   Refer to afa_dev_cf_sata() on how it's done in really optimal way.

>>>To what ? - there is no ata or afa_dev_cf_sata ?

>>    Very funny. Meant to be ata_dev_is_sata(), of course.

> We don't have one of those either - do you mean ata_id_is_sata ? If so
> then yes that looks like it might be slightly cleaner although its
> probably one instruction difference from the .s files.

    That extra *if* cost more than instruction I think.

> I'll redo it that way

    OK, just leave ata_dev_is_cfa() alone and rename the patch.

WBR, Sergei

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

[Jeff Garzik]

Sergei Shtylyov wrote:
> Alan Cox wrote:
> 
>>>>>> +    if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
>>>>>> +        return 0;
>>>>>> +    return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;
> 
>>>>>   Refer to afa_dev_cf_sata() on how it's done in really optimal way.
> 
>>>> To what ? - there is no ata or afa_dev_cf_sata ?
> 
>>>    Very funny. Meant to be ata_dev_is_sata(), of course.
> 
>> We don't have one of those either - do you mean ata_id_is_sata ? If so
>> then yes that looks like it might be slightly cleaner although its
>> probably one instruction difference from the .s files.
> 
>    That extra *if* cost more than instruction I think.

Either way, this is irrelevant, since this isn't used in any hot path 
that I am aware of... :)

Alan just posted a reasonable explanation in the "The logic is this" 
email, maybe we can reboot the discussion from there?

Responding to a side point, I don't think its a big deal to combine 
fixes and improvements into a single patch, if you are dealing with the 
same few lines of code.  Just make sure the patch description (and/or 
code comment) enumerates the fixes and improvements both...

	Jeff

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

[Sergei Shtylyov]

Hello, I wrote:

>>>>>> +    if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
>>>>>> +        return 0;
>>>>>> +    return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;

>>>>>   Refer to afa_dev_cf_sata() on how it's done in really optimal way.

>>>> To what ? - there is no ata or afa_dev_cf_sata ?

>>>    Very funny. Meant to be ata_dev_is_sata(), of course.

>> We don't have one of those either - do you mean ata_id_is_sata ?

   Sure I have, just got used to its original name, ide_dev_is_sata().

>> If so then yes that looks like it might be slightly cleaner although its
>> probably one instruction difference from the .s files.

>    That extra *if* cost more than instruction I think.

    At least 2 on x86.

>> I'll redo it that way

>    OK, just leave ata_dev_is_cfa() alone and rename the patch.

    Just reposted my patch with better description.

MBR, Sergei

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

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>>>>>>> +    if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
>>>>>>> +        return 0;
>>>>>>> +    return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;

>>>>>>   Refer to afa_dev_cf_sata() on how it's done in really optimal way.

>>>>> To what ? - there is no ata or afa_dev_cf_sata ?

>>>>    Very funny. Meant to be ata_dev_is_sata(), of course.

>>> We don't have one of those either - do you mean ata_id_is_sata ? If so
>>> then yes that looks like it might be slightly cleaner although its
>>> probably one instruction difference from the .s files.

>>    That extra *if* cost more than instruction I think.

> Either way, this is irrelevant, since this isn't used in any hot path 
> that I am aware of... :)

> Alan just posted a reasonable explanation in the "The logic is this" 
> email, maybe we can reboot the discussion from there?

    Please read all the thread, and you'll see that Alan's CFA patch was 
totally wrong in that part from the very start -- CF devices don't report ATA 
standard support in word 80, that's forbidden (!) by the CF specs since at 
least 2.1.

> Responding to a side point, I don't think its a big deal to combine 
> fixes and improvements into a single patch, if you are dealing with the 
> same few lines of code.

    Not the case here -- the fix is very local, improvements are spread over 
several inlines.

>     Jeff

MBR, Sergei

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

[Sergei Shtylyov]

Alan Cox wrote:

>>    I have explained everything in the prior mail. I hadn't expect you to 
>>start the patch fencing.

> I don't see an explanation just points I queried.

    Have you seen this mail:

http://marc.info/?l=linux-ide&m=123299798729575

    If you haven't, read it. After you have, the argument must be over.

> I shall draw the obvious conclusion from the fact you don't feel like providing one

> The logic is this

> ATA-3 or higher - that word has a defined meaning
> ATA < 3 that word should be 0x0000
> pre ATA (EIDE) or head up backside implementations that would will be
> anything but usually 0x0000 or 0xFFFF

> We cannot test for ATA < 3 because there is no version bit for it

    That's not quite true, read the ATA-3 standard better.

> Therefore we want to check

> 	CFA signature -> CFA (good for CFA 1.1 and later devices using it)
> 	ATA >= 3 claimed - word is trustable bit is 0 or means CFA

    The problem is that the CF specs explicitly forbid (!) to report anything 
in word 80 -- it's reserved and must be 0.

> Yes the implementation is paranoid, but having done ten years working for
> a distro dealing with PC hardware in volume day in and day out I've yet

    Working while checking word 82 ISO word 83? Who are you trying to cheat?

> to regret being paranoid.

    Again, I'm not seeing this kind of version paranoia in any place but the 
one that was wrong: ata_id_is_cfa().

> Assuming every piece of hardware sucks, nobody
> ever read the standard and every BIOS table is wrong is a staple part of
> writing a robust OS for the PC platform.

    Yes, we just had report of the CF drive which is totally unidentifiable... 
but perhaps you still should start reading the specs as more manufacturers are 
doing it now and following them?

> Alan

MBR, Sergei

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

[Alan Cox]

> totally wrong in that part from the very start -- CF devices don't report ATA 
> standard support in word 80, that's forbidden (!) by the CF specs since at 
> least 2.1.

And the ATA world says that if word 80 doesn't report any standards then
the word is potentially undefined....welcome to PC hell

Perhaps the best we can do is to test

	word 80 == 0 && word 83 bit set && word 83 valid

Fortunately the use is almost entirely to print the right CFA/ATA string
at boot ?

Now Sergei if you'd said that explicitly (or if you did before I didn't
see it) it would have been a bit simpler to work out why you were arguing
the needed for these changes.

Alan

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

[Sergei Shtylyov]

Alan Cox wrote:

>>totally wrong in that part from the very start -- CF devices don't report ATA 
>>standard support in word 80, that's forbidden (!) by the CF specs since at 
>>least 2.1.

> And the ATA world says that if word 80 doesn't report any standards then
> the word is potentially undefined....

    It's all your fantasy this time. :-D
    Do start reading the specs attentively. All ATA standards only have that 
if bits 14:15 are not 1:0, the words are not valid (as a variant, if the word 
is 0 or 0xFFFF).

 > welcome to PC hell

    I've started on PCs, and spent "the best years of my life" with them -- no 
need to welcome me. :-)

> Perhaps the best we can do is to test

> 	word 80 == 0 && word 83 bit set && word 83 valid

> Fortunately the use is almost entirely to print the right CFA/ATA string
> at boot ?

    What about PIO/DMA modes?

> Now Sergei if you'd said that explicitly (or if you did before I didn't
> see it) it would have been a bit simpler to work out why you were arguing
> the needed for these changes.

    If you think that I now have plenty of time to look into all the CF and 
ATA standard, you are very wrong. However, you caused me to lose much time on 
that pointless argument...

> Alan

MBR, Sergei

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

[Sergei Shtylyov]

Alan Cox wrote:

>>    I have explained everything in the prior mail. I hadn't expect you to 
>>start the patch fencing.

> I don't see an explanation just points I queried. I shall draw the obvious
> conclusion from the fact you don't feel like providing one

> The logic is this

> ATA-3 or higher - that word has a defined meaning
> ATA < 3 that word should be 0x0000

    Dunno why but I removed my remark: they are reserved and so "shall be 
zero" starting aith ATA-1. I've already stated that.

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

>> I shall draw the obvious conclusion from the fact you don't feel like 
>> providing one

>> The logic is this

>> ATA-3 or higher - that word has a defined meaning
>> ATA < 3 that word should be 0x0000
>> pre ATA (EIDE) or head up backside implementations that would will be
>> anything but usually 0x0000 or 0xFFFF

>> We cannot test for ATA < 3 because there is no version bit for it

>    That's not quite true, read the ATA-3 standard better.

>> Therefore we want to check

>>     CFA signature -> CFA (good for CFA 1.1 and later devices using it)
>>     ATA >= 3 claimed - word is trustable bit is 0 or means CFA

>    The problem is that the CF specs explicitly forbid (!) to report 
> anything in word 80 -- it's reserved and must be 0.
> 
>> Yes the implementation is paranoid, but having done ten years working for
>> a distro dealing with PC hardware in volume day in and day out I've yet

>    Working while checking word 82 ISO word 83? Who are you trying to cheat?

    I'm sorry, that was totally off base. I've misread this whole paragraph in 
haste. :-<
    The rest of my evening was wasted, sigh.

MBR, Sergei

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

[Alan Cox]

>     I'm sorry, that was totally off base. I've misread this whole paragraph in 
> haste. :-<
>     The rest of my evening was wasted, sigh.

If it got the CF code fixed it wasn't a waste.

Do I understand rightly that the agreed things to do are

- redo the version check patch using the signed check as you suggested
and remove the changes to the CFA check from it
- make the CFA code check word 80 == 0 as a sanity check
- apply your change to check the bit in the right command word for CFA

?

Alan

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

[Sergei Shtylyov]

Alan Cox wrote:

>>    I'm sorry, that was totally off base. I've misread this whole paragraph in 
>>haste. :-<
>>    The rest of my evening was wasted, sigh.

> If it got the CF code fixed it wasn't a waste.

> Do I understand rightly that the agreed things to do are

> - redo the version check patch using the signed check as you suggested
> and remove the changes to the CFA check from it

    Yes, those shouldn't have been intermixed from the very start.

> - make the CFA code check word 80 == 0 as a sanity check

    That'll work unless we bump into a drive that does follow ATA in that 
matter. My vote would still be for ignoring the version...

> - apply your change to check the bit in the right command word for CFA

    My change wasn't only that. I will repost it after some sleep -- with word 
80 check for 0 if you're so adamant about it.

> Alan

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

>>>    I'm sorry, that was totally off base. I've misread this whole 
>>> paragraph in haste. :-<
>>>    The rest of my evening was wasted, sigh.

>> If it got the CF code fixed it wasn't a waste.

>> Do I understand rightly that the agreed things to do are

>> - redo the version check patch using the signed check as you suggested
>> and remove the changes to the CFA check from it

>    Yes, those shouldn't have been intermixed from the very start.

>> - make the CFA code check word 80 == 0 as a sanity check

>    That'll work unless we bump into a drive that does follow ATA in that 
> matter.

    ... because it also does specify the CFA feature set (minus the exotic 
PIO/DMA modes). That's what you get for the closed standards. And yet there 
seem to be CF drives in the wild that follow neither... :-/

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

>>> - make the CFA code check word 80 == 0 as a sanity check
>
>>    That'll work unless we bump into a drive that does follow ATA in 
>> that matter.
>
>    ... because it also does specify the CFA feature set (minus the 
> exotic PIO/DMA modes). That's what you get for the closed standards. 
> And yet there seem to be CF drives in the wild that follow neither... :-/

  ... or are just not supporting the CFA feature set -- see 
http://marc.info/?l=linux-ide&m=123263947330207 if you've missed that 
Kingston's wonder. :-/

MBR, Sergei

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>> totally wrong in that part from the very start -- CF devices don't report ATA 
>> standard support in word 80, that's forbidden (!) by the CF specs since at 
>> least 2.1.
>>     
>
> And the ATA world says that if word 80 doesn't report any standards then
> the word is potentially undefined....welcome to PC hell
>
> Perhaps the best we can do is to test
>
> 	word 80 == 0 && word 83 bit set && word 83 valid
>
> Fortunately the use is almost entirely to print the right CFA/ATA string
> at boot ?
>   

    There was intent to use it for filtering out DMA modes on incapable 
CF slots, for the lack of better criterion -- these slots usually has a 
master/slave switch connected, so can't be tied to the drive #, only to 
the channel # and the board's DMI ID.

> Now Sergei if you'd said that explicitly (or if you did before I didn't
> see it) it would have been a bit simpler to work out why you were arguing
> the needed for these changes.
>   

   I was arguing entirely out of thinking that the version check is not 
really needed. The fact that CF spec. forbids reporting it was a 
surprise to me as well.

> Alan
>   

MBR, Sergei

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

> This patch keeps the version checks but incorporates the other suggestions
> Sergei made including a better ata version check for the usual case where
> we want to know "is version >= x" rather than "what version do you
> support".
>
> Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
>   
[...]
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 88c2428..c17b62a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2212,7 +2212,7 @@ retry:
>  		 * Note that ATA4 says lba is mandatory so the second check
>  		 * shoud never trigger.
>  		 */
> -		if (ata_id_major_version(id) < 4 || !ata_id_has_lba(id)) {
> +		if (!ata_id_has_version(id, 4) || !ata_id_has_lba(id)) {
>  			err_mask = ata_dev_init_params(dev, id[3], id[6]);
>  			if (err_mask) {
>  				rc = -EIO;
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index a53318b..e35d8de 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -675,6 +675,13 @@ static inline unsigned int ata_id_major_version(const u16 *id)
>  	return mver;
>  }
>  
> +static inline int ata_id_has_version(const u16 *id, int v)
> +{
> +	if (id[ATA_ID_MAJOR_VER] == 0xFFFF)
> +		return 0;
> +	return (id[ATA_ID_MAJOR_VER] & (1 << v)) ? 1 : 0;
> +}
> + 
>   
> @@ -691,7 +698,7 @@ static inline int ata_id_is_sata(const u16 *id)
>  static inline int ata_id_has_tpm(const u16 *id)
>  {
>  	/* The TPM bits are only valid on ATA8 */
> -	if (ata_id_major_version(id) < 8)
> +	if (!ata_id_has_version(id, 8))
>  		return 0;

   Note that this is not equivalent to the old code which didn't require 
the bit v to be set, just some bits above it.

MBR, Sergei