* [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()
@ 2022-05-13 20:50 Sergey Shtylyov
2022-05-16 11:29 ` Damien Le Moal
2022-06-08 6:47 ` Damien Le Moal
0 siblings, 2 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-05-13 20:50 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
to get a disk capacity implicitly uses the *int* type for that calculation
and casting the result to 'u64' before returning ensues a sign extension.
Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
a sign extension instruction and so in a more compact code...
Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
drivers/ata/libata-core.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
Index: libata/drivers/ata/libata-core.c
===================================================================
--- libata.orig/drivers/ata/libata-core.c
+++ libata/drivers/ata/libata-core.c
@@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
} else {
if (ata_id_current_chs_valid(id))
- return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
- id[ATA_ID_CUR_SECTORS];
+ return (u32)id[ATA_ID_CUR_CYLS] *
+ (u32)id[ATA_ID_CUR_HEADS] *
+ (u32)id[ATA_ID_CUR_SECTORS];
else
- return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
- id[ATA_ID_SECTORS];
+ return (u32)id[ATA_ID_CYLS] *
+ (u32)id[ATA_ID_HEADS] *
+ (u32)id[ATA_ID_SECTORS];
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()
2022-05-13 20:50 [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors() Sergey Shtylyov
@ 2022-05-16 11:29 ` Damien Le Moal
2022-05-17 19:43 ` Sergey Shtylyov
2022-06-08 6:47 ` Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2022-05-16 11:29 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
On 2022/05/13 22:50, Sergey Shtylyov wrote:
> The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
> to get a disk capacity implicitly uses the *int* type for that calculation
> and casting the result to 'u64' before returning ensues a sign extension.
> Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
> a sign extension instruction and so in a more compact code...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> ---
> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
>
> drivers/ata/libata-core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> Index: libata/drivers/ata/libata-core.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-core.c
> +++ libata/drivers/ata/libata-core.c
> @@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
> return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
> } else {
> if (ata_id_current_chs_valid(id))
> - return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
> - id[ATA_ID_CUR_SECTORS];
> + return (u32)id[ATA_ID_CUR_CYLS] *
> + (u32)id[ATA_ID_CUR_HEADS] *
> + (u32)id[ATA_ID_CUR_SECTORS];
> else
While at it, you can drop this useless "else". The 2 else above this one are
actually also useless...
> - return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
> - id[ATA_ID_SECTORS];
> + return (u32)id[ATA_ID_CYLS] *
> + (u32)id[ATA_ID_HEADS] *
> + (u32)id[ATA_ID_SECTORS];
Given that the function returns an u64, I would cast everything to u64. That
will avoid overflows too, which was possible before, eventhough no problems seem
to have been reported... Who uses CHS these days :)
> }
> }
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()
2022-05-16 11:29 ` Damien Le Moal
@ 2022-05-17 19:43 ` Sergey Shtylyov
2022-05-22 22:26 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Shtylyov @ 2022-05-17 19:43 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
Hello!
On 5/16/22 2:29 PM, Damien Le Moal wrote:
>> The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
>> to get a disk capacity implicitly uses the *int* type for that calculation
>> and casting the result to 'u64' before returning ensues a sign extension.
>> Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
>> a sign extension instruction and so in a more compact code...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>
>> ---
>> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
>>
>> drivers/ata/libata-core.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> Index: libata/drivers/ata/libata-core.c
>> ===================================================================
>> --- libata.orig/drivers/ata/libata-core.c
>> +++ libata/drivers/ata/libata-core.c
>> @@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
>> return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
>> } else {
>> if (ata_id_current_chs_valid(id))
>> - return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
>> - id[ATA_ID_CUR_SECTORS];
>> + return (u32)id[ATA_ID_CUR_CYLS] *
>> + (u32)id[ATA_ID_CUR_HEADS] *
>> + (u32)id[ATA_ID_CUR_SECTORS];
>> else
>
> While at it, you can drop this useless "else". The 2 else above this one are
> actually also useless...
OK. But I think it's all a matter of a separate patch. I don't want to touch
the LBA branches in this same patch...
>> - return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
>> - id[ATA_ID_SECTORS];
>> + return (u32)id[ATA_ID_CYLS] *
>> + (u32)id[ATA_ID_HEADS] *
>> + (u32)id[ATA_ID_SECTORS];
>
> Given that the function returns an u64, I would cast everything to u64. That
I don't think this is a good idea. Looking at the produced x86 32-bit code,
gcc produces an extra (3rd) multiplication instruction for no value.
> will avoid overflows too, which was possible before,
No, it wasn't possible. Any possible CHS capacity always fits into 32 bits --
max # of sectors per track is 255, max # of heads is only 16.
What actually seems to make sense is changing the order of multiplications
to first multiply # of sectors by # of heads and than multiply that by # of
cylinders...
> eventhough no problems seem
> to have been reported...
Because there's not problem. :-)
The current CHS capacity is stored in the words 57-58 (so 32-bit) and we
could read it from there instead of the multiplications... BUT I do remember
the disks (IIRC Fujitsu... but I'm not sure now -- that was back in 90s!)
that had totally wrong value in these words... so the code we have now is
a good thing! :-)
> Who uses CHS these days :)
Indeed, the CHS days are long gone... :-)
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()
2022-05-17 19:43 ` Sergey Shtylyov
@ 2022-05-22 22:26 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2022-05-22 22:26 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
On 2022/05/18 4:43, Sergey Shtylyov wrote:
> Hello!
>
> On 5/16/22 2:29 PM, Damien Le Moal wrote:
>
>>> The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
>>> to get a disk capacity implicitly uses the *int* type for that calculation
>>> and casting the result to 'u64' before returning ensues a sign extension.
>>> Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
>>> a sign extension instruction and so in a more compact code...
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>> analysis tool.
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>>
>>> ---
>>> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
>>>
>>> drivers/ata/libata-core.c | 10 ++++++----
>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> Index: libata/drivers/ata/libata-core.c
>>> ===================================================================
>>> --- libata.orig/drivers/ata/libata-core.c
>>> +++ libata/drivers/ata/libata-core.c
>>> @@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
>>> return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
>>> } else {
>>> if (ata_id_current_chs_valid(id))
>>> - return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
>>> - id[ATA_ID_CUR_SECTORS];
>>> + return (u32)id[ATA_ID_CUR_CYLS] *
>>> + (u32)id[ATA_ID_CUR_HEADS] *
>>> + (u32)id[ATA_ID_CUR_SECTORS];
>>> else
>>
>> While at it, you can drop this useless "else". The 2 else above this one are
>> actually also useless...
>
> OK. But I think it's all a matter of a separate patch. I don't want to touch
> the LBA branches in this same patch...
OK.
>
>>> - return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
>>> - id[ATA_ID_SECTORS];
>>> + return (u32)id[ATA_ID_CYLS] *
>>> + (u32)id[ATA_ID_HEADS] *
>>> + (u32)id[ATA_ID_SECTORS];
>>
>> Given that the function returns an u64, I would cast everything to u64. That
>
> I don't think this is a good idea. Looking at the produced x86 32-bit code,
> gcc produces an extra (3rd) multiplication instruction for no value.
>
>> will avoid overflows too, which was possible before,
>
> No, it wasn't possible. Any possible CHS capacity always fits into 32 bits --
> max # of sectors per track is 255, max # of heads is only 16.
> What actually seems to make sense is changing the order of multiplications
> to first multiply # of sectors by # of heads and than multiply that by # of
> cylinders...
OK.
>
>> eventhough no problems seem
>> to have been reported...
>
> Because there's not problem. :-)
> The current CHS capacity is stored in the words 57-58 (so 32-bit) and we
> could read it from there instead of the multiplications... BUT I do remember
> the disks (IIRC Fujitsu... but I'm not sure now -- that was back in 90s!)
> that had totally wrong value in these words... so the code we have now is
> a good thing! :-)
>
>> Who uses CHS these days :)
>
> Indeed, the CHS days are long gone... :-)
>
> [...]
>
> MBR, Sergey
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()
2022-05-13 20:50 [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors() Sergey Shtylyov
2022-05-16 11:29 ` Damien Le Moal
@ 2022-06-08 6:47 ` Damien Le Moal
2022-06-09 20:15 ` Sergey Shtylyov
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2022-06-08 6:47 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide
On 5/14/22 05:50, Sergey Shtylyov wrote:
> The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
> to get a disk capacity implicitly uses the *int* type for that calculation
> and casting the result to 'u64' before returning ensues a sign extension.
> Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
> a sign extension instruction and so in a more compact code...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> ---
> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
>
> drivers/ata/libata-core.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> Index: libata/drivers/ata/libata-core.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-core.c
> +++ libata/drivers/ata/libata-core.c
> @@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
> return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
> } else {
> if (ata_id_current_chs_valid(id))
> - return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
> - id[ATA_ID_CUR_SECTORS];
> + return (u32)id[ATA_ID_CUR_CYLS] *
> + (u32)id[ATA_ID_CUR_HEADS] *
> + (u32)id[ATA_ID_CUR_SECTORS];
> else
> - return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
> - id[ATA_ID_SECTORS];
> + return (u32)id[ATA_ID_CYLS] *
> + (u32)id[ATA_ID_HEADS] *
> + (u32)id[ATA_ID_SECTORS];
> }
> }
>
Applied to for-5.20. Thanks !
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()
2022-06-08 6:47 ` Damien Le Moal
@ 2022-06-09 20:15 ` Sergey Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-06-09 20:15 UTC (permalink / raw)
To: Damien Le Moal, linux-ide
On 6/8/22 9:47 AM, Damien Le Moal wrote:
>> The code multiplying the # of cylinders/heads/sectors in ata_id_n_sectors()
>> to get a disk capacity implicitly uses the *int* type for that calculation
>> and casting the result to 'u64' before returning ensues a sign extension.
>> Explicitly casting the 'u16' typed multipliers to 'u32' results in avoiding
>> a sign extension instruction and so in a more compact code...
>>
>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>>
>> ---
>> This patch is against the 'for-next' branch of Damien's 'libata.git' repo.
>>
>> drivers/ata/libata-core.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> Index: libata/drivers/ata/libata-core.c
>> ===================================================================
>> --- libata.orig/drivers/ata/libata-core.c
>> +++ libata/drivers/ata/libata-core.c
>> @@ -1107,11 +1107,13 @@ static u64 ata_id_n_sectors(const u16 *i
>> return ata_id_u32(id, ATA_ID_LBA_CAPACITY);
>> } else {
>> if (ata_id_current_chs_valid(id))
>> - return id[ATA_ID_CUR_CYLS] * id[ATA_ID_CUR_HEADS] *
>> - id[ATA_ID_CUR_SECTORS];
>> + return (u32)id[ATA_ID_CUR_CYLS] *
>> + (u32)id[ATA_ID_CUR_HEADS] *
>> + (u32)id[ATA_ID_CUR_SECTORS];
>> else
>> - return id[ATA_ID_CYLS] * id[ATA_ID_HEADS] *
>> - id[ATA_ID_SECTORS];
>> + return (u32)id[ATA_ID_CYLS] *
>> + (u32)id[ATA_ID_HEADS] *
>> + (u32)id[ATA_ID_SECTORS];
>> }
>> }
>>
>
> Applied to for-5.20. Thanks !
Actually I was going to redo it (changing the order of multiplications), but well,
it's OK as is...
MBR, Sergey
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-09 20:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-13 20:50 [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors() Sergey Shtylyov
2022-05-16 11:29 ` Damien Le Moal
2022-05-17 19:43 ` Sergey Shtylyov
2022-05-22 22:26 ` Damien Le Moal
2022-06-08 6:47 ` Damien Le Moal
2022-06-09 20:15 ` Sergey Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox