Staging: winbond: usb_free_urb(NULL) is safe

drivers/staging/winbond/wb35reg.c +4 -7

 		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

drivers/staging/winbond/wb35rx.c +1 -3

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

drivers/staging/winbond/wb35reg.c unsigned char Wb35Reg_BurstWrite(struct · line 67

Quoted 10 lines

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c

drivers/staging/winbond/wb35reg.c +1 -2

@@ -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

Quoted 3 lines · Julia Lawall

drivers/staging/winbond/wb35reg.c

Quoted 3 lines

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c

drivers/staging/winbond/wb35reg.c +0 -0

Quoted 12 lines

drivers/staging/winbond/wb35reg.c unsigned char Wb35Reg_BurstWrite(struct · line 67

Quoted 7 lines

drivers/staging/winbond/wb35reg.c +1 -2

@@ -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.

Quoted 4 lines

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.

Quoted 36 lines · Julia Lawall

Quoted 6 lines · Harsh Kumar

Quoted 3 lines · Julia Lawall

drivers/staging/winbond/wb35reg.c

Quoted 3 lines

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c

drivers/staging/winbond/wb35reg.c +0 -0

02:11:35.000000000 +0530

Quoted 12 lines

Quoted 12 lines

drivers/staging/winbond/wb35reg.c unsigned char Wb35Reg_BurstWrite(struct · line 67

Quoted 7 lines

drivers/staging/winbond/wb35reg.c +1 -2

@@ -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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 6 lines · Harsh Kumar

Quoted 3 lines · Julia Lawall

drivers/staging/winbond/wb35reg.c

Quoted 3 lines

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c

drivers/staging/winbond/wb35reg.c +0 -0

02:11:35.000000000 +0530

Quoted 12 lines

Quoted 12 lines

drivers/staging/winbond/wb35reg.c unsigned char Wb35Reg_BurstWrite(struct · line 67

Quoted 7 lines

drivers/staging/winbond/wb35reg.c +1 -2

@@ -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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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

Quoted 45 lines · Harsh Kumar

Quoted 41 lines · Julia Lawall

Quoted 6 lines · Harsh Kumar

Quoted 3 lines · Julia Lawall

drivers/staging/winbond/wb35reg.c

Quoted 3 lines

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c

drivers/staging/winbond/wb35reg.c +0 -0

02:11:35.000000000 +0530

Quoted 12 lines

Quoted 12 lines

drivers/staging/winbond/wb35reg.c unsigned char Wb35Reg_BurstWrite(struct · line 67

Quoted 7 lines

drivers/staging/winbond/wb35reg.c +1 -2

@@ -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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 1 line

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

Quoted 5 lines

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c

drivers/staging/winbond/wb35reg.c +0 -0

@@ -64,12 +64,11 @@ unsigned char Wb35Reg_BurstWrite(struct
 

drivers/staging/winbond/wb35reg.c unsigned char Wb35Reg_BurstWrite(struct · line 67

Quoted 5 lines

drivers/staging/winbond/wb35reg.c +1 -2

 		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.