Re: [PATCH ath-next] wifi: ath12k: accept num_users equal to HTT_PPDU_STATS_MAX_USERS

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

On 11/1/2026 12:37 PM, Goran Sustek wrote:
> ath12k_htt_pull_ppdu_stats() rejects a PPDU stats message when the
> common TLV reports num_users >= HTT_PPDU_STATS_MAX_USERS. The check
> guards a loop that iterates i < num_users over user_stats[], which has
> HTT_PPDU_STATS_MAX_USERS entries, so num_users equal to
> HTT_PPDU_STATS_MAX_USERS stays in bounds. Commit 1bc44a505a22 ("wifi:
> ath12k: fix possible out-of-bound read in ath12k_htt_pull_ppdu_stats()")
> rejects that value, which is off by one.
> 
> QCN9274 firmware WLAN.WBE.1.6-01243 sends num_users 8 in messages that
> otherwise parse cleanly. One AP logged the resulting warning 468 times
> between 2026-08-17 and 2026-11-01, always with the value 8, and never
> logged "Failed to parse tlv":
> 
>   HTT PPDU STATS event has unexpected num_users 8, should be smaller than 8
> 
> Accept num_users == HTT_PPDU_STATS_MAX_USERS.
> 
> Rejecting a message has no functional effect: the PPDU entry stays on
> the list, its per-user statistics are applied when it is evicted, and
> the only caller ignores the return value. A larger value is therefore
> not worth a warning either, so report it at debug level, as done in
> "wifi: ath12k: downgrade log level for CE buffer enqueue failure".
> 
> Fixes: 1bc44a505a22 ("wifi: ath12k: fix possible out-of-bound read in ath12k_htt_pull_ppdu_stats()")
> Signed-off-by: Goran Sustek <gsustek@gmail.com>
> ---
> Notes for reviewers:
> 
> The check guards two loops that can never run. Both are gated on
> ppdu_info->tlv_bitmap, frame_type, delay_ba and bar_num_users, and the
> first also on the per-user delay_ba; none of these is written anywhere
> in ath12k, not since the initial driver (2022). The parser that would
> fill them (per-user common TLV, MU-BAR user count) was never ported, so
> ath12k_copy_to_delay_stats(), ath12k_copy_to_bar() and the peer's
> ppdu_stats_delayba/delayba_flag are unreachable as well.
> 
> If you prefer, I have a patch ready that removes the loops, the helpers,
> the fields and this check instead (about 90 lines, no functional
> change). Keeping them only makes sense if delayed block ack accounting
> is going to be implemented. In that case the BAR restore loop also
> needs a bound (it iterates bar_num_users over user_stats[] unchecked),
> and note that the per-user slot finder only uses
> HTT_PPDU_STATS_MAX_USERS - 1 slots.
> 
> Compile tested only (W=1).
> 
>  drivers/net/wireless/ath/ath12k/dp_htt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp_htt.c b/drivers/net/wireless/ath/ath12k/dp_htt.c
> index 72069640bd38..9a409dd44d98 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_htt.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_htt.c
> @@ -487,12 +487,12 @@ static int ath12k_htt_pull_ppdu_stats(struct ath12k_base *ab,
>  		goto exit;
>  	}
>  
> -	if (ppdu_info->ppdu_stats.common.num_users >= HTT_PPDU_STATS_MAX_USERS) {
> +	if (ppdu_info->ppdu_stats.common.num_users > HTT_PPDU_STATS_MAX_USERS) {
>  		spin_unlock_bh(&dp_pdev->ppdu_list_lock);
> -		ath12k_warn(ab,
> -			    "HTT PPDU STATS event has unexpected num_users %u, should be smaller than %u\n",
> -			    ppdu_info->ppdu_stats.common.num_users,
> -			    HTT_PPDU_STATS_MAX_USERS);
> +		ath12k_dbg(ab, ATH12K_DBG_DP_HTT,
> +			   "HTT PPDU STATS event has unexpected num_users %u, should not exceed %u\n",
> +			   ppdu_info->ppdu_stats.common.num_users,
> +			   HTT_PPDU_STATS_MAX_USERS);

please don't change the warning. this is flagging a discrepancy between the
driver and the firmware and should be reported if it occurs in the future
(i.e. if a future firmware supports more users but the driver isn't updated)

>  		ret = -EINVAL;
>  		goto exit;
>  	}
> 
> base-commit: a3ddbb45768717b5c9f923489387cf55c838d1ad

My review agent says there is lot of associated code that is also treating
HTT_PPDU_STATS_MAX_USERS inconsistently. Much of this is directly inherited
from ath11k. I'm posting these finding here so that the developers can further
evaluate what approach to take with this patch.

BCC: Qualcomm's ath12k internal review list

/jeff


❯ can you provide a list of references to the patched version of the ath12k
  code where num_users == 8 isn't properly handled?

Here's the corrected and complete list, now reflecting that the firmware
genuinely sends 8 users and the 8th is silently discarded. All sites are in
drivers/net/wireless/ath/ath12k/dp_htt.c unless noted.

1. The 8th user is silently dropped during parse (root cause)

- dp_htt.c:49 — ath12k_get_ppdu_user_index(), loop i <
HTT_PPDU_STATS_MAX_USERS - 1 (0..6). Can only allocate slots 0..6, so the 8th
user never gets a slot and the function returns -EINVAL for it.
- dp_htt.c:95-96, :115-116, :135-137 — the three parse cases (USR_RATE,
USR_COMPLTN_COMMON, USR_COMPLTN_ACK_BA_STATUS): cur_user =
ath12k_get_ppdu_user_index(...); if (cur_user < 0) return -EINVAL;. For the
8th user this returns -EINVAL, so its rate / completion / ack-BA TLVs are not
stored.
- dp_htt.c:176-178 — ath12k_dp_htt_tlv_iter(): if (ret == -ENOMEM) return ret;
— only -ENOMEM aborts. The -EINVAL from the 8th user is swallowed, the
iterator continues and returns 0 (:183). This is why the drop is silent and
:486 ("Failed to parse tlv") never fires — reconciling the commit's field
evidence with 8 real users.

2. The 8th user's stats are never reported (consumer side)

- dp_htt.c:365 — ath12k_htt_update_ppdu_stats(), loop user <
HTT_PPDU_STATS_MAX_USERS - 1 (0..6). At eviction this pushes per-peer TX stats
for users 0..6 only; index 7 is never consumed (:195 indexes user_stats[user]
with that capped counter). Even if slot 7 were populated, its stats would not
be reported.

3. Loops whose ceiling the patch widened past what sections 1–2 can fill

- dp_htt.c:504-512 — delay-BA backup loop, for (i = 0; i < common.num_users;
i++). The patch (:490) now lets num_users == 8 through, so i == 7 reads
user_stats[7] (:505 peer_id, :510 usr_stats) — a slot section 1 guarantees is
empty. In-bounds and benign today (write at :512 gated by usr_stats->delay_ba
at :511, which is 0 for the empty slot), but it's the concrete ceiling
mismatch: num_users admits 8 while the allocator fills only 7.
- dp_htt.c:519-527 — BAR restore loop, for (i = 0; i < bar_num_users; i++),
reading user_stats[7] at :520/:525. Driven by the separate bar_num_users
counter (dp_htt.h:1296), never bounded by the :490 guard before or after the
patch — same empty-slot-7 exposure, pre-existing and independent of this
change.

4. The patched guard itself

- dp_htt.c:490 — if (ppdu_info->ppdu_stats.common.num_users >
HTT_PPDU_STATS_MAX_USERS). Correctly stops rejecting num_users == 8, but it's
a necessary-not-sufficient part of "handling 8 users": without the - 1
removals in sections 1–2, accepting the message just means the 8th user is
discarded a bit later and more quietly instead of the whole message being
dropped.

Supporting declarations

- dp_htt.h:1282 — #define HTT_PPDU_STATS_MAX_USERS 8
- dp_htt.h:1287 — struct htt_ppdu_user_stats
user_stats[HTT_PPDU_STATS_MAX_USERS]; (slots 0..7)
- dp_htt.h:1115 — u8 num_users;
- dp_htt.h:1296 — u32 bar_num_users;

Excluded after checking (not this path)

- dp_mon.c:905-909, :1049-1053 — ppdu_info->num_users bounded by
HAL_MAX_UL_MU_USERS, separate monitor-mode struct.
- dp.c:355-417 — TCL bank-profile num_users, unrelated.
- debugfs_htt_stats.c:2166 / .h:1216 — a separate __le32 num_users display
field.

Bottom line: to actually store and report the 8th user, the minimal change is
removing - 1 at :49 and :365 (both in-bounds against the 8-slot array), on top
of the existing :490 relaxation. Sections 1–2 are the real gap; section 3 is
the cosmetic ceiling mismatch that disappears once slot 7 is legitimately
populated.