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 4CFDEC4338F
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Jouni Malinen <jouni@codeaurora.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        Sathishkumar Muruganandam <murugana@codeaurora.org>
Subject: Re: [PATCH 1/2] ath11k: fix 4-addr tx failure for AP and STA modes
References: <20210720213147.90042-1-jouni@codeaurora.org>
Date:   Thu, 16 Sep 2021 12:14:02 +0300
In-Reply-To: <20210720213147.90042-1-jouni@codeaurora.org> (Jouni Malinen's
        message of "Wed, 21 Jul 2021 00:31:46 +0300")
Message-ID: <87wnngzxut.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

Jouni Malinen <jouni@codeaurora.org> writes:

> From: Sathishkumar Muruganandam <murugana@codeaurora.org>
>
> Ath11k FW requires peer parameter WMI_PEER_USE_4ADDR to be set for
> 4-addr peers allowing 4-address frame transmission to those peers.
>
> Add ath11k driver callback for sta_set_4addr() to queue new workq
> set_4addr_wk only once based on new boolean, use_4addr_set.
>
> sta_set_4addr() will be called during 4-addr STA association cases
> applicable for both AP and STA modes.
>
> In ath11k_sta_set_4addr_wk(),
>
> AP mode:
>         WMI_PEER_USE_4ADDR will be set for the corresponding
>         associated 4-addr STA(s)
>
> STA mode:
>         WMI_PEER_USE_4ADDR will be set for the AP to which the
>         4-addr STA got associated.
>
> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01238-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Sathishkumar Muruganandam <murugana@codeaurora.org>
> Signed-off-by: Jouni Malinen <jouni@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath11k/core.h |  3 ++
>  drivers/net/wireless/ath/ath11k/mac.c  | 48 ++++++++++++++++++++++++--
>  2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
> index 018fb2385f2a..11c8dffd0236 100644
> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -362,6 +362,7 @@ struct ath11k_sta {
>  	enum hal_pn_type pn_type;
>  
>  	struct work_struct update_wk;
> +	struct work_struct set_4addr_wk;
>  	struct rate_info txrate;
>  	struct rate_info last_txrate;
>  	u64 rx_duration;
> @@ -374,6 +375,8 @@ struct ath11k_sta {
>  	/* protected by conf_mutex */
>  	bool aggr_mode;
>  #endif
> +
> +	bool use_4addr_set;
>  };

[...]

> +static void ath11k_mac_op_sta_set_4addr(struct ieee80211_hw *hw,
> +					struct ieee80211_vif *vif,
> +					struct ieee80211_sta *sta, bool enabled)
> +{
> +	struct ath11k *ar = hw->priv;
> +	struct ath11k_sta *arsta = (struct ath11k_sta *)sta->drv_priv;
> +
> +	if (enabled && !arsta->use_4addr_set) {
> +		ieee80211_queue_work(ar->hw, &arsta->set_4addr_wk);
> +		arsta->use_4addr_set = true;
> +	}
> +}

There's no locking for accessing arsta->use_4addr_set. Most likely it's
just a theoretical issue but wanted to point out anyway. Maybe mac80211
handles serialisation of this callback?

I'm going to apply this patch anyway so that I get my patch queue
smaller, we can fix any issues in a followup patch.

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

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