On Fri, 2014-02-21 at 15:38:56 +0800, Dan Carpenter wrote:
> I don't think this is the right thing because it is needless code.
> Overall it doesn't really simplify anything.
>
> You are worried that reviewers will be confused and think there is a
> leak in et131x_rx_dma_memory_alloc() and then add a kfree() which breaks
> the code. [...]
AFAIK, this doesn't break the code. the relevant code in
et131x_rx_dma_memory_free() is:
/* Free the FBR Lookup Table */
kfree(rx_ring->fbr[0]);
kfree(rx_ring->fbr[1]);
By setting rx->fbr[0] to NULL after freeing rx->fbr[0] in
et131x_rx_dma_memory_alloc(), et131x_rx_dma_memory_free() won't free
rx->fbr[0] twice, and kree(NULL) is legal.
> I think you are right that a reviewer would initially wonder
> why the code does not call kfree() but then you do a search for fb[0]
> and it becomes obvious.
The reviewer won't want to search after adding the kfree(). :)
>
> If we add your patch and the reviewer does a search for fb[0] then it is
> confusing what the right thing to do is.
My fault. I should remove that two lines of code in
et131x_rx_dma_memory_free(), although they don't break the code.
>
> These kinds of "free later" code, are not the most common way to do
> things in the kernel but they are used other places as well.
When the freeing code is huge and will let the real code logic not so
clear, it's OK to do things that way. I don't want to eliminate them at
all. My change just let a piece of memory "freed earlier", to make the
code a little more readable.
I doubt no one except me care about this, and it doesn't need the effort
to send a v2 about this(remove two lines of code in et131x_rx_dma_memory_free)?
>
> regards,
> dan carpenter
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
