On 8/12/2022 9:09 AM, Kalle Valo wrote:
From: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
(Patches split into one patch per file for easier review, but the final
commit will be one big patch. See the cover letter for more info.)
Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
---
drivers/net/wireless/ath/ath12k/dp_rx.c | 4308 +++++++++++++++++++++++++++++++
1 file changed, 4308 insertions(+)
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
review part 2
snip
+static int ath12k_dp_rx_pdev_srng_alloc(struct ath12k *ar)
+{
+ struct ath12k_pdev_dp *dp = &ar->dp;
+ struct ath12k_base *ab = ar->ab;
+ int i;
+ int ret;
+ u32 mac_id = dp->mac_id;
+
+ if (!ar->ab->hw_params->rxdma1_enable) {
+ /* init mon status buffer reap timer */
+ timer_setup(&ar->ab->mon_reap_timer,
+ ath12k_dp_service_mon_ring, 0);
+ return 0;
+ }
the timer_setup() seems like strange logic to have in a function named
ath12k_dp_rx_pdev_srng_alloc() -- that doesn't have anything to do with
srng allocation
perhaps that should be in a separate properly-named function?
snip
+static void ath12k_dp_reo_cmd_free(struct ath12k_dp *dp, void *ctx,
+ enum hal_reo_cmd_status status)
+{
+ struct ath12k_dp_rx_tid *rx_tid = ctx;
+
+ if (status != HAL_REO_CMD_SUCCESS)
+ ath12k_warn(dp->ab, "failed to flush rx tid hw desc, tid %d status %d\n",
+ rx_tid->tid, status);
+
+ dma_unmap_single(dp->ab->dev, rx_tid->paddr, rx_tid->size,
+ DMA_BIDIRECTIONAL);
+ kfree(rx_tid->vaddr);
you don't want to set rx_tid->vaddr = NULL so that there isn't a
dangling pointer?
+}
+
+static int ath12k_dp_reo_cmd_send(struct ath12k_base *ab, struct ath12k_dp_rx_tid *rx_tid,
+ enum hal_reo_cmd_type type,
+ struct ath12k_hal_reo_cmd *cmd,
+ void (*cb)(struct ath12k_dp *dp, void *ctx,
+ enum hal_reo_cmd_status status))
+{
+ struct ath12k_dp *dp = &ab->dp;
+ struct ath12k_dp_rx_reo_cmd *dp_cmd;
+ struct hal_srng *cmd_ring;
+ int cmd_num;
+
+ cmd_ring = &ab->hal.srng_list[dp->reo_cmd_ring.ring_id];
+ cmd_num = ath12k_hal_reo_cmd_send(ab, cmd_ring, type, cmd);
+
+ /* cmd_num should start from 1, during failure return the error code */
+ if (cmd_num < 0)
+ return cmd_num;
+
+ /* reo cmd ring descriptors has cmd_num starting from 1 */
+ if (cmd_num == 0)
+ return -EINVAL;
+
+ if (!cb)
+ return 0;
+
+ /* Can this be optimized so that we keep the pending command list only
+ * for tid delete command to free up the resoruce on the command status
s/resoruce/resource/
+ * indication?
+ */
+ dp_cmd = kzalloc(sizeof(*dp_cmd), GFP_ATOMIC);
+
+ if (!dp_cmd)
+ return -ENOMEM;
+
+ memcpy(&dp_cmd->data, rx_tid, sizeof(struct ath12k_dp_rx_tid));
sizeof(*rx_tid) is preferred
+ dp_cmd->cmd_num = cmd_num;
+ dp_cmd->handler = cb;
+
+ spin_lock_bh(&dp->reo_cmd_lock);
+ list_add_tail(&dp_cmd->list, &dp->reo_cmd_list);
+ spin_unlock_bh(&dp->reo_cmd_lock);
+
+ return 0;
+}
+
+static void ath12k_dp_reo_cache_flush(struct ath12k_base *ab,
+ struct ath12k_dp_rx_tid *rx_tid)
+{
+ struct ath12k_hal_reo_cmd cmd = {0};
+ unsigned long tot_desc_sz, desc_sz;
+ int ret;
+
+ tot_desc_sz = rx_tid->size;
+ desc_sz = ath12k_hal_reo_qdesc_size(0, HAL_DESC_REO_NON_QOS_TID);
+
+ while (tot_desc_sz > desc_sz) {
+ tot_desc_sz -= desc_sz;
+ cmd.addr_lo = lower_32_bits(rx_tid->paddr + tot_desc_sz);
+ cmd.addr_hi = upper_32_bits(rx_tid->paddr);
+ ret = ath12k_dp_reo_cmd_send(ab, rx_tid,
+ HAL_REO_CMD_FLUSH_CACHE, &cmd,
+ NULL);
+ if (ret)
+ ath12k_warn(ab,
+ "failed to send HAL_REO_CMD_FLUSH_CACHE, tid %d (%d)\n",
+ rx_tid->tid, ret);
+ }
+
+ memset(&cmd, 0, sizeof(cmd));
+ cmd.addr_lo = lower_32_bits(rx_tid->paddr);
+ cmd.addr_hi = upper_32_bits(rx_tid->paddr);
+ cmd.flag |= HAL_REO_CMD_FLG_NEED_STATUS;
why |= instead of just =? flag will always be 0 at this point
+ ret = ath12k_dp_reo_cmd_send(ab, rx_tid,
+ HAL_REO_CMD_FLUSH_CACHE,
+ &cmd, ath12k_dp_reo_cmd_free);
+ if (ret) {
+ ath12k_err(ab, "failed to send HAL_REO_CMD_FLUSH_CACHE cmd, tid %d (%d)\n",
+ rx_tid->tid, ret);
+ dma_unmap_single(ab->dev, rx_tid->paddr, rx_tid->size,
+ DMA_BIDIRECTIONAL);
+ kfree(rx_tid->vaddr);
you don't want to set rx_tid->vaddr = NULL so that there isn't a
dangling pointer?
+ }
+}
+
+static void ath12k_dp_rx_tid_del_func(struct ath12k_dp *dp, void *ctx,
+ enum hal_reo_cmd_status status)
+{
+ struct ath12k_base *ab = dp->ab;
+ struct ath12k_dp_rx_tid *rx_tid = ctx;
+ struct ath12k_dp_rx_reo_cache_flush_elem *elem, *tmp;
+
+ if (status == HAL_REO_CMD_DRAIN) {
+ goto free_desc;
+ } else if (status != HAL_REO_CMD_SUCCESS) {
+ /* Shouldn't happen! Cleanup in case of other failure? */
+ ath12k_warn(ab, "failed to delete rx tid %d hw descriptor %d\n",
+ rx_tid->tid, status);
+ return;
+ }
+
+ elem = kzalloc(sizeof(*elem), GFP_ATOMIC);
+ if (!elem)
+ goto free_desc;
+
+ elem->ts = jiffies;
+ memcpy(&elem->data, rx_tid, sizeof(*rx_tid));
+
+ spin_lock_bh(&dp->reo_cmd_lock);
+ list_add_tail(&elem->list, &dp->reo_cmd_cache_flush_list);
+ dp->reo_cmd_cache_flush_count++;
+
+ /* Flush and invalidate aged REO desc from HW cache */
+ list_for_each_entry_safe(elem, tmp, &dp->reo_cmd_cache_flush_list,
+ list) {
+ if (dp->reo_cmd_cache_flush_count > ATH12K_DP_RX_REO_DESC_FREE_THRES ||
+ time_after(jiffies, elem->ts +
+ msecs_to_jiffies(ATH12K_DP_RX_REO_DESC_FREE_TIMEOUT_MS))) {
+ list_del(&elem->list);
+ dp->reo_cmd_cache_flush_count--;
+ spin_unlock_bh(&dp->reo_cmd_lock);
+
+ ath12k_dp_reo_cache_flush(ab, &elem->data);
+ kfree(elem);
+ spin_lock_bh(&dp->reo_cmd_lock);
is this really a safe iteration if you unlock & lock in the middle?
what prevents the tmp node from being deleted during this window?
+ }
+ }
+ spin_unlock_bh(&dp->reo_cmd_lock);
+
+ return;
+free_desc:
+ dma_unmap_single(ab->dev, rx_tid->paddr, rx_tid->size,
+ DMA_BIDIRECTIONAL);
+ kfree(rx_tid->vaddr);
you don't want to set rx_tid->vaddr = NULL so that there isn't a
dangling pointer?
+}
snip
+void ath12k_dp_rx_peer_tid_delete(struct ath12k *ar,
+ struct ath12k_peer *peer, u8 tid)
+{
+ struct ath12k_hal_reo_cmd cmd = {0};
+ struct ath12k_dp_rx_tid *rx_tid = &peer->rx_tid[tid];
+ int ret;
+
+ if (!rx_tid->active)
+ return;
+
+ cmd.flag = HAL_REO_CMD_FLG_NEED_STATUS;
+ cmd.addr_lo = lower_32_bits(rx_tid->paddr);
+ cmd.addr_hi = upper_32_bits(rx_tid->paddr);
+ cmd.upd0 |= HAL_REO_CMD_UPD0_VLD;
why |= instead of just =?
+ ret = ath12k_dp_reo_cmd_send(ar->ab, rx_tid,
+ HAL_REO_CMD_UPDATE_RX_QUEUE, &cmd,
+ ath12k_dp_rx_tid_del_func);
+ if (ret) {
+ ath12k_err(ar->ab, "failed to send HAL_REO_CMD_UPDATE_RX_QUEUE cmd, tid %d (%d)\n",
+ tid, ret);
+ dma_unmap_single(ar->ab->dev, rx_tid->paddr, rx_tid->size,
+ DMA_BIDIRECTIONAL);
+ kfree(rx_tid->vaddr);
you don't want to set rx_tid->vaddr = NULL so that there isn't a
dangling pointer?
+ }
+
+ ath12k_peer_rx_tid_qref_reset(ar->ab, peer->peer_id, tid);
+
+ rx_tid->active = false;
+}
snip
+int ath12k_dp_rx_peer_tid_setup(struct ath12k *ar, const u8 *peer_mac, int vdev_id,
+ u8 tid, u32 ba_win_sz, u16 ssn,
+ enum hal_pn_type pn_type)
+{
+ struct ath12k_base *ab = ar->ab;
+ struct ath12k_dp *dp = &ab->dp;
+ struct hal_rx_reo_queue *addr_aligned;
+ struct ath12k_peer *peer;
+ struct ath12k_dp_rx_tid *rx_tid;
+ u32 hw_desc_sz;
+ void *vaddr;
+ dma_addr_t paddr;
+ int ret;
+
+ spin_lock_bh(&ab->base_lock);
+
+ peer = ath12k_peer_find(ab, vdev_id, peer_mac);
+ if (!peer) {
+ spin_unlock_bh(&ab->base_lock);
+ ath12k_warn(ab, "failed to find the peer to set up rx tid\n");
should you log the peer_mac?
snip
+int ath12k_dp_rx_peer_pn_replay_config(struct ath12k_vif *arvif,
+ const u8 *peer_addr,
+ enum set_key_cmd key_cmd,
+ struct ieee80211_key_conf *key)
+{
+ struct ath12k *ar = arvif->ar;
+ struct ath12k_base *ab = ar->ab;
+ struct ath12k_hal_reo_cmd cmd = {0};
+ struct ath12k_peer *peer;
+ struct ath12k_dp_rx_tid *rx_tid;
+ u8 tid;
+ int ret = 0;
+
+ /* NOTE: Enable PN/TSC replay check offload only for unicast frames.
+ * We use mac80211 PN/TSC replay check functionality for bcast/mcast
+ * for now.
+ */
+ if (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE))
+ return 0;
+
+ cmd.flag |= HAL_REO_CMD_FLG_NEED_STATUS;
+ cmd.upd0 |= HAL_REO_CMD_UPD0_PN |
why |= instead of just = for both of the above?
+ HAL_REO_CMD_UPD0_PN_SIZE |
+ HAL_REO_CMD_UPD0_PN_VALID |
+ HAL_REO_CMD_UPD0_PN_CHECK |
+ HAL_REO_CMD_UPD0_SVLD;
+
+ switch (key->cipher) {
+ case WLAN_CIPHER_SUITE_TKIP:
+ case WLAN_CIPHER_SUITE_CCMP:
+ case WLAN_CIPHER_SUITE_CCMP_256:
+ case WLAN_CIPHER_SUITE_GCMP:
+ case WLAN_CIPHER_SUITE_GCMP_256:
+ if (key_cmd == SET_KEY) {
+ cmd.upd1 |= HAL_REO_CMD_UPD1_PN_CHECK;
+ cmd.pn_size = 48;
+ }
+ break;
+ default:
+ break;
+ }
+
+ spin_lock_bh(&ab->base_lock);
+
+ peer = ath12k_peer_find(ab, arvif->vdev_id, peer_addr);
+ if (!peer) {
+ ath12k_warn(ab, "failed to find the peer to configure pn replay detection\n");
should you log the peer_addr?
also it is preferable to unlock first, and then log
snip
+static int ath12k_get_ppdu_user_index(struct htt_ppdu_stats *ppdu_stats,
+ u16 peer_id)
+{
+ int i;
+
+ for (i = 0; i < HTT_PPDU_STATS_MAX_USERS - 1; i++) {
+ if (ppdu_stats->user_stats[i].is_valid_peer_id) {
+ if (peer_id == ppdu_stats->user_stats[i].peer_id)
+ return i;
+ } else {
+ return i;
+ }
is the user_stats[] array maintained in a manner where the
"is_valid_peer_id" records are always at the beginning of the array?
if not, then don't you have an issue if entry 0 has is_valid_peer_id =
false and entry 1 has is_valid_peer_id = true and peer_id is matching
since in that case you'd return 0 instead of 1?
+ }
+
+ return -EINVAL;
+}
+
+static int ath12k_htt_tlv_ppdu_stats_parse(struct ath12k_base *ab,
+ u16 tag, u16 len, const void *ptr,
+ void *data)
+{
+ struct htt_ppdu_stats_info *ppdu_info;
+ struct htt_ppdu_user_stats *user_stats;
+ int cur_user;
+ u16 peer_id;
+
+ ppdu_info = (struct htt_ppdu_stats_info *)data;
unnecessary typecast from void*
+
+ switch (tag) {
+ case HTT_PPDU_STATS_TAG_COMMON:
+ if (len < sizeof(struct htt_ppdu_stats_common)) {
+ ath12k_warn(ab, "Invalid len %d for the tag 0x%x\n",
+ len, tag);
+ return -EINVAL;
+ }
+ memcpy(&ppdu_info->ppdu_stats.common, ptr,
+ sizeof(struct htt_ppdu_stats_common));
+ break;
+ case HTT_PPDU_STATS_TAG_USR_RATE:
+ if (len < sizeof(struct htt_ppdu_stats_user_rate)) {
+ ath12k_warn(ab, "Invalid len %d for the tag 0x%x\n",
+ len, tag);
+ return -EINVAL;
+ }
+
+ peer_id = ((struct htt_ppdu_stats_user_rate *)ptr)->sw_peer_id;
+ cur_user = ath12k_get_ppdu_user_index(&ppdu_info->ppdu_stats,
+ peer_id);
+ if (cur_user < 0)
+ return -EINVAL;
+ user_stats = &ppdu_info->ppdu_stats.user_stats[cur_user];
+ user_stats->peer_id = peer_id;
+ user_stats->is_valid_peer_id = true;
+ memcpy(&user_stats->rate, ptr,
+ sizeof(struct htt_ppdu_stats_user_rate));
+ user_stats->tlv_flags |= BIT(tag);
+ break;
+ case HTT_PPDU_STATS_TAG_USR_COMPLTN_COMMON:
+ if (len < sizeof(struct htt_ppdu_stats_usr_cmpltn_cmn)) {
+ ath12k_warn(ab, "Invalid len %d for the tag 0x%x\n",
+ len, tag);
+ return -EINVAL;
+ }
+
+ peer_id = ((struct htt_ppdu_stats_usr_cmpltn_cmn *)ptr)->sw_peer_id;
+ cur_user = ath12k_get_ppdu_user_index(&ppdu_info->ppdu_stats,
+ peer_id);
+ if (cur_user < 0)
+ return -EINVAL;
+ user_stats = &ppdu_info->ppdu_stats.user_stats[cur_user];
+ user_stats->peer_id = peer_id;
+ user_stats->is_valid_peer_id = true;
+ memcpy(&user_stats->cmpltn_cmn, ptr,
+ sizeof(struct htt_ppdu_stats_usr_cmpltn_cmn));
+ user_stats->tlv_flags |= BIT(tag);
+ break;
+ case HTT_PPDU_STATS_TAG_USR_COMPLTN_ACK_BA_STATUS:
+ if (len <
+ sizeof(struct htt_ppdu_stats_usr_cmpltn_ack_ba_status)) {
+ ath12k_warn(ab, "Invalid len %d for the tag 0x%x\n",
+ len, tag);
+ return -EINVAL;
+ }
+
+ peer_id =
+ ((struct htt_ppdu_stats_usr_cmpltn_ack_ba_status *)ptr)->sw_peer_id;
descendant should be indented to aid readability
is it necessary to split this line?
+ cur_user = ath12k_get_ppdu_user_index(&ppdu_info->ppdu_stats,
+ peer_id);
+ if (cur_user < 0)
+ return -EINVAL;
+ user_stats = &ppdu_info->ppdu_stats.user_stats[cur_user];
+ user_stats->peer_id = peer_id;
+ user_stats->is_valid_peer_id = true;
+ memcpy(&user_stats->ack_ba, ptr,
+ sizeof(struct htt_ppdu_stats_usr_cmpltn_ack_ba_status));
+ user_stats->tlv_flags |= BIT(tag);
+ break;
+ }
+ return 0;
+}
snip
+static void ath12k_copy_to_delay_stats(struct ath12k_peer *peer,
+ struct htt_ppdu_user_stats *usr_stats)
+{
+ peer->ppdu_stats_delayba.sw_peer_id = usr_stats->rate.sw_peer_id;
+ peer->ppdu_stats_delayba.info0 = usr_stats->rate.info0;
+ peer->ppdu_stats_delayba.ru_end = usr_stats->rate.ru_end;
+ peer->ppdu_stats_delayba.ru_start = usr_stats->rate.ru_start;
+ peer->ppdu_stats_delayba.info1 = usr_stats->rate.info1;
+ peer->ppdu_stats_delayba.rate_flags = usr_stats->rate.rate_flags;
+ peer->ppdu_stats_delayba.resp_rate_flags = usr_stats->rate.resp_rate_flags;
+
+ peer->delayba_flag = true;
+}
+
+static void ath12k_copy_to_bar(struct ath12k_peer *peer,
+ struct htt_ppdu_user_stats *usr_stats)
+{
+ usr_stats->rate.sw_peer_id = peer->ppdu_stats_delayba.sw_peer_id;
+ usr_stats->rate.info0 = peer->ppdu_stats_delayba.info0;
+ usr_stats->rate.ru_end = peer->ppdu_stats_delayba.ru_end;
+ usr_stats->rate.ru_start = peer->ppdu_stats_delayba.ru_start;
+ usr_stats->rate.info1 = peer->ppdu_stats_delayba.info1;
+ usr_stats->rate.rate_flags = peer->ppdu_stats_delayba.rate_flags;
+ usr_stats->rate.resp_rate_flags = peer->ppdu_stats_delayba.resp_rate_flags;
+
+ peer->delayba_flag = false;
+}
see my comment in peer.h review about the above two functions
snip
+static void ath12k_htt_pktlog(struct ath12k_base *ab, struct sk_buff *skb)
+{
+ struct htt_pktlog_msg *data = (struct htt_pktlog_msg *)skb->data;
+ struct ath_pktlog_hdr *hdr = (struct ath_pktlog_hdr *)data;
+ struct ath12k *ar;
+ u8 pdev_id;
+
+ pdev_id = u32_get_bits(data->hdr, HTT_T2H_PPDU_STATS_INFO_PDEV_ID);
+ ar = ath12k_mac_get_ar_by_pdev_id(ab, pdev_id);
+ if (!ar) {
+ ath12k_warn(ab, "invalid pdev id %d on htt pktlog\n", pdev_id);
+ return;
+ }
+ hdr->m_timestamp = ar->pdev->timestamp;
+
+ trace_ath12k_htt_pktlog(ar, data->payload, hdr->size,
+ ar->ab->pktlog_defs_checksum);
+}
I am really confused about the above function.
The first two declarations have both data and hdr point to the skb->data.
So one would assume these are related structures. But instead we have:
struct ath_pktlog_hdr {
u16 flags;
u16 missed_cnt;
u16 log_type;
u16 size;
u32 timestamp;
u32 type_specific_data;
struct mlo_timestamp m_timestamp;
u8 payload[];
};
struct htt_pktlog_msg {
u32 hdr;
u8 payload[0];
};
this means the pdev_id = u32_get_bits() logic is somehow extracting the
pdev_id from the fields that hold the flags+missed_cnt
and the logic to write hdr->m_timestamp is somewhere in the middle of
the htt_pktlog_msg payload[].
now the only thing that I can guess is that the htt_pktlog_msg payload[]
begins with:
u16 log_type;
u16 size;
u32 timestamp;
u32 type_specific_data;
struct mlo_timestamp m_timestamp;
but if so, that is less than clear from the code and declarations
+
+static void ath12k_htt_backpressure_event_handler(struct ath12k_base *ab,
+ struct sk_buff *skb)
+{
+ u32 *data = (u32 *)skb->data;
+ u8 pdev_id, ring_type, ring_id, pdev_idx;
+ u16 hp, tp;
+ u32 backpressure_time;
+ struct ath12k_bp_stats *bp_stats;
+
+ pdev_id = u32_get_bits(*data, HTT_BACKPRESSURE_EVENT_PDEV_ID_M);
+ ring_type = u32_get_bits(*data, HTT_BACKPRESSURE_EVENT_RING_TYPE_M);
+ ring_id = u32_get_bits(*data, HTT_BACKPRESSURE_EVENT_RING_ID_M);
+ ++data;
+
+ hp = u32_get_bits(*data, HTT_BACKPRESSURE_EVENT_HP_M);
+ tp = u32_get_bits(*data, HTT_BACKPRESSURE_EVENT_TP_M);
+ ++data;
+
+ backpressure_time = *data;
I believe the above code would be much clearer if skb->data was typecast
to a struct pointer and assignments referenced members of the struct,
just like most/all of the functions that follow.
and as I've written before, I further believe that having accessor
functions/macros would make it even clearer.
snip
+static void ath12k_dp_rx_h_undecap_nwifi(struct ath12k *ar,
+ struct sk_buff *msdu,
+ enum hal_encrypt_type enctype,
+ struct ieee80211_rx_status *status)
+{
+ struct ath12k_base *ab = ar->ab;
+ struct ath12k_skb_rxcb *rxcb = ATH12K_SKB_RXCB(msdu);
+ u8 decap_hdr[DP_MAX_NWIFI_HDR_LEN];
+ struct ieee80211_hdr *hdr;
+ size_t hdr_len;
+ u8 *crypto_hdr;
+ u16 qos_ctl = 0;
nit: initializer always overwritten
+
+ /* pull decapped header */
+ hdr = (struct ieee80211_hdr *)msdu->data;
+ hdr_len = ieee80211_hdrlen(hdr->frame_control);
+ skb_pull(msdu, hdr_len);
+
+ /* Rebuild qos header */
+ hdr->frame_control |= __cpu_to_le16(IEEE80211_STYPE_QOS_DATA);
+
+ /* Reset the order bit as the HT_Control header is stripped */
+ hdr->frame_control &= ~(__cpu_to_le16(IEEE80211_FCTL_ORDER));
+
+ qos_ctl = rxcb->tid;
snip
+static void ath12k_get_dot11_hdr_from_rx_desc(struct ath12k *ar,
+ struct sk_buff *msdu,
+ struct ath12k_skb_rxcb *rxcb,
+ struct ieee80211_rx_status *status,
+ enum hal_encrypt_type enctype)
+{
+ struct hal_rx_desc *rx_desc = rxcb->rx_desc;
+ struct ath12k_base *ab = ar->ab;
+ size_t hdr_len, crypto_len;
+ struct ieee80211_hdr *hdr;
+ u16 qos_ctl = 0;
nit: initializer always overwritten
snip
+ qos_ctl = rxcb->tid;
+ if (ath12k_dp_rx_h_mesh_ctl_present(ab, rx_desc))
+ qos_ctl |= IEEE80211_QOS_CTL_MESH_CONTROL_PRESENT;
+
+ /* TODO: Add other QoS ctl fields when required */
+ memcpy(msdu->data + (hdr_len - IEEE80211_QOS_CTL_LEN),
+ &qos_ctl, IEEE80211_QOS_CTL_LEN);
+ }
+}
+
snip
+static void ath12k_dp_rx_deliver_msdu(struct ath12k *ar, struct napi_struct *napi,
+ struct sk_buff *msdu,
+ struct ieee80211_rx_status *status)
+{
+ struct ath12k_base *ab = ar->ab;
+ static const struct ieee80211_radiotap_he known = {
+ .data1 = cpu_to_le16(IEEE80211_RADIOTAP_HE_DATA1_DATA_MCS_KNOWN |
+ IEEE80211_RADIOTAP_HE_DATA1_BW_RU_ALLOC_KNOWN),
+ .data2 = cpu_to_le16(IEEE80211_RADIOTAP_HE_DATA2_GI_KNOWN),
+ };
+ struct ieee80211_radiotap_he *he = NULL;
nit: initializer always overwritten
+ struct ieee80211_rx_status *rx_status;
+ struct ieee80211_sta *pubsta = NULL;
+ struct ath12k_peer *peer;
+ struct ath12k_skb_rxcb *rxcb = ATH12K_SKB_RXCB(msdu);
+ u8 decap = DP_RX_DECAP_TYPE_RAW;
+ bool is_mcbc = rxcb->is_mcbc;
+ bool is_eapol = rxcb->is_eapol;
+
+ if (status->encoding == RX_ENC_HE && !(status->flag & RX_FLAG_RADIOTAP_HE) &&
+ !(status->flag & RX_FLAG_SKIP_MONITOR)) {
+ he = skb_push(msdu, sizeof(known));
+ memcpy(he, &known, sizeof(known));
this is only use of 'he' hence initializer is unnecessary
+ status->flag |= RX_FLAG_RADIOTAP_HE;
+ }
+
+ if (!(status->flag & RX_FLAG_ONLY_MONITOR))
+ decap = ath12k_dp_rx_h_decap_type(ab, rxcb->rx_desc);
+
+ spin_lock_bh(&ab->base_lock);
+ peer = ath12k_dp_rx_h_find_peer(ab, msdu);
+ if (peer && peer->sta)
+ pubsta = peer->sta;
could just unconditionally have pubsta = peer ? peer->sta : NULL
then initializer would be unnecessary
snip
+static int ath12k_dp_rx_process_msdu(struct ath12k *ar,
+ struct sk_buff *msdu,
+ struct sk_buff_head *msdu_list,
+ struct ieee80211_rx_status *rx_status)
+{
+ struct ath12k_base *ab = ar->ab;
+ struct hal_rx_desc *rx_desc, *lrx_desc;
+ struct ath12k_skb_rxcb *rxcb;
+ struct sk_buff *last_buf;
+ u8 l3_pad_bytes;
+ u16 msdu_len;
+ int ret;
+ u32 hal_rx_desc_sz = ar->ab->hw_params->hal_desc_sz;
+
+ last_buf = ath12k_dp_rx_get_msdu_last_buf(msdu_list, msdu);
+ if (!last_buf) {
+ ath12k_warn(ab,
+ "No valid Rx buffer to access MSDU_END tlv\n");
+ ret = -EIO;
+ goto free_out;
+ }
+
+ rx_desc = (struct hal_rx_desc *)msdu->data;
+ lrx_desc = (struct hal_rx_desc *)last_buf->data;
+ if (!ath12k_dp_rx_h_msdu_done(ab, lrx_desc)) {
+ ath12k_warn(ab, "msdu_done bit in msdu_end is not set\n");
+ ret = -EIO;
+ goto free_out;
+ }
+
+ rxcb = ATH12K_SKB_RXCB(msdu);
+ rxcb->rx_desc = rx_desc;
+ msdu_len = ath12k_dp_rx_h_msdu_len(ab, lrx_desc);
+ l3_pad_bytes = ath12k_dp_rx_h_l3pad(ab, lrx_desc);
+
+ if (rxcb->is_frag) {
+ skb_pull(msdu, hal_rx_desc_sz);
+ } else if (!rxcb->is_continuation) {
+ if ((msdu_len + hal_rx_desc_sz) > DP_RX_BUFFER_SIZE) {
+ ret = -EINVAL;
+ ath12k_warn(ab, "invalid msdu len %u\n", msdu_len);
+ ath12k_dbg_dump(ab, ATH12K_DBG_DATA, NULL, "", rx_desc,
+ sizeof(struct hal_rx_desc));
+ goto free_out;
+ }
+ skb_put(msdu, hal_rx_desc_sz + l3_pad_bytes + msdu_len);
+ skb_pull(msdu, hal_rx_desc_sz + l3_pad_bytes);
+ } else {
+ ret = ath12k_dp_rx_msdu_coalesce(ar, msdu_list,
+ msdu, last_buf,
+ l3_pad_bytes, msdu_len);
+ if (ret) {
+ ath12k_warn(ab,
+ "failed to coalesce msdu rx buffer%d\n", ret);
+ goto free_out;
+ }
+ }
+
+ ath12k_dp_rx_h_ppdu(ar, rx_desc, rx_status);
+ ath12k_dp_rx_h_mpdu(ar, msdu, rx_desc, rx_status);
+
+ rx_status->flag |= RX_FLAG_SKIP_MONITOR | RX_FLAG_DUP_VALIDATED;
+
+ return 0;
+
+free_out:
+ return ret;
+}
+
+static void ath12k_dp_rx_process_received_packets(struct ath12k_base *ab,
+ struct napi_struct *napi,
+ struct sk_buff_head *msdu_list,
+ int ring_id)
+{
+ struct ieee80211_rx_status rx_status = {0};
initializing just on entry seems wrong, see below
+ struct ath12k_skb_rxcb *rxcb;
+ struct sk_buff *msdu;
+ struct ath12k *ar;
+ u8 mac_id;
+ int ret;
+
+ if (skb_queue_empty(msdu_list))
+ return;
+
+ rcu_read_lock();
+
+ while ((msdu = __skb_dequeue(msdu_list))) {
+ rxcb = ATH12K_SKB_RXCB(msdu);
+ mac_id = rxcb->mac_id;
+ ar = ab->pdevs[mac_id].ar;
+ if (!rcu_dereference(ab->pdevs_active[mac_id])) {
+ dev_kfree_skb_any(msdu);
+ continue;
+ }
+
+ if (test_bit(ATH12K_CAC_RUNNING, &ar->dev_flags)) {
+ dev_kfree_skb_any(msdu);
+ continue;
+ }
+
+ ret = ath12k_dp_rx_process_msdu(ar, msdu, msdu_list, &rx_status);
you don't need to reinitialize rx_status for every iteration of the loop?
+ if (ret) {
+ ath12k_dbg(ab, ATH12K_DBG_DATA,
+ "Unable to process msdu %d", ret);
+ dev_kfree_skb_any(msdu);
+ continue;
+ }
+
+ ath12k_dp_rx_deliver_msdu(ar, napi, msdu, &rx_status);
+ }
+
+ rcu_read_unlock();
+}
snip
+int ath12k_dp_rx_process_err(struct ath12k_base *ab, struct napi_struct *napi,
+ int budget)
+{
+ u32 msdu_cookies[HAL_NUM_RX_MSDUS_PER_LINK_DESC];
+ struct dp_link_desc_bank *link_desc_banks;
+ enum hal_rx_buf_return_buf_manager rbm;
+ struct hal_rx_msdu_link *link_desc_va;
+ int tot_n_bufs_reaped, quota, ret, i;
+ struct hal_reo_dest_ring *reo_desc;
+ struct dp_rxdma_ring *rx_ring;
+ struct dp_srng *reo_except;
+ u32 desc_bank, num_msdus;
+ struct hal_srng *srng;
+ struct ath12k_dp *dp;
+ int mac_id;
+ struct ath12k *ar;
+ dma_addr_t paddr;
+ bool is_frag;
+ u8 drop = 0;
bool?
snip
+int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab,
+ struct napi_struct *napi, int budget)
+{
+ struct ath12k *ar;
+ struct ath12k_dp *dp = &ab->dp;
+ struct dp_rxdma_ring *rx_ring;
+ struct hal_rx_wbm_rel_info err_info;
+ struct hal_srng *srng;
+ struct sk_buff *msdu;
+ struct sk_buff_head msdu_list[MAX_RADIOS];
+ struct ath12k_skb_rxcb *rxcb;
+ void *rx_desc;
+ int mac_id;
+ int num_buffs_reaped = 0;
+ int total_num_buffs_reaped = 0;
+ struct ath12k_rx_desc_info *desc_info;
+ int ret, i;
+
+ for (i = 0; i < ab->num_radios; i++)
+ __skb_queue_head_init(&msdu_list[i]);
+
+ srng = &ab->hal.srng_list[dp->rx_rel_ring.ring_id];
+ rx_ring = &dp->rx_refill_buf_ring;
+
+ spin_lock_bh(&srng->lock);
+
+ ath12k_hal_srng_access_begin(ab, srng);
+
+ while (budget) {
+ rx_desc = ath12k_hal_srng_dst_get_next_entry(ab, srng);
+ if (!rx_desc)
+ break;
+
+ ret = ath12k_hal_wbm_desc_parse_err(ab, rx_desc, &err_info);
+ if (ret) {
+ ath12k_warn(ab,
+ "failed to parse rx error in wbm_rel ring desc %d\n",
+ ret);
+ continue;
+ }
+
+ desc_info = (struct ath12k_rx_desc_info *)err_info.rx_desc;
+
+ /* retry manual desc retrieval if hw cc is not done */
+ if (!desc_info) {
+ desc_info = ath12k_dp_get_rx_desc(ab, err_info.cookie);
+ if (!desc_info) {
+ ath12k_warn(ab, "Invalid cookie in manual desc retrival");
+ continue;
+ }
+ }
+
+ /* FIXME: Extract mac id correctly. Since descs are not tied
+ * to mac, we can extract from vdev id in ring desc.
+ */
+ mac_id = 0;
+
+ if (desc_info->magic != ATH12K_DP_RX_DESC_MAGIC)
+ ath12k_warn(ab, "WBM RX err, Check HW CC implementation");
+
+ msdu = desc_info->skb;
+ desc_info->skb = NULL;
+
+ spin_lock_bh(&dp->rx_desc_lock);
+ list_move_tail(&desc_info->list, &dp->rx_desc_free_list);
+ spin_unlock_bh(&dp->rx_desc_lock);
+
+ rxcb = ATH12K_SKB_RXCB(msdu);
+ dma_unmap_single(ab->dev, rxcb->paddr,
+ msdu->len + skb_tailroom(msdu),
+ DMA_FROM_DEVICE);
+
+ num_buffs_reaped++;
+ total_num_buffs_reaped++;
why are two separate counters needed? seem they will have the same value
always
+
+ if (!err_info.continuation)
+ budget--;
+
+ if (err_info.push_reason !=
+ HAL_REO_DEST_RING_PUSH_REASON_ERR_DETECTED) {
+ dev_kfree_skb_any(msdu);
+ continue;
+ }
+
+ rxcb->err_rel_src = err_info.err_rel_src;
+ rxcb->err_code = err_info.err_code;
+ rxcb->rx_desc = (struct hal_rx_desc *)msdu->data;
+ __skb_queue_tail(&msdu_list[mac_id], msdu);
+
+ rxcb->is_first_msdu = err_info.first_msdu;
+ rxcb->is_last_msdu = err_info.last_msdu;
+ rxcb->is_continuation = err_info.continuation;
+ }
+
+ ath12k_hal_srng_access_end(ab, srng);
+
+ spin_unlock_bh(&srng->lock);
+
+ if (!total_num_buffs_reaped)
+ goto done;
+
+ ath12k_dp_rx_bufs_replenish(ab, 0, rx_ring, num_buffs_reaped,
+ ab->hw_params->hal_params->rx_buf_rbm, true);
+
+ rcu_read_lock();
+ for (i = 0; i < ab->num_radios; i++) {
+ if (!rcu_dereference(ab->pdevs_active[i])) {
+ __skb_queue_purge(&msdu_list[i]);
+ continue;
+ }
+
+ ar = ab->pdevs[i].ar;
+
+ if (test_bit(ATH12K_CAC_RUNNING, &ar->dev_flags)) {
+ __skb_queue_purge(&msdu_list[i]);
+ continue;
+ }
+
+ while ((msdu = __skb_dequeue(&msdu_list[i])) != NULL)
+ ath12k_dp_rx_wbm_err(ar, napi, msdu, &msdu_list[i]);
+ }
+ rcu_read_unlock();
+done:
+ return total_num_buffs_reaped;
+}
snip
