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

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

[Daeseok Youn]

When dgap_tty_init() and dgap_tty_register_ports() are failed,
these are needed to free some memory properly.

It can be handled by calling dgap_tty_uninit() and dgap_cleanup_board().
But tty's ports are not registered yet when these function are failed,
so it need to switch with boolean value whether tty's ports are
registered.

Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
---
V2: remove "brd->nasync = 0" for avoiding to call some destroyer
for unregistering tty's ports.

I'm not sure about add a parameter in dgap_tty_uninit() for checking
whether tty's ports are registered.
please review this. And if I am wrong, let me know.

 drivers/staging/dgap/dgap.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 65d651e..03a3a4b 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -124,7 +124,7 @@ static void dgap_tty_send_xchar(struct tty_struct *tty, char ch);
 
 static int dgap_tty_register(struct board_t *brd);
 static int dgap_tty_init(struct board_t *);
-static void dgap_tty_uninit(struct board_t *);
+static void dgap_tty_uninit(struct board_t *, bool);
 static void dgap_carrier(struct channel_t *ch);
 static void dgap_input(struct channel_t *ch);
 
@@ -620,7 +620,7 @@ static void dgap_cleanup_module(void)
 
 	for (i = 0; i < dgap_numboards; ++i) {
 		dgap_remove_ports_sysfiles(dgap_board[i]);
-		dgap_tty_uninit(dgap_board[i]);
+		dgap_tty_uninit(dgap_board[i], true);
 		dgap_cleanup_board(dgap_board[i]);
 	}
 
@@ -954,19 +954,23 @@ static int dgap_firmware_load(struct pci_dev *pdev, int card_type)
 	 * Do tty device initialization.
 	 */
 	ret = dgap_tty_init(brd);
-	if (ret < 0) {
-		dgap_tty_uninit(brd);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_cleanup;
 
 	ret = dgap_tty_register_ports(brd);
 	if (ret)
-		return ret;
+		goto err_cleanup;
 
 	brd->state = BOARD_READY;
 	brd->dpastatus = BD_RUNNING;
 
 	return 0;
+
+err_cleanup:
+	dgap_tty_uninit(brd, false);
+	dgap_cleanup_board(brd);
+	return ret;
+
 }
 
 /*
@@ -1488,19 +1492,20 @@ static int dgap_tty_init(struct board_t *brd)
  * Uninitialize the TTY portion of this driver.  Free all memory and
  * resources.
  */
