[PATCH 1/6] staging: dgap: driver.c: removes smatch warning "redundant null check"

[PATCH 1/6] staging: dgap: driver.c: removes smatch warning "redundant null check"

[Lidza Louina]

This patch removes these smatch warnings from dgap_driver.c:

redundant null check on dgap_config_buf calling kfree()
redundant null check on brd->flipbuf calling kfree()
redundant null check on brd->flipflagbuf calling kfree()

Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
---
 drivers/staging/dgap/dgap_driver.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/dgap/dgap_driver.c b/drivers/staging/dgap/dgap_driver.c
index 65d7ee0..c0dd119 100644
--- a/drivers/staging/dgap/dgap_driver.c
+++ b/drivers/staging/dgap/dgap_driver.c
@@ -416,8 +416,7 @@ void dgap_cleanup_module(void)
 		unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
 	}
 
-	if (dgap_config_buf)
-		kfree(dgap_config_buf);
+	kfree(dgap_config_buf);
 
 	for (i = 0; i < dgap_NumBoards; ++i) {
 		dgap_remove_ports_sysfiles(dgap_Board[i]);
@@ -484,10 +483,8 @@ static void dgap_cleanup_board(struct board_t *brd)
 		}
 	}
 
-	if (brd->flipbuf)
-		kfree(brd->flipbuf);
-	if (brd->flipflagbuf)
-		kfree(brd->flipflagbuf);
+	kfree(brd->flipbuf);
+	kfree(brd->flipflagbuf);
 
 	dgap_Board[brd->boardnum] = NULL;
 
-- 
1.8.1.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/6] staging: dgap: fep5.c: removes smatch warning "missing break? reassigning 'ch->pscan_state'"

[Lidza Louina]

This patch removes this smatch warning:
warn: missing break? reassigning 'ch->pscan_state'

Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
---
 drivers/staging/dgap/dgap_fep5.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dgap/dgap_fep5.c b/drivers/staging/dgap/dgap_fep5.c
index 4464f02..29de349 100644
--- a/drivers/staging/dgap/dgap_fep5.c
+++ b/drivers/staging/dgap/dgap_fep5.c
@@ -1587,10 +1587,6 @@ void dgap_parity_scan(struct channel_t *ch, unsigned char *cbuf, unsigned char *
 	while (l--) {
 		c = *in++;
 		switch (ch->pscan_state) {
-		default:
-			/* reset to sanity and fall through */
-			ch->pscan_state = 0;
-
 		case 0:
 			/* No FF seen yet */
 			if (c == (unsigned char) '\377') {
@@ -1642,6 +1638,11 @@ void dgap_parity_scan(struct channel_t *ch, unsigned char *cbuf, unsigned char *
 
 			count += 1;
 			ch->pscan_state = 0;
+			break;
+		default:
+			/* reset to sanity and fall through */
+			ch->pscan_state = 0;
+			break;
 		}       
 	}
 	*len = count;
-- 
1.8.1.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/6] staging: dgap: tty.c: removes smatch warning "ignoring unreachable code"

[Lidza Louina]

This patch removes this smatch warning:
info: ignoring unreachable code.

There were instances where there was extra code after
the default action in switch statements. These default
actions ended with a break so the code wasn't being run
at anytime. This patch removes that extra code.

Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
---
 drivers/staging/dgap/dgap_tty.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
index 2b26152..7f909b8 100644
--- a/drivers/staging/dgap/dgap_tty.c
+++ b/drivers/staging/dgap/dgap_tty.c
@@ -3513,10 +3513,6 @@ static int dgap_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
 			return(-EINVAL);
 		}
 
-		DGAP_UNLOCK(ch->ch_lock, lock_flags2);
-		DGAP_UNLOCK(bd->bd_lock, lock_flags);
-		return(-ENOIOCTLCMD);
-
 	case DIGI_GETA:
 		/* get information for ditty */
 		DGAP_UNLOCK(ch->ch_lock, lock_flags2);
@@ -3586,12 +3582,4 @@ static int dgap_tty_ioctl(struct tty_struct *tty, unsigned int cmd,
 
 		return(-ENOIOCTLCMD);
 	}
-
-	DGAP_UNLOCK(ch->ch_lock, lock_flags2);
-	DGAP_UNLOCK(bd->bd_lock, lock_flags);
-
-	DPR_IOCTL(("dgap_tty_ioctl end - cmd %s (%x), arg %lx\n", 
-		dgap_ioctl_name(cmd), cmd, arg));
-                        
-	return(0);
 }
-- 
1.8.1.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/6] staging: dgap: tty.c: removes smatch warnings "redundant null check"

[Lidza Louina]

This patch removes these smatch warnings:
redundant null check on dgap_TmpWriteBuf calling kfree()
redundant null check on brd->SerialDriver->ttys calling kfree()
redundant null check on brd->PrintDriver->ttys calling kfree()

The code checked to see if these variables are null
before freeing. This check isn't needed.

Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
---
 drivers/staging/dgap/dgap_tty.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
index 7f909b8..924e2bf 100644
--- a/drivers/staging/dgap/dgap_tty.c
+++ b/drivers/staging/dgap/dgap_tty.c
@@ -497,10 +497,8 @@ int dgap_tty_init(struct board_t *brd)
  */
 void dgap_tty_post_uninit(void)
 {
-	if (dgap_TmpWriteBuf) {
-		kfree(dgap_TmpWriteBuf);
-		dgap_TmpWriteBuf = NULL;
-	}
+	kfree(dgap_TmpWriteBuf);
+	dgap_TmpWriteBuf = NULL;
 }
 
 
@@ -522,10 +520,8 @@ void dgap_tty_uninit(struct board_t *brd)
 			tty_unregister_device(brd->SerialDriver, i);
 		}
 		tty_unregister_driver(brd->SerialDriver);
