Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 2B3EAC43639
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Jouni Malinen <jouni@codeaurora.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        Seevalamuthu Mariappan <seevalam@codeaurora.org>,
        Miles Hu <milehu@codeaurora.org>,
        Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
Subject: Re: [PATCH 3/3] ath11k: monitor mode clean up to use separate APIs
References: <20210721162053.46290-1-jouni@codeaurora.org>
        <20210721162053.46290-4-jouni@codeaurora.org>
        <87ilyuunvh.fsf@codeaurora.org>
Date:   Tue, 21 Sep 2021 16:42:05 +0300
In-Reply-To: <87ilyuunvh.fsf@codeaurora.org> (Kalle Valo's message of "Tue, 21
        Sep 2021 15:14:26 +0300")
Message-ID: <8735pyujte.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Kalle Valo <kvalo@codeaurora.org> writes:

> Jouni Malinen <jouni@codeaurora.org> writes:
>
>> From: Seevalamuthu Mariappan <seevalam@codeaurora.org>
>>
>> If monitor interface is enabled in co-exist mode, only local traffic are
>> captured. It's caused by missing monitor vdev in co-exist mode. So,
>> monitor mode clean up is done with separate Monitor APIs. For this,
>> introduce monitor_started and monitor_vdev_created boolean flags.
>>
>> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-01725-QCAHKSWPL_SILICONZ-1
>>
>> Co-developed-by: Miles Hu <milehu@codeaurora.org>
>> Signed-off-by: Miles Hu <milehu@codeaurora.org>
>> Co-developed-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
>> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
>> Signed-off-by: Seevalamuthu Mariappan <seevalam@codeaurora.org>
>> Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
>> ---
>>  drivers/net/wireless/ath/ath11k/core.h  |   5 --
>>  drivers/net/wireless/ath/ath11k/dp_rx.c |   2 +-
>>  drivers/net/wireless/ath/ath11k/dp_tx.c |   9 +-
>>  drivers/net/wireless/ath/ath11k/mac.c   | 112 ++++++++++++++----------
>>  4 files changed, 73 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
>> index 3cddab695031..0ad5a935b52b 100644
>> --- a/drivers/net/wireless/ath/ath11k/core.h
>> +++ b/drivers/net/wireless/ath/ath11k/core.h
>> @@ -192,10 +192,6 @@ enum ath11k_dev_flags {
>>  	ATH11K_FLAG_HTC_SUSPEND_COMPLETE,
>>  };
>>  
>> -enum ath11k_monitor_flags {
>> -	ATH11K_FLAG_MONITOR_ENABLED,
>> -};
>> -
>>  struct ath11k_vif {
>>  	u32 vdev_id;
>>  	enum wmi_vdev_type vdev_type;
>> @@ -478,7 +474,6 @@ struct ath11k {
>>  
>>  	unsigned long dev_flags;
>>  	unsigned int filter_flags;
>> -	unsigned long monitor_flags;
>>  	u32 min_tx_power;
>>  	u32 max_tx_power;
>>  	u32 txpower_limit_2g;
>> diff --git a/drivers/net/wireless/ath/ath11k/dp_rx.c b/drivers/net/wireless/ath/ath11k/dp_rx.c
>> index 9a224817630a..6fde70914e1a 100644
>> --- a/drivers/net/wireless/ath/ath11k/dp_rx.c
>> +++ b/drivers/net/wireless/ath/ath11k/dp_rx.c
>> @@ -5029,7 +5029,7 @@ int ath11k_dp_rx_process_mon_rings(struct ath11k_base *ab, int mac_id,
>>  	struct ath11k *ar = ath11k_ab_to_ar(ab, mac_id);
>>  	int ret = 0;
>>  
>> -	if (test_bit(ATH11K_FLAG_MONITOR_ENABLED, &ar->monitor_flags))
>> +	if (ar->monitor_started)
>>  		ret = ath11k_dp_mon_process_rx(ab, mac_id, napi, budget);
>>  	else
>>  		ret = ath11k_dp_rx_process_mon_status(ab, mac_id, napi, budget);
>
> Moving from test_bit() to a boolean looks racy to me, I don't see how
> monitor_started is serialised.
>
> And why move away from monitor_flags and having separate booleans
> anyway? I would monitor_conf_enabled and monitor_started from patch 2 to
> use monitor_flags.

In the pending branch I changed back to monitor_flags.

>> @@ -1076,11 +1076,16 @@ int ath11k_dp_tx_htt_monitor_mode_ring_config(struct ath11k *ar, bool reset)
>>  
>>  	for (i = 0; i < ab->hw_params.num_rxmda_per_pdev; i++) {
>>  		ring_id = dp->rx_mon_status_refill_ring[i].refill_buf_ring.ring_id;
>> -		if (!reset)
>> +		if (!reset) {
>>  			tlv_filter.rx_filter =
>>  					HTT_RX_MON_FILTER_TLV_FLAGS_MON_STATUS_RING;
>> -		else
>> +		} else {
>>  			tlv_filter = ath11k_mac_mon_status_filter_default;
>> +#ifdef CONFIG_ATH11K_DEBUGFS
>> +			if (ar->debug.extd_rx_stats)
>> +				tlv_filter.rx_filter = ar->debug.rx_filter;
>> +#endif
>
> This should use ath11k_debugfs_is_extd_rx_stats_enabled and
> ath11k_debugfs_rx_filter(), then the ifdef is not needed.

I also fixed this in the pending branch.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches