Re: [PATCH 7/7] wifi: ath12k: add monitor interface support on QCN9274

["Nicolas Escande" <nico.escande@gmail.com>]

On Fri Jan 17, 2025 at 2:06 PM CET, Karthikeyan Periyasamy wrote:
>
>
> On 1/17/2025 5:03 PM, Nicolas Escande wrote:
> > On Tue Jan 7, 2025 at 3:10 AM CET, Karthikeyan Periyasamy wrote:
> >> From: P Praneesh <quic_ppranees@quicinc.com>
> >>
> >> Currently, the monitor interface is not supported. To support the monitor
> >> interface, configure the monitor vdev state identifier, configure the HTT
> >> filter setup, subscribe the mac80211 WANT_MONITOR_VIF feature and prevent
> >> monitor interface to transmit packet. Therefore, add these procedures to
> >> add monitor interface support and enable the monitor interface support on
> >> the QCN9274 platform through the hardware parameter.
> >>
> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> >> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> >>
> >> Signed-off-by: P Praneesh <quic_ppranees@quicinc.com>
> >> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
> >> ---
> > [...]
> >> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
> >> index b8f0df6f7a05..81d1ff918f48 100644
> >> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
> >> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
> >> @@ -1,7 +1,7 @@
> >>   // SPDX-License-Identifier: BSD-3-Clause-Clear
> >>   /*
> >>    * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
> >> - * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> >> + * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
> >>    */
> >>   
> >>   #include "core.h"
> >> @@ -1277,6 +1277,8 @@ int ath12k_dp_tx_htt_rx_monitor_mode_ring_config(struct ath12k *ar, bool reset)
> >>   					HTT_RX_MON_MO_CTRL_FILTER_FLASG3 |
> >>   					HTT_RX_MON_FP_DATA_FILTER_FLASG3 |
> >>   					HTT_RX_MON_MO_DATA_FILTER_FLASG3;
> >> +	} else {
> >> +		tlv_filter.rxmon_disable = true;
> > Shouldn't this be
> > 	tlv_filter = ath12k_mac_mon_status_filter_default;
> > To match de default value ?
>
> In standalone monitor case, this will enable the default filter 
> unnecessary, no ?

I'm not too familiar with the driver but clearly this is the part that breaks
sta info update when running a monitor while an ap is running.

And as this is a valid interface combination it should work right ?

So this change works around the problem I reported earlier, I just confirmed it
on my setup. But maybe this is not the right fix indeed but hopefully it
points towards the culprit.

>
>
> >>   	}
> >>   
> >>   	if (ab->hw_params->rxdma1_enable) {
> >> diff --git a/drivers/net/wireless/ath/ath12k/hw.c b/drivers/net/wireless/ath/ath12k/hw.c
> >> index a106ebed7870..021a4b565e8b 100644
> >> --- a/drivers/net/wireless/ath/ath12k/hw.c
> >> +++ b/drivers/net/wireless/ath/ath12k/hw.c
> >> @@ -1,7 +1,7 @@
> >>   // SPDX-License-Identifier: BSD-3-Clause-Clear
> >>   /*
> >>    * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
> >> - * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> >> + * Copyright (c) 2021-2025 Qualcomm Innovation Center, Inc. All rights reserved.
> >>    */
> >>   
> >>   #include <linux/types.h>
> >> @@ -1049,7 +1049,7 @@ static const struct ath12k_hw_params ath12k_hw_params[] = {
> >>   					BIT(NL80211_IFTYPE_AP) |
> >>   					BIT(NL80211_IFT
>
> YPE_MESH_POINT) |
> >>   					BIT(NL80211_IFTYPE_AP_VLAN),
> >> -		.supports_monitor = false,
> >> +		.supports_monitor = true,
> >>   
> >>   		.idle_ps = false,
> >>   		.download_calib = true,
> >> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> >> index abf7c7ed8967..27d44b508884 100644
> >> --- a/drivers/net/wireless/ath/ath12k/mac.c
> >> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> >> @@ -1264,6 +1264,12 @@ static int ath12k_mac_monitor_start(struct ath12k *ar)
> >>   		return ret;
> >>   	}
> >>   
> >> +	ret = ath12k_dp_tx_htt_monitor_mode_ring_config(ar, false);
> > This is already done a few lines after. Shouldn't the one after be removed
>
> yes, will remove in the next version of the patch.