On Tue, Aug 9, 2011 at 3:37 PM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> On Wed, 2011-07-27 at 13:22 +0300, Guy Eilam wrote:
>
>> +++ b/net/mac80211/mlme.c
>> @@ -1495,10 +1495,14 @@ static bool ieee80211_assoc_success(struct ieee80211_work *wk,
>>
>> ifmgd->aid = aid;
>>
>> - sta = sta_info_alloc(sdata, cbss->bssid, GFP_KERNEL);
>> - if (!sta) {
>> - printk(KERN_DEBUG "%s: failed to alloc STA entry for"
>> - " the AP\n", sdata->name);
>> + mutex_lock(&sdata->local->sta_mtx);
>> + /*
>> + * station info was already allocated and inserted before
>> + * the association and should be available to us
>> + */
>> + sta = sta_info_get_rx(sdata, cbss->bssid);
>> + if (WARN_ON(!sta)) {
>> + mutex_unlock(&sdata->local->sta_mtx);
>> return false;
>> }
>>
>> @@ -1569,8 +1573,10 @@ static bool ieee80211_assoc_success(struct ieee80211_work *wk,
>> if (elems.wmm_param)
>> set_sta_flags(sta, WLAN_STA_WME);
>>
>> - err = sta_info_insert(sta);
>> + err = sta_info_insert_notify(sta);
>
> I think I'd prefer if you could just call sta_info_insert() again with
> the existing station entry?
I can't do that because I have a lock at this point. _insert_notify()
is exactly the same as _insert(), but it expects to have sta_mtx
locked already.
>
>> sta = NULL;
>> + /* sta_info_insert_notify changed the mutex lock with rcu lock */
>> + rcu_read_unlock();
>
> This was previously part of sta_info_insert() right?
Yes. I can insert it to _insert_notify().
>
>> +++ b/net/mac80211/rx.c
>> @@ -842,8 +842,21 @@ ieee80211_rx_h_check(struct ieee80211_rx_data *rx)
>> ieee80211_is_pspoll(hdr->frame_control)) &&
>> rx->sdata->vif.type != NL80211_IFTYPE_ADHOC &&
>> rx->sdata->vif.type != NL80211_IFTYPE_WDS &&
>> - (!rx->sta || !test_sta_flags(rx->sta, WLAN_STA_ASSOC))))
>> + (!rx->sta || !test_sta_flags(rx->sta, WLAN_STA_ASSOC)))) {
>> + if (rx->sta && rx->sta->dummy &&
>> + ieee80211_is_data(hdr->frame_control)) {
>> + u16 ethertype;
>> + u8 *payload;
>> +
>> + payload = rx->skb->data +
>> + ieee80211_hdrlen(hdr->frame_control);
>> + ethertype = (payload[6] << 8) | payload[7];
>> + if (cpu_to_be16(ethertype) ==
>> + rx->sdata->control_port_protocol)
>
> Is there a frame length check here? Also can you just indent the
> rx->sdata part with four spaces instead of a tab so it's more clearly
> part of the if please.
I should probably check ieee80211_is_data_present() instead of just
ieee80211_is_data().
>
>> +++ b/net/mac80211/sta_info.c
>> @@ -102,6 +102,30 @@ struct sta_info *sta_info_get(struct ieee80211_sub_if_data *sdata,
>> lockdep_is_held(&local->sta_mtx));
>> while (sta) {
>> if (sta->sdata == sdata &&
>> + !sta->dummy &&
>
> maybe put those on one line
>
>> @@ -450,17 +510,28 @@ static int sta_info_insert_mgd(struct sta_info *sta) __acquires(RCU)
>> */
>>
>> spin_lock_irqsave(&local->sta_lock, flags);
>> - /* check if STA exists already */
>> - if (sta_info_get_bss(sdata, sta->sta.addr)) {
>> - spin_unlock_irqrestore(&local->sta_lock, flags);
>> - mutex_unlock(&local->sta_mtx);
>> - rcu_read_lock();
>> - return -EEXIST;
>> + /*
>> + * check if STA exists already.
>> + * only accept a scenario of a second call to sta_info_insert_mgd
>> + * with a dummy station entry that was inserted earlier
>> + * in that case - assume that the dummy station flag should
>> + * be removed.
>> + */
>> + exist_sta = sta_info_get_bss_rx(sdata, sta->sta.addr);
>> + if (exist_sta) {
>> + if (exist_sta == sta && sta->dummy && !sta_list_add) {
>> + sta->dummy = false;
>
> two things: why "sta_list_add"? I think it can be implicit.
>
> Secondly, I think sta->dummy must only be set to false in
> finish_insert(), when it would typically add it into the hash table etc.
> to preserve the right order of things and not introduce races with
> finding a non-dummy station that hasn't been fully inserted yet.
You are correct, the dummy=false shuold be moved to _finish_insert().
And I can remove the sta_list_add from the _mdg()/_non_ibss() function.
But I do need the sta_list_add flag in the sta_info_finish_insert() function,
because it have no ability to distinguish between a newly inserted dummy
station and a reinsertion of one (I don't want to add another check to
that function).
>
>> #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
>> - wiphy_debug(local->hw.wiphy, "Inserted STA %pM\n", sta->sta.addr);
>> + wiphy_debug(local->hw.wiphy, "Inserted %sSTA %pM\n",
>> + sta->dummy ? "Dummy " : "", sta->sta.addr);
>> #endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
>
> I think you should lowercase "dummy" in debug messages where it's and
> adjective :)
>
>> /* move reference to rcu-protected */
>> rcu_read_lock();
>> mutex_unlock(&local->sta_mtx);
>>
>> - if (ieee80211_vif_is_mesh(&sdata->vif))
>> + if (ieee80211_vif_is_mesh(&sdata->vif) && !sta->dummy)
>> mesh_accept_plinks_update(sdata);
>
> why this? can't have a dummy on mesh anyway, no?
>
>> @@ -435,9 +440,15 @@ rcu_dereference_protected_tid_tx(struct sta_info *sta, int tid)
>> struct sta_info *sta_info_get(struct ieee80211_sub_if_data *sdata,
>> const u8 *addr);
>>
>> +struct sta_info *sta_info_get_rx(struct ieee80211_sub_if_data *sdata,
>> + const u8 *addr);
>> +
>> struct sta_info *sta_info_get_bss(struct ieee80211_sub_if_data *sdata,
>> const u8 *addr);
>>
>> +struct sta_info *sta_info_get_bss_rx(struct ieee80211_sub_if_data *sdata,
>> + const u8 *addr);
>> +
>> static inline
>> void for_each_sta_info_type_check(struct ieee80211_local *local,
>> const u8 *addr,
>> @@ -458,6 +469,22 @@ void for_each_sta_info_type_check(struct ieee80211_local *local,
>> _sta = nxt, \
>> nxt = _sta ? rcu_dereference(_sta->hnext) : NULL \
>> ) \
>> + /* run code only if address matches and it's not a dummy sta */ \
>> + if (memcmp(_sta->sta.addr, (_addr), ETH_ALEN) == 0 &&\
>
> please keep the backslashes lined up
>
> Ok the only real problem is the place where you set dummy=false, but I'd
> also make the patch & code simpler by making it more implicit -- instead
> of having the sta_list_add argument everywhere I'd just check if it's a
> dummy being re-inserted etc.
>
> johannes
>
>
Guy.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
