On Thu, Nov 15, 2012 at 01:27:20PM +0000, Mark Einon wrote:
> for (rfdct = 0; rfdct < rx_ring->num_rfd; rfdct++) {
> - rfd = kmem_cache_alloc(rx_ring->recv_lookaside,
> - GFP_ATOMIC | GFP_DMA);
> + rfd = kcalloc(1, sizeof(struct rfd), GFP_ATOMIC | GFP_DMA);
Why not:
rfd = kzalloc(sizeof(*rfd), GFP_ATOMIC | GFP_DMA);
>
> if (!rfd) {
> - dev_err(&adapter->pdev->dev,
> - "Couldn't alloc RFD out of kmem_cache\n");
> + dev_err(&adapter->pdev->dev, "Couldn't alloc RFD\n");
> status = -ENOMEM;
> continue;
This continue here is a funny thing. I think we should bail out
instead of continuing.
> }
> @@ -2543,8 +2516,8 @@ static int et131x_init_recv(struct et131x_adapter *adapter)
>
> rx_ring->num_rfd = numrfd;
>
> - if (status != 0) {
> - kmem_cache_free(rx_ring->recv_lookaside, rfd);
> + if (status) {
> + kfree(rfd);
This only frees one rfd but we allocated several in a loop.
Checking "if (status)" is the same as checking:
if (numrfd <= NIC_MIN_NUM_RFD)
It's more readable to get rid of the status variable and check
against the minimum directly.
It's strange to me that NIC_MIN_NUM_RFD is one less than the minimum
number of rfds. Off by one?
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
