On Mon, Jun 4, 2012 at 9:16 AM, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote: > On Sun, 2012-06-03 at 23:30 +0300, Arik Nemtsov wrote: > >> +++ b/net/mac80211/rx.c >> @@ -3023,6 +3023,9 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb) >> if (WARN_ON(!sband)) >> goto drop; >> >> + /* make sure we get the latest values for the next variable checks */ >> + smp_rmb(); >> + > > That doesn't make a lot of sense I think? This entire thing is > inherently racy, you could be processing RX already even while just > setting the variable in the other code. I'm not really sure what the > effect of this here is, but it's certainly a hot-path. Yea I wondered about that myself, but I saw spinlocks are being used later inside ieee80211_rx_handlers(), so there's a memory barrier anyway IIUC. You're right that we still can get very old RX from before the reconfig, but this is not the one I was worried about (and it doesn't happen in practice). This one worries me (we actually had something similar) 1. reconfig starts 2. AP starts beaconing 3. remote sta sends ADDBA 4. remote sta is added by mac80211 Since in_reconfig is set before the start of the AP, in this sense we won't get state changes during the reconfig. > >> + /* >> + * Stop all Rx during the reconfig. We don't want state changes >> + * or driver callbacks while this is in progress. >> + */ >> + local->in_reconfig = true; >> + smp_wmb(); > > > If anything, you should probably put this right into > ieee80211_restart_hw() rather than the reconfig function? It's not > needed for suspend/resume anyway since we check that separately. Sure, even earlier is better I guess. Arik -- 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
