diff mbox series

USB: realtek_cr: fix return check for dma functions

Message ID 20200811151505.12222-1-trix@redhat.com (mailing list archive)
State New, archived
Headers show
Series USB: realtek_cr: fix return check for dma functions | expand

Commit Message

Tom Rix Aug. 11, 2020, 3:15 p.m. UTC 
From: Tom Rix <trix@redhat.com>

clang static analysis reports this representative problem

realtek_cr.c:639:3: warning: The left expression of the compound
  assignment is an uninitialized value. The computed value will
  also be garbage
    SET_BIT(value, 2);
    ^~~~~~~~~~~~~~~~~

value is set by a successful call to rts51x_read_mem()

	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
	if (retval < 0)
		return -EIO;

A successful call to rts51x_read_mem returns 0, failure can
return positive and negative values.  This check is wrong
for a number of functions.  Fix the retval check.

Fixes: 065e60964e29 ("ums_realtek: do not use stack memory for DMA")
Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/usb/storage/realtek_cr.c | 36 ++++++++++++++++----------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Alan Stern Aug. 11, 2020, 4:03 p.m. UTC | #1 
On Tue, Aug 11, 2020 at 08:15:05AM -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> clang static analysis reports this representative problem
> 
> realtek_cr.c:639:3: warning: The left expression of the compound
>   assignment is an uninitialized value. The computed value will
>   also be garbage
>     SET_BIT(value, 2);
>     ^~~~~~~~~~~~~~~~~
> 
> value is set by a successful call to rts51x_read_mem()
> 
> 	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
> 	if (retval < 0)
> 		return -EIO;
> 
> A successful call to rts51x_read_mem returns 0, failure can
> return positive and negative values.  This check is wrong
> for a number of functions.  Fix the retval check.
> 
> Fixes: 065e60964e29 ("ums_realtek: do not use stack memory for DMA")
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  drivers/usb/storage/realtek_cr.c | 36 ++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> index 3789698d9d3c..b983753e2368 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -481,16 +481,16 @@ static int enable_oscillator(struct us_data *us)
>  	u8 value;
>  
>  	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
> -	if (retval < 0)
> +	if (retval != STATUS_SUCCESS)
>  		return -EIO;

Instead of changing all these call sites, wouldn't it be a lot easier 
just to change rts51x_read_mem() to make it always return a negative 
value (such as -EIO) when there's an error?

Alan Stern
Tom Rix Aug. 11, 2020, 5:29 p.m. UTC | #2 
On 8/11/20 9:03 AM, Alan Stern wrote:
> On Tue, Aug 11, 2020 at 08:15:05AM -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> clang static analysis reports this representative problem
>>
>> realtek_cr.c:639:3: warning: The left expression of the compound
>>   assignment is an uninitialized value. The computed value will
>>   also be garbage
>>     SET_BIT(value, 2);
>>     ^~~~~~~~~~~~~~~~~
>>
>> value is set by a successful call to rts51x_read_mem()
>>
>> 	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
>> 	if (retval < 0)
>> 		return -EIO;
>>
>> A successful call to rts51x_read_mem returns 0, failure can
>> return positive and negative values.  This check is wrong
>> for a number of functions.  Fix the retval check.
>>
>> Fixes: 065e60964e29 ("ums_realtek: do not use stack memory for DMA")
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>  drivers/usb/storage/realtek_cr.c | 36 ++++++++++++++++----------------
>>  1 file changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
>> index 3789698d9d3c..b983753e2368 100644
>> --- a/drivers/usb/storage/realtek_cr.c
>> +++ b/drivers/usb/storage/realtek_cr.c
>> @@ -481,16 +481,16 @@ static int enable_oscillator(struct us_data *us)
>>  	u8 value;
>>  
>>  	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
>> -	if (retval < 0)
>> +	if (retval != STATUS_SUCCESS)
>>  		return -EIO;
> Instead of changing all these call sites, wouldn't it be a lot easier 
> just to change rts51x_read_mem() to make it always return a negative 
> value (such as -EIO) when there's an error?
>
> Alan Stern

I thought about that but there was already existing (retval != STATUS_SUCCESS) checks for these calls.

Tom

