Francesco Magliocca <franciman12@xxxxxxxxx> writes:
> 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 <enrico@xxxxxxxxxx>
> Signed-off-by: Enrico Lumetti <enrico@xxxxxxxxxx>
> Signed-off-by: Francesco Magliocca <franciman12@xxxxxxxxx>
> Link: https://lore.kernel.org/ath10k/CAH4F6usFu8-A6k5Z7rU9__iENcSC6Zr-NtRhh_aypR74UvN1uQ@xxxxxxxxxxxxxx/
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.
> --- 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?
> +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.
> +/* 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.
> +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.
> +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.
> +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?
> 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
