[Patch 2/2] Staging: winbond: Memory & urb freed

[Patch 2/2] Staging: winbond: Memory & urb freed

[Harsh Kumar]

Memory & urb should be freed before exiting from the function, I think.

Note: This patch is to be applied after applying the earlier patch i.e [patch 1/2]

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

---
 drivers/staging/winbond/wb35reg.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff -uprN a/drivers/staging/winbond/wb35reg.c b/drivers/staging/winbond/wb35reg.c
--- a/drivers/staging/winbond/wb35reg.c	2013-05-31 20:03:17.000000000 +0530
+++ b/drivers/staging/winbond/wb35reg.c	2013-05-31 20:21:39.856010583 +0530
@@ -69,6 +69,9 @@ unsigned char Wb35Reg_BurstWrite(struct
 	/* Start EP0VM */
 	Wb35Reg_EP0VM_start(pHwData);
 
+	kfree(reg_queue);
+	usb_free_urb(urb);
+
 	return true;
 }
 
@@ -211,6 +214,9 @@ unsigned char Wb35Reg_Write(struct hw_da
 	/* Start EP0VM */
 	Wb35Reg_EP0VM_start(pHwData);
 
+	kfree(reg_queue);
+	usb_free_urb(urb);
+
 	return true;
 }
 
@@ -277,6 +283,9 @@ unsigned char Wb35Reg_WriteWithCallbackV
 	/* Start EP0VM */
 	Wb35Reg_EP0VM_start(pHwData);
 
+	kfree(reg_queue);
+	usb_free_urb(urb);
+
 	return true;
 }
 
@@ -381,6 +390,9 @@ unsigned char Wb35Reg_Read(struct hw_dat
 	/* Start EP0VM */
 	Wb35Reg_EP0VM_start(pHwData);
 
+	kfree(reg_queue);
+	usb_free_urb(urb);
+
 	return true;
 }
 

Re: [Patch 2/2] Staging: winbond: Memory & urb freed

[Dan Carpenter]

On Fri, May 31, 2013 at 09:02:11PM +0530, Harsh Kumar wrote:
> Memory & urb should be freed before exiting from the function, I think.
> 

They are freed in Wb35Reg_EP0VM_complete() so this patch will make
the system crash right away.  Btw, there are tons of real bugs that
I know about but which I don't fix because I don't know what the
right thing to do is.

> Note: This patch is to be applied after applying the earlier patch i.e [patch 1/2]
> 

In a patch series it's assumed that we have to apply them in order,
but if you need to add a comment like this the right place to do it
is under the --- line and before the patch.

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

Side comments not intended for the perment git log go here.

>  drivers/staging/winbond/wb35reg.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

regards,
dan carpenter

Re: [Patch 2/2] Staging: winbond: Memory & urb freed

[Harsh Kumar]

On Friday 31 May 2013 09:27 PM, Dan Carpenter wrote:
> On Fri, May 31, 2013 at 09:02:11PM +0530, Harsh Kumar wrote:
>> Memory & urb should be freed before exiting from the function, I think.
>>
> 
> They are freed in Wb35Reg_EP0VM_complete() so this patch will make
> the system crash right away.  Btw, there are tons of real bugs that
> I know about but which I don't fix because I don't know what the
> right thing to do is.
> 

Ohh! Sorry, I missed it. That is why I was not sure about submitting this 
change. I need to be more thorough in checking this stuff. 

I have general question - Generally, shouldn't allocation and freeing up the 
memory be in the same function so that it is easy to make sure that everything 
is freed up after completion of task? Is it because there maybe certain cases 
where that may not be feasible or desirable to do so?

Harsh

Re: [Patch 2/2] Staging: winbond: Memory & urb freed

[Dan Carpenter]

On Fri, May 31, 2013 at 09:48:58PM +0530, Harsh Kumar wrote:
> 
> 
> On Friday 31 May 2013 09:27 PM, Dan Carpenter wrote:
> > On Fri, May 31, 2013 at 09:02:11PM +0530, Harsh Kumar wrote:
> >> Memory & urb should be freed before exiting from the function, I think.
> >>
> > 
> > They are freed in Wb35Reg_EP0VM_complete() so this patch will make
> > the system crash right away.  Btw, there are tons of real bugs that
> > I know about but which I don't fix because I don't know what the
> > right thing to do is.
> > 
> 
> Ohh! Sorry, I missed it. That is why I was not sure about submitting this 
> change. I need to be more thorough in checking this stuff. 
> 
> I have general question - Generally, shouldn't allocation and freeing up the 
> memory be in the same function so that it is easy to make sure that everything 
> is freed up after completion of task? Is it because there maybe certain cases 
> where that may not be feasible or desirable to do so?

Unfortunately kernel programming will never be 100% easy...  :P  In
the end you will learn the tricks just like I did.  Here was my
thought process here:
1) The commit message was not convincing.
2) The memory was saved to reg->reg_first.
3) Follow the Wb35Reg_EP0VM_start() call to the urb_submit point.
4) See that we pass Wb35Reg_EP0VM_complete() to urb_submit function.
   I have worked on usb drivers before so I suspected that
   Wb35Reg_EP0VM_complete() frees the urb.
5) Vefiried that this is true.  Done.

