Re: [PATCH v2 2/2] ata: libata-core: fix sloppy parameter type in ata_exec_internal[_sg]()

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

On 6/18/22 02:27, Sergey Shtylyov wrote:
> Hello!
> 
> On 6/17/22 10:44 AM, Damien Le Moal wrote:
> 
>>> Make the 'timeout' parameter to ata_exec_internal_sg() *unsigned int* as
>>> msecs_to_jiffies() that it calls takes just *unsigned int* for the time in
>>> milliseconds. Then follow the suit with ata_exec_internal(), its only
>>> caller; also fix up ata_dev_set_feature(), the only ata_exec_internal()'s
>>> caller  that explicitly passes *unsigned long* variable for timeout...
>>
>> Checking this, struct ata_eh_cmd_timeout_ent uses an unsigned long timeout
>> and ata_internal_cmd_timeout() returns an unsigned long which is stored
>> into the unsigned int timeout variable.
> 
>    I know. All timeouts stored in those tables fit into *unsigned int* (except
> the last ones which get truncated from ULONG_MAX to UINT_MAX anyways).
>    Note that *unsigned long* has always been silently truncated to *unsigned int*
> when calling msecs_to_jiffies() in ata_exec_internal_sg(); nothing changes with
> my patch, it just gets truncated a bit earlier...
> 
>> So it may be good to add another
>> prep patch before this one to cleanup the auto_timeout stuff (struct
>> ata_eh_cmd_timeout_ent and ata_internal_cmd_timeout()).
> 
>    It shouldn't matter whether we do it before or after this patch, due to
> truncation which happens even now (I have such a patch, but no description
> yet). I was planning to address that as a part of the large patch series,
> most probably next week (and I wrote about that in the cover letter)...
> 
>> Hmm ? Thoughts ?
> 
>    Was I clear enough? :-)
> 
>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>>> analysis tool.
> 
>    I wanted to fix SVACE's reports 1st, then the rest of the sloppy timeout
> typing...

Fine. But in the spirit of fixing everything with these types, please add
a patch to also convert the timeout tables to unsigned int. It is never a
good idea to have code silently truncate long to int as that can be a pain
to debug. So let's go all the way and fix this.

If you cannot do the additional patch, I will do it.

> 
>>>
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> [...]
> 
> MBR, Sergey


-- 
Damien Le Moal
Western Digital Research