| Message ID | 1370895442-21784-2-git-send-email-thomas@cozybit.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, 2013-06-10 at 13:17 -0700, Thomas Pedersen wrote: > + struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; > + u32 bit; > + > + /* if we race with running work, worst case this work becomes a noop */ > + for_each_set_bit(bit, (unsigned long *)&changed, > + sizeof(changed) * BITS_PER_BYTE) This isn't valid, it happens to work on little endian platforms but will fail on big endian 64-bit ones, because you have this in memory (0 is the lowest order nibble): 76 54 32 10 -- -- -- -- and now you point an unsigned long pointer to it, so you interpret the "--" as the lowest bits. More generally, I'd argue that mesh is being a bit odd here, flushing stations turing mesh stop can and will actually cause a BSS info update after the mesh interface has already been stopped (beaconing has been disabled in the driver.) This seems rather odd. Maybe it would be better to move the beacon update out of mesh_sta_cleanup() and into ieee80211_mesh_housekeeping() in some way? Although it'd also have to be done in the station handling in cfg.c but that shouldn't be a problem? Note also that the way you did this is rather odd, ieee80211_stop_mesh() could cause to schedule out to the workqueue for the update, but then the update won't happen. It's a bit racy though, because you could stop and restart the mesh and then the workqueue runs or something? Overall this approach seems a bit brittle? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 11, 2013 at 4:30 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2013-06-10 at 13:17 -0700, Thomas Pedersen wrote: > >> + struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; >> + u32 bit; >> + >> + /* if we race with running work, worst case this work becomes a noop */ >> + for_each_set_bit(bit, (unsigned long *)&changed, >> + sizeof(changed) * BITS_PER_BYTE) > > This isn't valid, it happens to work on little endian platforms but will > fail on big endian 64-bit ones, because you have this in memory (0 is > the lowest order nibble): > > 76 54 32 10 -- -- -- -- > and now you point an unsigned long pointer to it, so you interpret the > "--" as the lowest bits. OK I was just trying to make the compiler happy, but that makes sense. Assigning changed (u32) to an unsigned long then getting the address of that should move the u32 into the lower half of an unsigned long on a 64-bit BE system? Thanks for explaining this. > More generally, I'd argue that mesh is being a bit odd here, flushing > stations turing mesh stop can and will actually cause a BSS info update > after the mesh interface has already been stopped (beaconing has been > disabled in the driver.) This seems rather odd. Maybe it would be better > to move the beacon update out of mesh_sta_cleanup() and into > ieee80211_mesh_housekeeping() in some way? Although it'd also have to be > done in the station handling in cfg.c but that shouldn't be a problem? Yes it is odd to queue a bss info update but never do so. I don't know if it really matters though. The problem is mesh_sta_cleanup() is called from several paths: mac80211 mesh runtime, stop_mesh(), and cfg80211. I think this is a fairly clean way of satisfying all the users (mesh work queued from stop_mesh() is a noop if we check ifmsh->mesh_id_len instead of the wdev->mesh_id_len). It sounds like you'd like beacon updates to be asynchronous, which this patch already accomplishes :) > Note also that the way you did this is rather odd, ieee80211_stop_mesh() > could cause to schedule out to the workqueue for the update, but then > the update won't happen. It's a bit racy though, because you could stop > and restart the mesh and then the workqueue runs or something? Overall > this approach seems a bit brittle? I guess if you clear the ifmsh->wrkq_flags at the end of stop_mesh() this wouldn't happen. Also as long as the check to ensure no mesh work is performed while not joined is in place, we should be ok. -- Thomas -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 7a6f1a0..f79156d 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -543,6 +543,7 @@ struct ieee80211_if_mesh { struct timer_list mesh_path_root_timer; unsigned long wrkq_flags; + unsigned long mbss_changed; u8 mesh_id[IEEE80211_MAX_MESH_ID_LEN]; size_t mesh_id_len; diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index d5faf91..2499679 100644 --- a/net/mac80211/mesh.c +++ b/net/mac80211/mesh.c @@ -161,11 +161,8 @@ void mesh_sta_cleanup(struct sta_info *sta) del_timer_sync(&sta->plink_timer); } - if (changed) { - sdata_lock(sdata); + if (changed) ieee80211_mbss_info_change_notify(sdata, changed); - sdata_unlock(sdata); - } } int mesh_rmc_init(struct ieee80211_sub_if_data *sdata) @@ -719,14 +716,15 @@ ieee80211_mesh_rebuild_beacon(struct ieee80211_sub_if_data *sdata) void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata, u32 changed) { - if (sdata->vif.bss_conf.enable_beacon && - (changed & (BSS_CHANGED_BEACON | - BSS_CHANGED_HT | - BSS_CHANGED_BASIC_RATES | - BSS_CHANGED_BEACON_INT))) - if (ieee80211_mesh_rebuild_beacon(sdata)) - return; - ieee80211_bss_info_change_notify(sdata, changed); + struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; + u32 bit; + + /* if we race with running work, worst case this work becomes a noop */ + for_each_set_bit(bit, (unsigned long *)&changed, + sizeof(changed) * BITS_PER_BYTE) + set_bit(BIT(bit), &ifmsh->mbss_changed); + set_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags); + ieee80211_queue_work(&sdata->local->hw, &sdata->work); } int ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata) @@ -969,6 +967,26 @@ out: sdata_unlock(sdata); } +static void mesh_bss_info_changed(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; + u32 bit, changed = 0; + + for_each_set_bit(bit, (unsigned long *)&ifmsh->mbss_changed, + sizeof(changed) * BITS_PER_BYTE) + changed |= test_and_clear_bit(BIT(bit), &ifmsh->mbss_changed); + + if (sdata->vif.bss_conf.enable_beacon && + (changed & (BSS_CHANGED_BEACON | + BSS_CHANGED_HT | + BSS_CHANGED_BASIC_RATES | + BSS_CHANGED_BEACON_INT))) + if (ieee80211_mesh_rebuild_beacon(sdata)) + return; + + ieee80211_bss_info_change_notify(sdata, changed); +} + void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata) { struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; @@ -999,6 +1017,8 @@ void ieee80211_mesh_work(struct ieee80211_sub_if_data *sdata) if (test_and_clear_bit(MESH_WORK_DRIFT_ADJUST, &ifmsh->wrkq_flags)) mesh_sync_adjust_tbtt(sdata); + if (test_and_clear_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags)) + mesh_bss_info_changed(sdata); out: sdata_unlock(sdata); } diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h index 8b4d9a3..be28f9b 100644 --- a/net/mac80211/mesh.h +++ b/net/mac80211/mesh.h @@ -57,6 +57,7 @@ enum mesh_path_flags { * grow * @MESH_WORK_ROOT: the mesh root station needs to send a frame * @MESH_WORK_DRIFT_ADJUST: time to compensate for clock drift relative to other + * @MESH_WORK_MBSS_CHANGED: rebuild beacon and notify driver of BSS changes * mesh nodes */ enum mesh_deferred_task_flags { @@ -65,6 +66,7 @@ enum mesh_deferred_task_flags { MESH_WORK_GROW_MPP_TABLE, MESH_WORK_ROOT, MESH_WORK_DRIFT_ADJUST, + MESH_WORK_MBSS_CHANGED, }; /**
Fixes yet another deadlock on calling sta_info_flush() with the sdata_lock() held. Should make it easier to reason about locking in the future, since the sdata_lock() is now held on all mesh work. Signed-off-by: Thomas Pedersen <thomas@cozybit.com> --- v2: read all changed bits & drop macro (Johannes) net/mac80211/ieee80211_i.h | 1 + net/mac80211/mesh.c | 44 ++++++++++++++++++++++++++++++++------------ net/mac80211/mesh.h | 2 ++ 3 files changed, 35 insertions(+), 12 deletions(-)