Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()

[DaeSeok Youn]

Hi, Dan.

2014-05-27 19:20 GMT+09:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> 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.
Yes, I knew dgap_tty_init() do only allocate some memory for channel.
But if one of function in dgap_firmware_load() is failed, it seems to
need all of resources in board.
Calling functions before calling dgap_tty_init() has memory
allocations and registration of tty related so when dgap_tty_init()
failed, all of resource in a board need to free and unregister.

And also, in dgap_cleanup_board() will free channels, so I called
dgap_cleanup_board() in err_cleanup label.

I understood your explanation but I didn't get why dgap_tty_uninit()
is not needed. Because If dgap_tty_init() is failed, and then
dgap_firmware_load() returns failure. That means it need to cleanup
previous allocation or registration before dgap_tty_init() is called.
example, dgap_tty_register() function.

If I'm wrong, please let me know.
Thanks.

regards,
Daeseok Youn.
>
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/