drivers/ieee1394/csr1212.c: remove dead code
PATCH 1/1 drivers/ieee1394/csr1212.c: remove dead code
The Coverity checker spotted that the same check was already done above.
drivers/ieee1394/csr1212.c renamed from drivers/ieee1394/csr1212.c.old +1 -5
@@ -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 ;
Quoted 11 lines · Adrian Bunk
The Coverity checker spotted that the same check was already done above.
Signed-off-by Adrian Bunk <[email protected]>drivers/ieee1394/csr1212.c renamed from drivers/ieee1394/csr1212.c.old +0
-0@@ -1616,12 +1616,8 @@* and make cache regions for them */ for (dentry = csr->root_kv->value.directory.dentries_head; dentry; dentry = dentry->next) {
drivers/ieee1394/csr1212.c line 1623
Quoted 10 lines
drivers/ieee1394/csr1212.c renamed from drivers/ieee1394/csr1212.c.old +1
-5-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.
Signature · 3 lines
Stefan Richter -=====-=-=-= =-== =-=-= http://arcgraph.de/sr/
Quoted 2 lines · Stefan Richter
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.
drivers/ieee1394/csr1212.c +4 -4
@@ -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; } }
Quoted 14 lines
Signature · 13 lines
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 [email protected] https://lists.sourceforge.net/lists/listinfo/linux1394-devel
drivers/ieee1394/csr1212.c int csr1212_parse_csr(struct csr1212_csr · line 1614
Quoted 9 lines · Jody McIntyre
drivers/ieee1394/csr1212.c +2
-1@@ -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;
Quoted 5 lines
drivers/ieee1394/csr1212.c +0
-0/* 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) {
drivers/ieee1394/csr1212.c int csr1212_parse_csr(struct csr1212_csr
Quoted 10 lines
drivers/ieee1394/csr1212.c +2
-3if (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.
Signature · 3 lines
Stefan Richter -=====-=-=-= =-== =-=-= http://arcgraph.de/sr/
Review
Quoted 9 lines · Stefan Richter
(quoted diff) +3
-3- 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 :/
Quoted 2 lines
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
Quoted 4 lines
Signature · 3 lines
Stefan Richter -=====-=-=-= =-== =-=-= http://arcgraph.de/sr/
PATCH 1/2 csr1212: check results of keyval reads
In reply to Stefan Richtercsr1212_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.
drivers/ieee1394/csr1212.c +4 -3
@@ -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
In reply to Stefan RichterDon't read the keyval if there's already a valid one in place. May not be necessary but shouldn't hurt.
drivers/ieee1394/csr1212.c +2 -1
@@ -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;