* usb: storage: suspicious code
@ 2017-02-15 5:06 Gustavo A. R. Silva
2017-02-15 7:01 ` [usb-storage] " Oliver Neukum
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-15 5:06 UTC (permalink / raw)
To: stern, gregkh
Cc: linux-usb, usb-storage, linux-kernel, Peter Senna Tschudi,
Gustavo A. R. Silva
Hello,
I ran into the following piece of code at drivers/usb/storage/jumpshot.c:305 (linux-next), and it seems a little bit suspicious:
// read the result. apparently the bulk write can complete
// before the jumpshot drive is finished writing. so we loop
// here until we get a good return code
waitcount = 0;
do {
result = jumpshot_get_status(us);
if (result != USB_STOR_TRANSPORT_GOOD) {
// I have not experimented to find the smallest value.
//
msleep(50);
}
} while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
if (result != USB_STOR_TRANSPORT_GOOD)
usb_stor_dbg(us, "Gah! Waitcount = 10. Bad write!?\n");
Variable 'waitcount' is never updated inside the do-while loop. So, either it isn't needed at all or line 316 should be modified (++waitcount < 10)
In case 'waitcount' isn't needed, lines 318 and 319 should be removed.
Can someone help me to clarify this so I can write a patch to fix this code?
Thank you
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [usb-storage] usb: storage: suspicious code
2017-02-15 5:06 usb: storage: suspicious code Gustavo A. R. Silva
@ 2017-02-15 7:01 ` Oliver Neukum
2017-02-15 7:14 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Oliver Neukum @ 2017-02-15 7:01 UTC (permalink / raw)
To: Gustavo A. R. Silva, stern, gregkh
Cc: linux-usb, usb-storage, linux-kernel, Peter Senna Tschudi
Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:
Hi,
> waitcount = 0;
> do {
> result = jumpshot_get_status(us);
> if (result != USB_STOR_TRANSPORT_GOOD) {
> // I have not experimented to find the smallest
> value.
> //
> msleep(50);
> }
> } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
> 10));
>
> if (result != USB_STOR_TRANSPORT_GOOD)
> usb_stor_dbg(us, "Gah! Waitcount = 10. Bad
> write!?\n");
>
> Variable 'waitcount' is never updated inside the do-while loop. So,
> either it isn't needed at all or line 316 should be modified
> (++waitcount < 10)
you are correct. Waitcount needs to be incremented.
HTH
Oliver
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [usb-storage] usb: storage: suspicious code
2017-02-15 7:01 ` [usb-storage] " Oliver Neukum
@ 2017-02-15 7:14 ` Gustavo A. R. Silva
2017-02-15 7:39 ` [PATCH] usb: storage: add missing pre-increment to variable Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-15 7:14 UTC (permalink / raw)
To: Oliver Neukum
Cc: stern, gregkh, linux-usb, usb-storage, linux-kernel, Peter Senna Tschudi
Hi Oliver,
Quoting Oliver Neukum <oneukum@suse.com>:
> Am Dienstag, den 14.02.2017, 23:06 -0600 schrieb Gustavo A. R. Silva:
>
> Hi,
>
>> waitcount = 0;
>> do {
>> result = jumpshot_get_status(us);
>> if (result != USB_STOR_TRANSPORT_GOOD) {
>> // I have not experimented to find the smallest
>> value.
>> //
>> msleep(50);
>> }
>> } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount <
>> 10));
>>
>> if (result != USB_STOR_TRANSPORT_GOOD)
>> usb_stor_dbg(us, "Gah! Waitcount = 10. Bad
>> write!?\n");
>>
>> Variable 'waitcount' is never updated inside the do-while loop. So,
>> either it isn't needed at all or line 316 should be modified
>> (++waitcount < 10)
>
> you are correct. Waitcount needs to be incremented.
>
Thanks for clarifying, I'll send a patch shortly.
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] usb: storage: add missing pre-increment to variable
2017-02-15 7:14 ` Gustavo A. R. Silva
@ 2017-02-15 7:39 ` Gustavo A. R. Silva
2017-02-15 15:26 ` [usb-storage] " Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-15 7:39 UTC (permalink / raw)
To: oneukum, stern, gregkh
Cc: linux-usb, usb-storage, linux-kernel, Peter Senna Tschudin,
Gustavo A. R. Silva
Add missing pre-increment to 'waitcount' variable used in do-while loop.
Addresses-Coverity-ID: 1011631
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/usb/storage/jumpshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
index 011e527..a26c4bb 100644
--- a/drivers/usb/storage/jumpshot.c
+++ b/drivers/usb/storage/jumpshot.c
@@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
//
msleep(50);
}
- } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
+ } while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
if (result != USB_STOR_TRANSPORT_GOOD)
usb_stor_dbg(us, "Gah! Waitcount = 10. Bad write!?\n");
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable
2017-02-15 7:39 ` [PATCH] usb: storage: add missing pre-increment to variable Gustavo A. R. Silva
@ 2017-02-15 15:26 ` Alan Stern
2017-02-20 23:07 ` Gustavo A. R. Silva
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2017-02-15 15:26 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: oneukum, gregkh, linux-usb, usb-storage, linux-kernel,
Peter Senna Tschudin
On Wed, 15 Feb 2017, Gustavo A. R. Silva wrote:
> Add missing pre-increment to 'waitcount' variable used in do-while loop.
>
> Addresses-Coverity-ID: 1011631
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> drivers/usb/storage/jumpshot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
> index 011e527..a26c4bb 100644
> --- a/drivers/usb/storage/jumpshot.c
> +++ b/drivers/usb/storage/jumpshot.c
> @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
> //
> msleep(50);
> }
> - } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
> + } while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
>
> if (result != USB_STOR_TRANSPORT_GOOD)
> usb_stor_dbg(us, "Gah! Waitcount = 10. Bad write!?\n");
>
This has already been reported and fixed. See
http://marc.info/?l=linux-usb&m=148604164024557&w=2
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [usb-storage] [PATCH] usb: storage: add missing pre-increment to variable
2017-02-15 15:26 ` [usb-storage] " Alan Stern
@ 2017-02-20 23:07 ` Gustavo A. R. Silva
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-20 23:07 UTC (permalink / raw)
To: Alan Stern
Cc: oneukum, gregkh, linux-usb, usb-storage, linux-kernel,
Peter Senna Tschudin
Hi Alan,
Quoting Alan Stern <stern@rowland.harvard.edu>:
> On Wed, 15 Feb 2017, Gustavo A. R. Silva wrote:
>
>> Add missing pre-increment to 'waitcount' variable used in do-while loop.
>>
>> Addresses-Coverity-ID: 1011631
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>> drivers/usb/storage/jumpshot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
>> index 011e527..a26c4bb 100644
>> --- a/drivers/usb/storage/jumpshot.c
>> +++ b/drivers/usb/storage/jumpshot.c
>> @@ -313,7 +313,7 @@ static int jumpshot_write_data(struct us_data *us,
>> //
>> msleep(50);
>> }
>> - } while ((result != USB_STOR_TRANSPORT_GOOD) && (waitcount < 10));
>> + } while ((result != USB_STOR_TRANSPORT_GOOD) && (++waitcount < 10));
>>
>> if (result != USB_STOR_TRANSPORT_GOOD)
>> usb_stor_dbg(us, "Gah! Waitcount = 10. Bad write!?\n");
>>
>
> This has already been reported and fixed. See
>
> http://marc.info/?l=linux-usb&m=148604164024557&w=2
>
Awesome. Thanks for the info.
--
Gustavo A. R. Silva
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-20 23:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15 5:06 usb: storage: suspicious code Gustavo A. R. Silva
2017-02-15 7:01 ` [usb-storage] " Oliver Neukum
2017-02-15 7:14 ` Gustavo A. R. Silva
2017-02-15 7:39 ` [PATCH] usb: storage: add missing pre-increment to variable Gustavo A. R. Silva
2017-02-15 15:26 ` [usb-storage] " Alan Stern
2017-02-20 23:07 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.