On 6/24/22 03:33, Sergey Shtylyov wrote: > 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... :-) 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. > >>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static >>> analysis tool. >>> >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> > [...] > > MBR, Sergey -- Damien Le Moal Western Digital Research
