[PATCH] libata: Don't trust current capacity values in identify words 57-58

[PATCH] libata: Don't trust current capacity values in identify words 57-58

[Robert Hancock]

Hanno Böck reported a problem where an old Conner CP30254 240MB hard drive
was reported as 1.1TB in capacity by libata:

http://lkml.org/lkml/2009/2/13/134

This was caused by libata trusting the drive's reported current capacity in 
sectors in identify words 57 and 58 if the drive does not support LBA and the
current CHS translation values appear valid. Unfortunately it seems older
ATA specs were vague about what this field should contain and a number of drives
used values with wrong byte order or that were totally bogus. There's no
unique information that it conveys and so we can just calculate the number
of sectors from the reported current CHS values.

Signed-off-by: Robert Hancock <hancockrwd@gmail.com>

---

Should likely go into -next for testing for a while. Of course, the fact that
this problem showed up in the first place shows that the number of people
that test libata with such boat anchors is fairly small :-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9fbf059..5389cbc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1327,7 +1327,7 @@ static u64 ata_id_n_sectors(const u16 *id)
 			return ata_id_u32(id, 60);
 	} else {
 		if (ata_id_current_chs_valid(id))
-			return ata_id_u32(id, 57);
+			return id[54] * id[55] * id[56];
 		else
 			return id[1] * id[3] * id[6];
 	}

Re: [PATCH] libata: Don't trust current capacity values in identify words 57-58

[Sergei Shtylyov]

Robert Hancock wrote:

> Hanno Böck reported a problem where an old Conner CP30254 240MB hard drive
> was reported as 1.1TB in capacity by libata:

> http://lkml.org/lkml/2009/2/13/134

> This was caused by libata trusting the drive's reported current capacity in 
> sectors in identify words 57 and 58 if the drive does not support LBA and the
> current CHS translation values appear valid. Unfortunately it seems older
> ATA specs were vague about what this field should contain and a number of drives
> used values with wrong byte order or that were totally bogus. There's no
> unique information that it conveys and so we can just calculate the number
> of sectors from the reported current CHS values.

> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>

> ---

> Should likely go into -next for testing for a while. Of course, the fact that
> this problem showed up in the first place shows that the number of people
> that test libata with such boat anchors is fairly small :-)

    It's been tested well enough for years in the IDE core I think.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9fbf059..5389cbc 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1327,7 +1327,7 @@ static u64 ata_id_n_sectors(const u16 *id)
