On Thu, 2011-11-24 at 13:33 -0500, Nikolay Martynov wrote:
> > Looks OK to me. Did you run it through sparse too? :-)
> >
> Please forgive my lack of experience, but could you please point me
> to some docs where I can read about sparse and how to use it? I'd
> really appreciate this.
Sure, FWIW, I'm just asking because I received lots of patches recently
that later got sparse warnings. I guess you normally run something like
make M=net/mac80211
To run sparse, you add C=1 (just run sparse if cc runs) or C=2 (run
sparse on all files). Obviously you need to install sparse first, your
distribution probably has packages.
> Also, I one more thing I forgot to mention in original cover letter.
> Some time before this patch I've posted patch for ath9k: "[PATCH v3]
> ath9k: improve ath_tx_aggr_stop to avoid TID stuck in cleanup state".
> I've noticed that bug fixed in ath9k patch is triggered much more
> often when this patch to mac80211 applied. Probably because of some
> timing issues when TID is being closed from both ends at about same
> time.
> What I mean to say is that this patch to mac80211 should probably be
> applied after that patch to ath9k to avoid making ath9k less stable.
You should send that info to the list not just me, I've added the list
back.
> And one more thing, if you do not mind.
> 'ieee80211_stop_tx_ba_session' tests tx_tid->state for
> HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
> '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
> sta->lock is not being held. It seems to be that this could lead to
> problems if these two functions get called at about same time (which
> could easily happen when this patch is applied). Although actual
> window when lock is not help is really small. But I think the way it
> is currently done could still lead to problems. I think
> 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
> to be covered by spin lock. I can include this as a part of next
> version of my patch. I'd really appreciate your thoughts about this.
Yeah that does indeed seem like an issue -- please send a separate patch
outside of this series and add Cc: [email protected] (to the patch
description not to the actual email, if you're not familiar with the
stable submission rules please check for those)
johannes
Hi
2011/11/24 Johannes Berg <[email protected]>:
> On Thu, 2011-11-24 at 13:33 -0500, Nikolay Martynov wrote:
>
>> > Looks OK to me. Did you run it through sparse too? :-)
>> >
>> ? Please forgive my lack of experience, but could you please point me
>> to some docs where I can read about sparse and how to use it? I'd
>> really appreciate this.
>
> Sure, FWIW, I'm just asking because I received lots of patches recently
> that later got sparse warnings. I guess you normally run something like
>
> make M=net/mac80211
>
> To run sparse, you add C=1 (just run sparse if cc runs) or C=2 (run
> sparse on all files). Obviously you need to install sparse first, your
> distribution probably has packages.
>
I wasn't able to find any sparse warnings introduced by my changes,
everything looks fine.
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]
On Thu, 2011-11-24 at 23:43 -0500, Nikolay Martynov wrote:
> >> > Looks OK to me. Did you run it through sparse too? :-)
> I wasn't able to find any sparse warnings introduced by my changes,
> everything looks fine.
Great, thanks for checking.
johannes
On Thu, 2011-11-24 at 19:39 +0100, Johannes Berg wrote:
> > And one more thing, if you do not mind.
> > 'ieee80211_stop_tx_ba_session' tests tx_tid->state for
> > HT_AGG_STATE_STOPPING holding only spinlock on sta->lock.
> > '___ieee80211_stop_tx_ba_session' sets this bit when spin lock on
> > sta->lock is not being held. It seems to be that this could lead to
> > problems if these two functions get called at about same time (which
> > could easily happen when this patch is applied). Although actual
> > window when lock is not help is really small. But I think the way it
> > is currently done could still lead to problems. I think
> > 'set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);' should be moved up
> > to be covered by spin lock. I can include this as a part of next
> > version of my patch. I'd really appreciate your thoughts about this.
>
> Yeah that does indeed seem like an issue
Actually I think the right way to fix it would be the change below, do
you agree? I think just moving the set_bit doesn't help because other
code might call right into ___ieee80211_stop_tx_ba_session (or with just
two underscores on front)
johannes
--- wireless-testing.orig/net/mac80211/agg-tx.c 2011-11-09 10:07:34.000000000 +0100
+++ wireless-testing/net/mac80211/agg-tx.c 2011-11-24 19:44:51.000000000 +0100
@@ -162,6 +162,12 @@ int ___ieee80211_stop_tx_ba_session(stru
return -ENOENT;
}
+ /* if we're already stopping ignore any new requests to stop */
+ if (test_bit(HT_AGG_STATE_STOPPING, &tid_tx->state)) {
+ spin_unlock_bh(&sta->lock);
+ return -EALREADY;
+ }
+
if (test_bit(HT_AGG_STATE_WANT_START, &tid_tx->state)) {
/* not even started yet! */
ieee80211_assign_tid_tx(sta, tid, NULL);
@@ -170,6 +176,8 @@ int ___ieee80211_stop_tx_ba_session(stru
return 0;
}
+ set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
+
spin_unlock_bh(&sta->lock);
#ifdef CONFIG_MAC80211_HT_DEBUG
@@ -177,8 +185,6 @@ int ___ieee80211_stop_tx_ba_session(stru
sta->sta.addr, tid);
#endif /* CONFIG_MAC80211_HT_DEBUG */
- set_bit(HT_AGG_STATE_STOPPING, &tid_tx->state);
-
del_timer_sync(&tid_tx->addba_resp_timer);
/*