[Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Harsh Kumar]

usb_free_urb(NULL) is safe. So, the check was removed. Also, couple of checkpatch.pl fixes

Signed-off-by: Harsh Kumar <harsh1kumar@gmail.com>

---
 drivers/staging/winbond/phy_calibration.c |    2 +-
 drivers/staging/winbond/wb35reg.c         |   14 +++++---------
 drivers/staging/winbond/wb35rx.c          |    3 +--
 3 files changed, 7 insertions(+), 12 deletions(-)

diff -uprN a/drivers/staging/winbond/phy_calibration.c b/drivers/staging/winbond/phy_calibration.c
--- a/drivers/staging/winbond/phy_calibration.c	2013-05-28 00:52:15.000000000 +0530
+++ b/drivers/staging/winbond/phy_calibration.c	2013-05-28 01:54:07.000000000 +0530
@@ -296,7 +296,7 @@ void _sin_cos(s32 angle, s32 *sin, s32 *
 	}
 }
 
-static unsigned char hal_get_dxx_reg(struct hw_data *pHwData, u16 number, u32 * pValue)
+static unsigned char hal_get_dxx_reg(struct hw_data *pHwData, u16 number, u32 *pValue)
 {
 	if (number < 0x1000)
 		number += 0x1000;
diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
--- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
+++ b/drivers/staging/winbond/wb35reg.c	2013-05-28 02:11:35.000000000 +0530
@@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
 
 		return true;
 	} else {
-		if (urb)
-			usb_free_urb(urb);
+		usb_free_urb(urb);
 		kfree(reg_queue);
 		return false;
 	}
-   return false;
+	return false;
 }
 
 void Wb35Reg_Update(struct hw_data *pHwData,  u16 RegisterNo,  u32 RegisterValue)
@@ -206,8 +205,7 @@ unsigned char Wb35Reg_Write(struct hw_da
 
 		return true;
 	} else {
-		if (urb)
-			usb_free_urb(urb);
+		usb_free_urb(urb);
 		kfree(reg_queue);
 		return false;
 	}
@@ -270,8 +268,7 @@ unsigned char Wb35Reg_WriteWithCallbackV
 		Wb35Reg_EP0VM_start(pHwData);
 		return true;
 	} else {
-		if (urb)
-			usb_free_urb(urb);
+		usb_free_urb(urb);
 		kfree(reg_queue);
 		return false;
 	}
@@ -374,8 +371,7 @@ unsigned char Wb35Reg_Read(struct hw_dat
 
 		return true;
 	} else {
-		if (urb)
-			usb_free_urb(urb);
+		usb_free_urb(urb);
 		kfree(reg_queue);
 		return false;
 	}
diff -uprN a/drivers/staging/winbond/wb35rx.c b/drivers/staging/winbond/wb35rx.c
--- a/drivers/staging/winbond/wb35rx.c	2013-05-28 00:52:40.000000000 +0530
+++ b/drivers/staging/winbond/wb35rx.c	2013-05-28 02:25:16.000000000 +0530
@@ -343,8 +343,7 @@ void Wb35Rx_destroy(struct hw_data *pHwD
 	} while (pWb35Rx->EP3vm_state != VM_STOP);
 	msleep(10); /* Delay for waiting function exit */
 
-	if (pWb35Rx->RxUrb)
-		usb_free_urb(pWb35Rx->RxUrb);
+	usb_free_urb(pWb35Rx->RxUrb);
 	pr_debug("Wb35Rx_destroy OK\n");
 }
 

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Julia Lawall]

> diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
> --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
> +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28 02:11:35.000000000 +0530
> @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
>  
>  		return true;
>  	} else {
> -		if (urb)
> -			usb_free_urb(urb);
> +		usb_free_urb(urb);

I took a look at this case.  Wouldn't it be nicer to check for failures 
one by one, as done almost everywhere else in the kernel?  Then you would 
know what had been successfully allocated and what has to be freed.

julia



>  		kfree(reg_queue);
>  		return false;
>  	}
> -   return false;
> +	return false;
>  }
>  
>  void Wb35Reg_Update(struct hw_data *pHwData,  u16 RegisterNo,  u32 RegisterValue)
> @@ -206,8 +205,7 @@ unsigned char Wb35Reg_Write(struct hw_da
>  
>  		return true;
>  	} else {
> -		if (urb)
> -			usb_free_urb(urb);
> +		usb_free_urb(urb);
>  		kfree(reg_queue);
>  		return false;
>  	}
> @@ -270,8 +268,7 @@ unsigned char Wb35Reg_WriteWithCallbackV
>  		Wb35Reg_EP0VM_start(pHwData);
>  		return true;
>  	} else {
> -		if (urb)
> -			usb_free_urb(urb);
> +		usb_free_urb(urb);
>  		kfree(reg_queue);
>  		return false;
>  	}
> @@ -374,8 +371,7 @@ unsigned char Wb35Reg_Read(struct hw_dat
>  
>  		return true;
>  	} else {
> -		if (urb)
> -			usb_free_urb(urb);
> +		usb_free_urb(urb);
>  		kfree(reg_queue);
>  		return false;
>  	}
> diff -uprN a/drivers/staging/winbond/wb35rx.c b/drivers/staging/winbond/wb35rx.c
> --- a/drivers/staging/winbond/wb35rx.c	2013-05-28 00:52:40.000000000 +0530
> +++ b/drivers/staging/winbond/wb35rx.c	2013-05-28 02:25:16.000000000 +0530
> @@ -343,8 +343,7 @@ void Wb35Rx_destroy(struct hw_data *pHwD
>  	} while (pWb35Rx->EP3vm_state != VM_STOP);
>  	msleep(10); /* Delay for waiting function exit */
>  
> -	if (pWb35Rx->RxUrb)
> -		usb_free_urb(pWb35Rx->RxUrb);
> +	usb_free_urb(pWb35Rx->RxUrb);
>  	pr_debug("Wb35Rx_destroy OK\n");
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Harsh Kumar]

On Thursday 30 May 2013 10:41 AM, Julia Lawall wrote:
>> diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
>> --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
>> +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28 02:11:35.000000000 +0530
>> @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
>>  
>>  		return true;
>>  	} else {
>> -		if (urb)
>> -			usb_free_urb(urb);
>> +		usb_free_urb(urb);
> 
> I took a look at this case.  Wouldn't it be nicer to check for failures 
> one by one, as done almost everywhere else in the kernel?  Then you would 
> know what had been successfully allocated and what has to be freed.
> 
> julia
> 
> 
> 

Do you want that values of urb and reg_queue to be checked separately to see which has 
failed? That will be more logical. But, then what should be done with the knowledge of 
what has failed? Should there be a print or should the return value change?

Actually, the return values of these functions like Wb35Reg_BurstWrite() have not 
been used where these functions are called. Maybe, we need to check whether write & 
read are successful or not. Maybe, that also needs to be changed.

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Julia Lawall]

On Thu, 30 May 2013, Harsh Kumar wrote:

>
>
> On Thursday 30 May 2013 10:41 AM, Julia Lawall wrote:
> >> diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
> >> --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
> >> +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28
02:11:35.000000000 +0530
> >> @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
> >>
> >>  		return true;
> >>  	} else {
> >> -		if (urb)
> >> -			usb_free_urb(urb);
> >> +		usb_free_urb(urb);
> >
> > I took a look at this case.  Wouldn't it be nicer to check for failures
> > one by one, as done almost everywhere else in the kernel?  Then you would
> > know what had been successfully allocated and what has to be freed.
> >
> > julia
> >
> >
> >
>
> Do you want that values of urb and reg_queue to be checked separately to see which has
> failed? That will be more logical. But, then what should be done with the knowledge of
> what has failed? Should there be a print or should the return value change?

I don't know much about the driver, so a safe thing to do would be just to
keep the current semantics.  When the kzalloc fails, just return false.
When the usb_alloc_urb fails, just kfree and then return false.

Also, currently there is a return false at the end of the function that is
dead code.  Perhaps things could be reorganized so that that is not
necessary.  Usually, after an allocation, the if just takes care of the
error case, and the fallthrough case continues in the normal way.

> Actually, the return values of these functions like Wb35Reg_BurstWrite() have not
> been used where these functions are called. Maybe, we need to check whether write &
> read are successful or not. Maybe, that also needs to be changed.

I don't know.

julia

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Julia Lawall]

On Thu, 30 May 2013, Harsh Kumar wrote:

>
>
> On Thursday 30 May 2013 12:58 PM, Julia Lawall wrote:
> >
> >
> > On Thu, 30 May 2013, Harsh Kumar wrote:
> >
> >>
> >>
> >> On Thursday 30 May 2013 10:41 AM, Julia Lawall wrote:
> >>>> diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
> >>>> --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
> >>>> +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28
> > 02:11:35.000000000 +0530
> >>>> @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
> >>>>
> >>>>  		return true;
> >>>>  	} else {
> >>>> -		if (urb)
> >>>> -			usb_free_urb(urb);
> >>>> +		usb_free_urb(urb);
> >>>
> >>> I took a look at this case.  Wouldn't it be nicer to check for failures
> >>> one by one, as done almost everywhere else in the kernel?  Then you would
> >>> know what had been successfully allocated and what has to be freed.
> >>>
> >>> julia
> >>>
> >>>
> >>>
> >>
> >> Do you want that values of urb and reg_queue to be checked separately to see which has
> >> failed? That will be more logical. But, then what should be done with the knowledge of
> >> what has failed? Should there be a print or should the return value change?
> >
> > I don't know much about the driver, so a safe thing to do would be just to
> > keep the current semantics.  When the kzalloc fails, just return false.
> > When the usb_alloc_urb fails, just kfree and then return false.
> >
> > Also, currently there is a return false at the end of the function that is
> > dead code.  Perhaps things could be reorganized so that that is not
> > necessary.  Usually, after an allocation, the if just takes care of the
> > error case, and the fallthrough case continues in the normal way.
> >
>
> Okay, got it. I will reorganize the stuff here.

I think that some of the other cases should be changed in the same way.
It is just in the destroy function that the if was not needed.

julia

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Harsh Kumar]

