airo: remove pointless check

PATCH In review Dan Carpenter 2 participants 3 messages Sep 10, 2010 – Sep 11, 2010

Subsystems (2)

Kernel janitors Networking drivers (wireless)

lore

Share & tools

lore

Dan Carpenter

author

2010-09-10 11:57 UTC #0 lore raw

PATCH 1/1 airo: remove pointless check

It doesn't make sense to check "!ai->config.rates" here. "ai->config.rates" is the address of an eight bytes array and it can't ever be null here. Also if it were NULL then trying to set:

	ai->config.rates[i] = basic_rate | 0x80;

would cause an oops.

Signed-off-by Dan Carpenter <[email protected]>

drivers/net/wireless/airo.c +1 -2

index 1d05445..d806497 100644
@@ -3886,8 +3886,7 @@ static u16 setup_card(struct airo_info *ai, u8 *mac, int lock)
 		}
 		if ( basic_rate > 0 ) {
 			for( i = 0; i < 8; i++ ) {
-				if ( ai->config.rates[i] = basic_rate ||
-				     !ai->config.rates ) {
+				if (ai->config.rates[i] = basic_rate) {
 					ai->config.rates[i] = basic_rate | 0x80;
 					break;
 				}

Stanislaw Gruszka

2010-09-10 13:33 UTC #1 lore raw

Quoted 13 lines · Dan Carpenter

It doesn't make sense to check "!ai->config.rates" here. "ai->config.rates" is the address of an eight bytes array and it can't ever be null here. Also if it were NULL then trying to set:
	ai->config.rates[i] = basic_rate | 0x80;
would cause an oops.
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/wireless/airo.c +0 -0
index 1d05445..d806497 100644

drivers/net/wireless/airo.c static u16 setup_card(struct airo_info *ai, u8 *mac, int lock) · line 3892

Quoted 9 lines

drivers/net/wireless/airo.c +1 -2

@@ -3886,8 +3886,7 @@ static u16 setup_card(struct airo_info *ai, u8 *mac, int lock)
 		}
 		if ( basic_rate > 0 ) {
 			for( i = 0; i < 8; i++ ) {
-				if ( ai->config.rates[i] == basic_rate ||
-				     !ai->config.rates ) {
+				if (ai->config.rates[i] == basic_rate) {
 					ai->config.rates[i] = basic_rate | 0x80;
 					break;
 				}

I think code author wonted to do "!ai->config.rates[i], what mean "if rate is invalid use basic rate".

I'm not sure if basic_rate module parameter is used by anyone. It's not described, seems it was created for development purpose. I think we can remove it ...

Stanislaw

Dan Carpenter

author

2010-09-11 23:48 UTC #2 lore raw

PATCH v2 1/1 airo: remove "basic_rate" module option

The "basic_rate" module option is not implemented correctly. If the rate was set to zero it was supposed to set it to "basic_rate | 0x80". Unfortunately the check to see if what zero was wrong and it checked "!ai->config.rates" (which is always false) instead of "!ai->config.rates[i]".

This option was just used for development and it wasn't documented anywhere. Instead of fixing it, we can just remove it.

Reported-by Stanislaw Gruszka <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

---
V2: Version 1 was just a cleanup to please the static checkers but didn't fix anything. Stanislaw suggests that we can remove the feature entirely so that's what I've done in version 2.

drivers/net/wireless/airo.c +0 ~~-11~~

index 1d05445..c20cad7 100644
@@ -217,7 +217,6 @@ static char *statsLabels[] = {
    (no spaces) list of rates (up to 8). */
 
 static int rates[8];
-static int basic_rate;
 static char *ssids[3];
 
 static int io[4];

@@ -250,7 +249,6 @@ MODULE_LICENSE("Dual BSD/GPL");
 MODULE_SUPPORTED_DEVICE("Aironet 4500, 4800 and Cisco 340/350");
 module_param_array(io, int, NULL, 0);
 module_param_array(irq, int, NULL, 0);
-module_param(basic_rate, int, 0);
 module_param_array(rates, int, NULL, 0);
 module_param_array(ssids, charp, NULL, 0);
 module_param(auto_wep, int, 0);

@@ -3884,15 +3882,6 @@ static u16 setup_card(struct airo_info *ai, u8 *mac, int lock)
 				ai->config.rates[i] = rates[i];
 			}
 		}
-		if ( basic_rate > 0 ) {
-			for( i = 0; i < 8; i++ ) {
-				if ( ai->config.rates[i] = basic_rate ||
-				     !ai->config.rates ) {
-					ai->config.rates[i] = basic_rate | 0x80;
-					break;
-				}
-			}
-		}
 		set_bit (FLAG_COMMIT, &ai->flags);
 	}