[PATCH] usb: storage: freecom: drop useless assignment in init_freecom()

[PATCH] usb: storage: freecom: drop useless assignment in init_freecom()

[Sergey Shtylyov]

In init_freecom(), the results of usb_stor_control_msg() calls are stored
in the local variable and then printed out by usb_stor_dbg() (if enabled),
except for the 1st call, the result of which is completely ignored.  Drop
the useless assignment.

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 'usb-next' branch of Greg KH's 'usb.git' repo...

 drivers/usb/storage/freecom.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: usb/drivers/usb/storage/freecom.c
===================================================================
--- usb.orig/drivers/usb/storage/freecom.c
+++ usb/drivers/usb/storage/freecom.c
@@ -446,7 +446,7 @@ static int init_freecom(struct us_data *
 	 * all our packets.  No need to allocate any extra buffer space.
 	 */
 
-	result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
+	usb_stor_control_msg(us, us->recv_ctrl_pipe,
 			0x4c, 0xc0, 0x4346, 0x0, buffer, 0x20, 3*HZ);
 	buffer[32] = '\0';
 	usb_stor_dbg(us, "String returned from FC init is: %s\n", buffer);

Re: [PATCH] usb: storage: freecom: drop useless assignment in init_freecom()

[Greg Kroah-Hartman]

On Wed, Nov 29, 2023 at 11:43:50PM +0300, Sergey Shtylyov wrote:
> In init_freecom(), the results of usb_stor_control_msg() calls are stored
> in the local variable and then printed out by usb_stor_dbg() (if enabled),
> except for the 1st call, the result of which is completely ignored.  Drop
> the useless assignment.

Instead, you should check the return value and handle it properly, don't
just drop the checking entirely, that's not good.

thanks,

greg k-h

Re: [PATCH] usb: storage: freecom: drop useless assignment in init_freecom()

[Sergey Shtylyov]

On 12/4/23 10:04 AM, Greg Kroah-Hartman wrote:
[...]
>> In init_freecom(), the results of usb_stor_control_msg() calls are stored
>> in the local variable and then printed out by usb_stor_dbg() (if enabled),
>> except for the 1st call, the result of which is completely ignored.  Drop
>> the useless assignment.
> 
> Instead, you should check the return value and handle it properly, don't
> just drop the checking entirely, that's not good.

   Hmm... I wonder if you'd actually read the patch...
   I'm not dropping any checking because there's none, even at the further
call sites of usb_stor_control_msg() -- the most init_freecom() currently
is doing is printing out the result of the calls...

> thanks,
> 
> greg k-h

MBR, Sergey

Re: [PATCH] usb: storage: freecom: drop useless assignment in init_freecom()

[Sergey Shtylyov]

On 12/7/23 7:16 PM, Sergey Shtylyov wrote:

[...]
>>> In init_freecom(), the results of usb_stor_control_msg() calls are stored
>>> in the local variable and then printed out by usb_stor_dbg() (if enabled),
>>> except for the 1st call, the result of which is completely ignored.  Drop
>>> the useless assignment.
>>
>> Instead, you should check the return value and handle it properly, don't
>> just drop the checking entirely, that's not good.
> 
>    Hmm... I wonder if you'd actually read the patch...
>    I'm not dropping any checking because there's none, even at the further
> call sites of usb_stor_control_msg() -- the most init_freecom() currently
> is doing is printing out the result of the calls...

   Alan, haven't heard your opinion on this patch... What do you think?

[...]

MBR, Sergey

Re: [PATCH] usb: storage: freecom: drop useless assignment in init_freecom()

[Alan Stern]

On Mon, Jan 22, 2024 at 09:26:51PM +0300, Sergey Shtylyov wrote:
> On 12/7/23 7:16 PM, Sergey Shtylyov wrote:
> 
> [...]
> >>> In init_freecom(), the results of usb_stor_control_msg() calls are stored
> >>> in the local variable and then printed out by usb_stor_dbg() (if enabled),
> >>> except for the 1st call, the result of which is completely ignored.  Drop
> >>> the useless assignment.
> >>
> >> Instead, you should check the return value and handle it properly, don't
> >> just drop the checking entirely, that's not good.
> > 
> >    Hmm... I wonder if you'd actually read the patch...
> >    I'm not dropping any checking because there's none, even at the further
> > call sites of usb_stor_control_msg() -- the most init_freecom() currently
> > is doing is printing out the result of the calls...
> 
>    Alan, haven't heard your opinion on this patch... What do you think?

Oh, sorry about that.  There's nothing wrong with the patch.  None of 
the return values in that function are used for anything other than 
debug log messages.  (It's a little surprising that the original author 
of this driver didn't put any error checking here.)

Greg, feel free to merge the patch.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern