[PATCH AUTOSEL 6.19-5.10] wifi: ath10k: fix lock protection in ath10k_wmi_event_peer_sta_ps_state_chg()

[PATCH AUTOSEL 6.19-5.10] wifi: ath10k: fix lock protection in ath10k_wmi_event_peer_sta_ps_state_chg()

[Sasha Levin]

From: Ziyi Guo <n7l8m4@u.northwestern.edu>

[ Upstream commit 820ba7dd6859ef8b1eaf6014897e7aa4756fc65d ]

ath10k_wmi_event_peer_sta_ps_state_chg() uses lockdep_assert_held() to
assert that ar->data_lock should be held by the caller, but neither
ath10k_wmi_10_2_op_rx() nor ath10k_wmi_10_4_op_rx() acquire this lock
before calling this function.

The field arsta->peer_ps_state is documented as protected by
ar->data_lock in core.h, and other accessors (ath10k_peer_ps_state_disable,
ath10k_dbg_sta_read_peer_ps_state) properly acquire this lock.

Add spin_lock_bh()/spin_unlock_bh() around the peer_ps_state update,
and remove the lockdep_assert_held() to be aligned with new locking,
following the pattern used by other WMI event handlers in the driver.

Signed-off-by: Ziyi Guo <n7l8m4@u.northwestern.edu>
Reviewed-by: Baochen Qiang <baochen.qiang@oss.qualcomm.com>
Link: https://patch.msgid.link/20260123175611.767731-1-n7l8m4@u.northwestern.edu
[removed excess blank line]
Signed-off-by: Jeff Johnson <jeff.johnson@oss.qualcomm.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Analysis of wifi: ath10k: fix lock protection in
ath10k_wmi_event_peer_sta_ps_state_chg()

### 1. Commit Message Analysis

The commit message is clear and well-structured:
- **Subject**: Explicitly says "fix lock protection" — this is a
  locking/synchronization bug fix
- **Body**: Explains that `lockdep_assert_held()` asserted
  `ar->data_lock` should be held by callers, but **no caller actually
  held it**. This means the assertion was always wrong (or always
  disabled), and the field `arsta->peer_ps_state` was being accessed
  without the required lock protection.
- **Reviewed-by**: Baochen Qiang from Qualcomm reviewed it, lending
  credibility
- The commit references that `core.h` documents `peer_ps_state` as
  protected by `ar->data_lock`, and other accessors properly acquire it
  — meaning this was the only broken path.

### 2. Code Change Analysis

The change is minimal and surgical:

1. **Removes** `lockdep_assert_held(&ar->data_lock)` — the callers never
   held this lock, so the assertion was incorrect (and likely only
   checked with CONFIG_LOCKDEP enabled, which is why it didn't always
   trigger)

2. **Adds** `spin_lock_bh(&ar->data_lock)` /
   `spin_unlock_bh(&ar->data_lock)` around the single line
   `arsta->peer_ps_state = __le32_to_cpu(ev->peer_ps_state)` — this is
   the actual fix, properly protecting the field with the documented
   lock

The lock scope is minimal — only around the single write to
`peer_ps_state`, placed after the RCU-protected station lookup and
before the RCU read unlock. This is clean and correct.

### 3. Bug Classification

This is a **data race / missing synchronization** bug:
- The field `peer_ps_state` is documented as requiring `ar->data_lock`
  protection
- Other readers/writers of this field properly acquire the lock
- This WMI event handler was the only path that didn't hold the lock
- Without the lock, concurrent reads (from
  `ath10k_dbg_sta_read_peer_ps_state`) and writes could race, leading to
  torn reads or inconsistent state

### 4. Scope and Risk Assessment

- **Lines changed**: ~4 lines effective (remove 2 lines, add 3 lines
  including lock/unlock)
- **Files changed**: 1 file (`drivers/net/wireless/ath/ath10k/wmi.c`)
- **Risk**: Very low. The change adds proper locking around a single
  field access, following the established pattern used by all other
  accessors. The lock is `spin_lock_bh`, which is safe in this softirq
  context.
- **Could it break something?** Extremely unlikely — it adds a lock that
  was already supposed to be held, and uses the same locking pattern as
  other paths in the driver.

### 5. User Impact

- **Affected hardware**: ath10k WiFi devices (Qualcomm 802.11ac chipsets
  — very common in laptops and embedded systems)
- **Trigger**: WMI peer power-save state change events from firmware
- **Consequence of bug**: Data race on `peer_ps_state` field. While this
  is a 32-bit field and the race may not always cause visible corruption
  on most architectures, it violates the documented locking contract and
  could cause issues with lockdep-enabled kernels (warnings/splats). On
  architectures without atomic 32-bit writes, it could cause torn reads.
- **Severity**: Medium — it's a real locking bug in a commonly-used WiFi
  driver

### 6. Stability Indicators

- Reviewed by Qualcomm engineer (Baochen Qiang)
- Accepted by the ath10k maintainer (Jeff Johnson)
- Small, obvious, and following established patterns in the same driver

### 7. Dependency Check

- No dependencies on other commits
- The code being modified has existed in ath10k for a long time (the
  `lockdep_assert_held` suggests the locking was always intended but
  never correctly implemented in this path)
- Should apply cleanly to any stable tree that has the ath10k driver
  with this function

### 8. Stable Kernel Criteria

- **Obviously correct?** Yes — adds the documented lock around a field
  access
- **Fixes a real bug?** Yes — data race / missing synchronization
- **Small and contained?** Yes — 4 lines in 1 file
- **No new features?** Correct — pure bug fix
- **Tested?** Reviewed by subsystem experts, accepted by maintainer

### Conclusion

This is a textbook stable-worthy fix: a small, surgical correction to a
locking bug in a widely-used WiFi driver. It properly adds the lock that
was always documented as required but never acquired in this code path.
The risk is minimal and the fix follows established patterns in the same
driver.

**YES**

 drivers/net/wireless/ath/ath10k/wmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index b4aad6604d6d9..ce22141e5efd9 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -5289,8 +5289,6 @@ ath10k_wmi_event_peer_sta_ps_state_chg(struct ath10k *ar, struct sk_buff *skb)
 	struct ath10k_sta *arsta;
 	u8 peer_addr[ETH_ALEN];
 
-	lockdep_assert_held(&ar->data_lock);
-
 	ev = (struct wmi_peer_sta_ps_state_chg_event *)skb->data;
 	ether_addr_copy(peer_addr, ev->peer_macaddr.addr);
 
@@ -5305,7 +5303,9 @@ ath10k_wmi_event_peer_sta_ps_state_chg(struct ath10k *ar, struct sk_buff *skb)
 	}
 
 	arsta = (struct ath10k_sta *)sta->drv_priv;
+	spin_lock_bh(&ar->data_lock);
 	arsta->peer_ps_state = __le32_to_cpu(ev->peer_ps_state);
+	spin_unlock_bh(&ar->data_lock);
 
 exit:
 	rcu_read_unlock();
-- 
2.51.0