mbox series

[0/3] ath11k: Fix no data captured in monitor co-exist mode

Message ID 20210721162053.46290-1-jouni@codeaurora.org
Headers show
Series ath11k: Fix no data captured in monitor co-exist mode | expand

Message

Jouni Malinen July 21, 2021, 4:20 p.m. UTC
From: Seevalamuthu Mariappan <seevalam@codeaurora.org>

In AP+monitor coexistance, monitor interface is capturing only
local traffic. This patchset changes monitor mode approach to
address monitor mode issues.

Seevalamuthu Mariappan (3):
  ath11k: move static function ath11k_mac_vdev_setup_sync to top
  ath11k: add separate APIs for monitor mode
  ath11k: monitor mode clean up to use separate APIs

 drivers/net/wireless/ath/ath11k/core.h  |  10 +-
 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   | 429 ++++++++++++++++++++++++++++----
 4 files changed, 387 insertions(+), 63 deletions(-)

Comments

Kalle Valo Sept. 16, 2021, 9:35 a.m. UTC | #1
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


Seevalamuthu, in upstream IPQ8074 doesn't even support monitor mode:

static const struct ath11k_hw_params ath11k_hw_params[] = {
	{
		.hw_rev = ATH11K_HW_IPQ8074,
		.name = "ipq8074 hw2.0",
...
		.interface_modes = BIT(NL80211_IFTYPE_STATION) |
					BIT(NL80211_IFTYPE_AP) |
					BIT(NL80211_IFTYPE_MESH_POINT),

So I wonder how did you test this? Is this something which is only
tested on ancient QSDK kernels and not with upstream kernels?

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo Sept. 16, 2021, 9:38 a.m. UTC | #2
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

>

> Seevalamuthu, in upstream IPQ8074 doesn't even support monitor mode:

>

> static const struct ath11k_hw_params ath11k_hw_params[] = {

> 	{

> 		.hw_rev = ATH11K_HW_IPQ8074,

> 		.name = "ipq8074 hw2.0",

> ...

> 		.interface_modes = BIT(NL80211_IFTYPE_STATION) |

> 					BIT(NL80211_IFTYPE_AP) |

> 					BIT(NL80211_IFTYPE_MESH_POINT),

>

> So I wonder how did you test this? Is this something which is only

> tested on ancient QSDK kernels and not with upstream kernels?


Actually, ignore that. I forgot that there was a separate boolean for
the monitor mode:

		.supports_monitor = true,

Sorry for the noise.

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo Sept. 21, 2021, 12:14 p.m. UTC | #3
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.

> @@ -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.

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo Sept. 21, 2021, 1:26 p.m. UTC | #4
Jouni Malinen <jouni@codeaurora.org> writes:

> From: Seevalamuthu Mariappan <seevalam@codeaurora.org>

>

> Add separate APIs for monitor_vdev_create/monitor_vdev_delete

> and monitor_vdev_start/monitor_vdev_stop.

>

> 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/mac.c  | 313 ++++++++++++++++++++++++-

>  2 files changed, 312 insertions(+), 6 deletions(-)

>

> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h

> index 6a6cabdd3e30..3cddab695031 100644

> --- a/drivers/net/wireless/ath/ath11k/core.h

> +++ b/drivers/net/wireless/ath/ath11k/core.h

> @@ -488,7 +488,8 @@ struct ath11k {

>  	u32 chan_tx_pwr;

>  	u32 num_stations;

>  	u32 max_num_stations;

> -	bool monitor_present;

> +	bool monitor_conf_enabled;

> +	bool monitor_started;

>  	/* To synchronize concurrent synchronous mac80211 callback operations,

>  	 * concurrent debugfs configuration and concurrent FW statistics events.

>  	 */

> @@ -563,6 +564,7 @@ struct ath11k {

>  	struct ath11k_per_peer_tx_stats cached_stats;

>  	u32 last_ppdu_id;

>  	u32 cached_ppdu_id;

> +	int monitor_vdev_id;

>  #ifdef CONFIG_ATH11K_DEBUGFS

>  	struct ath11k_debug debug;

>  #endif

> @@ -571,6 +573,7 @@ struct ath11k {

>  #endif

>  	bool dfs_block_radar_events;

>  	struct ath11k_thermal thermal;

> +	bool monitor_vdev_created;

>  };

>  

>  struct ath11k_band_cap {

> diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c

> index 3fd9a79801cb..e446817ac8b0 100644

> --- a/drivers/net/wireless/ath/ath11k/mac.c

> +++ b/drivers/net/wireless/ath/ath11k/mac.c

> @@ -745,14 +745,314 @@ static inline int ath11k_mac_vdev_setup_sync(struct ath11k *ar)

>  	return ar->last_wmi_vdev_start_status ? -EINVAL : 0;

>  }

>  

> -static int ath11k_mac_op_config(struct ieee80211_hw *hw, u32 changed)

> +static void

> +ath11k_mac_get_any_chandef_iter(struct ieee80211_hw *hw,

> +				struct ieee80211_chanctx_conf *conf,

> +				void *data)

>  {

> -	/* mac80211 requires this op to be present and that's why

> -	 * there's an empty function, this can be extended when

> -	 * required.

> -	 */

> +	struct cfg80211_chan_def **def = data;

> +

> +	*def = &conf->def;

> +}

> +

> +static int ath11k_mac_monitor_vdev_start(struct ath11k *ar, int vdev_id,

> +					 struct cfg80211_chan_def *chandef)

> +{

> +	struct ieee80211_channel *channel;

> +	struct wmi_vdev_start_req_arg arg = {};

> +	int ret;

> +

> +	lockdep_assert_held(&ar->conf_mutex);

> +

> +	channel = chandef->chan;

> +

> +	arg.vdev_id = vdev_id;

> +	arg.channel.freq = channel->center_freq;

> +	arg.channel.band_center_freq1 = chandef->center_freq1;

> +	arg.channel.band_center_freq2 = chandef->center_freq2;

> +

> +	arg.channel.mode = ath11k_phymodes[chandef->chan->band][chandef->width];

> +	arg.channel.chan_radar =

> +			!!(channel->flags & IEEE80211_CHAN_RADAR);

> +

> +	arg.channel.min_power = 0;

> +	arg.channel.max_power = channel->max_power * 2;

> +	arg.channel.max_reg_power = channel->max_reg_power * 2;

> +	arg.channel.max_antenna_gain = channel->max_antenna_gain * 2;

> +

> +	arg.pref_tx_streams = ar->num_tx_chains;

> +	arg.pref_rx_streams = ar->num_rx_chains;

> +

> +	arg.channel.passive = !!(chandef->chan->flags & IEEE80211_CHAN_NO_IR);

> +

> +	reinit_completion(&ar->vdev_setup_done);

> +	reinit_completion(&ar->vdev_delete_done);

>  

> +	ret = ath11k_wmi_vdev_start(ar, &arg, false);

> +	if (ret) {

> +		ath11k_warn(ar->ab, "failed to request monitor vdev %i start: %d\n",

> +			    vdev_id, ret);

> +		return ret;

> +	}

> +	ret = ath11k_mac_vdev_setup_sync(ar);

> +	if (ret) {

> +		ath11k_warn(ar->ab, "failed to synchronize setup for monitor vdev %i start: %d\n",

> +			    vdev_id, ret);

> +		return ret;

> +	}

> +	ret = ath11k_wmi_vdev_up(ar, vdev_id, 0, ar->mac_addr);

> +	if (ret) {

> +		ath11k_warn(ar->ab, "failed to put up monitor vdev %i: %d\n",

> +			    vdev_id, ret);

> +		goto vdev_stop;

> +	}

> +	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i started\n",

> +		   vdev_id);

>  	return 0;

> +

> +vdev_stop:

> +	reinit_completion(&ar->vdev_setup_done);

> +

> +	ret = ath11k_wmi_vdev_stop(ar, vdev_id);

> +	if (ret) {

> +		ath11k_warn(ar->ab, "failed to stop monitor vdev %i after start failure: %d\n",

> +			    vdev_id, ret);

> +		return ret;

> +	}

> +

> +	ret = ath11k_mac_vdev_setup_sync(ar);

> +	if (ret)

> +		ath11k_warn(ar->ab, "failed to synchronize setup for vdev %i stop: %d\n",

> +			    vdev_id, ret);


I added return ret here for consistency..

> +	return ret;


I don't thinks this is right, in an error path (vdev_stop label) we
return 0? I changed this to -EIO.

> +static int ath11k_mac_monitor_vdev_stop(struct ath11k *ar)

> +{

> +	int ret;

> +

> +	lockdep_assert_held(&ar->conf_mutex);

> +

> +	reinit_completion(&ar->vdev_setup_done);

> +

> +	ret = ath11k_wmi_vdev_stop(ar, ar->monitor_vdev_id);

> +	if (ret)

> +		ath11k_warn(ar->ab, "failed to request monitor vdev %i stop: %d\n",

> +			    ar->monitor_vdev_id, ret);

> +

> +	ret = ath11k_mac_vdev_setup_sync(ar);

> +	if (ret)

> +		ath11k_warn(ar->ab, "failed to synchronize monitor vdev %i stop: %d\n",

> +			    ar->monitor_vdev_id, ret);

> +

> +	ret = ath11k_wmi_vdev_down(ar, ar->monitor_vdev_id);

> +	if (ret)

> +		ath11k_warn(ar->ab, "failed to put down monitor vdev %i: %d\n",

> +			    ar->monitor_vdev_id, ret);

> +

> +	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i stopped\n",

> +		   ar->monitor_vdev_id);

> +	return ret;

> +}


I was not sure what's the idea of error path handling here, we print
warnings but still continue the normal execution. I changed this so that
we bail out in the first error and if everything goes well we return 0.

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo Sept. 21, 2021, 1:42 p.m. UTC | #5
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
Kalle Valo Sept. 21, 2021, 3:55 p.m. UTC | #6
Kalle Valo <kvalo@codeaurora.org> writes:

>> +vdev_stop:

>> +	reinit_completion(&ar->vdev_setup_done);

>> +

>> +	ret = ath11k_wmi_vdev_stop(ar, vdev_id);

>> +	if (ret) {

>> +		ath11k_warn(ar->ab, "failed to stop monitor vdev %i after start failure: %d\n",

>> +			    vdev_id, ret);

>> +		return ret;

>> +	}

>> +

>> +	ret = ath11k_mac_vdev_setup_sync(ar);

>> +	if (ret)

>> +		ath11k_warn(ar->ab, "failed to synchronize setup for vdev %i stop: %d\n",

>> +			    vdev_id, ret);

>

> I added return ret here for consistency..

>

>> +	return ret;

>

> I don't thinks this is right, in an error path (vdev_stop label) we

> return 0? I changed this to -EIO.

>

>> +static int ath11k_mac_monitor_vdev_stop(struct ath11k *ar)

>> +{

>> +	int ret;

>> +

>> +	lockdep_assert_held(&ar->conf_mutex);

>> +

>> +	reinit_completion(&ar->vdev_setup_done);

>> +

>> +	ret = ath11k_wmi_vdev_stop(ar, ar->monitor_vdev_id);

>> +	if (ret)

>> +		ath11k_warn(ar->ab, "failed to request monitor vdev %i stop: %d\n",

>> +			    ar->monitor_vdev_id, ret);

>> +

>> +	ret = ath11k_mac_vdev_setup_sync(ar);

>> +	if (ret)

>> + ath11k_warn(ar->ab, "failed to synchronize monitor vdev %i stop:

>> %d\n",

>> +			    ar->monitor_vdev_id, ret);

>> +

>> +	ret = ath11k_wmi_vdev_down(ar, ar->monitor_vdev_id);

>> +	if (ret)

>> +		ath11k_warn(ar->ab, "failed to put down monitor vdev %i: %d\n",

>> +			    ar->monitor_vdev_id, ret);

>> +

>> +	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i stopped\n",

>> +		   ar->monitor_vdev_id);

>> +	return ret;

>> +}

>

> I was not sure what's the idea of error path handling here, we print

> warnings but still continue the normal execution. I changed this so that

> we bail out in the first error and if everything goes well we return 0.


I found quite a few missing error checks, too many to list here but
fixed in the pending branch:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=8b2f8d11422e7909ff02db456cda41728f621de4

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5ff318be206b3d2a0bfdcfaf0ac52cc3b4ecdeae

Please double check, compile tested only.

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Seevalamuthu Mariappan Sept. 22, 2021, 3:53 p.m. UTC | #7
On 2021-09-21 21:25, Kalle Valo wrote:
> Kalle Valo <kvalo@codeaurora.org> writes:

> 

>>> +vdev_stop:

>>> +	reinit_completion(&ar->vdev_setup_done);

>>> +

>>> +	ret = ath11k_wmi_vdev_stop(ar, vdev_id);

>>> +	if (ret) {

>>> +		ath11k_warn(ar->ab, "failed to stop monitor vdev %i after start 

>>> failure: %d\n",

>>> +			    vdev_id, ret);

>>> +		return ret;

>>> +	}

>>> +

>>> +	ret = ath11k_mac_vdev_setup_sync(ar);

>>> +	if (ret)

>>> +		ath11k_warn(ar->ab, "failed to synchronize setup for vdev %i stop: 

>>> %d\n",

>>> +			    vdev_id, ret);

>> 

>> I added return ret here for consistency..

>> 

>>> +	return ret;

>> 

>> I don't thinks this is right, in an error path (vdev_stop label) we

>> return 0? I changed this to -EIO.

>> 

>>> +static int ath11k_mac_monitor_vdev_stop(struct ath11k *ar)

>>> +{

>>> +	int ret;

>>> +

>>> +	lockdep_assert_held(&ar->conf_mutex);

>>> +

>>> +	reinit_completion(&ar->vdev_setup_done);

>>> +

>>> +	ret = ath11k_wmi_vdev_stop(ar, ar->monitor_vdev_id);

>>> +	if (ret)

>>> +		ath11k_warn(ar->ab, "failed to request monitor vdev %i stop: 

>>> %d\n",

>>> +			    ar->monitor_vdev_id, ret);

>>> +

>>> +	ret = ath11k_mac_vdev_setup_sync(ar);

>>> +	if (ret)

>>> + ath11k_warn(ar->ab, "failed to synchronize monitor vdev %i stop:

>>> %d\n",

>>> +			    ar->monitor_vdev_id, ret);

>>> +

>>> +	ret = ath11k_wmi_vdev_down(ar, ar->monitor_vdev_id);

>>> +	if (ret)

>>> +		ath11k_warn(ar->ab, "failed to put down monitor vdev %i: %d\n",

>>> +			    ar->monitor_vdev_id, ret);

>>> +

>>> +	ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "mac monitor vdev %i stopped\n",

>>> +		   ar->monitor_vdev_id);

>>> +	return ret;

>>> +}

>> 

>> I was not sure what's the idea of error path handling here, we print

>> warnings but still continue the normal execution. I changed this so 

>> that

>> we bail out in the first error and if everything goes well we return 

>> 0.

> 

> I found quite a few missing error checks, too many to list here but

> fixed in the pending branch:

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=8b2f8d11422e7909ff02db456cda41728f621de4

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=5ff318be206b3d2a0bfdcfaf0ac52cc3b4ecdeae

> 

> Please double check, compile tested only.


Thanks for the fixes Kalle. It looks fine and tested the same.

Regards,
Seevalamuthu M