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
