| Message ID | 20240427162644.2470886-1-quic_tamizhr@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: ath12k: Remove unsupported and unused ring configurations | expand |
On 4/27/2024 9:56 PM, Tamizh Chelvam Raja wrote: > Currently in driver doing memory allocation for tx_monitor, > tcl_cmd_ring and tcl_status ring. Here driver support for > tx_monitor mode is not there and memory for tcl_cmd and tcl_status > rings are allocated by firmware and it uses that memory instead of > host allocated. So avoid these unused ring setup configuration. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Tamizh Chelvam Raja (3): > wifi: ath12k: fix calling correct function for rx monitor mode > wifi: ath12k: Remove unsupported tx monitor handling > wifi: ath12k: Remove unused tcl_*_ring configuration > > v2: > * Rebased on top of ToT > > drivers/net/wireless/ath/ath12k/dp.c | 16 ------- > drivers/net/wireless/ath/ath12k/dp.h | 2 - > drivers/net/wireless/ath/ath12k/dp_mon.c | 40 +---------------- > drivers/net/wireless/ath/ath12k/dp_rx.c | 56 ------------------------ > drivers/net/wireless/ath/ath12k/dp_tx.c | 44 +------------------ > drivers/net/wireless/ath/ath12k/dp_tx.h | 1 - > 6 files changed, 2 insertions(+), 157 deletions(-) > > It nice to see code removal in general :) But I've also seen some concerns internally around code removal especially when the code will have to be re-added in future while properly supporting the feature. I guess the cover letter may need to clarify those concerns at least for internal review. Other than that, the series LGTM Vasanth
On 4/27/2024 9:56 PM, Tamizh Chelvam Raja wrote: > Currently in ath12k_dp_tx_htt_monitor_mode_ring_config() > ath12k_dp_tx_htt_tx_monitor_mode_ring_config() function wrongly called twice. > Fix that by calling ath12k_dp_tx_htt_rx_monitor_mode_ring_config(). > > Currently monitor mode is disabled in driver so the change is compile > tested and boot sequence verified. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/dp_tx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c > index 9b6d7d72f57c..2fb9fc42db11 100644 > --- a/drivers/net/wireless/ath/ath12k/dp_tx.c > +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c > @@ -1044,7 +1044,7 @@ int ath12k_dp_tx_htt_monitor_mode_ring_config(struct ath12k *ar, bool reset) > struct ath12k_base *ab = ar->ab; > int ret; > > - ret = ath12k_dp_tx_htt_tx_monitor_mode_ring_config(ar, reset); > + ret = ath12k_dp_tx_htt_rx_monitor_mode_ring_config(ar, reset); > if (ret) { > ath12k_err(ab, "failed to setup tx monitor filter %d\n", ret); > return ret; Looks like you modifying in wrong place because the debug log information is not matching the rx message. no ?
On 4/28/2024 8:59 AM, Vasanthakumar Thiagarajan wrote: > > > On 4/27/2024 9:56 PM, Tamizh Chelvam Raja wrote: >> Currently in driver doing memory allocation for tx_monitor, >> tcl_cmd_ring and tcl_status ring. Here driver support for >> tx_monitor mode is not there and memory for tcl_cmd and tcl_status >> rings are allocated by firmware and it uses that memory instead of >> host allocated. So avoid these unused ring setup configuration. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 >> >> Tamizh Chelvam Raja (3): >> wifi: ath12k: fix calling correct function for rx monitor mode >> wifi: ath12k: Remove unsupported tx monitor handling >> wifi: ath12k: Remove unused tcl_*_ring configuration >> >> v2: >> * Rebased on top of ToT >> >> drivers/net/wireless/ath/ath12k/dp.c | 16 ------- >> drivers/net/wireless/ath/ath12k/dp.h | 2 - >> drivers/net/wireless/ath/ath12k/dp_mon.c | 40 +---------------- >> drivers/net/wireless/ath/ath12k/dp_rx.c | 56 ------------------------ >> drivers/net/wireless/ath/ath12k/dp_tx.c | 44 +------------------ >> drivers/net/wireless/ath/ath12k/dp_tx.h | 1 - >> 6 files changed, 2 insertions(+), 157 deletions(-) >> >> > > It nice to see code removal in general :) > But I've also seen some concerns internally around code removal > especially when the code will have to be re-added in future while > properly supporting the feature. I guess the cover letter > may need to clarify those concerns at least for internal review. > This is mainly to avoid unnecessary memory allocation for the unused rings. And this can be added in the future while enabling the feature. > Other than that, the series LGTM > > Vasanth Tamizh
On 4/29/2024 9:54 AM, Tamizh Chelvam Raja wrote: > On 4/28/2024 8:59 AM, Vasanthakumar Thiagarajan wrote: >> >> >> On 4/27/2024 9:56 PM, Tamizh Chelvam Raja wrote: >>> Currently in driver doing memory allocation for tx_monitor, >>> tcl_cmd_ring and tcl_status ring. Here driver support for >>> tx_monitor mode is not there and memory for tcl_cmd and tcl_status >>> rings are allocated by firmware and it uses that memory instead of >>> host allocated. So avoid these unused ring setup configuration. >>> >>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 >>> >>> Tamizh Chelvam Raja (3): >>> wifi: ath12k: fix calling correct function for rx monitor mode >>> wifi: ath12k: Remove unsupported tx monitor handling >>> wifi: ath12k: Remove unused tcl_*_ring configuration >>> >>> v2: >>> * Rebased on top of ToT >>> >>> drivers/net/wireless/ath/ath12k/dp.c | 16 ------- >>> drivers/net/wireless/ath/ath12k/dp.h | 2 - >>> drivers/net/wireless/ath/ath12k/dp_mon.c | 40 +---------------- >>> drivers/net/wireless/ath/ath12k/dp_rx.c | 56 ------------------------ >>> drivers/net/wireless/ath/ath12k/dp_tx.c | 44 +------------------ >>> drivers/net/wireless/ath/ath12k/dp_tx.h | 1 - >>> 6 files changed, 2 insertions(+), 157 deletions(-) >>> >>> >> >> It nice to see code removal in general :) >> But I've also seen some concerns internally around code removal >> especially when the code will have to be re-added in future while >> properly supporting the feature. I guess the cover letter >> may need to clarify those concerns at least for internal review. >> > This is mainly to avoid unnecessary memory allocation for the unused rings. > And this can be added in the future while enabling the feature. I agree. I think I somehow overlooked the memory optimization part. Thanks for the clarification. Vasanth