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@xxxxxx> >>> >>> --- >>> 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