ata: ata_id_is_ssd() bugfix

PATCH In review Bartlomiej Zolnierkiewicz 4 participants 6 messages Oct 18, 2008 – Nov 7, 2008 libata

Share & tools

Bartlomiej Zolnierkiewicz

author

2008-10-18 20:26 UTC #0 lore raw

PATCH 1/1 ata: ata_id_is_ssd() bugfix

We need to explicitly check for major and minor version of supported ATA spec as earlier revisions used word 217 for different purposes.

[ The issue was originally spotted by Alan Cox. ]

This patch fixes regression introduced by:
commit 8bff7c6b0f63c7ee9c5e3a076338d74125b8debb
("libata: set queue SSD flag for SSD devices").

Cc Jeff Garzik <[email protected]>

Cc Alan Cox <[email protected]>

Cc Jens Axboe <[email protected]>

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
somebody owe me one for going through all these spec drafts... ;)

 include/linux/ata.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

include/linux/ata.h +7 -1

Index: b/include/linux/ata.h
===================================================================
@@ -75,6 +75,7 @@ enum {
 	ATA_ID_EIDE_PIO_IORDY	= 68,
 	ATA_ID_QUEUE_DEPTH	= 75,
 	ATA_ID_MAJOR_VER	= 80,
+	ATA_ID_MINOR_VER	= 81,
 	ATA_ID_COMMAND_SET_1	= 82,
 	ATA_ID_COMMAND_SET_2	= 83,
 	ATA_ID_CFSSE		= 84,

@@ -743,7 +744,12 @@ static inline int ata_id_is_cfa(const u1
 
 static inline int ata_id_is_ssd(const u16 *id)
 {
-	return id[ATA_ID_ROT_SPEED] == 0x01;
+	/* ATA8-ACS version 4c or higher (=> 4c or 6 at the moment) */
+	if (ata_id_major_version(id) >= 8 &&
+	    (id[ATA_ID_MINOR_VER] == 0x39 || id[ATA_ID_MINOR_VER] == 0x28) &&
+	    id[ATA_ID_ROT_SPEED] == 0x01)
+		return 1;
+	return 0;
 }
 
 static inline int ata_drive_40wire(const u16 *dev_id)

Jens Axboe

2008-10-19 18:05 UTC #1 lore raw

Quoted 3 lines · Bartlomiej Zolnierkiewicz

We need to explicitly check for major and minor version of supported ATA spec as earlier revisions used word 217 for different purposes.

What did they use to put in that word? Just curious if it does any harm, because as it stands, this patch will prevent ANY ssd from being correctly flagged as such. So I'm inclined to file this as too much spec fiddling, it'll do more harm than good.

Quoted 30 lines

[ The issue was originally spotted by Alan Cox. ]
This patch fixes regression introduced by:
commit 8bff7c6b0f63c7ee9c5e3a076338d74125b8debb
("libata: set queue SSD flag for SSD devices").
Cc Jeff Garzik <[email protected]>
Cc Alan Cox <[email protected]>
Cc Jens Axboe <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
somebody owe me one for going through all these spec drafts... ;)
 include/linux/ata.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Index: b/include/linux/ata.h
===================================================================
include/linux/ata.h +1 -0
@@ -75,6 +75,7 @@ enum {
 	ATA_ID_EIDE_PIO_IORDY	= 68,
 	ATA_ID_QUEUE_DEPTH	= 75,
 	ATA_ID_MAJOR_VER	= 80,
+	ATA_ID_MINOR_VER	= 81,
 	ATA_ID_COMMAND_SET_1	= 82,
 	ATA_ID_COMMAND_SET_2	= 83,
 	ATA_ID_CFSSE		= 84,

include/linux/ata.h static inline int ata_id_is_cfa(const u1 · line 752

Quoted 10 lines

include/linux/ata.h +6 -1

@@ -743,7 +744,12 @@ static inline int ata_id_is_cfa(const u1
 
 static inline int ata_id_is_ssd(const u16 *id)
 {
-	return id[ATA_ID_ROT_SPEED] == 0x01;
+	/* ATA8-ACS version 4c or higher (=> 4c or 6 at the moment) */
+	if (ata_id_major_version(id) >= 8 &&
+	    (id[ATA_ID_MINOR_VER] == 0x39 || id[ATA_ID_MINOR_VER] == 0x28) &&
+	    id[ATA_ID_ROT_SPEED] == 0x01)
+		return 1;
+	return 0;

Is the check even correct? It'll match version 8 AND the currently listed minor version, not newer.

Signature · 1 line

Jens Axboe

Bartlomiej Zolnierkiewicz

author

2008-10-19 18:52 UTC #2 lore raw

Quoted 10 lines · Jens Axboe

Quoted 3 lines · Bartlomiej Zolnierkiewicz
We need to explicitly check for major and minor version of supported ATA spec as earlier revisions used word 217 for different purposes.
What did they use to put in that word? Just curious if it does any harm,
because as it stands, this patch will prevent ANY ssd from being correctly flagged as such. So I'm inclined to file this as too much spec fiddling, it'll do more harm than good.

IIRC it was marked as Reserved for < ATA8 (however I didn't go through all previous specs) and some ATA8 minor versions (i.e. 3f) put "NV Cache Read Transfer Speed in MB/s" there.

Well, we may also drop minor versions checking assuming that no NV Cache actually will have 1MB/s speed and major version checking assuming that no vendor put anything special there but this would need an ACK from Alan and maybe a comment on why we do it...

Quoted 30 lines

Quoted 30 lines
[ The issue was originally spotted by Alan Cox. ]
This patch fixes regression introduced by:
commit 8bff7c6b0f63c7ee9c5e3a076338d74125b8debb
("libata: set queue SSD flag for SSD devices").
Cc Jeff Garzik <[email protected]>
Cc Alan Cox <[email protected]>
Cc Jens Axboe <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
somebody owe me one for going through all these spec drafts... ;)
 include/linux/ata.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Index: b/include/linux/ata.h
===================================================================
include/linux/ata.h +1 -0
@@ -75,6 +75,7 @@ enum {
 	ATA_ID_EIDE_PIO_IORDY	= 68,
 	ATA_ID_QUEUE_DEPTH	= 75,
 	ATA_ID_MAJOR_VER	= 80,
+	ATA_ID_MINOR_VER	= 81,
 	ATA_ID_COMMAND_SET_1	= 82,
 	ATA_ID_COMMAND_SET_2	= 83,
 	ATA_ID_CFSSE		= 84,
@@ -743,7 +744,12 @@ static inline int ata_id_is_cfa(const u1
 

Quoted 12 lines

include/linux/ata.h static inline int ata_id_is_cfa(const u1 · line 752
Quoted 9 lines
include/linux/ata.h +6 -1
 static inline int ata_id_is_ssd(const u16 *id)
 {
-	return id[ATA_ID_ROT_SPEED] == 0x01;
+	/* ATA8-ACS version 4c or higher (=> 4c or 6 at the moment) */
+	if (ata_id_major_version(id) >= 8 &&
+	    (id[ATA_ID_MINOR_VER] == 0x39 || id[ATA_ID_MINOR_VER] == 0x28) &&
+	    id[ATA_ID_ROT_SPEED] == 0x01)
+		return 1;
+	return 0;
Is the check even correct? It'll match version 8 AND the currently listed minor version, not newer.

Checking minor versions is a non-trivial bussiness and improvements are welcomed (though ATA8-ACS version 6 is the newest revision ATM).

It could also be that the we should be checking something else than the ATA_ID_ROT_SPEED to detect SSDs reliably but I don't know and don't have time currently to look into it. In the future please post ATA patches to linux-ide@ so issues can be spotted and discussed early...

Thanks,
Bart

Jens Axboe

2008-10-19 19:12 UTC #3 lore raw

Quoted 12 lines · Bartlomiej Zolnierkiewicz

Quoted 10 lines · Jens Axboe
Quoted 3 lines · Bartlomiej Zolnierkiewicz
We need to explicitly check for major and minor version of supported ATA spec as earlier revisions used word 217 for different purposes.
What did they use to put in that word? Just curious if it does any harm,
because as it stands, this patch will prevent ANY ssd from being correctly flagged as such. So I'm inclined to file this as too much spec fiddling, it'll do more harm than good.

Quoted 8 lines

IIRC it was marked as Reserved for < ATA8 (however I didn't go through all previous specs) and some ATA8 minor versions (i.e. 3f) put "NV Cache Read Transfer Speed in MB/s" there.
Well, we may also drop minor versions checking assuming that no NV Cache actually will have 1MB/s speed and major version checking assuming that no vendor put anything special there but this would need an ACK from Alan and maybe a comment on why we do it...

OK, so it's just NV cache. I'd consider that a non-issue.

Quoted 43 lines

Quoted 30 lines
Quoted 30 lines
[ The issue was originally spotted by Alan Cox. ]
This patch fixes regression introduced by:
commit 8bff7c6b0f63c7ee9c5e3a076338d74125b8debb
("libata: set queue SSD flag for SSD devices").
Cc Jeff Garzik <[email protected]>
Cc Alan Cox <[email protected]>
Cc Jens Axboe <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
somebody owe me one for going through all these spec drafts... ;)
 include/linux/ata.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Index: b/include/linux/ata.h
===================================================================
include/linux/ata.h +1 -0
@@ -75,6 +75,7 @@ enum {
 	ATA_ID_EIDE_PIO_IORDY	= 68,
 	ATA_ID_QUEUE_DEPTH	= 75,
 	ATA_ID_MAJOR_VER	= 80,
+	ATA_ID_MINOR_VER	= 81,
 	ATA_ID_COMMAND_SET_1	= 82,
 	ATA_ID_COMMAND_SET_2	= 83,
 	ATA_ID_CFSSE		= 84,
@@ -743,7 +744,12 @@ static inline int ata_id_is_cfa(const u1
 
Quoted 12 lines
include/linux/ata.h static inline int ata_id_is_cfa(const u1 · line 752
Quoted 9 lines
include/linux/ata.h +6 -1
 static inline int ata_id_is_ssd(const u16 *id)
 {
-	return id[ATA_ID_ROT_SPEED] == 0x01;
+	/* ATA8-ACS version 4c or higher (=> 4c or 6 at the moment) */
+	if (ata_id_major_version(id) >= 8 &&
+	    (id[ATA_ID_MINOR_VER] == 0x39 || id[ATA_ID_MINOR_VER] == 0x28) &&
+	    id[ATA_ID_ROT_SPEED] == 0x01)
+		return 1;
+	return 0;
Is the check even correct? It'll match version 8 AND the currently listed minor version, not newer.

Quoted 6 lines

Checking minor versions is a non-trivial bussiness and improvements are welcomed (though ATA8-ACS version 6 is the newest revision ATM).
It could also be that the we should be checking something else than the ATA_ID_ROT_SPEED to detect SSDs reliably but I don't know and don't have time currently to look into it. In the future please post ATA

Rotation speed == 1 is THE way to check for an SSD. The problem is just that lots of drives are out there and don't claim ATA8 compliance, since it was finalized until last month. Most of them SHOULD use word 217 though, the ones I looked at sure do.

Quoted 1 line

patches to linux-ide@ so issues can be spotted and discussed early...

As I already mentioned, it was not even intended to go upstream. It was committed for testing and I left it in by mistake.

Signature · 1 line

Jens Axboe

Jeff Garzik

2008-10-28 04:28 UTC #4 lore raw

Quoted 66 lines · Jens Axboe

Quoted 10 lines · Bartlomiej Zolnierkiewicz
Quoted 9 lines · Jens Axboe
Quoted 3 lines · Bartlomiej Zolnierkiewicz
We need to explicitly check for major and minor version of supported ATA spec as earlier revisions used word 217 for different purposes.
What did they use to put in that word? Just curious if it does any harm,
because as it stands, this patch will prevent ANY ssd from being correctly flagged as such. So I'm inclined to file this as too much spec fiddling, it'll do more harm than good.
Quoted 8 lines
IIRC it was marked as Reserved for < ATA8 (however I didn't go through all previous specs) and some ATA8 minor versions (i.e. 3f) put "NV Cache Read Transfer Speed in MB/s" there.
Well, we may also drop minor versions checking assuming that no NV Cache actually will have 1MB/s speed and major version checking assuming that no vendor put anything special there but this would need an ACK from Alan and maybe a comment on why we do it...
OK, so it's just NV cache. I'd consider that a non-issue.
Quoted 44 lines
Quoted 30 lines
Quoted 30 lines
[ The issue was originally spotted by Alan Cox. ]
This patch fixes regression introduced by:
commit 8bff7c6b0f63c7ee9c5e3a076338d74125b8debb
("libata: set queue SSD flag for SSD devices").
Cc Jeff Garzik <[email protected]>
Cc Alan Cox <[email protected]>
Cc Jens Axboe <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
somebody owe me one for going through all these spec drafts... ;)
 include/linux/ata.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Index: b/include/linux/ata.h
===================================================================
include/linux/ata.h +1 -0
@@ -75,6 +75,7 @@ enum {
 	ATA_ID_EIDE_PIO_IORDY	= 68,
 	ATA_ID_QUEUE_DEPTH	= 75,
 	ATA_ID_MAJOR_VER	= 80,
+	ATA_ID_MINOR_VER	= 81,
 	ATA_ID_COMMAND_SET_1	= 82,
 	ATA_ID_COMMAND_SET_2	= 83,
 	ATA_ID_CFSSE		= 84,
@@ -743,7 +744,12 @@ static inline int ata_id_is_cfa(const u1
 
Quoted 11 lines
include/linux/ata.h static inline int ata_id_is_cfa(const u1 · line 752
Quoted 9 lines
include/linux/ata.h +6 -1
 static inline int ata_id_is_ssd(const u16 *id)
 {
-	return id[ATA_ID_ROT_SPEED] == 0x01;
+	/* ATA8-ACS version 4c or higher (=> 4c or 6 at the moment) */
+	if (ata_id_major_version(id) >= 8 &&
+	    (id[ATA_ID_MINOR_VER] == 0x39 || id[ATA_ID_MINOR_VER] == 0x28) &&
+	    id[ATA_ID_ROT_SPEED] == 0x01)
+		return 1;
+	return 0;
Is the check even correct? It'll match version 8 AND the currently listed minor version, not newer.
Checking minor versions is a non-trivial bussiness and improvements are welcomed (though ATA8-ACS version 6 is the newest revision ATM).

Quoted 8 lines

Quoted 3 lines
It could also be that the we should be checking something else than the ATA_ID_ROT_SPEED to detect SSDs reliably but I don't know and don't have time currently to look into it. In the future please post ATA
Rotation speed == 1 is THE way to check for an SSD. The problem is just that lots of drives are out there and don't claim ATA8 compliance, since it was finalized until last month. Most of them SHOULD use word 217 though, the ones I looked at sure do.

It seems highly unlikely that any drive that claims ATA8 would repurpose word 217, therefore upstream logic IMO should look like

	if (id[ATA_ID_ROT_SPEED] == 0x01 &&
	    ((ata_id_major_version(id) >= 8) ||
	     (a list of ATA6/7 exceptions that Jens wishes to supply)))

Regards,

Jeff

KueiHuan Chen

2008-11-07 14:37 UTC #5 lore raw

I had a transcend 32GB SSD. It use ATA8 but lack of support word 217. So this is not really work for all SSD currently...

Here is the spec of such SSD
http://www.transcend.com.tw/support/dlcenter/datasheet/SSD25S-JMI%20Datasheet%20v1.05.pdf

--
Best Regards,
KueiHuan-Chen

2008/10/28 Jeff Garzik <[email protected]>

Quoted 17 lines

It seems highly unlikely that any drive that claims ATA8 would repurpose word 217, therefore upstream logic IMO should look like
       if (id[ATA_ID_ROT_SPEED] == 0x01 &&
           ((ata_id_major_version(id) >= 8) ||
            (a list of ATA6/7 exceptions that Jens wishes to supply)))
Regards,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/