>  			return ata_id_u32(id, 60);
>  	} else {
>  		if (ata_id_current_chs_valid(id))
> -			return ata_id_u32(id, 57);
> +			return id[54] * id[55] * id[56];

    We have mnemonics for these 3 words in <linux/ata.h>...

MBR, Sergei

[PATCH v2] libata: Don't trust current capacity values in identify words 57-58

[Robert Hancock]

Hanno Böck reported a problem where an old Conner CP30254 240MB hard drive
was reported as 1.1TB in capacity by libata:

http://lkml.org/lkml/2009/2/13/134

This was caused by libata trusting the drive's reported current capacity in 
sectors in identify words 57 and 58 if the drive does not support LBA and the
current CHS translation values appear valid. Unfortunately it seems older
ATA specs were vague about what this field should contain and a number of drives
used values with wrong byte order or that were totally bogus. There's no
unique information that it conveys and so we can just calculate the number
of sectors from the reported current CHS values.

While we're at it, clean up this function to use named constants for the
identify word values.

Signed-off-by: Robert Hancock <hancockrwd@gmail.com>

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9fbf059..33b5549 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1322,14 +1322,16 @@ static u64 ata_id_n_sectors(const u16 *id)
 {
 	if (ata_id_has_lba(id)) {
 		if (ata_id_has_lba48(id))
-			return ata_id_u64(id, 100);
+			return ata_id_u64(id, ATA_ID_LBA_CAPACITY_2);
 		else
-			return ata_id_u32(id, 60);
+			return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
 	} else {
 		if (ata_id_current_chs_valid(id))
-			return ata_id_u32(id, 57);
+			return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
+			       id[ATA_ID_CUR_SECTORS];
 		else
-			return id[1] * id[3] * id[6];
+			return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
+			       id[ATA_ID_SECTORS];
 	}
 }
 

Re: [PATCH v2] libata: Don't trust current capacity values in identify words 57-58

[Sergei Shtylyov]

Hello.

Robert Hancock wrote:

> Hanno Böck reported a problem where an old Conner CP30254 240MB hard drive
> was reported as 1.1TB in capacity by libata:
>
> http://lkml.org/lkml/2009/2/13/134
>
> This was caused by libata trusting the drive's reported current capacity in 
> sectors in identify words 57 and 58 if the drive does not support LBA and the
> current CHS translation values appear valid. Unfortunately it seems older
> ATA specs were vague about what this field should contain and a number of drives
> used values with wrong byte order or that were totally bogus. There's no
> unique information that it conveys and so we can just calculate the number
> of sectors from the reported current CHS values.
>
> While we're at it, clean up this function to use named constants for the
> identify word values.
>
> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
>   

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

   ... though I'm not sure that the rename should be a part of this patch.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9fbf059..33b5549 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1322,14 +1322,16 @@ static u64 ata_id_n_sectors(const u16 *id)
>  {
>  	if (ata_id_has_lba(id)) {
>  		if (ata_id_has_lba48(id))
> -			return ata_id_u64(id, 100);
> +			return ata_id_u64(id, ATA_ID_LBA_CAPACITY_2);
>  		else
> -			return ata_id_u32(id, 60);
> +			return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
>  	} else {
>  		if (ata_id_current_chs_valid(id))
> -			return ata_id_u32(id, 57);
> +			return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
> +			       id[ATA_ID_CUR_SECTORS];
>   

   It won't hurt to comment on why the capacity is ignored. 
Unfortunately, the IDE code lacks such comment.

MBR, Sergei

Re: [PATCH v2] libata: Don't trust current capacity values in identify words 57-58

[Robert Hancock]

Robert Hancock wrote:
> Hanno Böck reported a problem where an old Conner CP30254 240MB hard drive
> was reported as 1.1TB in capacity by libata:
> 
> http://lkml.org/lkml/2009/2/13/134
> 
> This was caused by libata trusting the drive's reported current capacity in 
> sectors in identify words 57 and 58 if the drive does not support LBA and the
> current CHS translation values appear valid. Unfortunately it seems older
> ATA specs were vague about what this field should contain and a number of drives
> used values with wrong byte order or that were totally bogus. There's no
> unique information that it conveys and so we can just calculate the number
> of sectors from the reported current CHS values.
> 
> While we're at it, clean up this function to use named constants for the
> identify word values.
> 
> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>

Jeff, any thoughts on this one? I would suggest it's -upstream material 
(for .30)..

> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 9fbf059..33b5549 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1322,14 +1322,16 @@ static u64 ata_id_n_sectors(const u16 *id)
>  {
>  	if (ata_id_has_lba(id)) {
>  		if (ata_id_has_lba48(id))
> -			return ata_id_u64(id, 100);
> +			return ata_id_u64(id, ATA_ID_LBA_CAPACITY_2);
>  		else
> -			return ata_id_u32(id, 60);
> +			return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
>  	} else {
>  		if (ata_id_current_chs_valid(id))
> -			return ata_id_u32(id, 57);
> +			return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
> +			       id[ATA_ID_CUR_SECTORS];
>  		else
> -			return id[1] * id[3] * id[6];
> +			return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
> +			       id[ATA_ID_SECTORS];
>  	}
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] libata: Don't trust current capacity values in identify words 57-58

[Mark Lord]

Robert Hancock wrote:
> Hanno Böck reported a problem where an old Conner CP30254 240MB hard drive
> was reported as 1.1TB in capacity by libata:
> 
> http://lkml.org/lkml/2009/2/13/134
> 
> This was caused by libata trusting the drive's reported current capacity in 
> sectors in identify words 57 and 58 if the drive does not support LBA and the
> current CHS translation values appear valid. Unfortunately it seems older
> ATA specs were vague about what this field should contain and a number of drives
> used values with wrong byte order or that were totally bogus. There's no
> unique information that it conveys and so we can just calculate the number
> of sectors from the reported current CHS values.
> 
> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
..
>  	} else {
>  		if (ata_id_current_chs_valid(id))
> -			return ata_id_u32(id, 57);
> +			return id[54] * id[55] * id[56];
>  		else
>  			return id[1] * id[3] * id[6];
..

NAK.  That's not quite correct, either.

The LBA capacity can be larger than the CHS capacity,
so we have to use the reported LBA values if at all possible.

That's why ata_id_is_lba_capacity_ok() exists,
and why it looks so peculiar.

Some of those early drives really did require that kind of logic.

Cheers

Re: [PATCH] libata: Don't trust current capacity values in identify words 57-58

[Mark Lord]

Mark Lord wrote:
> Robert Hancock wrote:
>> Hanno Böck reported a problem where an old Conner CP30254 240MB hard 
>> drive
>> was reported as 1.1TB in capacity by libata:
>>
>> http://lkml.org/lkml/2009/2/13/134
>>
>> This was caused by libata trusting the drive's reported current 
>> capacity in sectors in identify words 57 and 58 if the drive does not 
>> support LBA and the
>> current CHS translation values appear valid. Unfortunately it seems older
>> ATA specs were vague about what this field should contain and a number 
>> of drives
>> used values with wrong byte order or that were totally bogus. There's no
>> unique information that it conveys and so we can just calculate the 
>> number
>> of sectors from the reported current CHS values.
>>
>> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
> ..
>>      } else {
>>          if (ata_id_current_chs_valid(id))
>> -            return ata_id_u32(id, 57);
>> +            return id[54] * id[55] * id[56];
>>          else
>>              return id[1] * id[3] * id[6];
> ..
> 
> NAK.  That's not quite correct, either.
> 
> The LBA capacity can be larger than the CHS capacity,
> so we have to use the reported LBA values if at all possible.
> 
> That's why ata_id_is_lba_capacity_ok() exists,
> and why it looks so peculiar.
> 
> Some of those early drives really did require that kind of logic.
..

Mind you, one can do better than that, too.

The "10% solution" in there right now is a bit of a (working) hack.
It really probably just needs to check for a flipped-LBA
that is within +/- one full cylinder of the CHS capacity.

Cheers

Re: [PATCH] libata: Don't trust current capacity values in identify words 57-58

[Robert Hancock]

Mark Lord wrote:
> Robert Hancock wrote:
>> Hanno Böck reported a problem where an old Conner CP30254 240MB hard 
>> drive
>> was reported as 1.1TB in capacity by libata:
>>
>> http://lkml.org/lkml/2009/2/13/134
>>
>> This was caused by libata trusting the drive's reported current 
>> capacity in sectors in identify words 57 and 58 if the drive does not 
>> support LBA and the
>> current CHS translation values appear valid. Unfortunately it seems older
>> ATA specs were vague about what this field should contain and a number 
>> of drives
>> used values with wrong byte order or that were totally bogus. There's no
>> unique information that it conveys and so we can just calculate the 
>> number
>> of sectors from the reported current CHS values.
>>
>> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
> ..
>>      } else {
>>          if (ata_id_current_chs_valid(id))
>> -            return ata_id_u32(id, 57);
>> +            return id[54] * id[55] * id[56];
>>          else
>>              return id[1] * id[3] * id[6];
> ..
> 
> NAK.  That's not quite correct, either.
> 
> The LBA capacity can be larger than the CHS capacity,
> so we have to use the reported LBA values if at all possible.
> 
> That's why ata_id_is_lba_capacity_ok() exists,
> and why it looks so peculiar.
> 
> Some of those early drives really did require that kind of logic.

This is the !ata_id_has_lba code path. If the drive supports LBA then 
the LBA capacity will always be used, that hasn't changed.

Re: [PATCH] libata: Don't trust current capacity values in identify words 57-58

[Mark Lord]

Robert Hancock wrote:
..
> This is the !ata_id_has_lba code path. If the drive supports LBA then 
> the LBA capacity will always be used, that hasn't changed.
..

Oh, good.  No problems then!

Cheers

Re: [PATCH] libata: Don't trust current capacity values in identify words 57-58

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

>> Hanno Böck reported a problem where an old Conner CP30254 240MB hard 
>> drive
>> was reported as 1.1TB in capacity by libata:
>>
>> http://lkml.org/lkml/2009/2/13/134
>>
>> This was caused by libata trusting the drive's reported current 
>> capacity in sectors in identify words 57 and 58 if the drive does not 
>> support LBA and the
>> current CHS translation values appear valid. Unfortunately it seems 
>> older
>> ATA specs were vague about what this field should contain and a 
>> number of drives
>> used values with wrong byte order or that were totally bogus. There's no
>> unique information that it conveys and so we can just calculate the 
>> number
>> of sectors from the reported current CHS values.
>>
>> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
> ..
>>      } else {
>>          if (ata_id_current_chs_valid(id))
>> -            return ata_id_u32(id, 57);
>> +            return id[54] * id[55] * id[56];
>>          else
>>              return id[1] * id[3] * id[6];
> ..
>
> NAK.  That's not quite correct, either.
>
> The LBA capacity can be larger than the CHS capacity,
> so we have to use the reported LBA values if at all possible.
>
> That's why ata_id_is_lba_capacity_ok() exists,
> and why it looks so peculiar.

   I think that checking LBA validity is a matter of another patch. This 
patch in itself should be sufficient.

> Some of those early drives really did require that kind of logic.

   I hightly doubt that this 240 MB drive is LBA capable at all.

> Cheers

MBR, Sergei