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 <[email protected]>
---
drivers/net/wireless/ath/ath12k/dp_tx.c | 38 ++++++++++++++++------
drivers/net/wireless/ath/ath12k/hal_desc.h | 20 ++++++++++++
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
+ * 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
--
2.34.1
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 <[email protected]>
> ---
> 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
On Fri Apr 19, 2024 at 7:03 PM CEST, Karthikeyan Kathirvel wrote:
[...]
> 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
> + * the upper layer
> + */
> + dev_kfree_skb_any(msdu);
Shouldn't this be ieee80211_free_txskb ? I'm not clear when to use which.
> + 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.
Thanks
On 4/22/2024 1:35 PM, Nicolas Escande wrote:
> On Fri Apr 19, 2024 at 7:03 PM CEST, Karthikeyan Kathirvel wrote:
> [...]
>> 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
>> + * the upper layer
>> + */
>> + dev_kfree_skb_any(msdu);
> Shouldn't this be ieee80211_free_txskb ? I'm not clear when to use which.
Yes, there is an internal patch is in pipeline, that will take care of
addressing all dev_kfree_skb_any() to ieee80211_free_txskb() in
ath12k_dp_tx_complete_msdu(), this patch doesn't take care of that.
>> + 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.
>
> Thanks
Karthikeyan Kathirvel <[email protected]> writes:
> On 4/22/2024 1:35 PM, Nicolas Escande wrote:
>> On Fri Apr 19, 2024 at 7:03 PM CEST, Karthikeyan Kathirvel wrote:
>>
>>> + 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
>>> + * the upper layer
>>> + */
>>> + dev_kfree_skb_any(msdu);
>>
>> Shouldn't this be ieee80211_free_txskb ? I'm not clear when to use
>> which.
>
> Yes, there is an internal patch is in pipeline, that will take care of
> addressing all dev_kfree_skb_any() to ieee80211_free_txskb() in
> ath12k_dp_tx_complete_msdu(), this patch doesn't take care of that.
Let's not add new bugs knowingly. Please fix this patch to use
ieee80211_free_txskb(), that is much more reliable than waiting for some
other patch.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 4/23/2024 3:12 PM, Kalle Valo wrote:
> Karthikeyan Kathirvel <[email protected]> writes:
>
>> On 4/22/2024 1:35 PM, Nicolas Escande wrote:
>>> On Fri Apr 19, 2024 at 7:03 PM CEST, Karthikeyan Kathirvel wrote:
>>>
>>>> + 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
>>>> + * the upper layer
>>>> + */
>>>> + dev_kfree_skb_any(msdu);
>>>
>>> Shouldn't this be ieee80211_free_txskb ? I'm not clear when to use
>>> which.
>>
>> Yes, there is an internal patch is in pipeline, that will take care of
>> addressing all dev_kfree_skb_any() to ieee80211_free_txskb() in
>> ath12k_dp_tx_complete_msdu(), this patch doesn't take care of that.
>
> Let's not add new bugs knowingly. Please fix this patch to use
> ieee80211_free_txskb(), that is much more reliable than waiting for some
> other patch.
>
Have addressed Nicolas comment(replaced dev_kfree_skb_any() with
ieee80211_free_txskb()) in latest version "[PATCH v3 0/2] wifi: ath12k:
drop failed transmitted frames from metric calculation"