compiler warning in wireless-testing.

Looks like this is the compiler's fault to me..but might be worth explicitly initializing the sta variable just to make it quiet.

Quoted 3 lines · Ben Greear

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.

Quoted 9 lines · Johannes Berg

Quoted 3 lines · Ben Greear

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

net/mac80211/mlme.c +0 -7

index 6cb5361..96fc201 100644
@@ -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);

net/mac80211/mlme.c +5 -0

@@ -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.

Quoted 59 lines · Eliad Peller

Quoted 9 lines · Johannes Berg

Quoted 3 lines · Ben Greear

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

net/mac80211/mlme.c +0 -7

index 6cb5361..96fc201 100644
@@ -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);

net/mac80211/mlme.c +5 -0

@@ -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,

Quoted 1 line

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.

Quoted 5 lines

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 :)