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

[Mark Hounschell <markh@compro.net>]

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