quoted

--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c

@@ -75,6 +75,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
                      .board_size = QCA988X_BOARD_DATA_SZ,
                      .board_ext_size = QCA988X_BOARD_EXT_DATA_SZ,
              },
+             .rx_desc_ops = &qca988x_rx_desc_ops,
              .hw_ops = &qca988x_ops,

Any particular reason why you didn't use the existing hw_ops?

I didn't use the hw_ops, because while the preWCN3990 cards have
different hw_ops implementations,
they all share the same rx_desc (i tried having a dedicated rx_desc
for each card, but this made the firmware seldom crash,
while the current memory layout seems to be the most stable) so having
a separate rx_desc_ops requires the least number of changes.

This looks, but I did some changes in the pending branch. Please check
my changes:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=c9a516b946c4b3ce25d422890ca78d0ca730b065

Below are my comments.

Woops, I apologise for the typos and extra whitespaces, I thought I
had gotten rid of them all.

Anyways, everything looks good to me.

I was a bit worried about performance regression due to the
introduction of getters and setters
for accessing the rx descriptor, but so far it seems to consistently
work fine. I cannot test on the WCN3990 unfortunately,
where we also pay for the further indirection due to function pointers.

Il giorno mer 22 dic 2021 alle ore 08:54 Kalle Valo [off-list ref]
ha scritto:

Francesco Magliocca [off-list ref] writes:

quoted

QCA6174 card often hangs with the current htt_rx_desc
memory layout in some circumstances, because its firmware
fails to handle length differences.
Therefore we must abstract the htt_rx_desc structure
and operations on it, to allow different wireless cards
to use different, unrelated rx descriptor structures.

Define a base htt_rx_desc structure and htt_rx_desc_qca_old
for use with the QCA family of ath10k supported cards
and htt_rx_desc_new for use with the WCN3990 card.

Define htt_rx_desc_ops which contains the abstract operations
to access the generic htt_rx_desc, give implementations
for each card and update htt_rx.c to use the defined
abstract interface to rx descriptors.

Fixes: e3def6f7ddf8 ("ath10k: Update rx descriptor for WCN3990 target")

Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00157-QCARMSWPZ-1

Co-developed-by: Enrico Lumetti <redacted>
Signed-off-by: Enrico Lumetti <redacted>
Signed-off-by: Francesco Magliocca <redacted>
Link: https://lore.kernel.org/ath10k/CAH4F6usFu8-A6k5Z7rU9__iENcSC6Zr-NtRhh_aypR74UvN1uQ@mail.gmail.com/

This looks, but I did some changes in the pending branch. Please check
my changes:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=c9a516b946c4b3ce25d422890ca78d0ca730b065

Below are my comments.

drivers/net/wireless/ath/ath10k/htt.c:141: line length of 98 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt.c:149: line length of 98 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:374: line length of 91 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1098: line length of 98 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1234: line length of 98 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1261: line length of 93 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1417: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1553: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1600: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1654: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1710: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1753: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1788: line length of 91 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1814: line length of 91 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1930: line length of 93 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:1962: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:2172: line length of 93 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:2211: line length of 93 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:3139: line length of 100 exceeds 90 columns
drivers/net/wireless/ath/ath10k/htt_rx.c:3148: line length of 92 exceeds 90 columns

ath10k-check warned about long lines, so I fixed those.

quoted

--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c

@@ -75,6 +75,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
                      .board_size = QCA988X_BOARD_DATA_SZ,
                      .board_ext_size = QCA988X_BOARD_EXT_DATA_SZ,
              },
+             .rx_desc_ops = &qca988x_rx_desc_ops,
              .hw_ops = &qca988x_ops,

Any particular reason why you didn't use the existing hw_ops?

quoted

+static void ath10k_rx_desc_wcn3990_get_offsets(struct htt_rx_ring_rx_desc_offsets *off)
+{
+#define desc_offset(x) (offsetof(struct htt_rx_desc_new, x) / 4)
+     off->mac80211_hdr_offset = __cpu_to_le16(desc_offset(rx_hdr_status));
+     off->msdu_payload_offset = __cpu_to_le16(desc_offset(msdu_payload));
+     off->ppdu_start_offset = __cpu_to_le16(desc_offset(ppdu_start));
+     off->ppdu_end_offset = __cpu_to_le16(desc_offset(ppdu_end));
+     off->mpdu_start_offset = __cpu_to_le16(desc_offset(mpdu_start));
+     off->mpdu_end_offset = __cpu_to_le16(desc_offset(mpdu_end));
+     off->msdu_start_offset = __cpu_to_le16(desc_offset(msdu_start));
+     off->msdu_end_offset = __cpu_to_le16(desc_offset(msdu_end));
+     off->rx_attention_offset = __cpu_to_le16(desc_offset(attention));
+     off->frag_info_offset = __cpu_to_le16(desc_offset(frag_info));
+#undef desc_metadata_offset
+#undef metadata_offset

I couldn't find these two defined anywhere so removed the undefs.

quoted

+/* rx descriptor for wcn3990 and possibly extensible for newer cards
+ * Buffers like this are placed on the rx ring.
+ */
+struct htt_rx_desc_new {
+     struct htt_rx_desc base;
      struct {
              struct rx_attention attention;
              struct rx_frag_info frag_info;

@@ -2210,6 +2221,240 @@ struct htt_rx_desc {
      u8 msdu_payload[];
 };

Using old and new is not a good idea when versioning something because
when a third one is added the naming will be hard. So I renamed old to
v1 and new to v2 everywhere in the patch.

quoted

+extern const struct ath10k_htt_rx_desc_ops qca988x_rx_desc_ops;
+extern const struct ath10k_htt_rx_desc_ops qca99x0_rx_desc_ops;
+extern const struct ath10k_htt_rx_desc_ops wcn3990_rx_desc_ops;

These should be use "ath10k_" prefix, but forgot to change that. Will do
it later.

quoted

+static inline struct rx_msdu_end_common      *
+ath10k_htt_rx_desc_get_msdu_end(struct ath10k_hw_params      *hw, struct htt_rx_desc *rxd)

Are there extra space before '*'? Need to check that.

quoted

+static inline struct rx_ppdu_end_common      *
+ath10k_htt_rx_desc_get_ppdu_end(struct ath10k_hw_params      *hw, struct htt_rx_desc *rxd)

Extra spaces?

quoted

 static struct ieee80211_channel *
 ath10k_htt_rx_h_peer_channel(struct ath10k *ar, struct htt_rx_desc *rxd)
 {
+     struct ath10k_hw_params *hw = &ar->hw_params;
+     struct rx_attention *rxd_attention = ath10k_htt_rx_desc_get_attention(hw, rxd);
+     struct rx_msdu_end_common *rxd_msdu_end_common = ath10k_htt_rx_desc_get_msdu_end(hw, rxd);
+     struct rx_mpdu_start *rxd_mpdu_start = ath10k_htt_rx_desc_get_mpdu_start(hw, rxd);
      struct ath10k_peer *peer;
      struct ath10k_vif *arvif;
      struct cfg80211_chan_def def;

@@ -1069,15 +1107,15 @@ ath10k_htt_rx_h_peer_channel(struct ath10k *ar, struct htt_rx_desc *rxd)
      if (!rxd)
              return NULL;

Here I moved the variable initialisation after the rxd null check to
avoid null pointer issues and also reduce long lines.

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

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