MBSSID and EMA support in AP mode
v13 cover MBSSID and EMA support in AP mode
This patchset adds support for multiple BSSID and
enhanced multi-BSSID advertisements for AP mode.
New patch added to split __ieee80211_beacon_get() first before MBSSID and EMA beacon handing.
Aloka Dixit (1):
mac80211: split beacon retrieval functions
John Crispin (2):
mac80211: MBSSID and EMA support in beacon handling
mac80211: MBSSID channel switch
include/net/mac80211.h | 66 +++++++ net/mac80211/cfg.c | 177 ++++++++++++++---- net/mac80211/ieee80211_i.h | 1 + net/mac80211/tx.c | 360 ++++++++++++++++++++++++++++--------- 4 files changed, 484 insertions(+), 120 deletions(-)
base-commit: 171964252189d8ad5672c730f2197aa73092db6e
Signature · 1 line
2.31.1
PATCH v13 1/3 mac80211: split beacon retrieval functions
Split __ieee80211_beacon_get() into a separate function for AP mode ieee80211_beacon_get_ap().
Also, move the code common to all modes (AP, adhoc and mesh) to a separate function ieee80211_beacon_get_finish().
---
v13:New addition to the patch series compared to v12. This change is added in a separate patch for better readability.
net/mac80211/tx.c | 203 +++++++++++++++++++++++++++------------------- 1 file changed, 118 insertions(+), 85 deletions(-)
net/mac80211/tx.c +118 -85
@@ -4979,6 +4979,115 @@ static int ieee80211_beacon_protect(struct sk_buff *skb,return 0 ; } +static void +ieee80211_beacon_get_finish (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct ieee80211_mutable_offsets *offs, +struct beacon_data *beacon, +struct sk_buff *skb, +struct ieee80211_chanctx_conf *chanctx_conf, +u16 csa_off_base) +{ +struct ieee80211_local *local =hw_to_local (hw); +struct ieee80211_sub_if_data *sdata =vif_to_sdata (vif); +struct ieee80211_tx_info *info; +enum nl80211_band band; +struct ieee80211_tx_rate_control txrc; + +/* CSA offsets */ +if (offs && beacon) { +u16 i; + +for (i =0 ; i <IEEE80211_MAX_CNTDWN_COUNTERS_NUM ; i++) { +u16 csa_off = beacon->cntdwn_counter_offsets[i]; + +if (!csa_off) +continue ; + + offs->cntdwn_counter_offs[i] = csa_off_base + csa_off; + } + } + + band = chanctx_conf->def.chan->band; + info =IEEE80211_SKB_CB (skb); + info->flags |=IEEE80211_TX_INTFL_DONT_ENCRYPT ; + info->flags |=IEEE80211_TX_CTL_NO_ACK ; + info->band = band; + +memset (&txrc,0 ,sizeof (txrc)); + txrc.hw = hw; + txrc.sband = local->hw.wiphy->bands[band]; + txrc.bss_conf = &sdata->vif.bss_conf; + txrc.skb = skb; + txrc.reported_rate.idx =-1 ; +if (sdata->beacon_rate_set && sdata->beacon_rateidx_mask[band]) + txrc.rate_idx_mask = sdata->beacon_rateidx_mask[band]; +else + txrc.rate_idx_mask = sdata->rc_rateidx_mask[band]; + txrc.bss = true; +rate_control_get_rate (sdata,NULL , &txrc); + + info->control.vif = vif; + info->flags |=IEEE80211_TX_CTL_CLEAR_PS_FILT | +IEEE80211_TX_CTL_ASSIGN_SEQ | +IEEE80211_TX_CTL_FIRST_FRAGMENT ; +} + +static struct sk_buff * +ieee80211_beacon_get_ap (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct ieee80211_mutable_offsets *offs, +bool is_template, +struct beacon_data *beacon, +struct ieee80211_chanctx_conf *chanctx_conf) +{ +struct ieee80211_local *local =hw_to_local (hw); +struct ieee80211_sub_if_data *sdata =vif_to_sdata (vif); +struct ieee80211_if_ap *ap = &sdata->u.ap; +struct sk_buff *skb =NULL ; +u16 csa_off_base =0 ; + +if (beacon->cntdwn_counter_offsets[0 ]) { +if (!is_template) +ieee80211_beacon_update_cntdwn (vif); + +ieee80211_set_beacon_cntdwn (sdata, beacon); + } + +/* headroom, head length, +* tail length and maximum TIM length +*/ + skb =dev_alloc_skb (local->tx_headroom + beacon->head_len + + beacon->tail_len +256 + + local->hw.extra_beacon_tailroom); +if (!skb) +return NULL ; + +skb_reserve (skb, local->tx_headroom); +skb_put_data (skb, beacon->head, beacon->head_len); + +ieee80211_beacon_add_tim (sdata, &ap->ps, skb, is_template); + +if (offs) { + offs->tim_offset = beacon->head_len; + offs->tim_length = skb->len - beacon->head_len; + offs->cntdwn_counter_offs[0 ] = beacon->cntdwn_counter_offsets[0 ]; + +/* for AP the csa offsets are from tail */ + csa_off_base = skb->len; + } + +if (beacon->tail) +skb_put_data (skb, beacon->tail, beacon->tail_len); + +if (ieee80211_beacon_protect (skb, local, sdata) <0 ) +return NULL ; + +ieee80211_beacon_get_finish (hw, vif, offs, beacon, skb, chanctx_conf, + csa_off_base); +return skb; +} +static struct sk_buff * __ieee80211_beacon_get(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
@@ -4988,12 +5097,8 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,struct ieee80211_local *local =hw_to_local (hw);struct beacon_data *beacon =NULL ;struct sk_buff *skb =NULL ; -struct ieee80211_tx_info *info;struct ieee80211_sub_if_data *sdata =NULL ; -enum nl80211_band band; -struct ieee80211_tx_rate_control txrc;struct ieee80211_chanctx_conf *chanctx_conf; -int csa_off_base =0 ;rcu_read_lock ();
@@ -5010,48 +5115,11 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw,struct ieee80211_if_ap *ap = &sdata->u.ap; beacon =rcu_dereference (ap->beacon); -if (beacon) { -if (beacon->cntdwn_counter_offsets[0 ]) { -if (!is_template) -ieee80211_beacon_update_cntdwn (vif); - -ieee80211_set_beacon_cntdwn (sdata, beacon); - } - -/* -* headroom, head length, -* tail length and maximum TIM length -*/ - skb =dev_alloc_skb (local->tx_headroom + - beacon->head_len + - beacon->tail_len +256 + - local->hw.extra_beacon_tailroom); -if (!skb) - goto out; - -skb_reserve (skb, local->tx_headroom); -skb_put_data (skb, beacon->head, beacon->head_len); - -ieee80211_beacon_add_tim (sdata, &ap->ps, skb, - is_template); - -if (offs) { - offs->tim_offset = beacon->head_len; - offs->tim_length = skb->len - beacon->head_len; - offs->cntdwn_counter_offs[0 ] = beacon->cntdwn_counter_offsets[0 ]; - -/* for AP the csa offsets are from tail */ - csa_off_base = skb->len; - } - -if (beacon->tail) -skb_put_data (skb, beacon->tail, - beacon->tail_len); - -if (ieee80211_beacon_protect (skb, local, sdata) <0 ) - goto out; - }else +if (!beacon) goto out; + + skb =ieee80211_beacon_get_ap (hw, vif, offs, is_template, + beacon, chanctx_conf); }else if (sdata->vif.type ==NL80211_IFTYPE_ADHOC ) {struct ieee80211_if_ibss *ifibss = &sdata->u.ibss;struct ieee80211_hdr *hdr;
@@ -5077,6 +5145,9 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, hdr = (struct ieee80211_hdr *) skb->data; hdr->frame_control =cpu_to_le16 (IEEE80211_FTYPE_MGMT |IEEE80211_STYPE_BEACON ); + +ieee80211_beacon_get_finish (hw, vif, offs, beacon, skb, + chanctx_conf,0 ); }else if (ieee80211_vif_is_mesh (&sdata->vif)) {struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
@@ -5116,51 +5187,13 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, }skb_put_data (skb, beacon->tail, beacon->tail_len); +ieee80211_beacon_get_finish (hw, vif, offs, beacon, skb, + chanctx_conf,0 ); }else {WARN_ON (1 ); goto out; } -/* CSA offsets */ -if (offs && beacon) { -int i; - -for (i =0 ; i <IEEE80211_MAX_CNTDWN_COUNTERS_NUM ; i++) { -u16 csa_off = beacon->cntdwn_counter_offsets[i]; - -if (!csa_off) -continue ; - - offs->cntdwn_counter_offs[i] = csa_off_base + csa_off; - } - } - - band = chanctx_conf->def.chan->band; - - info =IEEE80211_SKB_CB (skb); - - info->flags |=IEEE80211_TX_INTFL_DONT_ENCRYPT ; - info->flags |=IEEE80211_TX_CTL_NO_ACK ; - info->band = band; - -memset (&txrc,0 ,sizeof (txrc)); - txrc.hw = hw; - txrc.sband = local->hw.wiphy->bands[band]; - txrc.bss_conf = &sdata->vif.bss_conf; - txrc.skb = skb; - txrc.reported_rate.idx =-1 ; -if (sdata->beacon_rate_set && sdata->beacon_rateidx_mask[band]) - txrc.rate_idx_mask = sdata->beacon_rateidx_mask[band]; -else - txrc.rate_idx_mask = sdata->rc_rateidx_mask[band]; - txrc.bss = true; -rate_control_get_rate (sdata,NULL , &txrc); - - info->control.vif = vif; - - info->flags |=IEEE80211_TX_CTL_CLEAR_PS_FILT | -IEEE80211_TX_CTL_ASSIGN_SEQ | -IEEE80211_TX_CTL_FIRST_FRAGMENT ; out:rcu_read_unlock ();return skb;
Signature · 1 line
2.31.1
Re: [v13 1/3] mac80211: split beacon retrieval functions
Quoted 41 lines · Aloka Dixit
Split
__ieee80211_beacon_get()into a separate function for AP modeieee80211_beacon_get_ap().
Also, move the code common to all modes (AP, adhoc and mesh) to a separate functionieee80211_beacon_get_finish().Signed-off-by Aloka Dixit <[email protected]>---
v13:New addition to the patch series compared to v12. This change is added in a separate patch for better readability.net/mac80211/tx.c | 203 +++++++++++++++++++++++++++------------------- 1 file changed, 118 insertions(+), 85 deletions(-)net/mac80211/tx.c +19
-0@@ -4979,6 +4979,115 @@ static int ieee80211_beacon_protect(structsk_buff *skb, return 0; } +static void +ieee80211_beacon_get_finish (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct ieee80211_mutable_offsets *offs, +struct beacon_data *beacon, +struct sk_buff *skb, +struct ieee80211_chanctx_conf *chanctx_conf, +u16 csa_off_base) +{ +struct ieee80211_local *local =hw_to_local (hw); +struct ieee80211_sub_if_data *sdata =vif_to_sdata (vif); +struct ieee80211_tx_info *info; +enum nl80211_band band; +struct ieee80211_tx_rate_control txrc; + +/* CSA offsets */ +if (offs && beacon) { +u16 i; +
net/mac80211/tx.c static int ieee80211_beacon_protect(struct · line 5011
Quoted 10 lines
net/mac80211/tx.c +10
-0+for (i =0 ; i <IEEE80211_MAX_CNTDWN_COUNTERS_NUM ; i++) { +u16 csa_off = beacon->cntdwn_counter_offsets[i]; + +if (!csa_off) +continue ; + + offs->cntdwn_counter_offs[i] = csa_off_base + csa_off; + } + } +
I just now realized that the CSA offset part can be moved to the AP specific function. ieee80211_beacon_get_finish() won't even need csa_off_base as an input in that case. Will wait for other comments and then move it.
PATCH v13 2/3 mac80211: MBSSID and EMA beacon handling in AP mode
In reply to Aloka DixitFrom: John Crispin <[email protected]>
Add new fields in struct beacon_data to store all MBSSID elements.
For a non-EMA AP, generate a single beacon template which includes all MBSSID elements.
For an EMA AP, generate multiple beacon templates, each including a single MBSSID element. EMA profile periodicity equals the count of elements.
Move CSA offset to reflect the MBSSID element length.
Add new APIs for the drivers to retrieve MBSSID and EMA beacons:
- ieee80211_beacon_get_template_ema_index() - Generate the EMA beacon which includes the multiple BSSID element at the given index. Drivers can use this function in a loop as well NULL is returned.
- ieee80211_beacon_get_template_ema_list() - Generate all EMA templates. Drivers must call ieee80211_beacon_free_ema_list() to free the memory.
No change in the prototype for the existing API ieee80211_beacon_get_template() which should be used for non-EMA cases, both with and without multiple BSSID enabled.
---
v13:
- Reverted lock related changes around __ieee80211_beacon_get().
- Replaced the infinite loop to generate EMA beacons by a finite loop and also reordered code so that rcu_dereference(ap->beacon) happens only once.
- Removed ema_index from struct beacon_data and the function ieee80211_beacon_get_template_ema_next(), replaced by ieee80211_beacon_get_template_ema_index() as given in the description.
include/net/mac80211.h | 66 ++++++++++++++ net/mac80211/cfg.c | 163 +++++++++++++++++++++++++++------- net/mac80211/ieee80211_i.h | 1 + net/mac80211/tx.c | 175 ++++++++++++++++++++++++++++++++++--- 4 files changed, 363 insertions(+), 42 deletions(-)
include/net/mac80211.h +66 -0
@@ -4919,12 +4919,14 @@ void ieee80211_report_low_ack(struct ieee80211_sta *sta, u32 num_packets);* @cntdwn_counter_offs: array of IEEE80211_MAX_CNTDWN_COUNTERS_NUM offsets * to countdown counters. This array can contain zero values which * should be ignored. +* @mbssid_off: position of the multiple bssid element */ struct ieee80211_mutable_offsets {u16 tim_offset;u16 tim_length;u16 cntdwn_counter_offs[IEEE80211_MAX_CNTDWN_COUNTERS_NUM ]; +u16 mbssid_off; };/**
@@ -4951,6 +4953,70 @@ ieee80211_beacon_get_template(struct ieee80211_hw *hw,struct ieee80211_vif *vif, struct ieee80211_mutable_offsets *offs); +/** +* ieee80211_beacon_get_template_ema_index - EMA beacon template generation +* function for drivers using the sw offload path. +* @hw: pointer obtained from ieee80211_alloc_hw(). +* @vif: &struct ieee80211_vif pointer from the add_interface callback. +* @offs: &struct ieee80211_mutable_offsets pointer to struct that will +* receive the offsets that may be updated by the driver. +* @ema_index: index of the beacon in the EMA set, must be more than or equal +* to 0. +* +* This function follows the same rules as ieee80211_beacon_get_template() +* but returns a beacon template which includes multiple BSSID element at the +* requested index. +* +* Return: The beacon template. %NULL on error or if the given index is more +* than or equal to the number of EMA beacons. +*/ +struct sk_buff * +ieee80211_beacon_get_template_ema_index (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct ieee80211_mutable_offsets *offs, +u8 ema_index); + +/** +* struct ieee80211_ema_bcns - list entry of an EMA beacon +* @list: the list pointer. +* @skb: the skb containing this specific beacon +* @offs: &struct ieee80211_mutable_offsets pointer to struct that will +* receive the offsets that may be updated by the driver. +*/ +struct ieee80211_ema_bcns { +struct list_head list; +struct sk_buff *skb; +struct ieee80211_mutable_offsets offs; +}; + +/** +* ieee80211_beacon_get_template_ema_list - EMA beacon template generation +* function for drivers using the hw offload. +* @hw: pointer obtained from ieee80211_alloc_hw(). +* @vif: &struct ieee80211_vif pointer from the add_interface callback. +* @head: linked list head that will get populated with +* &struct ieee80211_ema_bcns pointers. Driver must call INIT_LIST_HEAD() +* before calling this function to initialize the list. +* +* This function follows the same rules as ieee80211_beacon_get_template() +* but returns a linked list of all beacon templates required to cover all +* profiles in the multiple BSSID set. Each template includes only one multiple +* BSSID element. Driver must call ieee80211_beacon_free_ema_list() to free +* the allocated memory after done. +*/ +void ieee80211_beacon_get_template_ema_list (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct list_head *head); + +/** +* ieee80211_beacon_free_ema_list - free an EMA beacon template list +* @head: linked list head containing &struct ieee80211_ema_bcns pointers. +* +* This function will free a list previously acquired by calling +* ieee80211_beacon_get_template_ema_list() +*/ +void ieee80211_beacon_free_ema_list (struct list_head *head); +/** * ieee80211_beacon_get_tim - beacon generation function * @hw: pointer obtained from ieee80211_alloc_hw().
net/mac80211/cfg.c +133 -30
291 lines not on this page · raw message
net/mac80211/ieee80211_i.h +1 -0
@@ -257,6 +257,7 @@ struct beacon_data {struct ieee80211_meshconf_ie *meshconf;u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM ];u8 cntdwn_current_counter; +struct cfg80211_mbssid_elems *mbssid_ies;struct rcu_head rcu_head; };
net/mac80211/tx.c +163 -12
234 lines not on this page · raw message
Signature · 1 line
2.31.1
Johannes Berg
maintainerRe: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5154
Quoted 10 lines · Aloka Dixit
net/mac80211/tx.c +9
-0+static struct sk_buff * +ieee80211_beacon_get_ap_mbssid (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct ieee80211_mutable_offsets *offs, +bool is_template, +struct beacon_data *beacon, +struct ieee80211_chanctx_conf *chanctx_conf, +int ema_index, +struct list_head *ema_list)
This function is called from ieee80211_beacon_get_ap(). That's called from __ieee80211_beacon_get(), under RCU read lock.
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5166
Quoted 4 lines
net/mac80211/tx.c +4
-0+for (i =0 ; i < beacon->mbssid_ies->cnt; i++) { +struct ieee80211_ema_bcns *bcn; + + bcn =kzalloc (sizeof (*bcn),GFP_KERNEL );
Therefore, you really cannot GFP_KERNEL allocate anything. But I really only saw this because I went back to my comments on v12 where this was still more obvious.
Given that we're already in v13 of this patch, let's take a step back. Much of what I've pointed out in the reviews really is automatically testable.
Can you please add support for this to hwsim and hostapd and add a few tests to the hostap hwsim tests? Given the complexity of this, being able to run the tests for it myself would give me a lot more confidence in it.
johannes
Re: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode
Quoted 16 lines · Johannes Berg
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5154
Quoted 10 lines · Aloka Dixit
net/mac80211/tx.c +9
-0+static struct sk_buff * +ieee80211_beacon_get_ap_mbssid (struct ieee80211_hw *hw, +struct ieee80211_vif *vif, +struct ieee80211_mutable_offsets *offs, +bool is_template, +struct beacon_data *beacon, +struct ieee80211_chanctx_conf *chanctx_conf, +int ema_index, +struct list_head *ema_list)This function is called from
ieee80211_beacon_get_ap(). That's called from__ieee80211_beacon_get(), under RCU read lock.
Quoted 9 lines
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5166
Quoted 4 lines
net/mac80211/tx.c +4
-0+for (i =0 ; i < beacon->mbssid_ies->cnt; i++) { +struct ieee80211_ema_bcns *bcn; + + bcn =kzalloc (sizeof (*bcn),GFP_KERNEL );Therefore, you really cannot
GFP_KERNELallocate anything. But I really only saw this because I went back to my comments on v12 where this was still more obvious.
Okay, I understand now that it is illegal because GFP_KERNEL is blocking.
I thought of following:
lock rcu -> get mbssid count first -> unlock rcu -> allocate memory.
But in that case, will have again: lock -> dereference to get beacon snapshot. Beacon can change in between so new count might be wrong. In general sounds complicated and wrong.
I read that GFP_ATOMIC should be used sparingly, mainly for interrupt handlers etc. Do you think this code path warrants its use?
Or should I look for some other function split?
Will add hwsim test cases before the next version but I genuinely did not see any issue during testing with current code.
So can you tell me which debug flags should be enabled to make such errors become obvious to someone like me who is new to these details in kernel programming?
Thanks!!
Johannes Berg
maintainerRe: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode
Hi!
Quoted 14 lines
Quoted 4 lines
This function is called from
ieee80211_beacon_get_ap(). That's called from__ieee80211_beacon_get(), under RCU read lock.Quoted 9 lines
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5166
Quoted 4 lines
net/mac80211/tx.c +4
-0+for (i =0 ; i < beacon->mbssid_ies->cnt; i++) { +struct ieee80211_ema_bcns *bcn; + + bcn =kzalloc (sizeof (*bcn),GFP_KERNEL );Therefore, you really cannot
GFP_KERNELallocate anything. But I really only saw this because I went back to my comments on v12 where this was still more obvious.
Quoted 2 lines
Okay, I understand now that it is illegal because
GFP_KERNELis blocking.
Right.
Quoted 6 lines
I thought of following:
lock rcu -> get mbssid count first -> unlock rcu -> allocate memory. But in that case, will have again: lock -> dereference to get beacon snapshot.
Beacon can change in between so new count might be wrong. In general sounds complicated and wrong.
Indeed. You could make it work (and count changing is highly unlikely!) by going back and checking if the count was correct in the critical section, and then going back if necessary (i.e. if it was wrong). But if you do this, you should do something like this pseudo-code:
rcu_read_lock();
repeat:calculated_size = calculate_size();rcu_read_unlock();
alloc = kzalloc(calculated_size, GFP_KERNEL);
// omitting error handling
rcu_read_lock();calculated_size = calculate_size();
if (ksize(alloc) < calculated_size)
goto repeat;
...
i.e. note the ksize(), since allocations are rounded up. Even if the count increased, you might not need a new allocation.
Also maybe anyway it'd make sense to allocate all of them together as an array, rather than individual pointers for each beacon?
Quoted 2 lines
I read that
GFP_ATOMICshould be used sparingly, mainly for interrupt handlers etc.
I guess once every beacon is still fairly sparingly though :)
Quoted 5 lines
Do you think this code path warrants its use?
Or should I look for some other function split?Will add hwsim test cases before the next version but I genuinely did not see any issue during testing with current code.
Sounds great, thanks!
Quoted 3 lines
So can you tell me which debug flags should be enabled to make such errors become obvious to someone like me who is new to these details in kernel programming?
Hmm. I guess you want at least CONFIG_DEBUG_ATOMIC_SLEEP for this case. But probably best to (also) turn on lockdep (CONFIG_PROVE_LOCKING=y), including all the RCU checks (CONFIG_PROVE_RCU=y).
With that, it really _should_ be obvious here once that code path executes at all, regardless of whether the kzalloc(GFP_KERNEL) actually sleeps or not.
johannes
Re: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode
Quoted 51 lines · Johannes Berg
Hi!
Quoted 14 lines
Quoted 4 lines
This function is called from
ieee80211_beacon_get_ap(). That's called from__ieee80211_beacon_get(), under RCU read lock.Quoted 9 lines
net/mac80211/tx.c ieee80211_beacon_get_ap(struct ieee80211_hw *hw, · line 5166
Quoted 4 lines
net/mac80211/tx.c +4
-0+for (i =0 ; i < beacon->mbssid_ies->cnt; i++) { +struct ieee80211_ema_bcns *bcn; + + bcn =kzalloc (sizeof (*bcn),GFP_KERNEL );Therefore, you really cannot
GFP_KERNELallocate anything. But I really only saw this because I went back to my comments on v12 where this was still more obvious.Quoted 2 lines
Okay, I understand now that it is illegal because
GFP_KERNELis blocking.Right.
Quoted 6 lines
I thought of following:
lock rcu -> get mbssid count first -> unlock rcu -> allocate memory. But in that case, will have again: lock -> dereference to get beacon snapshot.
Beacon can change in between so new count might be wrong. In general sounds complicated and wrong.Indeed. You could make it work (and count changing is highly unlikely!) by going back and checking if the count was correct in the critical section, and then going back if necessary (i.e. if it was wrong). But if you do this, you should do something like this pseudo-code:
rcu_read_lock();
repeat:calculated_size=calculate_size();rcu_read_unlock();alloc =
kzalloc(calculated_size, GFP_KERNEL);
// omitting error handling
rcu_read_lock();calculated_size=calculate_size();
if (ksize(alloc) <calculated_size)goto repeat;...
i.e. note the
ksize(), since allocations are rounded up. Even if the count increased, you might not need a new allocation.
Quoted 10 lines
Also maybe anyway it'd make sense to allocate all of them together as an array, rather than individual pointers for each beacon?
Quoted 2 lines
I read that
GFP_ATOMICshould be used sparingly, mainly for interrupt handlers etc.I guess once every beacon is still fairly sparingly though :)
Thank you so much for the quick response, will enable the debug flags henceforth.
With that, what would be a better way:
(1) Making it work with pseudo code, still using GFP_KERNEL or
(2) Changing to GFP_ATOMIC but otherwise keep the code fairly similar to v13 (preferably allocating an array instead of separate pointers as you suggested)?
Johannes Berg
maintainerRe: [v13 2/3] mac80211: MBSSID and EMA beacon handling in AP mode
Quoted 7 lines · Aloka Dixit
With that, what would be a better way:
(1) Making it work with pseudo code, still usingGFP_KERNELor
(2) Changing toGFP_ATOMICbut otherwise keep the code fairly similar to v13 (preferably allocating an array instead of separate pointers as you suggested)?
It's a good question. I'm not really into the code right now, but I'd say GFP_ATOMIC should be OK.
I'm not even sure the functions you're modifying are always guaranteed to be called in a context where you can sleep? E.g. I believe the driver can call ieee80211_beacon_get_tim() in atomic context itself, e.g. even from timer/interrupt to get the next beacon?
johannes
PATCH v13 3/3 mac80211: MBSSID channel switch
In reply to Aloka DixitFrom: John Crispin <[email protected]>
Trigger ieee80211_csa_finish() on the non-transmitting interfaces when channel switch concludes on the transmitting interface.
---
v13: Replaced list_for_each_entry_safe() by list_for_each_entry()
net/mac80211/cfg.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
net/mac80211/cfg.c +12 -2
@@ -3271,8 +3271,18 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif) {struct ieee80211_sub_if_data *sdata =vif_to_sdata (vif); -ieee80211_queue_work (&sdata->local->hw, - &sdata->csa_finalize_work); +if (vif->mbssid_tx_vif == vif) { +struct ieee80211_sub_if_data *child; + +list_for_each_entry (child, &sdata->local->interfaces, list) +if (child != sdata && child->vif.mbssid_tx_vif == vif && +ieee80211_sdata_running (child)) { +ieee80211_queue_work (&child->local->hw, + &child->csa_finalize_work); + } + } + +ieee80211_queue_work (&sdata->local->hw, &sdata->csa_finalize_work); }EXPORT_SYMBOL (ieee80211_csa_finish);
Signature · 1 line
2.31.1
Johannes Berg
maintainerRe: [v13 3/3] mac80211: MBSSID channel switch
Quoted 11 lines · Aloka Dixit
From: John Crispin <[email protected]>
Trigger
ieee80211_csa_finish()on the non-transmitting interfaces when channel switch concludes on the transmitting interface.Signed-off-by John Crispin <[email protected]>Co-developed-by Aloka Dixit <[email protected]>Signed-off-by Aloka Dixit <[email protected]>---
v13: Replacedlist_for_each_entry_safe()bylist_for_each_entry()
Uh, ouch. I guess I miscommunicated that.
What I really meant was that the list iteration here:
Quoted 5 lines
net/mac80211/cfg.c +4
-0+if (vif->mbssid_tx_vif == vif) { +struct ieee80211_sub_if_data *child; + + list_for_each_entry(child, &sdata->local->interfaces,list)
does not seem correct, since it's not locked, and you don't have any guarantees that it's not being modified since the only thing here is that the vif pointer is coming in valid from the driver?
johannes