Re: [PATCH v2 2/3] staging: dgnc: driver.c and tty.c: replaces dgnc_driver_kzmalloc with kzalloc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 27, 2013 at 10:13:27PM -0400, Lidza Louina wrote:
> @@ -501,7 +501,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
>  
>  	/* get the board structure and prep it */
>  	brd = dgnc_Board[dgnc_NumBoards] =
> -	(struct board_t *) dgnc_driver_kzmalloc(sizeof(struct board_t), GFP_KERNEL);
> +	(struct board_t *) kzalloc(sizeof(struct board_t), GFP_KERNEL);
>  	if (!brd) {
>  		APR(("memory allocation for board structure failed\n"));
>  		return(-ENOMEM);

So you didn't introduce this, but here are the style problems in this
section, in case you want to fix them in a later patch.

1) Bad indenting.
2) Unneeded casting.
3) Use sizeof(*brd) instead of sizeof(struct board_t).
4) board_t is a bad and wrong name for this data type.  "board" is too
   generic, and "_t" means "typedef" but this is a not a typedef.
5) Put the two assignments on two lines.  First assign "brd" and then
   initialize "dgnc_Board[dgnc_NumBoards]" after allocating
   "brd->msgbuf".
6) Comment is obvious and at the same time wrong.  It means "allocate
   the board structure" instead of "get".  Delete.
7) kmalloc() has its own error message which is much more useful.  No
   need to print a useless error message.  Also this error will never
   occur in real life so adding code for something which will never
   happen and it's a waste of time for people reading the code.
8) No parenthesis around the return.

> @@ -509,7 +509,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
>  
>  	/* make a temporary message buffer for the boot messages */
>  	brd->msgbuf = brd->msgbuf_head =
> -		(char *) dgnc_driver_kzmalloc(sizeof(char) * 8192, GFP_KERNEL);
> +		(char *) kzalloc(sizeof(char) * 8192, GFP_KERNEL);
>  	if (!brd->msgbuf) {
>  		kfree(brd);
>  		APR(("memory allocation for board msgbuf failed\n"));

9)  I think we know the sizeof(char)...  If we want to keep that then
    use kcalloc() instead of kzalloc().  But it's simplest to just say:

    brd->msgbuf = kzalloc(8192, GFP_KERNEL);

10) The error handling should be:

	if (!brd->msgbuf) {
		ret = -ENOMEM;
		goto err_free_brd;
	}

	[snip code in the middle]

	return 0;

err_free_brd:
	kfree(brd);

	return ret;

    We leak memory later on in this function...

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux