2014-09-25 16:50 GMT+09:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
> On Thu, Sep 25, 2014 at 09:26:47AM +0900, DaeSeok Youn wrote:
>> Hi,
>>
>> 2014-09-24 18:45 GMT+09:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
>> > On Tue, Sep 23, 2014 at 09:22:36AM +0900, Daeseok Youn wrote:
>> >> static void dgap_release_remap(struct board_t *brd)
>> >> {
>> >> - release_mem_region(brd->membase, 0x200000);
>> >> - release_mem_region(brd->membase + PCI_IO_OFFSET, 0x200000);
>> >> - iounmap(brd->re_map_membase);
>> >> - iounmap(brd->re_map_port);
>> >> + if (brd->re_map_membase) {
>> >
>> > I hate this if conditions. Let's not complicate dgap_release_remap(),
>> > let's fix the callers instead. Especially dgap_init_module() is a
>> > totally sucky function with bad unwinding.
>> ok. then,
>> if (brd->re_map_membase && brd->re_map_port)
>> dgap_release_remap(brd);
>>
>> right?
>
> What I'm saying is there should be no if statement because we should
> only call this function after the memory has been remapped so there
> should be no if conditions in this function at all.
OK. I got this. remove "if" statement in dgap_release_remap().
>
> The problem is that dgap_init_module() doesn't unwind, it just tries to
> free every variable without considering whether or not it was allocated.
> It's the "One Err" approach to error handling where you just have one
> error label that calls free on every single thing in the world. It
> *should* look more like this:
>
> static int dgap_init_module(void)
> {
> int rc;
>
> pr_info("%s, Digi International Part Number %s\n", DG_NAME, DG_PART);
>
> rc = dgap_start();
> if (rc)
> return rc;
>
> rc = pci_register_driver(&dgap_driver);
> if (rc)
> goto err_stop;
>
> rc = dgap_create_driver_sysfiles(&dgap_driver);
> if (rc)
> goto err_unregister;
>
> dgap_driver_state = DRIVER_READY;
>
> return 0;
>
> err_unregister: // <- note that the label names reflect where the label
> // is not where the goto is. dgap_start() uses
> // wrong/confusing label names.
> pci_unregister_driver(&dgap_driver);
> err_stop:
> dgap_stop(); // <- mirror image of dgap_start();
>
> return rc;
> }
>
> dgap_stop() is easy to write because it's based on the error handling
> in dgap_start().
>
> static void dgap_start(void)
> {
> spin_lock_irqsave(&dgap_poll_lock, lock_flags);
> dgap_poll_stop = 1;
> spin_unlock_irqrestore(&dgap_poll_lock, lock_flags);
>
> del_timer_sync(&dgap_poll_timer);
>
> device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
> class_destroy(dgap_class);
> unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
> }
>
> The existing dgap_init_module() "free every single thing in the entire
> universe" approach is buggy because it tries to delete the sysfs files
> before they have been allocated. This is called a "One Err Bug" because
> having just one error label makes the code too complicated for anyone to
> understand. You have to add all kinds of if conditions everywhere and
> it's a mess.
thanks for kind explanation.
I will send patches as your comments.
regards,
Daeseok Youn.
>
> regards,
> dan carpenter
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