-		if (brd->SerialDriver->ttys) {
-			kfree(brd->SerialDriver->ttys);
-			brd->SerialDriver->ttys = NULL;
-		}
+		kfree(brd->SerialDriver->ttys);
+		brd->SerialDriver->ttys = NULL;
 		put_tty_driver(brd->SerialDriver);
 		brd->dgap_Major_Serial_Registered = FALSE;
 	}
@@ -538,10 +534,8 @@ void dgap_tty_uninit(struct board_t *brd)
 			tty_unregister_device(brd->PrintDriver, i);
 		}
 		tty_unregister_driver(brd->PrintDriver);
-		if (brd->PrintDriver->ttys) {
-			kfree(brd->PrintDriver->ttys);
-			brd->PrintDriver->ttys = NULL;
-	        }
+		kfree(brd->PrintDriver->ttys);
+		brd->PrintDriver->ttys = NULL;
 		put_tty_driver(brd->PrintDriver);
 		brd->dgap_Major_TransparentPrint_Registered = FALSE;
 	}
-- 
1.8.1.2

[PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Lidza Louina]

This patch removes these warnings:
potential null dereference 'brd->SerialDriver'. (alloc_tty_driver returns null)
potential null dereference 'brd->PrintDriver'. (alloc_tty_driver returns null)

This warning popped up because there wasn't a check
to make sure that the serial and print drivers were
allocated and not null before being initialized. This
patch adds that check.

Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
---
 drivers/staging/dgap/dgap_tty.c | 103 +++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 49 deletions(-)

diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
index 924e2bf..8f0a824 100644
--- a/drivers/staging/dgap/dgap_tty.c
+++ b/drivers/staging/dgap/dgap_tty.c
@@ -220,66 +220,71 @@ int dgap_tty_register(struct board_t *brd)
 	DPR_INIT(("tty_register start"));
 
 	brd->SerialDriver = alloc_tty_driver(MAXPORTS);
+	if(brd->SerialDriver){
+		snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgap_%d_", brd->boardnum);
+		brd->SerialDriver->name = brd->SerialName;
+		brd->SerialDriver->name_base = 0;
+		brd->SerialDriver->major = 0;
+		brd->SerialDriver->minor_start = 0;
+		brd->SerialDriver->type = TTY_DRIVER_TYPE_SERIAL; 
+		brd->SerialDriver->subtype = SERIAL_TYPE_NORMAL;   
+		brd->SerialDriver->init_termios = DgapDefaultTermios;
+		brd->SerialDriver->driver_name = DRVSTR;
+		brd->SerialDriver->flags = (TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_HARDWARE_BREAK);
+
+		/* The kernel wants space to store pointers to tty_structs */
+		brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
+		if (!brd->SerialDriver->ttys)
+			return(-ENOMEM);
+
+	#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
+		brd->SerialDriver->refcount = brd->TtyRefCnt;
+	#endif
 
-	snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgap_%d_", brd->boardnum);
-	brd->SerialDriver->name = brd->SerialName;
-	brd->SerialDriver->name_base = 0;
-	brd->SerialDriver->major = 0;
-	brd->SerialDriver->minor_start = 0;
-	brd->SerialDriver->type = TTY_DRIVER_TYPE_SERIAL; 
-	brd->SerialDriver->subtype = SERIAL_TYPE_NORMAL;   
-	brd->SerialDriver->init_termios = DgapDefaultTermios;
-	brd->SerialDriver->driver_name = DRVSTR;
-	brd->SerialDriver->flags = (TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_HARDWARE_BREAK);
-
-	/* The kernel wants space to store pointers to tty_structs */
-	brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
-	if (!brd->SerialDriver->ttys)
+		/*
+		 * Entry points for driver.  Called by the kernel from
+		 * tty_io.c and n_tty.c.
+		 */
+		tty_set_operations(brd->SerialDriver, &dgap_tty_ops);
+	}
+	else
 		return(-ENOMEM);
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
-	brd->SerialDriver->refcount = brd->TtyRefCnt;
-#endif
-
-	/*
-	 * Entry points for driver.  Called by the kernel from
-	 * tty_io.c and n_tty.c.
-	 */
-	tty_set_operations(brd->SerialDriver, &dgap_tty_ops);
-
 	/*
 	 * If we're doing transparent print, we have to do all of the above
 	 * again, separately so we don't get the LD confused about what major
 	 * we are when we get into the dgap_tty_open() routine.
 	 */
 	brd->PrintDriver = alloc_tty_driver(MAXPORTS);
+	if(brd->PrintDriver){
+		snprintf(brd->PrintName, MAXTTYNAMELEN, "pr_dgap_%d_", brd->boardnum);
+		brd->PrintDriver->name = brd->PrintName;
+		brd->PrintDriver->name_base = 0;
+		brd->PrintDriver->major = 0;
+		brd->PrintDriver->minor_start = 0;
+		brd->PrintDriver->type = TTY_DRIVER_TYPE_SERIAL;   
+		brd->PrintDriver->subtype = SERIAL_TYPE_NORMAL;
+		brd->PrintDriver->init_termios = DgapDefaultTermios;
+		brd->PrintDriver->driver_name = DRVSTR;
+		brd->PrintDriver->flags = (TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_HARDWARE_BREAK);
+
+		/* The kernel wants space to store pointers to tty_structs */
+		brd->PrintDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
+		if (!brd->PrintDriver->ttys)
+			return(-ENOMEM);
+
+	#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
+		brd->PrintDriver->refcount = brd->TtyRefCnt;
+	#endif
 
-	snprintf(brd->PrintName, MAXTTYNAMELEN, "pr_dgap_%d_", brd->boardnum);
-	brd->PrintDriver->name = brd->PrintName;
-	brd->PrintDriver->name_base = 0;
-	brd->PrintDriver->major = 0;
-	brd->PrintDriver->minor_start = 0;
-	brd->PrintDriver->type = TTY_DRIVER_TYPE_SERIAL;   
-	brd->PrintDriver->subtype = SERIAL_TYPE_NORMAL;
-	brd->PrintDriver->init_termios = DgapDefaultTermios;
-	brd->PrintDriver->driver_name = DRVSTR;
-	brd->PrintDriver->flags = (TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV | TTY_DRIVER_HARDWARE_BREAK);
-
-	/* The kernel wants space to store pointers to tty_structs */
-	brd->PrintDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
-	if (!brd->PrintDriver->ttys)
+		/*
+		 * Entry points for driver.  Called by the kernel from
+		 * tty_io.c and n_tty.c.
+		 */
+		tty_set_operations(brd->PrintDriver, &dgap_tty_ops);
+	}
+	else
 		return(-ENOMEM);
-
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
-	brd->PrintDriver->refcount = brd->TtyRefCnt;
-#endif
-
-	/*
-	 * Entry points for driver.  Called by the kernel from
-	 * tty_io.c and n_tty.c.
-	 */
-	tty_set_operations(brd->PrintDriver, &dgap_tty_ops);
-
 	if (!brd->dgap_Major_Serial_Registered) {
 		/* Register tty devices */
 		rc = tty_register_driver(brd->SerialDriver);
-- 
1.8.1.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 6/6] staging: dgap: tty.c: removes smatch warning "unsigned '--un->un_open_count' is never less than zero"

[Lidza Louina]

This patch removes this smatch warning:
unsigned '--un->un_open_count' is never less than zero

The code decremented the un_open_count variable
and tested to see if it was less than zero. Because
un_open_count is unsigned and can't be below zero,
this test doesn't work. This patch tests
un_open_count against 0 without decrementing it.

Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
---
 drivers/staging/dgap/dgap_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
index 8f0a824..f496710 100644
--- a/drivers/staging/dgap/dgap_tty.c
+++ b/drivers/staging/dgap/dgap_tty.c
@@ -1442,7 +1442,7 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
 		un->un_open_count = 1;
 	}  
 
-	if (--un->un_open_count < 0) {
+	if (un->un_open_count == 0) {
 		APR(("bad serial port open count of %d\n", un->un_open_count));
 		un->un_open_count = 0;
 	}
-- 
1.8.1.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: dgap: tty.c: removes smatch warning "unsigned '--un->un_open_count' is never less than zero"

[Dan Carpenter]

On Mon, Sep 23, 2013 at 06:47:17PM -0400, Lidza Louina wrote:
> This patch removes this smatch warning:
> unsigned '--un->un_open_count' is never less than zero
> 
> The code decremented the un_open_count variable
> and tested to see if it was less than zero. Because
> un_open_count is unsigned and can't be below zero,
> this test doesn't work. This patch tests
> un_open_count against 0 without decrementing it.
> 
> Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
> ---
>  drivers/staging/dgap/dgap_tty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
> index 8f0a824..f496710 100644
> --- a/drivers/staging/dgap/dgap_tty.c
> +++ b/drivers/staging/dgap/dgap_tty.c
> @@ -1442,7 +1442,7 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
>  		un->un_open_count = 1;
>  	}  
>  
> -	if (--un->un_open_count < 0) {
> +	if (un->un_open_count == 0) {
>  		APR(("bad serial port open count of %d\n", un->un_open_count));
>  		un->un_open_count = 0;
>  	}

This fix isn't right.  We still need the decrement.  Probably the best
thing is to audit all the driver and make sure that un->un_open_count
can never be == 0.  But the next best, and still totally reasonable
thing to do is this:

	if (un->un_open_count == 0) {
		APR(("bad serial port open count of %d\n", un->un_open_count));
		un->un_open_count = 1;
	}
	un->un_open_count--;

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/6] staging: dgap: fep5.c: removes smatch warning "missing break? reassigning 'ch->pscan_state'"

[Dan Carpenter]

On Mon, Sep 23, 2013 at 06:47:13PM -0400, Lidza Louina wrote:
> This patch removes this smatch warning:
> warn: missing break? reassigning 'ch->pscan_state'

I would just leave this one as is.

With Smatch, I don't put a lot of effort into cutting down false
positives.  Eventually I do plan to parse the file and grep for "fall"
on the lines before the case statement.  That will silence most of the
missing break false positives.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/6] staging: dgap: fep5.c: removes smatch warning "missing break? reassigning 'ch->pscan_state'"

[Dan Carpenter]

On Tue, Sep 24, 2013 at 03:06:06AM +0300, Dan Carpenter wrote:
> On Mon, Sep 23, 2013 at 06:47:13PM -0400, Lidza Louina wrote:
> > This patch removes this smatch warning:
> > warn: missing break? reassigning 'ch->pscan_state'
> 
> I would just leave this one as is.
> 

Yes.  Actually, the fix isn't right.  Let's drop this one.

regards,
dan carpenter

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Dan Carpenter]

On Mon, Sep 23, 2013 at 06:47:16PM -0400, Lidza Louina wrote:
> This patch removes these warnings:
> potential null dereference 'brd->SerialDriver'. (alloc_tty_driver returns null)
> potential null dereference 'brd->PrintDriver'. (alloc_tty_driver returns null)
> 
> This warning popped up because there wasn't a check
> to make sure that the serial and print drivers were
> allocated and not null before being initialized. This
> patch adds that check.
> 
> Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
> ---
>  drivers/staging/dgap/dgap_tty.c | 103 +++++++++++++++++++++-------------------
>  1 file changed, 54 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
> index 924e2bf..8f0a824 100644
> --- a/drivers/staging/dgap/dgap_tty.c
> +++ b/drivers/staging/dgap/dgap_tty.c
> @@ -220,66 +220,71 @@ int dgap_tty_register(struct board_t *brd)
>  	DPR_INIT(("tty_register start"));
>  
>  	brd->SerialDriver = alloc_tty_driver(MAXPORTS);
> +	if(brd->SerialDriver){

Don't do it that way, flip it around.

	brd->SerialDriver = alloc_tty_driver(MAXPORTS);
	if (!brd->SerialDriver)
		return -ENOMEM;

	snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgap_%d_", brd->boardnum);
	brd->SerialDriver->name = brd->SerialName;

That way has fewer indents.

When you're writing code, you want it to read in a straight line going
down.  You don't want long if statement blocks or spaghetti code.  If
you hit an error deal with it immediately and continue with the story in
a straight line going down.

> +
> +		/* The kernel wants space to store pointers to tty_structs */
> +		brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
> +		if (!brd->SerialDriver->ttys)
> +			return(-ENOMEM);

On this if statement it's a little bit more complicated because you have
to free the memory you allocated earlier before returning.  This one
should look like:

	brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
	if (!brd->SerialDriver->ttys) {
		ret = -ENOMEM;
		goto err_put_driver;
	}
	tty_set_operations(brd->SerialDriver, &dgap_tty_ops);


At the end of the function there will be something like:

	return 0;

err_release_foo:
	release_foo();
err_free_something:
	free_some_more_stuff();
err_put_driver:
	put_tty_driver(brd->SerialDriver);

	return ret;
}

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 6/6] staging: dgap: tty.c: removes smatch warning "unsigned '--un->un_open_count' is never less than zero"

[Lidza Louina]

On Mon, Sep 23, 2013 at 8:00 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Sep 23, 2013 at 06:47:17PM -0400, Lidza Louina wrote:
>> This patch removes this smatch warning:
>> unsigned '--un->un_open_count' is never less than zero
>>
>> The code decremented the un_open_count variable
>> and tested to see if it was less than zero. Because
>> un_open_count is unsigned and can't be below zero,
>> this test doesn't work. This patch tests
>> un_open_count against 0 without decrementing it.
>>
>> Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
>> ---
>>  drivers/staging/dgap/dgap_tty.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
>> index 8f0a824..f496710 100644
>> --- a/drivers/staging/dgap/dgap_tty.c
>> +++ b/drivers/staging/dgap/dgap_tty.c
>> @@ -1442,7 +1442,7 @@ static void dgap_tty_close(struct tty_struct *tty, struct file *file)
>>               un->un_open_count = 1;
>>       }
>>
>> -     if (--un->un_open_count < 0) {
>> +     if (un->un_open_count == 0) {
>>               APR(("bad serial port open count of %d\n", un->un_open_count));
>>               un->un_open_count = 0;
>>       }
>
> This fix isn't right.  We still need the decrement.  Probably the best
> thing is to audit all the driver and make sure that un->un_open_count
> can never be == 0.  But the next best, and still totally reasonable
> thing to do is this:
>
>         if (un->un_open_count == 0) {
>                 APR(("bad serial port open count of %d\n", un->un_open_count));
>                 un->un_open_count = 1;
>         }
>         un->un_open_count--;
>
> regards,
> dan carpenter
>

Okay, I see why, I'll resend this.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/6] staging: dgap: fep5.c: removes smatch warning "missing break? reassigning 'ch->pscan_state'"

[Lidza Louina]

On Mon, Sep 23, 2013 at 8:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 24, 2013 at 03:06:06AM +0300, Dan Carpenter wrote:
>> On Mon, Sep 23, 2013 at 06:47:13PM -0400, Lidza Louina wrote:
>> > This patch removes this smatch warning:
>> > warn: missing break? reassigning 'ch->pscan_state'
>>
>> I would just leave this one as is.
>>
>
> Yes.  Actually, the fix isn't right.  Let's drop this one.
>
> regards,
> dan carpenter
>

The switch statement this code refers to has the
default behavior first then all the different cases. Isn't
it supposed to be the other way around where
the different cases are presented before the default?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/6] staging: dgap: fep5.c: removes smatch warning "missing break? reassigning 'ch->pscan_state'"

[Dan Carpenter]

On Tue, Sep 24, 2013 at 06:06:16AM -0400, Lidza Louina wrote:
> On Mon, Sep 23, 2013 at 8:10 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Sep 24, 2013 at 03:06:06AM +0300, Dan Carpenter wrote:
> >> On Mon, Sep 23, 2013 at 06:47:13PM -0400, Lidza Louina wrote:
> >> > This patch removes this smatch warning:
> >> > warn: missing break? reassigning 'ch->pscan_state'
> >>
> >> I would just leave this one as is.
> >>
> >
> > Yes.  Actually, the fix isn't right.  Let's drop this one.
> >
> > regards,
> > dan carpenter
> >
> 
> The switch statement this code refers to has the
> default behavior first then all the different cases. Isn't
> it supposed to be the other way around where
> the different cases are presented before the default?

Write a small test program and figure it out.  ;)

int main(int argc, char **argv)
{
	int num;

	if (argc != 2) {
		printf("no number\n");
		return 1;
	}

	num = atoi(argv[1]);

	switch (num) {
		default:
			printf("%d\n", num);
		case 1:
			printf("one\n");
			break;
		case 2:
			printf("two\n");

	}

	return 0;
}

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Lidza Louina]

On Mon, Sep 23, 2013 at 8:27 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Sep 23, 2013 at 06:47:16PM -0400, Lidza Louina wrote:
>> This patch removes these warnings:
>> potential null dereference 'brd->SerialDriver'. (alloc_tty_driver returns null)
>> potential null dereference 'brd->PrintDriver'. (alloc_tty_driver returns null)
>>
>> This warning popped up because there wasn't a check
>> to make sure that the serial and print drivers were
>> allocated and not null before being initialized. This
>> patch adds that check.
>>
>> Signed-off-by: Lidza Louina <lidza.louina@gmail.com>
>> ---
>>  drivers/staging/dgap/dgap_tty.c | 103 +++++++++++++++++++++-------------------
>>  1 file changed, 54 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/staging/dgap/dgap_tty.c b/drivers/staging/dgap/dgap_tty.c
>> index 924e2bf..8f0a824 100644
>> --- a/drivers/staging/dgap/dgap_tty.c
>> +++ b/drivers/staging/dgap/dgap_tty.c
>> @@ -220,66 +220,71 @@ int dgap_tty_register(struct board_t *brd)
>>       DPR_INIT(("tty_register start"));
>>
>>       brd->SerialDriver = alloc_tty_driver(MAXPORTS);
>> +     if(brd->SerialDriver){
>
> Don't do it that way, flip it around.
>
>         brd->SerialDriver = alloc_tty_driver(MAXPORTS);
>         if (!brd->SerialDriver)
>                 return -ENOMEM;
>
>         snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgap_%d_", brd->boardnum);
>         brd->SerialDriver->name = brd->SerialName;
>
> That way has fewer indents.
>
> When you're writing code, you want it to read in a straight line going
> down.  You don't want long if statement blocks or spaghetti code.  If
> you hit an error deal with it immediately and continue with the story in
> a straight line going down.
>
>> +
>> +             /* The kernel wants space to store pointers to tty_structs */
>> +             brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
>> +             if (!brd->SerialDriver->ttys)
>> +                     return(-ENOMEM);
>
> On this if statement it's a little bit more complicated because you have
> to free the memory you allocated earlier before returning.  This one
> should look like:
>
>         brd->SerialDriver->ttys = dgap_driver_kzmalloc(MAXPORTS * sizeof(struct tty_struct *), GFP_KERNEL);
>         if (!brd->SerialDriver->ttys) {
>                 ret = -ENOMEM;
>                 goto err_put_driver;
>         }
>         tty_set_operations(brd->SerialDriver, &dgap_tty_ops);
>
>
> At the end of the function there will be something like:
>
>         return 0;
>
> err_release_foo:
>         release_foo();
> err_free_something:
>         free_some_more_stuff();
> err_put_driver:
>         put_tty_driver(brd->SerialDriver);
>
>         return ret;
> }

Instead of writing:
        brd->SerialDriver = alloc_tty_driver(MAXPORTS);
        if (!brd->SerialDriver){
                goto free_stuff;
                return -ENOMEM;
        }

Would it correct if I wrote:
        brd->SerialDriver = alloc_tty_driver(MAXPORTS);
        if (!brd->SerialDriver){
                kfree(brd->SerialDriver);
                return -ENOMEM;
        }

Just to avoid writing goto statements? >_<
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Dan Carpenter]

On Tue, Sep 24, 2013 at 02:40:10PM -0400, Lidza Louina wrote:
> Instead of writing:
>         brd->SerialDriver = alloc_tty_driver(MAXPORTS);
>         if (!brd->SerialDriver){
>                 goto free_stuff;
>                 return -ENOMEM;
>         }
> 
> Would it correct if I wrote:
>         brd->SerialDriver = alloc_tty_driver(MAXPORTS);
>         if (!brd->SerialDriver){
>                 kfree(brd->SerialDriver);
>                 return -ENOMEM;
>         }
> 
> Just to avoid writing goto statements? >_<

No.  The problem with doing it that way is that we end up with multiple
calls to kfree(brd->SerialDriver);.  The error handling becomes more
complex and error prone.

This is explained in Documentation/CodingStyle.

It should be written in mirror format.  All the resources are freed in
the reverse order that they are allocated.

	one = kmalloc();
	if (!one)
		return -ENOMEM;

	two = kmalloc();
	if (!two) {
		ret = -ENOMEM;
		goto err_free_one;
	}

	three = kmalloc();
	if (!three) {
		ret = -ENOMEM;
		goto err_free_two;
	}
	ret = frob();
	if (ret)
		goto err_free_three;


err_free_three:
	kfree(three);
err_free_two:
	kfree(two);
err_free_one:
	kfree(one);

	return ret;

Most of the time, there shouldn't be any if statements in the cleanup.

A common mistake is to use GW-BASIC names.
err_1:
	kfree(foo);
err_2:
	kfree(bar);

That's not useful because it doesn't describe what happens at the label.

Another mistake is to choose label names based on the goto location.

	if (kmalloc_failed)
		goto err_kmalloc_failed;

I already know kmalloc failed so the label name is totally useless.

Once you have your error handling cleanup block at the end of the
function, then that should almost match the release() function.  Btw, I
notice this dgap_tty_register() function doesn't have a matching
unregister function and actually dgap_tty_register() is never called.
:P

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Lidza Louina]

