| Message ID | 05a7fcf4-3a2b-3012-34c6-ca1f00fa457c@omp.ru |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-scsi: fix result type of ata_ioc32() | expand |
On 6/23/22 05:57, Sergey Shtylyov 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. The implicit cast to unsigned long from int is not really an issue at all (the reverse cast 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 ? Because we still have an implicit cast in the user site. > > Found by Linux Verification Center (linuxtesting.org) with the SVACE static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > This patch is against the 'for-next' branch of Damien's 'libata.git' repo. > > Changes in version 2: > - changed the result type of ata_ioc32() to 'bool', updating the 'return' > statements as well; > - dropped "sloppy" from the patch subject; > - added a note about the object code to the patch description; > - changed * to ' in the patch description. > > drivers/ata/libata-scsi.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > Index: libata/drivers/ata/libata-scsi.c > =================================================================== > --- libata.orig/drivers/ata/libata-scsi.c > +++ libata/drivers/ata/libata-scsi.c > @@ -539,13 +539,13 @@ int ata_task_ioctl(struct scsi_device *s > return rc; > } > > -static int ata_ioc32(struct ata_port *ap) > +static bool ata_ioc32(struct ata_port *ap) > { > if (ap->flags & ATA_FLAG_PIO_DMA) > - return 1; > + return true; > if (ap->pflags & ATA_PFLAG_PIO32) > - return 1; > - return 0; > + return true; > + return false; > } > > /*
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@omp.ru> [...] MBR, Sergey
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@omp.ru> > [...] > > MBR, Sergey
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@omp.ru> >> [...] MBR, Sergey
Index: libata/drivers/ata/libata-scsi.c =================================================================== --- libata.orig/drivers/ata/libata-scsi.c +++ libata/drivers/ata/libata-scsi.c @@ -539,13 +539,13 @@ int ata_task_ioctl(struct scsi_device *s return rc; } -static int ata_ioc32(struct ata_port *ap) +static bool ata_ioc32(struct ata_port *ap) { if (ap->flags & ATA_FLAG_PIO_DMA) - return 1; + return true; if (ap->pflags & ATA_PFLAG_PIO32) - return 1; - return 0; + return true; + return false; } /*
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). Found by Linux Verification Center (linuxtesting.org) with the SVACE static analysis tool. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- This patch is against the 'for-next' branch of Damien's 'libata.git' repo. Changes in version 2: - changed the result type of ata_ioc32() to 'bool', updating the 'return' statements as well; - dropped "sloppy" from the patch subject; - added a note about the object code to the patch description; - changed * to ' in the patch description. drivers/ata/libata-scsi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)