On 6/24/22 1:52 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... Yes, inlining is to blame here -- luckily we have 'noinline'! :-) >>> 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... :-) > > Yes, I am aware of that. My point is that the commit message does not say > WHY it is OK. Need to mention that the cast is between unsigned types so > is fine, or something like that. OK, I'll try to come up with something... didn't quite expect that this patch would be so tough to get merged... :-) >>>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static >>>> analysis tool. >>>> >>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> >> [...] MBR, Sergey
