From 8fd20308ed53678c863a0ef52fb2c754e3adc63c Mon Sep 17 00:00:00 2001 From: Markus Fritsche Date: Fri, 8 May 2026 00:17:46 +0200 Subject: [PATCH 27/29] =?UTF-8?q?bes2600:=20Patch=20D=20=E2=80=94=20atomic?= =?UTF-8?q?ize=20ba=5Flock=20counters,=20drop=20the=20spinlock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The block-ack policy uses 4 int counters (ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx) bumped per data frame in the TX and RX hot paths under spin_lock_bh(&hw_priv->ba_lock). The lock was the heaviest per-frame synchronization cost remaining after Patch C v3 (which fixed the sdio_rx_work relay). Per the Opus structural critique (PR #8), this pattern matches mac80211 driver convention for per-frame statistics: atomic_t suffices, no lock needed. Field-by-field changes in struct bes2600_common: ba_acc, ba_cnt, ba_acc_rx, ba_cnt_rx: int -> atomic_t ba_armed: new atomic_t (timer-arm flag) ba_ena: bool -> atomic_t ba_lock: removed (spinlock_t deleted) ba_hist: int (single-writer = ba_timer) Producer hot path (txrx.c TX submit + RX receive): - atomic_add for the byte accumulator - atomic_inc for the frame counter - atomic_cmpxchg(&ba_armed, 0, 1) to claim the once-per-window mod_timer arm — at most ONE producer succeeds; race-free - no spin_lock_bh Consumer paths (sta.c bes2600_ba_timer, sta.c disconnect-reset, sta.c bes2600_ba_work, debug.c debugfs reader): - atomic_read snapshots all 4 counters into locals; the threshold predicate (acc/cnt >= THLD) tolerates approximate snapshots — the timer fires periodically, a single misclassification just delays the policy update by one tick - atomic_set zeroes the counters at end of timer-callback window; racing producer increments after the snapshot are lost (acceptable for stats; same approximation the original lock allowed under contention) - atomic_set(&ba_armed, 0) re-enables the next window's arm Followup-amenable simplification: ba_hist remains int because only the single ba_timer callback writes it; multiple writers would need to upgrade it too. This patch follows the cw1200-mainline-idiom established by Patch C v3 (structural fix, not bandaid). The cw1200 reference doesn't have a similar lock to compare; bes2600 inherited this from a later Bestechnic addition rather than the upstream tree. --- bes2600/bes2600.h | 26 ++++++++++------ bes2600/debug.c | 13 +++++--- bes2600/main.c | 2 +- bes2600/sta.c | 77 ++++++++++++++++++++++++++++------------------- bes2600/txrx.c | 23 ++++++++------ 5 files changed, 85 insertions(+), 56 deletions(-) diff --git a/drivers/staging/bes2600/bes2600.h b/drivers/staging/bes2600/bes2600.h index 84059c7..32bce5e 100644 --- a/drivers/staging/bes2600/bes2600.h +++ b/drivers/staging/bes2600/bes2600.h @@ -353,15 +353,23 @@ struct bes2600_common { * Keeping in common structure for the time being. Will be moved to VIFF * after the mechanism is clear */ u8 ba_tid_mask; - int ba_acc; /*TODO: Same as above */ - int ba_cnt; /*TODO: Same as above */ - int ba_cnt_rx; /*TODO: Same as above */ - int ba_acc_rx; /*TODO: Same as above */ - int ba_hist; /*TODO: Same as above */ - struct timer_list ba_timer;/*TODO: Same as above */ - spinlock_t ba_lock; /*TODO: Same as above */ - bool ba_ena; /*TODO: Same as above */ - struct work_struct ba_work; /*TODO: Same as above */ + /* + * Patch D: ba_lock removed. Per-frame TX/RX hot-path bumped these + * counters under spin_lock_bh; the lock did not protect any + * compound invariant that atomic ops can't satisfy. Counters are + * now atomic_t; ba_armed gates the once-per-window mod_timer + * arm via cmpxchg so concurrent TX/RX at a fresh window each + * try to claim the arm and exactly one succeeds. + */ + atomic_t ba_acc; + atomic_t ba_cnt; + atomic_t ba_cnt_rx; + atomic_t ba_acc_rx; + atomic_t ba_armed; + int ba_hist; + struct timer_list ba_timer; + atomic_t ba_ena; + struct work_struct ba_work; bool is_BT_Present; bool is_go_thru_go_neg; u8 conf_listen_interval; diff --git a/drivers/staging/bes2600/debug.c b/drivers/staging/bes2600/debug.c index 47e27be..0ab79c0 100644 --- a/drivers/staging/bes2600/debug.c +++ b/drivers/staging/bes2600/debug.c @@ -110,17 +110,20 @@ static int bes2600_status_show_common(struct seq_file *seq, void *v) int ba_cnt, ba_acc, ba_cnt_rx, ba_acc_rx, ba_avg = 0, ba_avg_rx = 0; bool ba_ena; - spin_lock_bh(&hw_priv->ba_lock); - ba_cnt = hw_priv->debug->ba_cnt; - ba_acc = hw_priv->debug->ba_acc; + /* + * Patch D: ba_lock removed. hw_priv->debug->ba_* are written only + * by the timer callback (single writer); reading without a lock is + * fine for stats. ba_ena is atomic_t. + */ + ba_cnt = hw_priv->debug->ba_cnt; + ba_acc = hw_priv->debug->ba_acc; ba_cnt_rx = hw_priv->debug->ba_cnt_rx; ba_acc_rx = hw_priv->debug->ba_acc_rx; - ba_ena = hw_priv->ba_ena; + ba_ena = !!atomic_read(&hw_priv->ba_ena); if (ba_cnt) ba_avg = ba_acc / ba_cnt; if (ba_cnt_rx) ba_avg_rx = ba_acc_rx / ba_cnt_rx; - spin_unlock_bh(&hw_priv->ba_lock); seq_puts(seq, "BES2600 Wireless LAN driver status\n"); seq_printf(seq, "Hardware: %d.%d\n", diff --git a/drivers/staging/bes2600/main.c b/drivers/staging/bes2600/main.c index 71dc4ae..8fc37b4 100644 --- a/drivers/staging/bes2600/main.c +++ b/drivers/staging/bes2600/main.c @@ -501,7 +501,7 @@ static struct ieee80211_hw *bes2600_init_common(size_t hw_priv_data_len) INIT_LIST_HEAD(&hw_priv->event_queue); INIT_WORK(&hw_priv->event_handler, bes2600_event_handler); INIT_WORK(&hw_priv->ba_work, bes2600_ba_work); - spin_lock_init(&hw_priv->ba_lock); + /* Patch D: ba_lock removed; ba_acc/ba_cnt/etc are atomic_t. */ timer_setup(&hw_priv->ba_timer, bes2600_ba_timer, 0); if (unlikely(bes2600_queue_stats_init(&hw_priv->tx_queue_stats, diff --git a/drivers/staging/bes2600/sta.c b/drivers/staging/bes2600/sta.c index 70b12f9..8af8150 100644 --- a/drivers/staging/bes2600/sta.c +++ b/drivers/staging/bes2600/sta.c @@ -2362,14 +2362,19 @@ void bes2600_join_work(struct work_struct *work) //WARN_ON(wsm_reset(hw_priv, &reset, priv->if_id)); WARN_ON(wsm_set_block_ack_policy(hw_priv, 0, hw_priv->ba_tid_mask, priv->if_id)); - spin_lock_bh(&hw_priv->ba_lock); - hw_priv->ba_ena = false; - hw_priv->ba_cnt = 0; - hw_priv->ba_acc = 0; + /* + * Patch D: ba_lock removed. Disconnect-reset clears the + * counters and the arm flag; producers racing here cannot + * cause harm — at worst they re-arm the timer and bump + * counters that will be cleared on the next timer tick. + */ + atomic_set(&hw_priv->ba_ena, 0); + atomic_set(&hw_priv->ba_cnt, 0); + atomic_set(&hw_priv->ba_acc, 0); hw_priv->ba_hist = 0; - hw_priv->ba_cnt_rx = 0; - hw_priv->ba_acc_rx = 0; - spin_unlock_bh(&hw_priv->ba_lock); + atomic_set(&hw_priv->ba_cnt_rx, 0); + atomic_set(&hw_priv->ba_acc_rx, 0); + atomic_set(&hw_priv->ba_armed, 0); mgmt_policy.protectedMgmtEnable = 0; mgmt_policy.unprotectedMgmtFramesAllowed = 1; @@ -2649,10 +2654,11 @@ void bes2600_ba_work(struct work_struct *work) return;*/ bes_devel("BA work****\n"); - spin_lock_bh(&hw_priv->ba_lock); -// tx_ba_tid_mask = hw_priv->ba_ena ? hw_priv->ba_tid_mask : 0; + /* + * Patch D: ba_lock removed. ba_tid_mask is u8 set once at init + * (main.c); reading it without a lock is fine. + */ tx_ba_tid_mask = hw_priv->ba_tid_mask; - spin_unlock_bh(&hw_priv->ba_lock); wsm_lock_tx(hw_priv); @@ -2665,37 +2671,49 @@ void bes2600_ba_work(struct work_struct *work) void bes2600_ba_timer(struct timer_list *t) { bool ba_ena; + int cnt, acc, cnt_rx, acc_rx; struct bes2600_common *hw_priv = timer_container_of(hw_priv, t, ba_timer); - spin_lock_bh(&hw_priv->ba_lock); - bes2600_debug_ba(hw_priv, hw_priv->ba_cnt, hw_priv->ba_acc, - hw_priv->ba_cnt_rx, hw_priv->ba_acc_rx); + /* + * Patch D: ba_lock removed. Snapshot atomic counters into locals + * for the predicate evaluation; producers may race incrementing + * after the snapshot but the resulting decision is approximate + * which the policy already tolerates (next timer tick re-evaluates). + */ + cnt = atomic_read(&hw_priv->ba_cnt); + acc = atomic_read(&hw_priv->ba_acc); + cnt_rx = atomic_read(&hw_priv->ba_cnt_rx); + acc_rx = atomic_read(&hw_priv->ba_acc_rx); + + bes2600_debug_ba(hw_priv, cnt, acc, cnt_rx, acc_rx); if (atomic_read(&hw_priv->scan.in_progress)) { - hw_priv->ba_cnt = 0; - hw_priv->ba_acc = 0; - hw_priv->ba_cnt_rx = 0; - hw_priv->ba_acc_rx = 0; - goto skip_statistic_update; + atomic_set(&hw_priv->ba_cnt, 0); + atomic_set(&hw_priv->ba_acc, 0); + atomic_set(&hw_priv->ba_cnt_rx, 0); + atomic_set(&hw_priv->ba_acc_rx, 0); + atomic_set(&hw_priv->ba_armed, 0); + return; } - if (hw_priv->ba_cnt >= BES2600_BLOCK_ACK_CNT && - (hw_priv->ba_acc / hw_priv->ba_cnt >= BES2600_BLOCK_ACK_THLD || - (hw_priv->ba_cnt_rx >= BES2600_BLOCK_ACK_CNT && - hw_priv->ba_acc_rx / hw_priv->ba_cnt_rx >= + if (cnt >= BES2600_BLOCK_ACK_CNT && + (acc / cnt >= BES2600_BLOCK_ACK_THLD || + (cnt_rx >= BES2600_BLOCK_ACK_CNT && + acc_rx / cnt_rx >= BES2600_BLOCK_ACK_THLD))) ba_ena = true; else ba_ena = false; - hw_priv->ba_cnt = 0; - hw_priv->ba_acc = 0; - hw_priv->ba_cnt_rx = 0; - hw_priv->ba_acc_rx = 0; + atomic_set(&hw_priv->ba_cnt, 0); + atomic_set(&hw_priv->ba_acc, 0); + atomic_set(&hw_priv->ba_cnt_rx, 0); + atomic_set(&hw_priv->ba_acc_rx, 0); + atomic_set(&hw_priv->ba_armed, 0); - if (ba_ena != hw_priv->ba_ena) { + if (ba_ena != !!atomic_read(&hw_priv->ba_ena)) { if (ba_ena || ++hw_priv->ba_hist >= BES2600_BLOCK_ACK_HIST) { - hw_priv->ba_ena = ba_ena; + atomic_set(&hw_priv->ba_ena, ba_ena ? 1 : 0); hw_priv->ba_hist = 0; #if 0 bes_devel("[STA] %s block ACK:\n", @@ -2705,9 +2723,6 @@ void bes2600_ba_timer(struct timer_list *t) } } else if (hw_priv->ba_hist) --hw_priv->ba_hist; - -skip_statistic_update: - spin_unlock_bh(&hw_priv->ba_lock); } int bes2600_vif_setup(struct bes2600_vif *priv) diff --git a/drivers/staging/bes2600/txrx.c b/drivers/staging/bes2600/txrx.c index 3aef009..536b198 100644 --- a/drivers/staging/bes2600/txrx.c +++ b/drivers/staging/bes2600/txrx.c @@ -996,14 +996,18 @@ bes2600_tx_h_ba_stat(struct bes2600_vif *priv, if (!ieee80211_is_data(t->hdr->frame_control)) return; - spin_lock_bh(&hw_priv->ba_lock); - hw_priv->ba_acc += t->skb->len - t->hdrlen; - if (!(hw_priv->ba_cnt_rx || hw_priv->ba_cnt)) { + /* + * Patch D: lock-free hot-path BA accounting. atomic_inc + atomic_add + * each per-frame; the once-per-window timer-arm uses cmpxchg on + * ba_armed so concurrent TX/RX can't both try to set the timer and + * we don't need cross-counter coherency on the ba_cnt/ba_cnt_rx pair. + */ + atomic_add(t->skb->len - t->hdrlen, &hw_priv->ba_acc); + atomic_inc(&hw_priv->ba_cnt); + if (atomic_cmpxchg(&hw_priv->ba_armed, 0, 1) == 0) { mod_timer(&hw_priv->ba_timer, jiffies + BES2600_BLOCK_ACK_INTERVAL); } - hw_priv->ba_cnt++; - spin_unlock_bh(&hw_priv->ba_lock); } static int @@ -1651,14 +1655,13 @@ bes2600_rx_h_ba_stat(struct bes2600_vif *priv, if (!priv->setbssparams_done) return; - spin_lock_bh(&hw_priv->ba_lock); - hw_priv->ba_acc_rx += skb_len - hdrlen; - if (!(hw_priv->ba_cnt_rx || hw_priv->ba_cnt)) { + /* Patch D: lock-free hot-path BA accounting; see TX side comment. */ + atomic_add(skb_len - hdrlen, &hw_priv->ba_acc_rx); + atomic_inc(&hw_priv->ba_cnt_rx); + if (atomic_cmpxchg(&hw_priv->ba_armed, 0, 1) == 0) { mod_timer(&hw_priv->ba_timer, jiffies + BES2600_BLOCK_ACK_INTERVAL); } - hw_priv->ba_cnt_rx++; - spin_unlock_bh(&hw_priv->ba_lock); } void bes2600_rx_cb(struct bes2600_vif *priv, -- 2.54.0