Received-SPF: pass (google.com: domain of linux-wireless+bounces-6585-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <b52f04d9-4569-4b4d-9e67-93ebc8775dae@quicinc.com>
Date: Fri, 19 Apr 2024 11:08:32 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: ath12k: drop failed transmitted frames from metric
 calculation.
Content-Language: en-US
To: Karthikeyan Kathirvel <quic_kathirve@quicinc.com>,
        <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240419170314.3117623-1-quic_kathirve@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240419170314.3117623-1-quic_kathirve@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/19/2024 10:03 AM, Karthikeyan Kathirvel wrote:
> In mesh node traffic, internal firmware-transmitted failures are
> reported as transmitted failures in mesh metric calculation, leading
> to the breakage of the mesh link.
> 
> Fix the issue by dropping the internal firmware-transmitted failures
> before updating the TX completion status to mac80211, in order to
> prevent false failure averaging in mesh metric calculation.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> 
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
> Signed-off-by: Karthikeyan Kathirvel <quic_kathirve@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/dp_tx.c    | 38 ++++++++++++++++------
>  drivers/net/wireless/ath/ath12k/hal_desc.h | 20 ++++++++++++

My Qualcomm Innovation Center copyright checker reports:
drivers/net/wireless/ath/ath12k/hal_desc.h copyright missing 2024

>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
> index 9b6d7d72f57c..164431b7eb4e 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
> @@ -481,18 +481,36 @@ static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
>  	/* skip tx rate update from ieee80211_status*/
>  	info->status.rates[0].idx = -1;
>  
> -	if (ts->status == HAL_WBM_TQM_REL_REASON_FRAME_ACKED &&
> -	    !(info->flags & IEEE80211_TX_CTL_NO_ACK)) {
> -		info->flags |= IEEE80211_TX_STAT_ACK;
> -		info->status.ack_signal = ATH12K_DEFAULT_NOISE_FLOOR +
> -					  ts->ack_rssi;
> -		info->status.flags = IEEE80211_TX_STATUS_ACK_SIGNAL_VALID;
> +	switch (ts->status) {
> +	case HAL_WBM_TQM_REL_REASON_FRAME_ACKED:
> +		if (!(info->flags & IEEE80211_TX_CTL_NO_ACK)) {
> +			info->flags |= IEEE80211_TX_STAT_ACK;
> +			info->status.ack_signal = ATH12K_DEFAULT_NOISE_FLOOR +
> +						  ts->ack_rssi;
> +			info->status.flags = IEEE80211_TX_STATUS_ACK_SIGNAL_VALID;
> +		}
> +		break;
> +	case HAL_WBM_TQM_REL_REASON_CMD_REMOVE_TX:
> +		if (info->flags & IEEE80211_TX_CTL_NO_ACK) {
> +			info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> +			break;
> +		}
> +		fallthrough;
> +	case HAL_WBM_TQM_REL_REASON_CMD_REMOVE_MPDU:
> +	case HAL_WBM_TQM_REL_REASON_DROP_THRESHOLD:
> +	case HAL_WBM_TQM_REL_REASON_CMD_REMOVE_AGED_FRAMES:
> +		/* The failure status is due to internal firmware tx failure
> +		 * hence drop the frame do not update the status of frame to

there is a separator missing here, so should be something like:
...drop the frame and do not update...
...drop the frame -- do not update...
...drop the frame; do not update...
...drop the frame (do not update...)

> +		 * the upper layer
> +		 */
> +		dev_kfree_skb_any(msdu);
> +		goto exit;
> +	default:
> +		ath12k_dbg(ab, ATH12K_DBG_DP_TX, "tx frame is not acked status %d\n",
> +			   ts->status);
> +		break;
>  	}
>  
> -	if (ts->status == HAL_WBM_TQM_REL_REASON_CMD_REMOVE_TX &&
> -	    (info->flags & IEEE80211_TX_CTL_NO_ACK))
> -		info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> -
>  	/* NOTE: Tx rate status reporting. Tx completion status does not have
>  	 * necessary information (for example nss) to build the tx rate.
>  	 * Might end up reporting it out-of-band from HTT stats.
> diff --git a/drivers/net/wireless/ath/ath12k/hal_desc.h b/drivers/net/wireless/ath/ath12k/hal_desc.h
> index 63340256d3f6..677f31879553 100644
> --- a/drivers/net/wireless/ath/ath12k/hal_desc.h
> +++ b/drivers/net/wireless/ath/ath12k/hal_desc.h
> @@ -2048,6 +2048,19 @@ struct hal_wbm_release_ring {
>   *	fw with fw_reason2.
>   * @HAL_WBM_TQM_REL_REASON_CMD_REMOVE_RESEAON3: Remove command initiated by
>   *	fw with fw_reason3.
> + * @HAL_WBM_TQM_REL_REASON_CMD_DISABLE_QUEUE: Remove command initiated by
> + *	fw with disable queue.
> + * @HAL_WBM_TQM_REL_REASON_CMD_TILL_NONMATCHING: Remove command initiated by
> + *	fw to remove all mpdu until 1st non-match.
> + * @HAL_WBM_TQM_REL_REASON_DROP_THRESHOLD: Dropped due to drop threshold
> + *	criteria
> + * @HAL_WBM_TQM_REL_REASON_DROP_LINK_DESC_UNAVAIL: Dropped due to link desc
> + *	not available
> + * @HAL_WBM_TQM_REL_REASON_DROP_OR_INVALID_MSDU: Dropped due drop bit set or
> + *	null flow
> + * @HAL_WBM_TQM_REL_REASON_MULTICAST_DROP: Dropped due mcast drop set for VDEV
> + * @HAL_WBM_TQM_REL_REASON_VDEV_MISMATCH_DROP: Dropped due to being set with
> + *	'TCL_drop_reason'
>   */
>  enum hal_wbm_tqm_rel_reason {
>  	HAL_WBM_TQM_REL_REASON_FRAME_ACKED,
> @@ -2058,6 +2071,13 @@ enum hal_wbm_tqm_rel_reason {
>  	HAL_WBM_TQM_REL_REASON_CMD_REMOVE_RESEAON1,
>  	HAL_WBM_TQM_REL_REASON_CMD_REMOVE_RESEAON2,
>  	HAL_WBM_TQM_REL_REASON_CMD_REMOVE_RESEAON3,
> +	HAL_WBM_TQM_REL_REASON_CMD_DISABLE_QUEUE,
> +	HAL_WBM_TQM_REL_REASON_CMD_TILL_NONMATCHING,
> +	HAL_WBM_TQM_REL_REASON_DROP_THRESHOLD,
> +	HAL_WBM_TQM_REL_REASON_DROP_LINK_DESC_UNAVAIL,
> +	HAL_WBM_TQM_REL_REASON_DROP_OR_INVALID_MSDU,
> +	HAL_WBM_TQM_REL_REASON_MULTICAST_DROP,
> +	HAL_WBM_TQM_REL_REASON_VDEV_MISMATCH_DROP,
>  };
>  
>  struct hal_wbm_buffer_ring {
> 
> base-commit: 363e7193eaf258fe7f04e8db560bd8a282a12cd9
actual code LGTM, will ack a v2 with the nits addressed