[2.6 patch] drivers/ieee1394/csr1212.c: remove dead code

[2.6 patch] drivers/ieee1394/csr1212.c: remove dead code

[Adrian Bunk]

The Coverity checker spotted that the same check was already done above.


Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- linux-2.6.15-rc1-mm2-full/drivers/ieee1394/csr1212.c.old	2005-11-20 22:50:14.000000000 +0100
+++ linux-2.6.15-rc1-mm2-full/drivers/ieee1394/csr1212.c	2005-11-20 22:50:36.000000000 +0100
@@ -1616,12 +1616,8 @@
 	 * and make cache regions for them */
 	for (dentry = csr->root_kv->value.directory.dentries_head;
 	     dentry; dentry = dentry->next) {
-		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
+		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM)
 			csr1212_get_keyval(csr, dentry->kv);
-
-			if (ret != CSR1212_SUCCESS)
-				return ret;
-		}
 	}
 
 	return CSR1212_SUCCESS;

Re: [2.6 patch] drivers/ieee1394/csr1212.c: remove dead code

[Stefan Richter]

Adrian Bunk wrote:
> The Coverity checker spotted that the same check was already done above.
> 
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 
> --- linux-2.6.15-rc1-mm2-full/drivers/ieee1394/csr1212.c.old	2005-11-20 22:50:14.000000000 +0100
> +++ linux-2.6.15-rc1-mm2-full/drivers/ieee1394/csr1212.c	2005-11-20 22:50:36.000000000 +0100
> @@ -1616,12 +1616,8 @@
>  	 * and make cache regions for them */
>  	for (dentry = csr->root_kv->value.directory.dentries_head;
>  	     dentry; dentry = dentry->next) {
> -		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
> +		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM)
>  			csr1212_get_keyval(csr, dentry->kv);
> -
> -			if (ret != CSR1212_SUCCESS)
> -				return ret;
> -		}
>  	}
>  
>  	return CSR1212_SUCCESS;

Yes, this is dead code. But when I looked through csr1212_parse_csr() 
which you are patching here, I wondered why the return code of 
csr1212_get_keyval() is never checked there. csr1212_get_keyval() 
performs memory allocations and bus reads. Shouldn't both calls of 
csr1212_get_keyval() be enclosed in something like this?

	if(!csr1212_get_keyval(...))
		return ~ CSR1212_SUCCESS;

Or for better yet, we should use _csr1212_read_keyval() instead so that 
we get more sensible error codes.
-- 
Stefan Richter
-=====-=-=-= =-== =-=-=
http://arcgraph.de/sr/

Re: [2.6 patch] drivers/ieee1394/csr1212.c: remove dead code

[Jody McIntyre]

On Mon, Nov 21, 2005 at 08:45:29PM +0100, Stefan Richter wrote:

> Or for better yet, we should use _csr1212_read_keyval() instead so that 
> we get more sensible error codes.

Good idea.  How about:


csr1212: check results of keyval reads

csr1212_parse_csr() did not properly check return values when reading
keyvals.  Fix this by using _csr1212_read_keyval() instead of
csr1212_get_keyval() and checking the return code.

Signed-off-by: Jody McIntyre <scjody@steamballoon.com>

Index: linux/drivers/ieee1394/csr1212.c
===================================================================
--- linux.orig/drivers/ieee1394/csr1212.c
+++ linux/drivers/ieee1394/csr1212.c
@@ -1610,16 +1610,16 @@ int csr1212_parse_csr(struct csr1212_csr
 	csr->root_kv->valid = 0;
 	csr->root_kv->next = csr->root_kv;
 	csr->root_kv->prev = csr->root_kv;
-	csr1212_get_keyval(csr, csr->root_kv);
+	if (_csr1212_read_keyval(csr, csr->root_kv) != CSR1212_SUCCESS)
+		return ret;
 
 	/* Scan through the Root directory finding all extended ROM regions
 	 * and make cache regions for them */
 	for (dentry = csr->root_kv->value.directory.dentries_head;
 	     dentry; dentry = dentry->next) {
 		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
-			csr1212_get_keyval(csr, dentry->kv);
-
-			if (ret != CSR1212_SUCCESS)
+			if (_csr1212_read_keyval(csr, dentry->kv) !=
+						CSR1212_SUCCESS)
 				return ret;
 		}
 	}

> -- 
> Stefan Richter
> -=====-=-=-= =-== =-=-=
> http://arcgraph.de/sr/
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
> Register for a JBoss Training Course.  Free Certification Exam
> for All Training Attendees Through End of 2005. For more info visit:
> http://ads.osdn.com/?ad_id=7628&alloc_id=16845&op=click
> _______________________________________________
> mailing list linux1394-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel

-- 

Re: [2.6 patch] drivers/ieee1394/csr1212.c: remove dead code

[Stefan Richter]

