Hello! On 6/18/22 9:45 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 I even wonder why C allows that... and the static analyzers don't seem to complain about that too... > as that can be a pain > to debug. So let's go all the way and fix this. OK. > If you cannot do the additional patch, I will do it. I can. I have the patch already, just without the description yet... >>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> >> [...] MBR, Sergey
