Staging: winbond: Memory & urb freed
Subsystems (2)
PATCH 2/2 Staging: winbond: Memory & urb freed
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]
--- drivers/staging/winbond/wb35reg.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
drivers/staging/winbond/wb35reg.c +12 -0
@@ -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; }
Quoted 2 lines · Harsh Kumar
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.
Quoted 2 lines
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.
Quoted 3 lines
Signed-off-by Harsh Kumar <[email protected]>---
Side comments not intended for the perment git log go here.
Quoted 3 lines
drivers/staging/winbond/wb35reg.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
regards,
dan carpenter
Quoted 9 lines · Dan Carpenter
Quoted 2 lines · Harsh Kumar
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
Quoted 13 lines · Harsh Kumar
Quoted 9 lines · Dan Carpenter
Quoted 2 lines · Harsh Kumar
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.
Quoted 7 lines
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
Quoted 37 lines · Dan Carpenter
Quoted 13 lines · Harsh Kumar
Quoted 9 lines · Dan Carpenter
Quoted 2 lines · Harsh Kumar
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.Quoted 7 lines
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 toreg->reg_first.
3) Follow theWb35Reg_EP0VM_start()call to theurb_submitpoint.
4) See that we passWb35Reg_EP0VM_complete()tourb_submitfunction. I have worked on usb drivers before so I suspected thatWb35Reg_EP0VM_complete()frees the urb.
5) Vefiried that this is true. Done.Or alternatively:
3) Findreg_firstinWb35Reg_EP0VM_complete()and see that we free it.
Quoted 6 lines
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
Quoted 46 lines · Julia Lawall
Quoted 37 lines · Dan Carpenter
Quoted 13 lines · Harsh Kumar
Quoted 9 lines · Dan Carpenter
Quoted 2 lines · Harsh Kumar
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.Quoted 7 lines
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 toreg->reg_first.
3) Follow theWb35Reg_EP0VM_start()call to theurb_submitpoint.
4) See that we passWb35Reg_EP0VM_complete()tourb_submitfunction. I have worked on usb drivers before so I suspected thatWb35Reg_EP0VM_complete()frees the urb.
5) Vefiried that this is true. Done.Or alternatively:
3) Findreg_firstinWb35Reg_EP0VM_complete()and see that we free it.Quoted 6 lines
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.
Quoted 5 lines
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