Re: [PATCH] ata: libata-core: fix sloppy typing in ata_id_n_sectors()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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