* [patch] airo: remove pointless check @ 2010-09-10 11:57 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2010-09-10 11:57 UTC (permalink / raw) To: John W. Linville Cc: Matthieu CASTET, Stanislaw Gruszka, linux-wireless, kernel-janitors 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 <error27@gmail.com> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c index 1d05445..d806497 100644 --- a/drivers/net/wireless/airo.c +++ b/drivers/net/wireless/airo.c @@ -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; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [patch] airo: remove pointless check @ 2010-09-10 11:57 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2010-09-10 11:57 UTC (permalink / raw) To: John W. Linville Cc: Matthieu CASTET, Stanislaw Gruszka, linux-wireless, kernel-janitors 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 <error27@gmail.com> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c index 1d05445..d806497 100644 --- a/drivers/net/wireless/airo.c +++ b/drivers/net/wireless/airo.c @@ -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; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] airo: remove pointless check 2010-09-10 11:57 ` Dan Carpenter @ 2010-09-10 13:34 ` Stanislaw Gruszka -1 siblings, 0 replies; 6+ messages in thread From: Stanislaw Gruszka @ 2010-09-10 13:34 UTC (permalink / raw) To: Dan Carpenter Cc: John W. Linville, Matthieu CASTET, linux-wireless, kernel-janitors On Fri, Sep 10, 2010 at 01:57:14PM +0200, Dan Carpenter wrote: > 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 <error27@gmail.com> > > diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c > index 1d05445..d806497 100644 > --- a/drivers/net/wireless/airo.c > +++ b/drivers/net/wireless/airo.c > @@ -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] airo: remove pointless check @ 2010-09-10 13:34 ` Stanislaw Gruszka 0 siblings, 0 replies; 6+ messages in thread From: Stanislaw Gruszka @ 2010-09-10 13:34 UTC (permalink / raw) To: Dan Carpenter Cc: John W. Linville, Matthieu CASTET, linux-wireless, kernel-janitors On Fri, Sep 10, 2010 at 01:57:14PM +0200, Dan Carpenter wrote: > 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 <error27@gmail.com> > > diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c > index 1d05445..d806497 100644 > --- a/drivers/net/wireless/airo.c > +++ b/drivers/net/wireless/airo.c > @@ -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [patch v2] airo: remove "basic_rate" module option 2010-09-10 13:34 ` Stanislaw Gruszka @ 2010-09-11 23:48 ` Dan Carpenter -1 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2010-09-11 23:48 UTC (permalink / raw) To: Stanislaw Gruszka Cc: John W. Linville, Matthieu CASTET, linux-wireless, kernel-janitors 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 <sgruszka@redhat.com> Signed-off-by: Dan Carpenter <error27@gmail.com> --- 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. diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c index 1d05445..c20cad7 100644 --- a/drivers/net/wireless/airo.c +++ b/drivers/net/wireless/airo.c @@ -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); } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [patch v2] airo: remove "basic_rate" module option @ 2010-09-11 23:48 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2010-09-11 23:48 UTC (permalink / raw) To: Stanislaw Gruszka Cc: John W. Linville, Matthieu CASTET, linux-wireless, kernel-janitors 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 <sgruszka@redhat.com> Signed-off-by: Dan Carpenter <error27@gmail.com> --- 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. diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c index 1d05445..c20cad7 100644 --- a/drivers/net/wireless/airo.c +++ b/drivers/net/wireless/airo.c @@ -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); } ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-11 23:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-10 11:57 [patch] airo: remove pointless check Dan Carpenter 2010-09-10 11:57 ` Dan Carpenter 2010-09-10 13:34 ` Stanislaw Gruszka 2010-09-10 13:34 ` Stanislaw Gruszka 2010-09-11 23:48 ` [patch v2] airo: remove "basic_rate" module option Dan Carpenter 2010-09-11 23:48 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.