The "brd->nasync = 0;" was wrong, yes, but my main complaint was that
you are writing complicated error handling. This v2 patch makes the
error handling even more complicated. If dgap_tty_init() fails it
should free the things it allocates itself, instead of the caller
handling errors for it.
It's not actually that hard. The only error handling we need to do in
dgap_tty_init() is if the kzalloc() fails:
1374 /*
1375 * Allocate channel memory that might not have been allocated
1376 * when the driver was first loaded.
1377 */
1378 for (i = 0; i < brd->nasync; i++) {
1379 if (!brd->channels[i]) {
1380 brd->channels[i] =
1381 kzalloc(sizeof(struct channel_t), GFP_KERNEL);
1382 if (!brd->channels[i])
1383 return -ENOMEM;
Instead of returning directly here we should free the previous
allocations.
1384 }
1385 }
The code is confusing because which ones did we allocate and which ones
were already non-NULL at the start of the function? In other words
the "if (!brd->channels[i]) {" test?
The answer is that the comment and the test seem to be wrong they were
all NULL at the start of the function. Just add a:
free_chan:
while (--i >= 0) {
kfree(brd->channels[i]);
brd->channels[i] = NULL;
}
return ret;
Actually, for these I would introduce an "int chan" variable just for
that loop instead of "i" which we re-use.
So then we remove the call to dgap_tty_uninit() from
dgap_firmware_load().
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