>
Alan Stern Aug. 11, 2020, 5:53 p.m. UTC | #3 
On Tue, Aug 11, 2020 at 10:29:29AM -0700, Tom Rix wrote:
> 
> On 8/11/20 9:03 AM, Alan Stern wrote:
> > On Tue, Aug 11, 2020 at 08:15:05AM -0700, trix@redhat.com wrote:
> >> From: Tom Rix <trix@redhat.com>
> >>
> >> clang static analysis reports this representative problem
> >>
> >> realtek_cr.c:639:3: warning: The left expression of the compound
> >>   assignment is an uninitialized value. The computed value will
> >>   also be garbage
> >>     SET_BIT(value, 2);
> >>     ^~~~~~~~~~~~~~~~~
> >>
> >> value is set by a successful call to rts51x_read_mem()
> >>
> >> 	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
> >> 	if (retval < 0)
> >> 		return -EIO;
> >>
> >> A successful call to rts51x_read_mem returns 0, failure can
> >> return positive and negative values.  This check is wrong
> >> for a number of functions.  Fix the retval check.
> >>
> >> Fixes: 065e60964e29 ("ums_realtek: do not use stack memory for DMA")
> >> Signed-off-by: Tom Rix <trix@redhat.com>
> >> ---
> >>  drivers/usb/storage/realtek_cr.c | 36 ++++++++++++++++----------------
> >>  1 file changed, 18 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> >> index 3789698d9d3c..b983753e2368 100644
> >> --- a/drivers/usb/storage/realtek_cr.c
> >> +++ b/drivers/usb/storage/realtek_cr.c
> >> @@ -481,16 +481,16 @@ static int enable_oscillator(struct us_data *us)
> >>  	u8 value;
> >>  
> >>  	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
> >> -	if (retval < 0)
> >> +	if (retval != STATUS_SUCCESS)
> >>  		return -EIO;
> > Instead of changing all these call sites, wouldn't it be a lot easier 
> > just to change rts51x_read_mem() to make it always return a negative 
> > value (such as -EIO) when there's an error?
> >
> > Alan Stern
> 
> I thought about that but there was already existing (retval != 
> STATUS_SUCCESS) checks for these calls.

The only values that routine currently returns are 
USB_STOR_TRANSPORT_ERROR, -EIO, and 0.  None of the callers distinguish 
between the first two values, so you can just change the first to the 
second.

Note that STATUS_SUCCESS is simply 0.

Alan Stern
Tom Rix Aug. 11, 2020, 6:54 p.m. UTC | #4 
On 8/11/20 10:53 AM, Alan Stern wrote:
> On Tue, Aug 11, 2020 at 10:29:29AM -0700, Tom Rix wrote:
>> On 8/11/20 9:03 AM, Alan Stern wrote:
>>> On Tue, Aug 11, 2020 at 08:15:05AM -0700, trix@redhat.com wrote:
>>>> From: Tom Rix <trix@redhat.com>
>>>>
>>>> clang static analysis reports this representative problem
>>>>
>>>> realtek_cr.c:639:3: warning: The left expression of the compound
>>>>   assignment is an uninitialized value. The computed value will
>>>>   also be garbage
>>>>     SET_BIT(value, 2);
>>>>     ^~~~~~~~~~~~~~~~~
>>>>
>>>> value is set by a successful call to rts51x_read_mem()
>>>>
>>>> 	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
>>>> 	if (retval < 0)
>>>> 		return -EIO;
>>>>
>>>> A successful call to rts51x_read_mem returns 0, failure can
>>>> return positive and negative values.  This check is wrong
>>>> for a number of functions.  Fix the retval check.
>>>>
>>>> Fixes: 065e60964e29 ("ums_realtek: do not use stack memory for DMA")
>>>> Signed-off-by: Tom Rix <trix@redhat.com>
>>>> ---
>>>>  drivers/usb/storage/realtek_cr.c | 36 ++++++++++++++++----------------
>>>>  1 file changed, 18 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
>>>> index 3789698d9d3c..b983753e2368 100644
>>>> --- a/drivers/usb/storage/realtek_cr.c
>>>> +++ b/drivers/usb/storage/realtek_cr.c
>>>> @@ -481,16 +481,16 @@ static int enable_oscillator(struct us_data *us)
>>>>  	u8 value;
>>>>  
>>>>  	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
>>>> -	if (retval < 0)
>>>> +	if (retval != STATUS_SUCCESS)
>>>>  		return -EIO;
>>> Instead of changing all these call sites, wouldn't it be a lot easier 
>>> just to change rts51x_read_mem() to make it always return a negative 
>>> value (such as -EIO) when there's an error?
>>>
>>> Alan Stern
>> I thought about that but there was already existing (retval != 
>> STATUS_SUCCESS) checks for these calls.
> The only values that routine currently returns are 
> USB_STOR_TRANSPORT_ERROR, -EIO, and 0.  None of the callers distinguish 
> between the first two values, so you can just change the first to the 
> second.
>
> Note that STATUS_SUCCESS is simply 0.

