[lvc-project] [PATCH] usb: storage: isd200: fix sloppy typing in isd200_scsi_to_ata()

[Alan Stern]

On Fri, Mar 22, 2024 at 12:49:23PM +0300, Sergey Shtylyov wrote:
> On 3/22/24 3:57 AM, Alan Stern wrote:
> [...]
> 
> >> When isd200_scsi_to_ata() emulates the SCSI READ CAPACITY command, the
> >> capacity local variable is needlessly typed as *unsigned long* -- which
> >> is 32-bit type on the 32-bit arches and 64-bit type on the 64-bit arches;
> >> this variable's value should always fit into 32 bits for both the ATA and
> >> the SCSI capacity data...
> >>
> >> While at it, arrange the local variable declarations in the reverse Xmas
> >> tree order...
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> >> analysis tool.
> >>
> >> Signed-off-by: Sergey Shtylyov <s.shtylyov at omp.ru>
> >>
> >> ---
> >>  drivers/usb/storage/isd200.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Index: usb/drivers/usb/storage/isd200.c
> >> ===================================================================
> >> --- usb.orig/drivers/usb/storage/isd200.c
> >> +++ usb/drivers/usb/storage/isd200.c
> >> @@ -1283,8 +1283,8 @@ static int isd200_scsi_to_ata(struct scs
> >>  
> >>  	case READ_CAPACITY:
> >>  	{
> >> -		unsigned long capacity;
> >>  		struct read_capacity_data readCapacityData;
> >> +		u32 capacity;
> > 
> > This is a bit peculiar.  Why bother to change the type of the capacity 
> > variable without also changing the types of lba and blockCount at the 
> > start of the routine?
> 
>    The reason is simple: Svace didn't complain about those.

You shouldn't trust automated code checkers too far.  Always verify 
their reports, and look around the vicinity to see if they missed 
something obvious.

>  I'll fix
> them up in v2 if you're not opposed to this patch...

Sure, go ahead.

Alan Stern