Re: [PATCH 3/3] staging: dgap: use dgap_release_remap() in dgap_cleanup_board()

[Dan Carpenter]

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.

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.

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/