* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
@ 2013-05-30 5:11 ` Julia Lawall
2013-05-30 6:50 ` Harsh Kumar
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2013-05-30 5:11 UTC (permalink / raw)
To: kernel-janitors
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
2013-05-30 5:11 ` Julia Lawall
@ 2013-05-30 6:50 ` Harsh Kumar
2013-05-30 7:28 ` Julia Lawall
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Harsh Kumar @ 2013-05-30 6:50 UTC (permalink / raw)
To: kernel-janitors
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
2013-05-30 5:11 ` Julia Lawall
2013-05-30 6:50 ` Harsh Kumar
@ 2013-05-30 7:28 ` Julia Lawall
2013-05-30 7:43 ` Julia Lawall
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2013-05-30 7:28 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
` (2 preceding siblings ...)
2013-05-30 7:28 ` Julia Lawall
@ 2013-05-30 7:43 ` Julia Lawall
2013-05-30 7:50 ` Harsh Kumar
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2013-05-30 7:43 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
` (3 preceding siblings ...)
2013-05-30 7:43 ` Julia Lawall
@ 2013-05-30 7:50 ` Harsh Kumar
2013-05-30 7:55 ` Harsh Kumar
2013-05-30 9:38 ` Pavel Machek
6 siblings, 0 replies; 8+ messages in thread
From: Harsh Kumar @ 2013-05-30 7:50 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
` (4 preceding siblings ...)
2013-05-30 7:50 ` Harsh Kumar
@ 2013-05-30 7:55 ` Harsh Kumar
2013-05-30 9:38 ` Pavel Machek
6 siblings, 0 replies; 8+ messages in thread
From: Harsh Kumar @ 2013-05-30 7:55 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: usb_free_urb(NULL) is safe
2013-05-30 4:47 [Patch] Staging: winbond: usb_free_urb(NULL) is safe Harsh Kumar
` (5 preceding siblings ...)
2013-05-30 7:55 ` Harsh Kumar
@ 2013-05-30 9:38 ` Pavel Machek
6 siblings, 0 replies; 8+ messages in thread
From: Pavel Machek @ 2013-05-30 9:38 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 8+ messages in thread