Yes, i noted all of these already. My change is consistent with the existing correct checks.  consistency is important.  returning a neg value to reuse the exiting check should mean the STATUS_SUCCESS != 0 checks are changed to neg check.  i can do this larger change if required.

Tom

>
> Alan Stern
>
Alan Stern Aug. 11, 2020, 7:43 p.m. UTC | #5 
On Tue, Aug 11, 2020 at 11:54:28AM -0700, Tom Rix wrote:
> 
> On 8/11/20 10:53 AM, Alan Stern wrote:

> >>> Instead of changing all these call sites, wouldn't it be a lot easier 
> >>> just to change rts51x_read_mem() to make it always return a negative 
> >>> value (such as -EIO) when there's an error?
> >>>
> >>> Alan Stern
> >> I thought about that but there was already existing (retval != 
> >> STATUS_SUCCESS) checks for these calls.
> > The only values that routine currently returns are 
> > USB_STOR_TRANSPORT_ERROR, -EIO, and 0.  None of the callers distinguish 
> > between the first two values, so you can just change the first to the 
> > second.
> >
> > Note that STATUS_SUCCESS is simply 0.
> 
> Yes, i noted all of these already. My change is consistent with the 
> existing correct checks.  consistency is important.  returning a neg 
> value to reuse the exiting check should mean the STATUS_SUCCESS != 0 
> checks are changed to neg check.

Do you mean the "retval == STATUS_SUCCESS" checks?  Those checks would 
end up doing exactly the same thing as they do now, since 
USB_STOR_TRANSPORT_ERROR and -EIO are both different from 0.

Yes, it is true that consistency is important.  But correctness is more 
important than consistency.

>  i can do this larger change if 
> required.

Let me put it this way: Suppose you changed the USB_STOR_TRANSPORT_ERROR 
in rts51x_read_mem() to -EIO, without changing anything else.  Wouldn't 
that fix the problem reported by the clang static analysis?  If not, why 
not?

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index 3789698d9d3c..b983753e2368 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -481,16 +481,16 @@  static int enable_oscillator(struct us_data *us)
 	u8 value;
 
 	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
-	if (retval < 0)
+	if (retval != STATUS_SUCCESS)
 		return -EIO;
 
 	value |= 0x04;
 	retval = rts51x_write_mem(us, 0xFE77, &value, 1);
-	if (retval < 0)
+	if (retval != STATUS_SUCCESS)
 		return -EIO;
 
 	retval = rts51x_read_mem(us, 0xFE77, &value, 1);
-	if (retval < 0)
+	if (retval != STATUS_SUCCESS)
 		return -EIO;
 
 	if (!(value & 0x04))
@@ -533,7 +533,7 @@  static int do_config_autodelink(struct us_data *us, int enable, int force)
 	u8 value;
 
 	retval = rts51x_read_mem(us, 0xFE47, &value, 1);
