| Message ID | 4d3fb70f-bf2f-96cc-a8fb-1ef658a24920@omp.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: storage: freecom: drop useless assignment in init_freecom() | expand |
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
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
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
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
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);
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(-)