* 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).