-	if (retval < 0)
+	if (retval != STATUS_SUCCESS)
 		return -EIO;
 
 	if (enable) {
@@ -549,7 +549,7 @@  static int do_config_autodelink(struct us_data *us, int enable, int force)
 
 	/* retval = rts51x_write_mem(us, 0xFE47, &value, 1); */
 	retval = __do_config_autodelink(us, &value, 1);
-	if (retval < 0)
+	if (retval != STATUS_SUCCESS)
 		return -EIO;
 
 	return 0;
@@ -565,7 +565,7 @@  static int config_autodelink_after_power_on(struct us_data *us)
 		return 0;
 
 	retval = rts51x_read_mem(us, 0xFE47, &value, 1);
-	if (retval < 0)
+	if (retval != STATUS_SUCCESS)
 		return -EIO;
 
 	if (auto_delink_en) {
@@ -580,7 +580,7 @@  static int config_autodelink_after_power_on(struct us_data *us)
 
 		/* retval = rts51x_write_mem(us, 0xFE47, &value, 1); */
 		retval = __do_config_autodelink(us, &value, 1);
-		if (retval < 0)
+		if (retval != STATUS_SUCCESS)
 			return -EIO;
 
 		retval = enable_oscillator(us);
@@ -602,18 +602,18 @@  static int config_autodelink_after_power_on(struct us_data *us)
 
 		/* retval = rts51x_write_mem(us, 0xFE47, &value, 1); */
 		retval = __do_config_autodelink(us, &value, 1);
-		if (retval < 0)
+		if (retval != STATUS_SUCCESS)
 			return -EIO;
 
 		if (CHECK_ID(chip, 0x0159, 0x5888)) {
 			value = 0xFF;
 			retval = rts51x_write_mem(us, 0xFE79, &value, 1);
-			if (retval < 0)
+			if (retval != STATUS_SUCCESS)
 				return -EIO;
 
 			value = 0x01;
 			retval = rts51x_write_mem(us, 0x48, &value, 1);
-			if (retval < 0)
+			if (retval != STATUS_SUCCESS)
 				return -EIO;
 		}
 	}
@@ -633,37 +633,37 @@  static int config_autodelink_before_power_down(struct us_data *us)
 
 	if (auto_delink_en) {
 		retval = rts51x_read_mem(us, 0xFE77, &value, 1);
-		if (retval < 0)
+		if (retval != STATUS_SUCCESS)
 			return -EIO;
 
 		SET_BIT(value, 2);
 		retval = rts51x_write_mem(us, 0xFE77, &value, 1);
-		if (retval < 0)
+		if (retval != STATUS_SUCCESS)
 			return -EIO;
 
 		if (CHECK_ID(chip, 0x0159, 0x5888)) {
 			value = 0x01;
 			retval = rts51x_write_mem(us, 0x48, &value, 1);
-			if (retval < 0)
+			if (retval != STATUS_SUCCESS)
 				return -EIO;
 		}
 
 		retval = rts51x_read_mem(us, 0xFE47, &value, 1);
-		if (retval < 0)
+		if (retval != STATUS_SUCCESS)
 			return -EIO;
 
 		SET_BIT(value, 0);
 		if (CHECK_ID(chip, 0x0138, 0x3882))
 			SET_BIT(value, 2);
 		retval = rts51x_write_mem(us, 0xFE77, &value, 1);
-		if (retval < 0)
+		if (retval != STATUS_SUCCESS)
 			return -EIO;
 	} else {
 		if (CHECK_ID(chip, 0x0159, 0x5889) ||
 		    CHECK_ID(chip, 0x0138, 0x3880) ||
 		    CHECK_ID(chip, 0x0138, 0x3882)) {
 			retval = rts51x_read_mem(us, 0xFE47, &value, 1);
-			if (retval < 0)
+			if (retval != STATUS_SUCCESS)
 				return -EIO;
 
 			if (CHECK_ID(chip, 0x0159, 0x5889) ||
@@ -677,14 +677,14 @@  static int config_autodelink_before_power_down(struct us_data *us)
 
 			/* retval = rts51x_write_mem(us, 0xFE47, &value, 1); */
 			retval = __do_config_autodelink(us, &value, 1);
-			if (retval < 0)
+			if (retval != STATUS_SUCCESS)
 				return -EIO;
 		}
 
 		if (CHECK_ID(chip, 0x0159, 0x5888)) {
 			value = 0x01;
 			retval = rts51x_write_mem(us, 0x48, &value, 1);
-			if (retval < 0)
+			if (retval != STATUS_SUCCESS)
 				return -EIO;
 		}
 	}