| Message ID | 20240419170314.3117623-1-quic_kathirve@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: drop failed transmitted frames from metric calculation. | expand |
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
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 <quic_kathirve@quicinc.com> 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.
On 4/23/2024 3:12 PM, Kalle Valo wrote: > Karthikeyan Kathirvel <quic_kathirve@quicinc.com> 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"
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 {
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 ++++++++++++ 2 files changed, 48 insertions(+), 10 deletions(-) base-commit: 363e7193eaf258fe7f04e8db560bd8a282a12cd9