-static void dgap_tty_uninit(struct board_t *brd)
+static void dgap_tty_uninit(struct board_t *brd, bool ports_registered)
 {
 	struct device *dev;
-	int i;
+	int i = 0;
 
 	if (brd->dgap_major_serial_registered) {
 		dgap_boards_by_major[brd->serial_driver->major] = NULL;
 		brd->dgap_serial_major = 0;
-		for (i = 0; i < brd->nasync; i++) {
+		while (ports_registered && i < brd->nasync) {
 			tty_port_destroy(&brd->serial_ports[i]);
 			dev = brd->channels[i]->ch_tun.un_sysfs;
 			dgap_remove_tty_sysfs(dev);
 			tty_unregister_device(brd->serial_driver, i);
+			i++;
 		}
 		tty_unregister_driver(brd->serial_driver);
 		put_tty_driver(brd->serial_driver);
@@ -1508,14 +1513,21 @@ static void dgap_tty_uninit(struct board_t *brd)
 		brd->dgap_major_serial_registered = FALSE;
 	}
 
+	/*
+	 * Clear index of tty's port for unregistering
+	 * ports of printer driver
+	 */
+	i = 0;
+
 	if (brd->dgap_major_transparent_print_registered) {
 		dgap_boards_by_major[brd->print_driver->major] = NULL;
 		brd->dgap_transparent_print_major = 0;
-		for (i = 0; i < brd->nasync; i++) {
+		while (ports_registered && i < brd->nasync) {
 			tty_port_destroy(&brd->printer_ports[i]);
 			dev = brd->channels[i]->ch_pun.un_sysfs;
 			dgap_remove_tty_sysfs(dev);
 			tty_unregister_device(brd->print_driver, i);
+			i++;
 		}
 		tty_unregister_driver(brd->print_driver);
 		put_tty_driver(brd->print_driver);
-- 
1.7.1

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

[Dan Carpenter]

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

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@oracle.com>:
> 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
>

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

[Dan Carpenter]

We are talking about different things I think.  What I'm saying is that
there is the normal way to do error handling in the kernel.  That's with
a series of labels like this:

	...
	return 0;

err_free_ttys:
	free_ttys();
err_free_channels:
	free_channels();
err_free_brd:
	free_brd();

	return ret;

In this code there are no if statements unless absolutely needed because
of an matching if statement in the allocation code.  The label names
describe what happens at the label.  It is in reverse order from the way
the variables were allocated.

The other thing is that at the end of dgap_tty_register() we have
unwinding like this.

  1304  unregister_serial_drv:
  1305          tty_unregister_driver(brd->serial_driver);
  1306  free_print_drv:
  1307          put_tty_driver(brd->print_driver);
  1308  free_serial_drv:
  1309          put_tty_driver(brd->serial_driver);

We can add a tty_unregister_driver(brd->print_driver) and create a
dgap_tty_unregister().

static void dgap_tty_unregister(struct board_t *brd)
{
	tty_unregister_driver(brd->print_driver);
	tty_unregister_driver(brd->serial_driver);
	put_tty_driver(brd->print_driver);
	put_tty_driver(brd->serial_driver);
}

Very simple and nice.

If you have one label it makes the code too complicated and it causes
bugs.  We call them "one err bugs" because there is one error label.

In your patch it has:
+       dgap_tty_uninit(brd, false);

But it should only be "false" if dgap_tty_init() failed.  If
dgap_tty_register_ports() fails then it should be "true".  Another
problem is that as you say, the earlier function are allocating
resources like dgap_tty_register() but only the last two function calls
have a "goto err_cleanup;" so the error handling is incomplete.

To be honest, I think once dgap the code is cleaned up this error
handling will be easy to write.  We shouldn't have things like:
brd->dgap_major_serial_registered = TRUE; because we will know which
order things were registered and unregister them in the reverse order.

regards,
dan carpenter

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

[DaeSeok Youn]

Hi, Dan

Please look at below my line comment.

2014-05-28 16:02 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> We are talking about different things I think.  What I'm saying is that
> there is the normal way to do error handling in the kernel.  That's with
> a series of labels like this:
>
>         ...
>         return 0;
>
> err_free_ttys:
>         free_ttys();
> err_free_channels:
>         free_channels();
> err_free_brd:
>         free_brd();
>
>         return ret;
>
> In this code there are no if statements unless absolutely needed because
> of an matching if statement in the allocation code.  The label names
> describe what happens at the label.  It is in reverse order from the way
> the variables were allocated.
>
> The other thing is that at the end of dgap_tty_register() we have
> unwinding like this.
>
>   1304  unregister_serial_drv:
>   1305          tty_unregister_driver(brd->serial_driver);
>   1306  free_print_drv:
>   1307          put_tty_driver(brd->print_driver);
>   1308  free_serial_drv:
>   1309          put_tty_driver(brd->serial_driver);
>
> We can add a tty_unregister_driver(brd->print_driver) and create a
> dgap_tty_unregister().
>
> static void dgap_tty_unregister(struct board_t *brd)
> {
>         tty_unregister_driver(brd->print_driver);
>         tty_unregister_driver(brd->serial_driver);
>         put_tty_driver(brd->print_driver);
>         put_tty_driver(brd->serial_driver);
> }
>
> Very simple and nice.
>
> If you have one label it makes the code too complicated and it causes
> bugs.  We call them "one err bugs" because there is one error label.
>
> In your patch it has:
> +       dgap_tty_uninit(brd, false);
>
> But it should only be "false" if dgap_tty_init() failed.  If
> dgap_tty_register_ports() fails then it should be "true".  Another
Yes, you're right. There were no error handle for tty_port_register_device() and
dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
It need to add error handlers for them, right?

> problem is that as you say, the earlier function are allocating
> resources like dgap_tty_register() but only the last two function calls
> have a "goto err_cleanup;" so the error handling is incomplete.
So remove "goto" in dgap_firmware_load() and add error handler in
dgap_tty_init() and dgap_tty_register_ports(), right?

I have a question of this. In case of this, how to complete the error handling?
I want to cleanup allocated resources from dgap_tty_register() when
dgap_tty_init() is failed.
(and also in case of failure of dgap_tty_register_ports())
I think it can be handled as your previous e-mail.
It can have a label name of "free_chan" in dgap_tty_init() but it also have
a cleanup for allocated resource from earlier functions like
dgap_tty_register().

Can I call some functions for cleanup allocated resource from the
earlier function in dgap_tty_init() with goto label?

>
> To be honest, I think once dgap the code is cleaned up this error
> handling will be easy to write.  We shouldn't have things like:
> brd->dgap_major_serial_registered = TRUE; because we will know which
> order things were registered and unregister them in the reverse order.
OK. I will look at the code with your comment.

Really thanks for kind explanation.
regards,
Daeseok Youn
>
> regards,
> dan carpenter
>

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

[Dan Carpenter]

On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
> > In your patch it has:
> > +       dgap_tty_uninit(brd, false);
> >
> > But it should only be "false" if dgap_tty_init() failed.  If
> > dgap_tty_register_ports() fails then it should be "true".  Another
> Yes, you're right. There were no error handle for tty_port_register_device() and
> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
> It need to add error handlers for them, right?

Eventually, yes.  But I don't see a simple way to fix
dgap_firmware_load() until after the code is cleaned up.

> 
> > problem is that as you say, the earlier function are allocating
> > resources like dgap_tty_register() but only the last two function calls
> > have a "goto err_cleanup;" so the error handling is incomplete.
> So remove "goto" in dgap_firmware_load() and add error handler in
> dgap_tty_init()

In the current code there isn't a goto in dgap_firmware_load().  Remove
the call to dgap_tty_uninit() and add error handling in dgap_tty_init().

That will clean up the code, and fix some NULL dereference bugs inside
dgap_tty_uninit().

> and dgap_tty_register_ports(), right?

Inside dgap_tty_register_ports(), then we should add a
kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
That is not a complete fix, but it is a part fix and it is clean.

> 
> I have a question of this. In case of this, how to complete the error handling?

[patch 1/x] staging: dgap: remove useless dgap_probe1() function
[patch 2/x] staging: dgap: unwind on error in dgap_found_board()
[patch 3/x] staging: dgap: remove bogus null test in dgap_tty_init()
	The ->channels[] were set to null in dgap_found_board().
[patch 4/x] staging: dgap: unwind on error in dgap_tty_init()
	This also removes the call to dgap_tty_uninit() in
	dgap_firmware_load()
[patch 5/x] staging: dgap: unwind on error in dgap_tty_register_ports()
[patch 6/x] staging: dgap: make dgap_config_buf a local buffer
[patch 7/x] staging: dgap: pass "dgap_numboards" to dgap_found_board()
	instead of using a global variable
[patch 8/x] staging: dgap: pass "brd" to dgap_after_config_loaded()
	instead of passing "dgap_numboards" and looking up brd again.
[patch 9/x] staging: dgap: rename dgap_finalize_board_init() to dgap_request_irq()

In the end, I hate dgap_tty_uninit() because it doesn't match
dgap_tty_init() at all.  It's poorly named.  We should rename it and
make another dgap_tty_init() which just sets the ->channels[] to NULL.

[patch x/x] staging: dgap: introduce dgap_tty_unregister()
	This is currently done in dgap_tty_uninit(), which is the wrong
	place.

I have started using a new todo list tag in my emails.  So I'm adding
this stuff to the todo list.

TODO-list: 2014-05-28: dgap: cleanups and bug fixes.

regards,
dan carpenter

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

[DaeSeok Youn]

Hi, Dan.

2014-05-28 19:11 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
>> > In your patch it has:
>> > +       dgap_tty_uninit(brd, false);
>> >
>> > But it should only be "false" if dgap_tty_init() failed.  If
>> > dgap_tty_register_ports() fails then it should be "true".  Another
>> Yes, you're right. There were no error handle for tty_port_register_device() and
>> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
>> It need to add error handlers for them, right?
>
> Eventually, yes.  But I don't see a simple way to fix
> dgap_firmware_load() until after the code is cleaned up.
>
>>
>> > problem is that as you say, the earlier function are allocating
>> > resources like dgap_tty_register() but only the last two function calls
>> > have a "goto err_cleanup;" so the error handling is incomplete.
>> So remove "goto" in dgap_firmware_load() and add error handler in
>> dgap_tty_init()
>
> In the current code there isn't a goto in dgap_firmware_load().  Remove
> the call to dgap_tty_uninit() and add error handling in dgap_tty_init().
Yes. I will try to fix it.
>
> That will clean up the code, and fix some NULL dereference bugs inside
> dgap_tty_uninit().
>
>> and dgap_tty_register_ports(), right?
>
> Inside dgap_tty_register_ports(), then we should add a
> kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
> That is not a complete fix, but it is a part fix and it is clean.
Actually, I sent a patch which is removing "kfree(brd->serial_ports)" and pushed
into staging-next branch.
see the 0ade4a34fd43 staging: dgap: remove unneeded kfree() in
dgap_tty_register_ports()
Because I think dgap_tty_uninit() will free "brd->serial_ports" with this patch.

Can I send a patch after revert "0ade4a34fd43" commit?
>
>>
>> I have a question of this. In case of this, how to complete the error handling?
>
> [patch 1/x] staging: dgap: remove useless dgap_probe1() function
> [patch 2/x] staging: dgap: unwind on error in dgap_found_board()
> [patch 3/x] staging: dgap: remove bogus null test in dgap_tty_init()
>         The ->channels[] were set to null in dgap_found_board().
> [patch 4/x] staging: dgap: unwind on error in dgap_tty_init()
>         This also removes the call to dgap_tty_uninit() in
>         dgap_firmware_load()
> [patch 5/x] staging: dgap: unwind on error in dgap_tty_register_ports()
> [patch 6/x] staging: dgap: make dgap_config_buf a local buffer
> [patch 7/x] staging: dgap: pass "dgap_numboards" to dgap_found_board()
>         instead of using a global variable
> [patch 8/x] staging: dgap: pass "brd" to dgap_after_config_loaded()
>         instead of passing "dgap_numboards" and looking up brd again.
> [patch 9/x] staging: dgap: rename dgap_finalize_board_init() to dgap_request_irq()
>
> In the end, I hate dgap_tty_uninit() because it doesn't match
> dgap_tty_init() at all.  It's poorly named.  We should rename it and
> make another dgap_tty_init() which just sets the ->channels[] to NULL.
>
> [patch x/x] staging: dgap: introduce dgap_tty_unregister()
>         This is currently done in dgap_tty_uninit(), which is the wrong
>         place.
oh.. too many patches for cleanup. :o
Can I send part of these for cleanup?

Thanks.

regards,
Daeseok Youn
>
> I have started using a new todo list tag in my emails.  So I'm adding
> this stuff to the todo list.
>
> TODO-list: 2014-05-28: dgap: cleanups and bug fixes.
>
> regards,
> dan carpenter
>
>

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

[Dan Carpenter]

On Thu, May 29, 2014 at 09:17:09AM +0900, DaeSeok Youn wrote:
> Hi, Dan.
> 
> 2014-05-28 19:11 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
> >> > In your patch it has:
> >> > +       dgap_tty_uninit(brd, false);
> >> >
> >> > But it should only be "false" if dgap_tty_init() failed.  If
> >> > dgap_tty_register_ports() fails then it should be "true".  Another
> >> Yes, you're right. There were no error handle for tty_port_register_device() and
> >> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
> >> It need to add error handlers for them, right?
> >
> > Eventually, yes.  But I don't see a simple way to fix
> > dgap_firmware_load() until after the code is cleaned up.
> >
> >>
> >> > problem is that as you say, the earlier function are allocating
> >> > resources like dgap_tty_register() but only the last two function calls
> >> > have a "goto err_cleanup;" so the error handling is incomplete.
> >> So remove "goto" in dgap_firmware_load() and add error handler in
> >> dgap_tty_init()
> >
> > In the current code there isn't a goto in dgap_firmware_load().  Remove
> > the call to dgap_tty_uninit() and add error handling in dgap_tty_init().
> Yes. I will try to fix it.
> >
> > That will clean up the code, and fix some NULL dereference bugs inside
> > dgap_tty_uninit().
> >
> >> and dgap_tty_register_ports(), right?
> >
> > Inside dgap_tty_register_ports(), then we should add a
> > kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
> > That is not a complete fix, but it is a part fix and it is clean.
> Actually, I sent a patch which is removing "kfree(brd->serial_ports)" and pushed
> into staging-next branch.
> see the 0ade4a34fd43 staging: dgap: remove unneeded kfree() in
> dgap_tty_register_ports()
> Because I think dgap_tty_uninit() will free "brd->serial_ports" with this patch.
> 
> Can I send a patch after revert "0ade4a34fd43" commit?

Oh, crud.  I missed that.  Yeah.  Let's revert it.

regards,
dan carpenter

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

[Greg KH]

On Thu, May 29, 2014 at 09:40:39AM +0300, Dan Carpenter wrote:
> On Thu, May 29, 2014 at 09:17:09AM +0900, DaeSeok Youn wrote:
> > Hi, Dan.
> > 
> > 2014-05-28 19:11 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > > On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
> > >> > In your patch it has:
> > >> > +       dgap_tty_uninit(brd, false);
> > >> >
> > >> > But it should only be "false" if dgap_tty_init() failed.  If
> > >> > dgap_tty_register_ports() fails then it should be "true".  Another
> > >> Yes, you're right. There were no error handle for tty_port_register_device() and
> > >> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
> > >> It need to add error handlers for them, right?
> > >
> > > Eventually, yes.  But I don't see a simple way to fix
> > > dgap_firmware_load() until after the code is cleaned up.
> > >
> > >>
> > >> > problem is that as you say, the earlier function are allocating
> > >> > resources like dgap_tty_register() but only the last two function calls
> > >> > have a "goto err_cleanup;" so the error handling is incomplete.
> > >> So remove "goto" in dgap_firmware_load() and add error handler in
> > >> dgap_tty_init()
> > >
> > > In the current code there isn't a goto in dgap_firmware_load().  Remove
> > > the call to dgap_tty_uninit() and add error handling in dgap_tty_init().
> > Yes. I will try to fix it.
> > >
> > > That will clean up the code, and fix some NULL dereference bugs inside
> > > dgap_tty_uninit().
> > >
> > >> and dgap_tty_register_ports(), right?
> > >
> > > Inside dgap_tty_register_ports(), then we should add a
> > > kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
> > > That is not a complete fix, but it is a part fix and it is clean.
> > Actually, I sent a patch which is removing "kfree(brd->serial_ports)" and pushed
> > into staging-next branch.
> > see the 0ade4a34fd43 staging: dgap: remove unneeded kfree() in
> > dgap_tty_register_ports()
> > Because I think dgap_tty_uninit() will free "brd->serial_ports" with this patch.
> > 
> > Can I send a patch after revert "0ade4a34fd43" commit?
> 
> Oh, crud.  I missed that.  Yeah.  Let's revert it.

Now reverted.

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

[Mark Hounschell]

On 05/28/2014 06:11 AM, Dan Carpenter wrote:
> On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote:
>>> In your patch it has:
>>> +       dgap_tty_uninit(brd, false);
>>>
>>> But it should only be "false" if dgap_tty_init() failed.  If
>>> dgap_tty_register_ports() fails then it should be "true".  Another
>> Yes, you're right. There were no error handle for tty_port_register_device() and
>> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
>> It need to add error handlers for them, right?
> 
> Eventually, yes.  But I don't see a simple way to fix
> dgap_firmware_load() until after the code is cleaned up.
> 
>>
>>> problem is that as you say, the earlier function are allocating
>>> resources like dgap_tty_register() but only the last two function calls
>>> have a "goto err_cleanup;" so the error handling is incomplete.
>> So remove "goto" in dgap_firmware_load() and add error handler in
>> dgap_tty_init()
> 
> In the current code there isn't a goto in dgap_firmware_load().  Remove
> the call to dgap_tty_uninit() and add error handling in dgap_tty_init().
> 
> That will clean up the code, and fix some NULL dereference bugs inside
> dgap_tty_uninit().
> 
>> and dgap_tty_register_ports(), right?
> 
> Inside dgap_tty_register_ports(), then we should add a
> kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails.
> That is not a complete fix, but it is a part fix and it is clean.
> 
>>
>> I have a question of this. In case of this, how to complete the error handling?
> 
> [patch 1/x] staging: dgap: remove useless dgap_probe1() function
> [patch 2/x] staging: dgap: unwind on error in dgap_found_board()
> [patch 3/x] staging: dgap: remove bogus null test in dgap_tty_init()
> 	The ->channels[] were set to null in dgap_found_board().
> [patch 4/x] staging: dgap: unwind on error in dgap_tty_init()
> 	This also removes the call to dgap_tty_uninit() in
> 	dgap_firmware_load()
> [patch 5/x] staging: dgap: unwind on error in dgap_tty_register_ports()
> [patch 6/x] staging: dgap: make dgap_config_buf a local buffer
> [patch 7/x] staging: dgap: pass "dgap_numboards" to dgap_found_board()
> 	instead of using a global variable
> [patch 8/x] staging: dgap: pass "brd" to dgap_after_config_loaded()
> 	instead of passing "dgap_numboards" and looking up brd again.
> [patch 9/x] staging: dgap: rename dgap_finalize_board_init() to dgap_request_irq()
> 
> In the end, I hate dgap_tty_uninit() because it doesn't match
> dgap_tty_init() at all.  It's poorly named.  We should rename it and
> make another dgap_tty_init() which just sets the ->channels[] to NULL.
> 
> [patch x/x] staging: dgap: introduce dgap_tty_unregister()
> 	This is currently done in dgap_tty_uninit(), which is the wrong
> 	place.
> 
> I have started using a new todo list tag in my emails.  So I'm adding
> this stuff to the todo list.
> 
> TODO-list: 2014-05-28: dgap: cleanups and bug fixes.
> 

I can think of some more things that could be added to that TODO-list.

All the configuration file stuff needs to go away. As Greg previously
said, we don't read configurations files in kernel modules. I'm pretty
sure all cards supported have unique device IDs, even though notes and
code in this driver imply otherwise. As long as they all do, with the
exception of those cards that use port extenders, I think this could be
done. Maybe we will never be able to support "port extenders". Unclear
yet. When we do this, the useintr and altpin configuration file stuff
will need converted to module options.

There was also some ioctl code removed entirely before the driver was
merged into staging. It was specific to the dgap_mgmt device (currently
dead as a result) that allowed userland some real-time monitoring and
configuration changes. These userland utilities were part of the
original Digi package and I'm sure are used by some. I personally never
used them, but...

Then there are some things you recommended in a previous email that I
haven't got to yet?

Is that TODO-list going to be commited?

Regards
Mark

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

[Dan Carpenter]

On Wed, May 28, 2014 at 10:14:03AM -0400, Mark Hounschell wrote:
> Is that TODO-list going to be commited?

The TODO-list is a new thing.  People are always complaining that there
isn't a TODO-list with task for people to do.  Now I can just search my
inbox and generate one automatically.  Fancy, huh?  :)

regards,
dan carpenter