On Tue, Sep 24, 2013 at 3:20 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Sep 24, 2013 at 02:40:10PM -0400, Lidza Louina wrote:
>> Instead of writing:
>>         brd->SerialDriver = alloc_tty_driver(MAXPORTS);
>>         if (!brd->SerialDriver){
>>                 goto free_stuff;
>>                 return -ENOMEM;
>>         }
>>
>> Would it correct if I wrote:
>>         brd->SerialDriver = alloc_tty_driver((MAXPORTS);
>>         if (!brd->SerialDriver){
>>                 kfree(brd->SerialDriver);
>>                 return -ENOMEM;
>>         }
>>
>> Just to avoid writing goto statements? >_<
>
> No.  The problem with doing it that way is that we end up with multiple
> calls to kfree(brd->SerialDriver);.  The error handling becomes more
> complex and error prone.
>
> This is explained in Documentation/CodingStyle.

Okay, I read that. Thanks.

> It should be written in mirror format.  All the resources are freed in
> the reverse order that they are allocated.
>
>         one = kmalloc();
>         if (!one)
>                 return -ENOMEM;
>
>         two = kmalloc();
>         if (!two) {
>                 ret = -ENOMEM;
>                 goto err_free_one;
>         }
>
>         three = kmalloc();
>         if (!three) {
>                 ret = -ENOMEM;
>                 goto err_free_two;
>         }
>         ret = frob();
>         if (ret)
>                 goto err_free_three;
>
>
> err_free_three:
>         kfree(three);
> err_free_two:
>         kfree(two);
> err_free_one:
>         kfree(one);
>
>         return ret;

I looked at other uses of the function alloc_tty_driver() in
the kernel and none of them seem to follow up with a
call to kfree(). Are they supposed to? I realize that
because the allocation failed, I wouldn't need to call
kfree afterward. >_< So this should be enough:

    if(!brd->PrintDriver){
        rc = -ENOMEM;
    }

The code then returns rc at the end of the function.

Is this code supposed to include a call to kfree? If so,
what makes this driver different from the others?

> Most of the time, there shouldn't be any if statements in the cleanup.
>
> A common mistake is to use GW-BASIC names.
> err_1:
>         kfree(foo);
> err_2:
>         kfree(bar);
>
> That's not useful because it doesn't describe what happens at the label.
>
> Another mistake is to choose label names based on the goto location.
>
>         if (kmalloc_failed)
>                 goto err_kmalloc_failed;
>
> I already know kmalloc failed so the label name is totally useless.

Okay, thanks for the tip. I'll keep that in mind
whenever I use goto statements. :)

> Once you have your error handling cleanup block at the end of the
> function, then that should almost match the release() function.  Btw, I
> notice this dgap_tty_register() function doesn't have a matching
> unregister function and actually dgap_tty_register() is never called.
> :P

Hmm, ok, I'll do that next on this driver. >_< Add
dgap_tty_unregister() and make sure it and
dgap_tty_register() get called.

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Dan Carpenter]

On Wed, Sep 25, 2013 at 01:22:08PM -0400, Lidza Louina wrote:
> 
> I looked at other uses of the function alloc_tty_driver() in
> the kernel and none of them seem to follow up with a
> call to kfree().

Read my first response again.  I showed how to do this.  Your setting
up a bunch of things in a line.  If any of them fail you need to
cleanup by releasing any allocations.

If you have an allocation from alloc_tty_driver() then you can't release
it with kfree() you need to use put_tty_driver().

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Lidza Louina]

On Wed, Sep 25, 2013 at 2:34 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Sep 25, 2013 at 01:22:08PM -0400, Lidza Louina wrote:
>>
>> I looked at other uses of the function alloc_tty_driver() in
>> the kernel and none of them seem to follow up with a
>> call to kfree().
>
> Read my first response again.  I showed how to do this.  Your setting
> up a bunch of things in a line.  If any of them fail you need to
> cleanup by releasing any allocations.
>
> If you have an allocation from alloc_tty_driver() then you can't release
> it with kfree() you need to use put_tty_driver().

Alrighty.

These are the examples I'd found in the kernel.

Case 1: tty/synclink.c: mgsl_init_tty(): The serial_driver is
allocated, it checks for an error and returns -ENOMEM:

    serial_driver = alloc_tty_driver(128);
    if (!serial_driver)
        return -ENOMEM;

The code doesn't call put_tty_driver until synclink_cleanup() is
called. In synclink, the put_tty_driver only gets called when
serial_driver is not null:

    if (serial_driver) {
        if ((rc = tty_unregister_driver(serial_driver)))
            printk("%s(%d) failed to unregister tty driver err=%d\n",
                     __FILE__,__LINE__,rc);
        put_tty_driver(serial_driver);
    }

This is the case for most of the drivers I found, it returns -ENOMEM
when the alloc fails, and calls put_tty_driver when something fails
afterward (like when registering the device fails).

Case 2: tty/rocket.c: rp_init(): rocket_driver is allocated using
alloc_tty_driver, and we return ret:
    int ret = -ENOMEM, pci_boards_found, isa_boards_found, i;

    rocket_driver = alloc_tty_driver(MAX_RP_PORTS);
    if (!rocket_driver)
        goto err;
    .............(some code).............
    err:
        return ret;

put_tty_driver() gets called when we can't find an IO region:

    if (controller && (!request_region(controller, 4, "Comtrol RocketPort"))) {
        printk(KERN_ERR "Unable to reserve IO region for first "
        "configured ISA RocketPort controller 0x%lx.  "
        "Driver exiting\n", controller);
        ret = -EBUSY;
        goto err_tty;
    }
    .............(some code).............
    err_tty:
        put_tty_driver(rocket_driver);

And after setting rocket_driver's flags, termios info, type, subtype,
etc., it tries to register the driver:

    ret = tty_register_driver(rocket_driver);
    if (ret < 0) {
        printk(KERN_ERR "Couldn't install tty RocketPort driver\n");
        goto err_controller;
    }
    .............(some code).............
    err_controller:
        if (controller)
            release_region(controller, 4);

I would think that err_controller would have a call to put_tty_driver.
Also I'd think that err_tty would go with the failed register_driver()
call and the err_controller would math the failed request_region. Bad
names? >_<

Case 3: tty/serial/msm_smd_tty.c: smd_tty_init(): This doesn't have a
matching put_tty_driver after alloc_tty_driver.

Case 4: tty/vt/vt.c: vty_init(): This code allocates the driver, then
calls a panic function:

    console_driver = alloc_tty_driver(MAX_NR_CONSOLES);
    if (!console_driver)
        panic("Couldn't allocate console driver\n");

