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

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

[Daeseok Youn]

Just simply use dgap_release_remap() in dgap_cleanup_board() for
releasing map memory.

Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
---
 drivers/staging/dgap/dgap.c |   25 ++++++++++---------------
 1 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 637ea8a..d0be1ce 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -683,17 +683,7 @@ static void dgap_cleanup_board(struct board_t *brd)
 
 	tasklet_kill(&brd->helper_tasklet);
 
-	if (brd->re_map_port) {
-		release_mem_region(brd->membase + 0x200000, 0x200000);
-		iounmap(brd->re_map_port);
-		brd->re_map_port = NULL;
-	}
-
-	if (brd->re_map_membase) {
-		release_mem_region(brd->membase, 0x200000);
-		iounmap(brd->re_map_membase);
-		brd->re_map_membase = NULL;
-	}
+	dgap_release_remap(brd);
 
 	/* Free all allocated channels structs */
 	for (i = 0; i < MAXPORTS ; i++)
@@ -1023,10 +1013,15 @@ static int dgap_do_remap(struct board_t *brd)
 
 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) {
+		release_mem_region(brd->membase, 0x200000);
+		iounmap(brd->re_map_membase);
+	}
+
+	if (brd->re_map_port) {
+		release_mem_region(brd->membase + PCI_IO_OFFSET, 0x200000);
+		iounmap(brd->re_map_port);
+	}
 }
 /*****************************************************************************
 *
-- 
1.7.1

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

[Dan Carpenter]

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.

Let's not apply this patch.

regards,
dan carpenter

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

[DaeSeok Youn]

Hi,

2014-09-24 18:45 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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?
>
> Let's not apply this patch.
oh.. this patch was already applied to staging tree.
I will make different patch for this.

thanks.

regards,
Daeseok Youn.
>
> regards,
> dan carpenter
>

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

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

[DaeSeok Youn]

2014-09-25 16:50 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> 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@oracle.com>:
>> > 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
>