Hello! On 6/23/22 2:45 AM, Damien Le Moal wrote: [...] >> While ata_ioc32() returns 'int', its result gets assigned to and compared >> with the 'unsigned long' variable 'val' in ata_sas_scsi_ioctl(), its only >> caller, which implies a problematic implicit cast -- fix that by returning >> 'bool' instead (actually, the object code doesn't seem to change, probably >> because the function is always inlined). > > Looks good. I would though prefer to change this commit message to simply > say that ata_ioc32() return value is clearly a bool. No, just changing *int* to 'bool' wasn't the purpose of this patch -- I would not have called it a fix if it was so. > The implicit cast to > unsigned long from int is not really an issue at all (the reverse cast In general case, it is an issue -- as it involves a sign extension (and thus a needless extra instruction on an x86_64 kernel)! However, with the possible *int* values just being 0 and 1, it's not much of issue indeed (except an extra instruction just being there)... In reality , that extra instruction is not there, probably due to ata_ioc32() being inlined... > would be an issue). And keep mentioning that the object code does not change. > > By the way, does your statis analyzer stop complaining after this change ? I don't really know -- all I have is the online report generated for the whole 5.10 kernel. I still can't re-run SVACE but maybe that will change soon... > Because we still have an implicit cast in the user site. A cast from 'bool' should be OK... :-) >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static >> analysis tool. >> >> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> [...] MBR, Sergey
