On Friday, August 22, 2014 04:23:19 PM Eric Dumazet wrote: > On Fri, 2014-08-22 at 23:53 +0200, Christian Lamparter wrote: > > > The sta_info->agg[tid] check is not needed (for reference, see [0]). > > (There is already a check in mac80211 which prevents the leak of > > sta_info->agg[tid] [1]). > > > > Regards > > Christian > > > > [0] <https://lkml.org/lkml/2014/8/20/725> > > [1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583> > > > > Hmpfff... this code is quite confusing. That's true. Furthermore, parts of the logic are also embedded in the mac80211-stack and above. So, it's very hard to see the whole big picture, just by looking at the driver code. > RCU is used both in tricky way (carl9170_ampdu_gc() is an example) > and a talisman (the part you remove) I know that game ;-). But fair enough: if you have concerns about the complexity of the code in question: I'm willing to help you and explain the quirks in detail if necessary. I think this is a valuable addition, since "external consultants" are hard to come by. > Why is rcu_assign_pointer(sta_info->agg[tid], tid_info); > done inside the spinlock protected region, I don't know. The pointer in sta_info->agg[tid] is used exclusively by the tx.c code... It is queried only if an outgoing frame has the IEEE80211_TX_CTL_AMPDU flag set. But for this flag to be set, the aggregation session has to be operational. This requires two calls to ampdu_action [0]. (first with: IEEE80211_AMPDU_TX_START and later with: IEEE80211_AMPDU_TX_OPERATIONAL). => If you want to make a patch to move this rcu_assign_pointer(...) after the spin_unlock_bh() - Then: Yes, GO FOR IT! > If this code relies on external protection, a comment would help its > comprehension for sure. > > For example, you could add a > BUG_ON(rcu_access_pointer(sta_info->agg[tid])); > so that we are sure requirements are not changed > in the callers one day. Maybe, but then: Is a "specific driver" the right place for this? Other drivers may also depend on ampdu_action not changing. As for the logic: The AMPDU handshake itself is part of the 802.11 spec. If you are interested you can get 802.11-2012 [1] and look into Section 9.21 "Block Acknowledgment". It contains a message sequence chart and details about the setup and tear down procedures for aggregation session [which is at the heart of the ampdu_action callback issue]. Note: mac80211 has a "software simulator" mac80211_hwsim [2]. It can be (and is) used to test most of the mac80211 functionality. So what do you think? Regards Christian [0] <https://www.kernel.org/doc/htmldocs/80211/aggregation.html> [1] <http://standards.ieee.org/findstds/standard/802.11-2012.html> [2] <http://wireless.kernel.org/en/users/Drivers/mac80211_hwsim> -- 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
