Re: [PATCH v2] wifi: ath11k: move .max_tx_ring to struct ath11k_hw_hal_params
"Alex G." <[email protected]>
| Newsgroups | org.infradead.lists.ath11k,org.kernel.vger.linux-kernel,org.kernel.vger.linux-wireless |
|---|---|
| Message-ID | <[email protected]> |
On Tuesday, December 16, 2025 1:14:27 AM CST Baochen Qiang wrote: > On 12/16/2025 12:35 PM, Alexandru Gagniuc wrote: > > ".max_tx_ring" is an upper bounds to indexing ".tcl2wbm_rbm_map". It > > is initialized in, core.c, a different file than the array. This > > spaghetti-like relation is fragile and not obvious. Accidentally > > setting ".max_tx_ring" too high leads to a hard to track out-of- > > bounds access and memory corruption. > > > > There is a small ambiguity on the meaning of "max_tx_ring": > > - The highest ring, max=3 implies there are 4 rings (0, 1, 2, 3) > > - The highest number to use for array indexing (there are 3 rings) > > > > Clarify this dependency by moving ".max_tx_ring" adjacent to the array > > ".tcl2wbm_rbm_map", and name it "num_tx_rings". Use ARRAY_SIZE() > > instead of #defines to initialize the length field. > > > > The ath11k_hw_hal_params_qca6390 uses fewer num_tx_rings than its map, > > so use a constant to express the correct value. Add a static_assert() > > to fail compilation if the constant is accidentally set too high. > > > > The intent is to make the code easier to understand rather than fix > > an existing bug. > > > > Signed-off-by: Alexandru Gagniuc <[email protected]> > > --- > > > > I am trying to make ath11k work on IPQ9574. My device uses a IPQ9570 > > with a QCN5024 as the 2.4 GHz wifi. > > > > I spent a few days tracking a memory corruption bug caused by > > erroneously setting ".max_tx_ring" too high. I think I would not have > > made this mistake if the initializations of .max_tx_ring and > > .tcl2wbm_rbm_map were right next to each other. > > > > Changes since v1: > > - use "num_tx_rings" name instead of "map_len" > > - make sure debugfs.c is correctly updated > > - add a static_assert for ath11k_hw_hal_params_qca6390 special case > > > > drivers/net/wireless/ath/ath11k/core.c | 12 +----------- > > drivers/net/wireless/ath/ath11k/debugfs.c | 2 +- > > drivers/net/wireless/ath/ath11k/dp.c | 12 ++++++------ > > drivers/net/wireless/ath/ath11k/dp_tx.c | 9 +++++---- > > drivers/net/wireless/ath/ath11k/hw.c | 19 +++++++++++++++++++ > > drivers/net/wireless/ath/ath11k/hw.h | 3 ++- > > drivers/net/wireless/ath/ath11k/mac.c | 2 +- > > 7 files changed, 35 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath11k/core.c > > b/drivers/net/wireless/ath/ath11k/core.c index > > 812686173ac8a..07199ceecbeb4 100644 > > --- a/drivers/net/wireless/ath/ath11k/core.c > > +++ b/drivers/net/wireless/ath/ath11k/core.c > > @@ -100,7 +100,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = false, > > .fix_l1ss = true, > > .credit_flow = false, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX, > > > > .hal_params = &ath11k_hw_hal_params_ipq8074, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = true, > > > > @@ -184,7 +183,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = false, > > .fix_l1ss = true, > > .credit_flow = false, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX, > > > > .hal_params = &ath11k_hw_hal_params_ipq8074, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = true, > > > > @@ -271,7 +269,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = false, > > .fix_l1ss = true, > > .credit_flow = true, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX_QCA6390, > > > > .hal_params = &ath11k_hw_hal_params_qca6390, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = false, > > > > @@ -358,7 +355,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = false, > > .fix_l1ss = true, > > .credit_flow = false, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX, > > > > .hal_params = &ath11k_hw_hal_params_ipq8074, > > .supports_dynamic_smps_6ghz = true, > > .alloc_cacheable_memory = true, > > > > @@ -445,7 +441,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = true, > > .fix_l1ss = false, > > .credit_flow = true, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX_QCA6390, > > > > .hal_params = &ath11k_hw_hal_params_qca6390, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = false, > > > > @@ -533,7 +528,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = true, > > .fix_l1ss = false, > > .credit_flow = true, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX_QCA6390, > > > > .hal_params = &ath11k_hw_hal_params_qca6390, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = false, > > > > @@ -619,7 +613,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = true, > > .fix_l1ss = false, > > .credit_flow = true, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX, > > > > .hal_params = &ath11k_hw_hal_params_wcn6750, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = false, > > > > @@ -662,7 +655,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .qmi_service_ins_id = ATH11K_QMI_WLFW_SERVICE_INS_ID_V01_IPQ8074, > > .ring_mask = &ath11k_hw_ring_mask_ipq8074, > > .credit_flow = false, > > > > - .max_tx_ring = 1, > > > > .spectral = { > > > > .fft_sz = 2, > > .fft_pad_sz = 0, > > > > @@ -698,7 +690,7 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = false, > > .idle_ps = false, > > .supports_suspend = false, > > > > - .hal_params = &ath11k_hw_hal_params_ipq8074, > > + .hal_params = &ath11k_hw_hal_params_ipq5018, > > > > .single_pdev_only = false, > > .coldboot_cal_mm = true, > > .coldboot_cal_ftm = true, > > > > @@ -789,7 +781,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = true, > > .fix_l1ss = false, > > .credit_flow = true, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX_QCA6390, > > > > .hal_params = &ath11k_hw_hal_params_qca6390, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = false, > > > > @@ -876,7 +867,6 @@ static const struct ath11k_hw_params > > ath11k_hw_params[] = {> > > .supports_regdb = true, > > .fix_l1ss = false, > > .credit_flow = true, > > > > - .max_tx_ring = DP_TCL_NUM_RING_MAX_QCA6390, > > > > .hal_params = &ath11k_hw_hal_params_qca6390, > > .supports_dynamic_smps_6ghz = false, > > .alloc_cacheable_memory = false, > > > > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c > > b/drivers/net/wireless/ath/ath11k/debugfs.c index > > 977f945b6e669..50f344803e8fd 100644 > > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > > @@ -707,7 +707,7 @@ static ssize_t ath11k_debugfs_dump_soc_dp_stats(struct > > file *file,> > > len += scnprintf(buf + len, size - len, "\nSOC TX STATS:\n"); > > len += scnprintf(buf + len, size - len, "\nTCL Ring Full Failures: \n"); > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) > > > > len += scnprintf(buf + len, size - len, "ring%d: %u\n", > > > > i, soc_stats- >tx_err.desc_na[i]); > > > > diff --git a/drivers/net/wireless/ath/ath11k/dp.c > > b/drivers/net/wireless/ath/ath11k/dp.c index 56b1a657e0b0f..c940de285276d > > 100644 > > --- a/drivers/net/wireless/ath/ath11k/dp.c > > +++ b/drivers/net/wireless/ath/ath11k/dp.c > > @@ -344,7 +344,7 @@ void ath11k_dp_stop_shadow_timers(struct ath11k_base > > *ab)> > > if (!ab->hw_params.supports_shadow_regs) > > > > return; > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) > > > > ath11k_dp_shadow_stop_timer(ab, &ab- >dp.tx_ring_timer[i]); > > > > ath11k_dp_shadow_stop_timer(ab, &ab->dp.reo_cmd_timer); > > > > @@ -359,7 +359,7 @@ static void ath11k_dp_srng_common_cleanup(struct > > ath11k_base *ab)> > > ath11k_dp_srng_cleanup(ab, &dp->wbm_desc_rel_ring); > > ath11k_dp_srng_cleanup(ab, &dp->tcl_cmd_ring); > > ath11k_dp_srng_cleanup(ab, &dp->tcl_status_ring); > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) { > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) { > > > > ath11k_dp_srng_cleanup(ab, &dp- >tx_ring[i].tcl_data_ring); > > ath11k_dp_srng_cleanup(ab, &dp- >tx_ring[i].tcl_comp_ring); > > > > } > > > > @@ -400,7 +400,7 @@ static int ath11k_dp_srng_common_setup(struct > > ath11k_base *ab)> > > goto err; > > > > } > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) { > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) { > > > > tcl_num = ab->hw_params.hal_params- >tcl2wbm_rbm_map[i].tcl_ring_num; > > wbm_num = ab->hw_params.hal_params- >tcl2wbm_rbm_map[i].wbm_ring_num; > > > > @@ -782,7 +782,7 @@ int ath11k_dp_service_srng(struct ath11k_base *ab, > > > > int i, j; > > int tot_work_done = 0; > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) { > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) { > > > > if (BIT(ab->hw_params.hal_params- >tcl2wbm_rbm_map[i].wbm_ring_num) & > > > > ab->hw_params.ring_mask->tx[grp_id]) > > > > ath11k_dp_tx_completion_handler(ab, i); > > > > @@ -1035,7 +1035,7 @@ void ath11k_dp_free(struct ath11k_base *ab) > > > > ath11k_dp_reo_cmd_list_cleanup(ab); > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) { > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) { > > > > spin_lock_bh(&dp->tx_ring[i].tx_idr_lock); > > idr_for_each(&dp->tx_ring[i].txbuf_idr, > > > > ath11k_dp_tx_pending_cleanup, ab); > > > > @@ -1086,7 +1086,7 @@ int ath11k_dp_alloc(struct ath11k_base *ab) > > > > size = sizeof(struct hal_wbm_release_ring) * DP_TX_COMP_RING_SIZE; > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) { > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) { > > > > idr_init(&dp->tx_ring[i].txbuf_idr); > > spin_lock_init(&dp->tx_ring[i].tx_idr_lock); > > dp->tx_ring[i].tcl_data_ring_id = i; > > > > diff --git a/drivers/net/wireless/ath/ath11k/dp_tx.c > > b/drivers/net/wireless/ath/ath11k/dp_tx.c index > > 562aba66582f3..86e1e6c27b36c 100644 > > --- a/drivers/net/wireless/ath/ath11k/dp_tx.c > > +++ b/drivers/net/wireless/ath/ath11k/dp_tx.c > > @@ -91,6 +91,7 @@ int ath11k_dp_tx(struct ath11k *ar, struct ath11k_vif > > *arvif,> > > struct hal_srng *tcl_ring; > > struct ieee80211_hdr *hdr = (void *)skb->data; > > struct dp_tx_ring *tx_ring; > > > > + size_t num_tx_rings = ab->hw_params.hal_params->num_tx_rings; > > > > void *hal_tcl_desc; > > u8 pool_id; > > u8 hal_ring_id; > > > > @@ -113,7 +114,7 @@ int ath11k_dp_tx(struct ath11k *ar, struct ath11k_vif > > *arvif,> > > tcl_ring_sel: > > tcl_ring_retry = false; > > > > - ti.ring_id = ring_selector % ab->hw_params.max_tx_ring; > > + ti.ring_id = ring_selector % num_tx_rings; > > > > ti.rbm_id = > > ab->hw_params.hal_params->tcl2wbm_rbm_map[ti.ring_id].rbm_id; > > > > ring_map |= BIT(ti.ring_id); > > > > @@ -126,7 +127,7 @@ int ath11k_dp_tx(struct ath11k *ar, struct ath11k_vif > > *arvif,> > > spin_unlock_bh(&tx_ring->tx_idr_lock); > > > > if (unlikely(ret < 0)) { > > > > - if (ring_map == (BIT(ab->hw_params.max_tx_ring) - 1) || > > + if (ring_map == (BIT(num_tx_rings) - 1) || > > > > !ab->hw_params.tcl_ring_retry) { > > > > atomic_inc(&ab->soc_stats.tx_err.misc_fail); > > return -ENOSPC; > > > > @@ -244,8 +245,8 @@ int ath11k_dp_tx(struct ath11k *ar, struct ath11k_vif > > *arvif,> > > * checking this ring earlier for each pkt tx. > > * Restart ring selection if some rings are not checked yet. > > */ > > > > - if (unlikely(ring_map != (BIT(ab- >hw_params.max_tx_ring)) - 1) && > > - ab->hw_params.tcl_ring_retry && ab- >hw_params.max_tx_ring > 1) { > > + if (unlikely(ring_map != (BIT(num_tx_rings)) - 1) && > > + ab->hw_params.tcl_ring_retry && num_tx_rings > 1) { > > > > tcl_ring_retry = true; > > ring_selector++; > > > > } > > > > diff --git a/drivers/net/wireless/ath/ath11k/hw.c > > b/drivers/net/wireless/ath/ath11k/hw.c index caa6dc12a790b..bbaacdf76af0a > > 100644 > > --- a/drivers/net/wireless/ath/ath11k/hw.c > > +++ b/drivers/net/wireless/ath/ath11k/hw.c > > @@ -2707,6 +2707,14 @@ const struct ath11k_hw_regs wcn6750_regs = { > > > > .hal_reo1_misc_ctl = 0x000005d8, > > > > }; > > > > +static const struct ath11k_hw_tcl2wbm_rbm_map > > ath11k_hw_tcl2wbm_rbm_map_ipq5018[] = { + { > > + .tcl_ring_num = 0, > > + .wbm_ring_num = 0, > > + .rbm_id = HAL_RX_BUF_RBM_SW0_BM, > > + }, > > +}; > > + > > > > static const struct ath11k_hw_tcl2wbm_rbm_map > > ath11k_hw_tcl2wbm_rbm_map_ipq8074[] = {> > > { > > > > .tcl_ring_num = 0, > > > > @@ -2822,19 +2830,30 @@ const struct ath11k_hw_regs ipq5018_regs = { > > > > .hal_wbm1_release_ring_base_lsb = 0x0000097c, > > > > }; > > > > +const struct ath11k_hw_hal_params ath11k_hw_hal_params_ipq5018 = { > > + .rx_buf_rbm = HAL_RX_BUF_RBM_SW3_BM, > > + .tcl2wbm_rbm_map = ath11k_hw_tcl2wbm_rbm_map_ipq5018, > > + .num_tx_rings = ARRAY_SIZE(ath11k_hw_tcl2wbm_rbm_map_ipq5018), > > +}; > > + > > > > const struct ath11k_hw_hal_params ath11k_hw_hal_params_ipq8074 = { > > > > .rx_buf_rbm = HAL_RX_BUF_RBM_SW3_BM, > > .tcl2wbm_rbm_map = ath11k_hw_tcl2wbm_rbm_map_ipq8074, > > > > + .num_tx_rings = ARRAY_SIZE(ath11k_hw_tcl2wbm_rbm_map_ipq8074), > > > > }; > > > > const struct ath11k_hw_hal_params ath11k_hw_hal_params_qca6390 = { > > > > .rx_buf_rbm = HAL_RX_BUF_RBM_SW1_BM, > > .tcl2wbm_rbm_map = ath11k_hw_tcl2wbm_rbm_map_ipq8074, > > > > + .num_tx_rings = DP_TCL_NUM_RING_MAX_QCA6390, > > > > }; > > > > +static_assert(ARRAY_SIZE(ath11k_hw_tcl2wbm_rbm_map_ipq8074) >= > > DP_TCL_NUM_RING_MAX_QCA6390); + > > instead of keeping this special handling, how about define its own > tcl2wbm_rbm_map for ath11k_hw_hal_params_qca6390?or even simply use the > newly introduced ath11k_hw_tcl2wbm_rbm_map_ipq5018? like > > const struct ath11k_hw_hal_params ath11k_hw_hal_params_qca6390 = { > .rx_buf_rbm = HAL_RX_BUF_RBM_SW1_BM, > .tcl2wbm_rbm_map = ath11k_hw_tcl2wbm_rbm_map_ipq5018, > .num_tx_rings = ARRAY_SIZE(ath11k_hw_tcl2wbm_rbm_map_ipq5018) > }; Sure, I will do it this way. I am ready to submit the IPQ9574 support, and this patch is a dependency. Should I include v3 of this patch in the IPQ9574 series, or submit v3 independently? Alex > > const struct ath11k_hw_hal_params ath11k_hw_hal_params_wcn6750 = { > > > > .rx_buf_rbm = HAL_RX_BUF_RBM_SW1_BM, > > .tcl2wbm_rbm_map = ath11k_hw_tcl2wbm_rbm_map_wcn6750, > > > > + .num_tx_rings = ARRAY_SIZE(ath11k_hw_tcl2wbm_rbm_map_wcn6750), > > > > }; > > > > static const struct cfg80211_sar_freq_ranges > > ath11k_hw_sar_freq_ranges_wcn6855[] = {> > > diff --git a/drivers/net/wireless/ath/ath11k/hw.h > > b/drivers/net/wireless/ath/ath11k/hw.h index 52d9f4c13b136..9db984ac4321c > > 100644 > > --- a/drivers/net/wireless/ath/ath11k/hw.h > > +++ b/drivers/net/wireless/ath/ath11k/hw.h > > @@ -134,6 +134,7 @@ struct ath11k_hw_tcl2wbm_rbm_map { > > > > struct ath11k_hw_hal_params { > > > > enum hal_rx_buf_return_buf_manager rx_buf_rbm; > > const struct ath11k_hw_tcl2wbm_rbm_map *tcl2wbm_rbm_map; > > > > + size_t num_tx_rings; > > > > }; > > > > struct ath11k_hw_params { > > > > @@ -198,7 +199,6 @@ struct ath11k_hw_params { > > > > bool supports_regdb; > > bool fix_l1ss; > > bool credit_flow; > > > > - u8 max_tx_ring; > > > > const struct ath11k_hw_hal_params *hal_params; > > bool supports_dynamic_smps_6ghz; > > bool alloc_cacheable_memory; > > > > @@ -291,6 +291,7 @@ extern const struct ce_ie_addr > > ath11k_ce_ie_addr_ipq5018;> > > extern const struct ce_remap ath11k_ce_remap_ipq5018; > > > > +extern const struct ath11k_hw_hal_params ath11k_hw_hal_params_ipq5018; > > > > extern const struct ath11k_hw_hal_params ath11k_hw_hal_params_ipq8074; > > extern const struct ath11k_hw_hal_params ath11k_hw_hal_params_qca6390; > > extern const struct ath11k_hw_hal_params ath11k_hw_hal_params_wcn6750; > > > > diff --git a/drivers/net/wireless/ath/ath11k/mac.c > > b/drivers/net/wireless/ath/ath11k/mac.c index > > 3276fe443502f..33ebe03380114 100644 > > --- a/drivers/net/wireless/ath/ath11k/mac.c > > +++ b/drivers/net/wireless/ath/ath11k/mac.c > > @@ -7392,7 +7392,7 @@ static void ath11k_mac_op_remove_interface(struct > > ieee80211_hw *hw,> > > idr_for_each(&ar->txmgmt_idr, > > > > ath11k_mac_vif_txmgmt_idr_remove, vif); > > > > - for (i = 0; i < ab->hw_params.max_tx_ring; i++) { > > + for (i = 0; i < ab->hw_params.hal_params->num_tx_rings; i++) { > > > > spin_lock_bh(&ab->dp.tx_ring[i].tx_idr_lock); > > idr_for_each(&ab->dp.tx_ring[i].txbuf_idr, > > > > ath11k_mac_vif_unref, vif);