On Thursday 30 May 2013 12:58 PM, Julia Lawall wrote:
> 
> 
> On Thu, 30 May 2013, Harsh Kumar wrote:
> 
>>
>>
>> On Thursday 30 May 2013 10:41 AM, Julia Lawall wrote:
>>>> diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
>>>> --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
>>>> +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28
> 02:11:35.000000000 +0530
>>>> @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
>>>>
>>>>  		return true;
>>>>  	} else {
>>>> -		if (urb)
>>>> -			usb_free_urb(urb);
>>>> +		usb_free_urb(urb);
>>>
>>> I took a look at this case.  Wouldn't it be nicer to check for failures
>>> one by one, as done almost everywhere else in the kernel?  Then you would
>>> know what had been successfully allocated and what has to be freed.
>>>
>>> julia
>>>
>>>
>>>
>>
>> Do you want that values of urb and reg_queue to be checked separately to see which has
>> failed? That will be more logical. But, then what should be done with the knowledge of
>> what has failed? Should there be a print or should the return value change?
> 
> I don't know much about the driver, so a safe thing to do would be just to
> keep the current semantics.  When the kzalloc fails, just return false.
> When the usb_alloc_urb fails, just kfree and then return false.
> 
> Also, currently there is a return false at the end of the function that is
> dead code.  Perhaps things could be reorganized so that that is not
> necessary.  Usually, after an allocation, the if just takes care of the
> error case, and the fallthrough case continues in the normal way.
> 

Okay, got it. I will reorganize the stuff here.

>> Actually, the return values of these functions like Wb35Reg_BurstWrite() have not
>> been used where these functions are called. Maybe, we need to check whether write &
>> read are successful or not. Maybe, that also needs to be changed.
> 
> I don't know.
> 
> julia

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Harsh Kumar]

On Thursday 30 May 2013 01:13 PM, Julia Lawall wrote:
> 
> 
> On Thu, 30 May 2013, Harsh Kumar wrote:
> 
>>
>>
>> On Thursday 30 May 2013 12:58 PM, Julia Lawall wrote:
>>>
>>>
>>> On Thu, 30 May 2013, Harsh Kumar wrote:
>>>
>>>>
>>>>
>>>> On Thursday 30 May 2013 10:41 AM, Julia Lawall wrote:
>>>>>> diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
>>>>>> --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
>>>>>> +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28
>>> 02:11:35.000000000 +0530
>>>>>> @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
>>>>>>
>>>>>>  		return true;
>>>>>>  	} else {
>>>>>> -		if (urb)
>>>>>> -			usb_free_urb(urb);
>>>>>> +		usb_free_urb(urb);
>>>>>
>>>>> I took a look at this case.  Wouldn't it be nicer to check for failures
>>>>> one by one, as done almost everywhere else in the kernel?  Then you would
>>>>> know what had been successfully allocated and what has to be freed.
>>>>>
>>>>> julia
>>>>>
>>>>>
>>>>>
>>>>
>>>> Do you want that values of urb and reg_queue to be checked separately to see which has
>>>> failed? That will be more logical. But, then what should be done with the knowledge of
>>>> what has failed? Should there be a print or should the return value change?
>>>
>>> I don't know much about the driver, so a safe thing to do would be just to
>>> keep the current semantics.  When the kzalloc fails, just return false.
>>> When the usb_alloc_urb fails, just kfree and then return false.
>>>
>>> Also, currently there is a return false at the end of the function that is
>>> dead code.  Perhaps things could be reorganized so that that is not
>>> necessary.  Usually, after an allocation, the if just takes care of the
>>> error case, and the fallthrough case continues in the normal way.
>>>
>>
>> Okay, got it. I will reorganize the stuff here.
> 
> I think that some of the other cases should be changed in the same way.
> It is just in the destroy function that the if was not needed.
> 
> julia

Yes, I understood that. I will change other cases as well. Thanks.

Harsh

Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe

[Pavel Machek]

On Thu 2013-05-30 07:11:24, Julia Lawall wrote:
> > diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
> > --- a/drivers/staging/winbond/wb35reg.c	2013-05-28 00:52:26.000000000 +0530
> > +++ b/drivers/staging/winbond/wb35reg.c	2013-05-28 02:11:35.000000000 +0530
> > @@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
> >  
> >  		return true;
> >  	} else {
> > -		if (urb)
> > -			usb_free_urb(urb);
> > +		usb_free_urb(urb);
> 
> I took a look at this case.  Wouldn't it be nicer to check for failures 
> one by one, as done almost everywhere else in the kernel?  Then you would 
> know what had been successfully allocated and what has to be freed.

Yeah, that code needs lot of improvements. No, we probably will not
make it perfect by single patch.

(Original patch:)

Acked-by: Pavel Machek <pavel@ucw.cz>

Does someone have the hardware, BTW? I have single instance here, I
need to power it up some day. It was apparently sold as an USB card,
too... (I have version built-in obscure netbook).

     	      	    	      	     		     	   Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html