Re: [PATCH 1/1] wifi: ath12k: add msdu_end structure for WCN7850

[Kalle Valo <kvalo@kernel.org>]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 8/14/2023 11:26 PM, Kang Yang wrote:
>> WCN7850 and QCN9274 use the same structure rx_msdu_end_qcn9274 for
>
> Suggest s/use/currently use/ to clarify this is the current behavior
> and not the expected behavior
>
>> msdu_end. But content of msdu_end on WCN7850 is different from that of
>> QCN9274. Need to update it for WCN7850, otherwise will get the wrong
>> values when using it.
>> For example, TID is no longer in WCN7850's msdu_end. But
>> ath12k_dp_rx_process_err and ath12k_dp_rx_process_wbm_err still get TID
>
> Please add () to function references
>
>> from msdu_end. So an uncertain value will be used in these two functions
>> on WCN7850.
>> Therefore, add new structure rx_msdu_end_wcn7850 for WCN7850.
>> Tested-on: WCN7850 hw2.0 PCI
>> WLAN.HMT.1.0-03427-QCAHMTSWPL_V1.0_V2.0_SILICONZ-1.15378.4
>> Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/hal.c     |  6 +--
>>   drivers/net/wireless/ath/ath12k/rx_desc.h | 53 ++++++++++++++++++++++-
>>   2 files changed, 55 insertions(+), 4 deletions(-)
>> diff --git a/drivers/net/wireless/ath/ath12k/hal.c
>> b/drivers/net/wireless/ath/ath12k/hal.c
>> index e7a150e7158e..19b4207ca048 100644
>> --- a/drivers/net/wireless/ath/ath12k/hal.c
>> +++ b/drivers/net/wireless/ath/ath12k/hal.c
>> @@ -824,8 +824,8 @@ static u8 ath12k_hw_wcn7850_rx_desc_get_msdu_nss(struct hal_rx_desc *desc)
>>     static u8 ath12k_hw_wcn7850_rx_desc_get_mpdu_tid(struct
>> hal_rx_desc *desc)
>>   {
>> -	return le16_get_bits(desc->u.wcn7850.msdu_end.info5,
>> -			     RX_MSDU_END_INFO5_TID);
>> +	return le32_get_bits(desc->u.wcn7850.mpdu_start.info2,
>> +			     RX_MPDU_START_INFO2_TID);
>>   }
>>     static u16 ath12k_hw_wcn7850_rx_desc_get_mpdu_peer_id(struct
>> hal_rx_desc *desc)
>> @@ -837,7 +837,7 @@ static void ath12k_hw_wcn7850_rx_desc_copy_end_tlv(struct hal_rx_desc *fdesc,
>>   						   struct hal_rx_desc *ldesc)
>>   {
>>   	memcpy(&fdesc->u.wcn7850.msdu_end, &ldesc->u.wcn7850.msdu_end,
>> -	       sizeof(struct rx_msdu_end_qcn9274));
>> +	       sizeof(struct rx_msdu_end_wcn7850));
>>   }
>>     static u32 ath12k_hw_wcn7850_rx_desc_get_mpdu_start_tag(struct
>> hal_rx_desc *desc)
>> diff --git a/drivers/net/wireless/ath/ath12k/rx_desc.h b/drivers/net/wireless/ath/ath12k/rx_desc.h
>> index f99556a253e5..8769d8f3e7ea 100644
>> --- a/drivers/net/wireless/ath/ath12k/rx_desc.h
>> +++ b/drivers/net/wireless/ath/ath12k/rx_desc.h
>> @@ -782,6 +782,57 @@ struct rx_msdu_end_qcn9274 {
>>   	__le32 info14;
>>   } __packed;
>>   +struct rx_msdu_end_wcn7850 {
>> +	__le16 info0;
>> +	__le16 phy_ppdu_id;
>> +	__le16 ip_hdr_cksum;
>> +	__le16 info1;
>> +	__le16 info2;
>> +	__le16 cumulative_l3_checksum;
>> +	__le32 rule_indication0;
>> +	__le32 rule_indication1;
>> +	__le16 info3;
>> +	__le16 l3_type;
>> +	__le32 ipv6_options_crc;
>> +	__le32 tcp_seq_num;
>> +	__le32 tcp_ack_num;
>> +	__le16 info4;
>> +	__le16 window_size;
>> +	__le16 tcp_udp_chksum;
>> +	__le16 info5;
>> +	__le16 sa_idx;
>> +	__le16 da_idx_or_sw_peer_id;
>> +	__le32 info6;
>> +	__le32 fse_metadata;
>> +	__le16 cce_metadata;
>> +	__le16 sa_sw_peer_id;
>> +	__le16 info7;
>> +	__le16 rsvd0;
>> +	__le16 cumulative_l4_checksum;
>> +	__le16 cumulative_ip_length;
>> +	__le32 info9;
>> +	__le32 info10;
>> +	__le32 info11;
>> +	__le32 toeplitz_hash_2_or_4;
>> +	__le32 flow_id_toeplitz;
>> +	__le32 info12;
>> +	__le32 ppdu_start_timestamp_31_0;
>> +	__le32 ppdu_start_timestamp_63_32;
>> +	__le32 phy_meta_data;
>> +	__le16 vlan_ctag_ci;
>> +	__le16 vlan_stag_ci;
>> +	__le32 rsvd[3];
>> +	__le32 info13;
>> +	__le32 info14;
>> +} __packed;
>> +
>> +/* These macro definitions are only used for WCN7850 */
>> +#define RX_MSDU_END_INFO5_MSDU_LIMIT_ERR       BIT(2)
>> +#define RX_MSDU_END_INFO5_IDX_TIMOUT           BIT(3)
>> +#define RX_MSDU_END_INFO5_IDX_INVLID           BIT(4)
>> +#define RX_MSDU_END_INFO5_WIFI_PARSE_ERR       BIT(5)
>> +#define RX_MSDU_END_INFO5_AMSDU_PARSER_ERR     BIT(6)
>> +
>
> What uses these macros?
> Is there a reason to not spell out TIMEOUT and INVALID?
>
> If there are two different structs with two different decodings for
> info5 then it seems strange to have macros which don't have the
> chipset in the macro name. So I'd expect these to be named
> RX_MSDU_END_WCN7850_INFO5_* and the QCN9274 ones to be named
> RX_MSDU_END_QCN9274_INFO5_*. Only if there are members that are
> identical between WCN7850 and QCN9274 would it make sense to not have
> a chipset-specific name.
>
>>   /* rx_msdu_end
>>    *
>>    * rxpcu_mpdu_filter_in_category
>> @@ -1410,7 +1461,7 @@ struct rx_pkt_hdr_tlv {
>>     struct hal_rx_desc_wcn7850 {
>>   	__le64 msdu_end_tag;
>> -	struct rx_msdu_end_qcn9274 msdu_end;
>> +	struct rx_msdu_end_wcn7850 msdu_end;
>>   	u8 rx_padding0[RX_BE_PADDING0_BYTES];
>>   	__le64 mpdu_start_tag;
>>   	struct rx_mpdu_start_qcn9274 mpdu_start;

Jeff, for some reason your mail didn't have Cc linux-wireless so
patchwork don't show your comments:

https://patchwork.kernel.org/project/linux-wireless/patch/20230815062610.59248-1-quic_kangyang@quicinc.com/

The problem is that if there are no comments in patchwork I will most
likely miss them. Adding linux-wireless back now.

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

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

-- 
ath12k mailing list
ath12k@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/ath12k