[PATCH 5/6] staging: et131x: Replace kmem_cache use with plain kmalloc/kfree

Dan Carpenter dan.carpenter at oracle.com
Thu Nov 15 13:50:01 UTC 2012


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




More information about the devel mailing list