The code doesn't call put_tty_driver at any time, and I'm not sure
what the panic function does. I grepped thru the tty drivers and
couldn't find a declaration or definition for it.

There are more drivers I didn't look at, but I figured this would be
enough for now.

Out of the 18 drivers I checked:
- Most of them returned -ENOMEM when allocating failed and most used
put_tty_driver when registering, requesting a region, or using kthread
failed (not all)
- One called put_tty_driver when the module_exit function was being
called: tty/hvc/hvc_console.c
- One had no put_tty_driver call after it was allocated
- One had a panic function when it encountered an error and I don't
know what panic() does, but it doesnt seem to call put_tty_driver

I think I was just looking at the bad ones. >_< Do the ones I caught
need fixing? :)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Dan Carpenter]

On Wed, Sep 25, 2013 at 06:13:47PM -0400, Lidza Louina wrote:
> On Wed, Sep 25, 2013 at 2:34 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, Sep 25, 2013 at 01:22:08PM -0400, Lidza Louina wrote:
> >>
> >> I looked at other uses of the function alloc_tty_driver() in
> >> the kernel and none of them seem to follow up with a
> >> call to kfree().
> >
> > Read my first response again.  I showed how to do this.  Your setting
> > up a bunch of things in a line.  If any of them fail you need to
> > cleanup by releasing any allocations.
> >
> > If you have an allocation from alloc_tty_driver() then you can't release
> > it with kfree() you need to use put_tty_driver().
> 
> Alrighty.
> 
> These are the examples I'd found in the kernel.
> 
> Case 1: tty/synclink.c: mgsl_init_tty(): The serial_driver is
> allocated, it checks for an error and returns -ENOMEM:
> 
>     serial_driver = alloc_tty_driver(128);
>     if (!serial_driver)
>         return -ENOMEM;
> 

It's not allocated, the allocation failed.  It does call
put_tty_driver() if the tty_register_driver() call fails so this
function is correct.


> The code doesn't call put_tty_driver until synclink_cleanup() is
> called. In synclink, the put_tty_driver only gets called when
> serial_driver is not null:
> 
>     if (serial_driver) {
>         if ((rc = tty_unregister_driver(serial_driver)))
>             printk("%s(%d) failed to unregister tty driver err=%d\n",
>                      __FILE__,__LINE__,rc);
>         put_tty_driver(serial_driver);
>     }
> 
> This is the case for most of the drivers I found, it returns -ENOMEM
> when the alloc fails, and calls put_tty_driver when something fails
> afterward (like when registering the device fails).

Yes.  That's correct.

> 
> Case 2: tty/rocket.c: rp_init(): rocket_driver is allocated using
> alloc_tty_driver, and we return ret:
>     int ret = -ENOMEM, pci_boards_found, isa_boards_found, i;
> 
>     rocket_driver = alloc_tty_driver(MAX_RP_PORTS);
>     if (!rocket_driver)
>         goto err;
>     .............(some code).............
>     err:
>         return ret;
> 
> put_tty_driver() gets called when we can't find an IO region:
> 
>     if (controller && (!request_region(controller, 4, "Comtrol RocketPort"))) {
>         printk(KERN_ERR "Unable to reserve IO region for first "
>         "configured ISA RocketPort controller 0x%lx.  "
>         "Driver exiting\n", controller);
>         ret = -EBUSY;
>         goto err_tty;
>     }
>     .............(some code).............
>     err_tty:
>         put_tty_driver(rocket_driver);
> 
> And after setting rocket_driver's flags, termios info, type, subtype,
> etc., it tries to register the driver:
> 
>     ret = tty_register_driver(rocket_driver);
>     if (ret < 0) {
>         printk(KERN_ERR "Couldn't install tty RocketPort driver\n");
>         goto err_controller;
>     }
>     .............(some code).............
>     err_controller:
>         if (controller)
>             release_region(controller, 4);
> 
> I would think that err_controller would have a call to put_tty_driver.
> Also I'd think that err_tty would go with the failed register_driver()
> call and the err_controller would math the failed request_region. Bad
> names? >_<

This function is also fine.  The names ok-ish.

err_tty puts the tty.
err_controller releases the controller region.
err_ttyu unregisters the tty.

The names are not great.  "ttyu" is a too short and who would know that
the 'u' stands for "unregister?"  It would be better to use
"err_unregister_tty:"

> 
> Case 3: tty/serial/msm_smd_tty.c: smd_tty_init(): This doesn't have a
> matching put_tty_driver after alloc_tty_driver.
> 

This one is buggy.  If tty_register_driver() fails then it should call
put_tty_driver().

> Case 4: tty/vt/vt.c: vty_init(): This code allocates the driver, then
> calls a panic function:
> 
>     console_driver = alloc_tty_driver(MAX_NR_CONSOLES);
>     if (!console_driver)
>         panic("Couldn't allocate console driver\n");
> 
> The code doesn't call put_tty_driver at any time, and I'm not sure
> what the panic function does. I grepped thru the tty drivers and
> couldn't find a declaration or definition for it.
> 

panic() means the kernel dies.  If you can't load the vt module then
there is no point in continueing and nothing you can do to recover.
vt is special and essential.

> There are more drivers I didn't look at, but I figured this would be
> enough for now.
> 
> Out of the 18 drivers I checked:
> - Most of them returned -ENOMEM when allocating failed and most used
> put_tty_driver when registering, requesting a region, or using kthread
> failed (not all)
> - One called put_tty_driver when the module_exit function was being
> called: tty/hvc/hvc_console.c

The hvc_console.c driver is fine.

> - One had no put_tty_driver call after it was allocated
> - One had a panic function when it encountered an error and I don't
> know what panic() does, but it doesnt seem to call put_tty_driver
> 
> I think I was just looking at the bad ones. >_< Do the ones I caught
> need fixing? :)