Jody McIntyre wrote:
> --- linux.orig/drivers/ieee1394/csr1212.c
> +++ linux/drivers/ieee1394/csr1212.c
> @@ -1610,16 +1610,16 @@ int csr1212_parse_csr(struct csr1212_csr
>  	csr->root_kv->valid = 0;
>  	csr->root_kv->next = csr->root_kv;
>  	csr->root_kv->prev = csr->root_kv;
> -	csr1212_get_keyval(csr, csr->root_kv);
> +	if (_csr1212_read_keyval(csr, csr->root_kv) != CSR1212_SUCCESS)
> +		return ret;

-	csr1212_get_keyval(csr, csr->root_kv);
+	ret = _csr1212_read_keyval(csr, csr->root_kv);
+	if (ret != CSR1212_SUCCESS)
+		return ret;

>  
>  	/* Scan through the Root directory finding all extended ROM regions
>  	 * and make cache regions for them */
>  	for (dentry = csr->root_kv->value.directory.dentries_head;
>  	     dentry; dentry = dentry->next) {
>  		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
> -			csr1212_get_keyval(csr, dentry->kv);
> -
> -			if (ret != CSR1212_SUCCESS)
> +			if (_csr1212_read_keyval(csr, dentry->kv) !=
> +						CSR1212_SUCCESS)
>  				return ret;
>  		}
>  	}
> 

-		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
-			csr1212_get_keyval(csr, dentry->kv);
-
+		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM &&
+		    !dentry->kv->valid) {
+			ret = _csr1212_read_keyval(csr, dentry->kv);
  			if (ret != CSR1212_SUCCESS)
  				return ret;
  		}


Although I am not quite sure if the check for !valid is really required. 
It certainly cannot hurt.
-- 
Stefan Richter
-=====-=-=-= =-== =-=-=
http://arcgraph.de/sr/

Re: [2.6 patch] drivers/ieee1394/csr1212.c: remove dead code

[Jody McIntyre]

On Mon, Nov 21, 2005 at 11:02:10PM +0100, Stefan Richter wrote:

> -		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
> -			csr1212_get_keyval(csr, dentry->kv);
> -
> +		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM &&
> +		    !dentry->kv->valid) {
> +			ret = _csr1212_read_keyval(csr, dentry->kv);
>  			if (ret != CSR1212_SUCCESS)
>  				return ret;
>  		}

Duh :/

> Although I am not quite sure if the check for !valid is really required. 
> It certainly cannot hurt.

That's a separate issue so it should be a separate patch.  I'll do it
though.

Cheers,
Jody

> -- 
> Stefan Richter
> -=====-=-=-= =-== =-=-=
> http://arcgraph.de/sr/

[PATCH 1/2] csr1212: check results of keyval reads

[Jody McIntyre]

csr1212_parse_csr() did not properly check return values when reading
keyvals.  Fix this by using _csr1212_read_keyval() instead of
csr1212_get_keyval() and checking the return code.

Signed-off-by: Jody McIntyre <scjody@steamballoon.com>

Index: linux/drivers/ieee1394/csr1212.c
===================================================================
--- linux.orig/drivers/ieee1394/csr1212.c
+++ linux/drivers/ieee1394/csr1212.c
@@ -1610,15 +1610,16 @@ int csr1212_parse_csr(struct csr1212_csr
 	csr->root_kv->valid = 0;
 	csr->root_kv->next = csr->root_kv;
 	csr->root_kv->prev = csr->root_kv;
-	csr1212_get_keyval(csr, csr->root_kv);
+	ret = _csr1212_read_keyval(csr, csr->root_kv);
+	if (ret != CSR1212_SUCCESS)
+		return ret;
 
 	/* Scan through the Root directory finding all extended ROM regions
 	 * and make cache regions for them */
 	for (dentry = csr->root_kv->value.directory.dentries_head;
 	     dentry; dentry = dentry->next) {
 		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
-			csr1212_get_keyval(csr, dentry->kv);
-
+			ret = _csr1212_read_keyval(csr, dentry->kv);
 			if (ret != CSR1212_SUCCESS)
 				return ret;
 		}

[PATCH 2/2] csr1212: add check for !valid

[Jody McIntyre]

Don't read the keyval if there's already a valid one in place.  May not be
necessary but shouldn't hurt.

Signed-off-by: Jody McIntyre <scjdy@steamballoon.com>


Index: linux/drivers/ieee1394/csr1212.c
===================================================================
--- linux.orig/drivers/ieee1394/csr1212.c
+++ linux/drivers/ieee1394/csr1212.c
@@ -1618,7 +1618,8 @@ int csr1212_parse_csr(struct csr1212_csr
 	 * and make cache regions for them */
 	for (dentry = csr->root_kv->value.directory.dentries_head;
 	     dentry; dentry = dentry->next) {
-		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM) {
+		if (dentry->kv->key.id == CSR1212_KV_ID_EXTENDED_ROM &&
+			!dentry->kv->valid) {
 			ret = _csr1212_read_keyval(csr, dentry->kv);
 			if (ret != CSR1212_SUCCESS)
 				return ret;