Or alternatively:
3) Find reg_first in Wb35Reg_EP0VM_complete() and see that we free
   it.

Make sure you have cscope configured in vim.  Also in vim the '*'
button searches.

If you think you have found a bug but you're not sure, feel free to
ask on kernel janitors.  Probably for new code it's not the right
mailing list but for code audits it's fine.

regards,
dan carpenter

Re: [Patch 2/2] Staging: winbond: Memory & urb freed

[Julia Lawall]

On Fri, 31 May 2013, Dan Carpenter wrote:

> On Fri, May 31, 2013 at 09:48:58PM +0530, Harsh Kumar wrote:
> >
> >
> > On Friday 31 May 2013 09:27 PM, Dan Carpenter wrote:
> > > On Fri, May 31, 2013 at 09:02:11PM +0530, Harsh Kumar wrote:
> > >> Memory & urb should be freed before exiting from the function, I think.
> > >>
> > >
> > > They are freed in Wb35Reg_EP0VM_complete() so this patch will make
> > > the system crash right away.  Btw, there are tons of real bugs that
> > > I know about but which I don't fix because I don't know what the
> > > right thing to do is.
> > >
> >
> > Ohh! Sorry, I missed it. That is why I was not sure about submitting this
> > change. I need to be more thorough in checking this stuff.
> >
> > I have general question - Generally, shouldn't allocation and freeing up the
> > memory be in the same function so that it is easy to make sure that everything
> > is freed up after completion of task? Is it because there maybe certain cases
> > where that may not be feasible or desirable to do so?
>
> Unfortunately kernel programming will never be 100% easy...  :P  In
> the end you will learn the tricks just like I did.  Here was my
> thought process here:
> 1) The commit message was not convincing.
> 2) The memory was saved to reg->reg_first.
> 3) Follow the Wb35Reg_EP0VM_start() call to the urb_submit point.
> 4) See that we pass Wb35Reg_EP0VM_complete() to urb_submit function.
>    I have worked on usb drivers before so I suspected that
>    Wb35Reg_EP0VM_complete() frees the urb.
> 5) Vefiried that this is true.  Done.
>
> Or alternatively:
> 3) Find reg_first in Wb35Reg_EP0VM_complete() and see that we free
>    it.
>
> Make sure you have cscope configured in vim.  Also in vim the '*'
> button searches.
>
> If you think you have found a bug but you're not sure, feel free to
> ask on kernel janitors.  Probably for new code it's not the right
> mailing list but for code audits it's fine.

I tend to look at other examples nearby.  If other similar functions are
freeing something, then it probably needs to be freed.  If not, then it
might seem more promising to try to figure out why.

julia

Re: [Patch 2/2] Staging: winbond: Memory & urb freed

[Harsh Kumar]

On Friday 31 May 2013 10:16 PM, Julia Lawall wrote:
> On Fri, 31 May 2013, Dan Carpenter wrote:
> 
>> On Fri, May 31, 2013 at 09:48:58PM +0530, Harsh Kumar wrote:
>>>
>>>
>>> On Friday 31 May 2013 09:27 PM, Dan Carpenter wrote:
>>>> On Fri, May 31, 2013 at 09:02:11PM +0530, Harsh Kumar wrote:
>>>>> Memory & urb should be freed before exiting from the function, I think.
>>>>>
>>>>
>>>> They are freed in Wb35Reg_EP0VM_complete() so this patch will make
>>>> the system crash right away.  Btw, there are tons of real bugs that
>>>> I know about but which I don't fix because I don't know what the
>>>> right thing to do is.
>>>>
>>>
>>> Ohh! Sorry, I missed it. That is why I was not sure about submitting this
>>> change. I need to be more thorough in checking this stuff.
>>>
>>> I have general question - Generally, shouldn't allocation and freeing up the
>>> memory be in the same function so that it is easy to make sure that everything
>>> is freed up after completion of task? Is it because there maybe certain cases
>>> where that may not be feasible or desirable to do so?
>>
>> Unfortunately kernel programming will never be 100% easy...  :P  In
>> the end you will learn the tricks just like I did.  Here was my
>> thought process here:
>> 1) The commit message was not convincing.
>> 2) The memory was saved to reg->reg_first.
>> 3) Follow the Wb35Reg_EP0VM_start() call to the urb_submit point.
>> 4) See that we pass Wb35Reg_EP0VM_complete() to urb_submit function.
>>    I have worked on usb drivers before so I suspected that
>>    Wb35Reg_EP0VM_complete() frees the urb.
>> 5) Vefiried that this is true.  Done.
>>
>> Or alternatively:
>> 3) Find reg_first in Wb35Reg_EP0VM_complete() and see that we free
>>    it.
>>
>> Make sure you have cscope configured in vim.  Also in vim the '*'
>> button searches.
>>
>> If you think you have found a bug but you're not sure, feel free to
>> ask on kernel janitors.  Probably for new code it's not the right
>> mailing list but for code audits it's fine.
> 
> I tend to look at other examples nearby.  If other similar functions are
> freeing something, then it probably needs to be freed.  If not, then it
> might seem more promising to try to figure out why.
> 
> julia

Thank you Dan & Julia. This will surely be very helpful.

Harsh