Only smd_tty_init().  The others are all ok.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 5/6] staging: dgap: tty.c: removes smatch warnings "potential null dereference"

[Lidza Louina]

On Wed, Sep 25, 2013 at 6:29 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Sep 25, 2013 at 06:13:47PM -0400, Lidza Louina wrote:
>> On Wed, Sep 25, 2013 at 2:34 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Wed, Sep 25, 2013 at 01:22:08PM -0400, Lidza Louina wrote:
>> >>
>> >> I looked at other uses of the function alloc_tty_driver() in
>> >> the kernel and none of them seem to follow up with a
>> >> call to kfree().
>> >
>> > Read my first response again.  I showed how to do this.  Your setting
>> > up a bunch of things in a line.  If any of them fail you need to
>> > cleanup by releasing any allocations.
>> >
>> > If you have an allocation from alloc_tty_driver() then you can't release
>> > it with kfree() you need to use put_tty_driver().
>>
>> Alrighty.
>>
>> These are the examples I'd found in the kernel.
>>
>> Case 1: tty/synclink.c: mgsl_init_tty(): The serial_driver is
>> allocated, it checks for an error and returns -ENOMEM:
>>
>>     serial_driver = alloc_tty_driver(128);
>>     if (!serial_driver)
>>         return -ENOMEM;
>>
>
> It's not allocated, the allocation failed.  It does call
> put_tty_driver() if the tty_register_driver() call fails so this
> function is correct.
>
>
>> The code doesn't call put_tty_driver until synclink_cleanup() is
>> called. In synclink, the put_tty_driver only gets called when
>> serial_driver is not null:
>>
>>     if (serial_driver) {
>>         if ((rc = tty_unregister_driver(serial_driver)))
>>             printk("%s(%d) failed to unregister tty driver err=%d\n",
>>                      __FILE__,__LINE__,rc);
>>         put_tty_driver(serial_driver);
>>     }
>>
>> This is the case for most of the drivers I found, it returns -ENOMEM
>> when the alloc fails, and calls put_tty_driver when something fails
>> afterward (like when registering the device fails).
>
> Yes.  That's correct.
>
>>
>> Case 2: tty/rocket.c: rp_init(): rocket_driver is allocated using
>> alloc_tty_driver, and we return ret:
>>     int ret = -ENOMEM, pci_boards_found, isa_boards_found, i;
>>
>>     rocket_driver = alloc_tty_driver(MAX_RP_PORTS);
>>     if (!rocket_driver)
>>         goto err;
>>     .............(some code).............
>>     err:
>>         return ret;
>>
>> put_tty_driver() gets called when we can't find an IO region:
>>
>>     if (controller && (!request_region(controller, 4, "Comtrol RocketPort"))) {
>>         printk(KERN_ERR "Unable to reserve IO region for first "
>>         "configured ISA RocketPort controller 0x%lx.  "
>>         "Driver exiting\n", controller);
>>         ret = -EBUSY;
>>         goto err_tty;
>>     }
>>     .............(some code).............
>>     err_tty:
>>         put_tty_driver(rocket_driver);
>>
>> And after setting rocket_driver's flags, termios info, type, subtype,
>> etc., it tries to register the driver:
>>
>>     ret = tty_register_driver(rocket_driver);
>>     if (ret < 0) {
>>         printk(KERN_ERR "Couldn't install tty RocketPort driver\n");
>>         goto err_controller;
>>     }
>>     .............(some code).............
>>     err_controller:
>>         if (controller)
>>             release_region(controller, 4);
>>
>> I would think that err_controller would have a call to put_tty_driver.
>> Also I'd think that err_tty would go with the failed register_driver()
>> call and the err_controller would math the failed request_region. Bad
>> names? >_<
>
> This function is also fine.  The names ok-ish.
>
> err_tty puts the tty.
> err_controller releases the controller region.
> err_ttyu unregisters the tty.
>
> The names are not great.  "ttyu" is a too short and who would know that
> the 'u' stands for "unregister?"  It would be better to use
> "err_unregister_tty:"
>
>>
>> Case 3: tty/serial/msm_smd_tty.c: smd_tty_init(): This doesn't have a
>> matching put_tty_driver after alloc_tty_driver.
>>
>
> This one is buggy.  If tty_register_driver() fails then it should call
> put_tty_driver().
>
>> Case 4: tty/vt/vt.c: vty_init(): This code allocates the driver, then
>> calls a panic function:
>>
>>     console_driver = alloc_tty_driver(MAX_NR_CONSOLES);
>>     if (!console_driver)
>>         panic("Couldn't allocate console driver\n");
>>
>> The code doesn't call put_tty_driver at any time, and I'm not sure
>> what the panic function does. I grepped thru the tty drivers and
>> couldn't find a declaration or definition for it.
>>
>
> panic() means the kernel dies.  If you can't load the vt module then
> there is no point in continueing and nothing you can do to recover.
> vt is special and essential.
>
>> There are more drivers I didn't look at, but I figured this would be
>> enough for now.
>>
>> Out of the 18 drivers I checked:
>> - Most of them returned -ENOMEM when allocating failed and most used
>> put_tty_driver when registering, requesting a region, or using kthread
>> failed (not all)
>> - One called put_tty_driver when the module_exit function was being
>> called: tty/hvc/hvc_console.c
>
> The hvc_console.c driver is fine.
>
>> - One had no put_tty_driver call after it was allocated
>> - One had a panic function when it encountered an error and I don't
>> know what panic() does, but it doesnt seem to call put_tty_driver
>>
>> I think I was just looking at the bad ones. >_< Do the ones I caught
>> need fixing? :)
>
> Only smd_tty_init().  The others are all ok.
>
> regards,
> dan carpenter

Alrighty, thanks. :)