Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 : > > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 : > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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.carpen...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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.carpen...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
Hi, Dan. 2014-05-28 19:11 GMT+09:00 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(). 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
Hi, Dan
Please look at below my line comment.
2014-05-28 16:02 GMT+09:00 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
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
Hi, Dan. 2014-05-28 19:11 GMT+09:00 Dan Carpenter dan.carpen...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
Hi, Dan
Please look at below my line comment.
2014-05-28 16:02 GMT+09:00 Dan Carpenter dan.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
Hi, Dan.
2014-05-27 19:20 GMT+09:00 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.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load()
Hi, Dan.
2014-05-27 19:20 GMT+09:00 Dan Carpenter dan.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
