* compiler warning in wireless-testing. @ 2012-03-15 18:37 Ben Greear 2012-03-15 18:39 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Ben Greear @ 2012-03-15 18:37 UTC (permalink / raw) To: linux-wireless@vger.kernel.org Looks like this is the compiler's fault to me..but might be worth explicitly initializing the sta variable just to make it quiet. CC [M] net/mac80211/mlme.o /home/greearb/git/linux.wireless-testing/net/mac80211/mlme.c: In function ‘ieee80211_prep_connection’: /home/greearb/git/linux.wireless-testing/net/mac80211/mlme.c:3058: warning: ‘sta’ may be used uninitialized in this function CC [M] net/mac80211/debugfs.o CC [M] net/mac80211/debugfs_netdev.o -- Ben Greear <greearb@candelatech.com> Candela Technologies Inc http://www.candelatech.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: compiler warning in wireless-testing. 2012-03-15 18:37 compiler warning in wireless-testing Ben Greear @ 2012-03-15 18:39 ` Johannes Berg 2012-03-15 18:53 ` Eliad Peller 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2012-03-15 18:39 UTC (permalink / raw) To: Ben Greear; +Cc: linux-wireless@vger.kernel.org On Thu, 2012-03-15 at 11:37 -0700, Ben Greear wrote: > Looks like this is the compiler's fault to me..but might be > worth explicitly initializing the sta variable just to make > it quiet. I disagree, doing that will only hide legitimate warnings if somebody changes the code. Initializing for compiler happiness is almost always the worst thing you can do. johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: compiler warning in wireless-testing. 2012-03-15 18:39 ` Johannes Berg @ 2012-03-15 18:53 ` Eliad Peller 2012-03-15 18:59 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Eliad Peller @ 2012-03-15 18:53 UTC (permalink / raw) To: Johannes Berg; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, Mar 15, 2012 at 8:39 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2012-03-15 at 11:37 -0700, Ben Greear wrote: >> Looks like this is the compiler's fault to me..but might be >> worth explicitly initializing the sta variable just to make >> it quiet. > > I disagree, doing that will only hide legitimate warnings if somebody > changes the code. Initializing for compiler happiness is almost always > the worst thing you can do. > what about diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 6cb5361..96fc201 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -3094,7 +3094,6 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata, struct ieee80211_local *local = sdata->local; struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; struct ieee80211_bss *bss = (void *)cbss->priv; - struct sta_info *sta; bool have_sta = false; int err; @@ -3107,12 +3106,6 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata, rcu_read_unlock(); } - if (!have_sta) { - sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL); - if (!sta) - return -ENOMEM; - } - mutex_lock(&local->mtx); ieee80211_recalc_idle(sdata->local); mutex_unlock(&local->mtx); @@ -3123,10 +3116,15 @@ static int ieee80211_prep_connection(struct ieee80211_sub_if_data *sdata, if (!have_sta) { struct ieee80211_supported_band *sband; + struct sta_info *sta; u32 rates = 0, basic_rates = 0; bool have_higher_than_11mbit; int min_rate = INT_MAX, min_rate_index = -1; + sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL); + if (!sta) + return -ENOMEM; + sband = sdata->local->hw.wiphy->bands[cbss->channel->band]; ieee80211_get_rates(sband, bss->supp_rates, it also makes the code more clear, imho. Eliad. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: compiler warning in wireless-testing. 2012-03-15 18:53 ` Eliad Peller @ 2012-03-15 18:59 ` Johannes Berg 2012-03-15 19:15 ` Eliad Peller 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2012-03-15 18:59 UTC (permalink / raw) To: Eliad Peller; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, 2012-03-15 at 20:53 +0200, Eliad Peller wrote: > On Thu, Mar 15, 2012 at 8:39 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: > > On Thu, 2012-03-15 at 11:37 -0700, Ben Greear wrote: > >> Looks like this is the compiler's fault to me..but might be > >> worth explicitly initializing the sta variable just to make > >> it quiet. > > > > I disagree, doing that will only hide legitimate warnings if somebody > > changes the code. Initializing for compiler happiness is almost always > > the worst thing you can do. > > > what about > > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c > index 6cb5361..96fc201 100644 > --- a/net/mac80211/mlme.c > +++ b/net/mac80211/mlme.c > @@ -3094,7 +3094,6 @@ static int ieee80211_prep_connection(struct > ieee80211_sub_if_data *sdata, > struct ieee80211_local *local = sdata->local; > struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; > struct ieee80211_bss *bss = (void *)cbss->priv; > - struct sta_info *sta; > bool have_sta = false; > int err; > > @@ -3107,12 +3106,6 @@ static int ieee80211_prep_connection(struct > ieee80211_sub_if_data *sdata, > rcu_read_unlock(); > } > > - if (!have_sta) { > - sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL); > - if (!sta) > - return -ENOMEM; > - } > - > mutex_lock(&local->mtx); > ieee80211_recalc_idle(sdata->local); > mutex_unlock(&local->mtx); > @@ -3123,10 +3116,15 @@ static int ieee80211_prep_connection(struct > ieee80211_sub_if_data *sdata, > > if (!have_sta) { > struct ieee80211_supported_band *sband; > + struct sta_info *sta; > u32 rates = 0, basic_rates = 0; > bool have_higher_than_11mbit; > int min_rate = INT_MAX, min_rate_index = -1; > > + sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL); > + if (!sta) > + return -ENOMEM; > + > sband = sdata->local->hw.wiphy->bands[cbss->channel->band]; > > ieee80211_get_rates(sband, bss->supp_rates, > > > it also makes the code more clear, imho. Yeah, I thought about that for a second, but I'm working on changes in this area that would essentially require me to revert this again ... I really want to do the allocation first so the failure path for that is easy. On the other hand, I'm adding channel type switching (see my RFC patchset) in the middle of this, and unwinding that would be a larger change that even touches the device. What may be possible instead is to replace the second have_sta in the second if with "if (sta)", which may make Ben's compiler a bit happier, but I have no way of even testing that since my compiler doesn't complain to start with. johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: compiler warning in wireless-testing. 2012-03-15 18:59 ` Johannes Berg @ 2012-03-15 19:15 ` Eliad Peller 2012-03-15 19:19 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Eliad Peller @ 2012-03-15 19:15 UTC (permalink / raw) To: Johannes Berg; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, Mar 15, 2012 at 8:59 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2012-03-15 at 20:53 +0200, Eliad Peller wrote: >> On Thu, Mar 15, 2012 at 8:39 PM, Johannes Berg >> <johannes@sipsolutions.net> wrote: >> > On Thu, 2012-03-15 at 11:37 -0700, Ben Greear wrote: >> >> Looks like this is the compiler's fault to me..but might be >> >> worth explicitly initializing the sta variable just to make >> >> it quiet. >> > >> > I disagree, doing that will only hide legitimate warnings if somebody >> > changes the code. Initializing for compiler happiness is almost always >> > the worst thing you can do. >> > >> what about >> >> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c >> index 6cb5361..96fc201 100644 >> --- a/net/mac80211/mlme.c >> +++ b/net/mac80211/mlme.c >> @@ -3094,7 +3094,6 @@ static int ieee80211_prep_connection(struct >> ieee80211_sub_if_data *sdata, >> struct ieee80211_local *local = sdata->local; >> struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; >> struct ieee80211_bss *bss = (void *)cbss->priv; >> - struct sta_info *sta; >> bool have_sta = false; >> int err; >> >> @@ -3107,12 +3106,6 @@ static int ieee80211_prep_connection(struct >> ieee80211_sub_if_data *sdata, >> rcu_read_unlock(); >> } >> >> - if (!have_sta) { >> - sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL); >> - if (!sta) >> - return -ENOMEM; >> - } >> - >> mutex_lock(&local->mtx); >> ieee80211_recalc_idle(sdata->local); >> mutex_unlock(&local->mtx); >> @@ -3123,10 +3116,15 @@ static int ieee80211_prep_connection(struct >> ieee80211_sub_if_data *sdata, >> >> if (!have_sta) { >> struct ieee80211_supported_band *sband; >> + struct sta_info *sta; >> u32 rates = 0, basic_rates = 0; >> bool have_higher_than_11mbit; >> int min_rate = INT_MAX, min_rate_index = -1; >> >> + sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL); >> + if (!sta) >> + return -ENOMEM; >> + >> sband = sdata->local->hw.wiphy->bands[cbss->channel->band]; >> >> ieee80211_get_rates(sband, bss->supp_rates, >> >> >> it also makes the code more clear, imho. > > Yeah, I thought about that for a second, but I'm working on changes in > this area that would essentially require me to revert this again ... > > I really want to do the allocation first so the failure path for that is > easy. On the other hand, I'm adding channel type switching (see my RFC > patchset) in the middle of this, and unwinding that would be a larger > change that even touches the device. > > What may be possible instead is to replace the second have_sta in the > second if with "if (sta)", which may make Ben's compiler a bit happier, > but I have no way of even testing that since my compiler doesn't > complain to start with. > that's possible, but then you'll have to initialize sta anyway :) Eliad. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: compiler warning in wireless-testing. 2012-03-15 19:15 ` Eliad Peller @ 2012-03-15 19:19 ` Johannes Berg 0 siblings, 0 replies; 6+ messages in thread From: Johannes Berg @ 2012-03-15 19:19 UTC (permalink / raw) To: Eliad Peller; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, 2012-03-15 at 21:15 +0200, Eliad Peller wrote: > > What may be possible instead is to replace the second have_sta in the > > second if with "if (sta)", which may make Ben's compiler a bit happier, > > but I have no way of even testing that since my compiler doesn't > > complain to start with. > > > that's possible, but then you'll have to initialize sta anyway :) But then at least that has a purpose you can see :) johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-15 19:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-15 18:37 compiler warning in wireless-testing Ben Greear 2012-03-15 18:39 ` Johannes Berg 2012-03-15 18:53 ` Eliad Peller 2012-03-15 18:59 ` Johannes Berg 2012-03-15 19:15 ` Eliad Peller 2012-03-15 19:19 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).