On 10 March 2014 17:27, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
> On Wed, 2014-03-05 at 15:27 +0100, Michal Kazior wrote:
>
>> +void ieee80211_recalc_csa_block_tx(struct ieee80211_local *local)
>> +{
>> + lockdep_assert_held(&local->mtx);
>> +
>> + if (ieee80211_csa_needs_block_tx(local))
>> + ieee80211_stop_queues_by_reason(&local->hw,
>> + IEEE80211_MAX_QUEUE_MAP,
>> + IEEE80211_QUEUE_STOP_REASON_CSA);
>> + else
>> + ieee80211_wake_queues_by_reason(&local->hw,
>> + IEEE80211_MAX_QUEUE_MAP,
>> + IEEE80211_QUEUE_STOP_REASON_CSA);
>> +}
>
> I don't like this function, even if technically stop is idempotent, it
> still seems strange to call this "recalc".
>
>> @@ -1092,7 +1131,11 @@ static int ieee80211_stop_ap(struct wiphy *wiphy, struct net_device *dev)
>> old_probe_resp = sdata_dereference(sdata->u.ap.probe_resp, sdata);
>>
>> /* abort any running channel switch */
>> + mutex_lock(&local->mtx);
>> sdata->vif.csa_active = false;
>> + ieee80211_recalc_csa_block_tx(local);
>> + mutex_unlock(&local->mtx);
>
> In fact, here you don't care about the stop queues part at all, afaict.
I wanted to make the stop/wake code decision in one place. I
understand you prefer to have:
if (can_wake)
wake()
and
if (!can_wake)
stop()
calls explicitly instead of the "recalc" function?
>> sdata->csa_chandef = params->chandef;
>> + sdata->csa_block_tx = params->block_tx;
>
> doesn't that more belong to the previous patch?
It belongs here.
CSA tx queue blocking is global. If you want to support multi- CSA
then you need to keep queues blocked until last CSA that requires tx
being blocked is completed. To do that you need to store which
interface CSA requested block_tx.
I should probably include this note in the commit message.
Michał
--
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
