Weird layout of struct hal_rx_desc_qcn9274

[Karthikeyan Periyasamy]

On 9/4/2024 8:29 PM, Nicolas Escande wrote:
> So I think I've stumbled upon another weird thing.
> 
> In rx_desc.h the definition of hal_rx_desc as used the firmware seems to be
> varying depending on the HW and on the use of reduced metadata (with some tlv
> registration magic that seems to select the info we retrieve, 8bytes at a time)
> 
> // For QCN9274 full metadata:
> 
> struct rx_msdu_end_qcn9274 {
> 	__le16 info0;
> 	__le16 phy_ppdu_id;
> 	__le16 ip_hdr_cksum;
> 	__le16 info1;
> ...
> };
> 
> struct hal_rx_desc_qcn9274 {
> 	struct rx_msdu_end_qcn9274 msdu_end;
> 	struct rx_mpdu_start_qcn9274 mpdu_start;
> 	u8 msdu_payload[];
> } __packed;
> 
> 
> // For QCN9274 with reduced metadata:
> 
> struct rx_msdu_end_qcn9274_compact {
> 	__le64 msdu_end_tag;
> 	__le16 sa_sw_peer_id;
> 	__le16 info5;
> ...
> };
> 
> struct hal_rx_desc_qcn9274_compact {
> 	struct rx_msdu_end_qcn9274_compact msdu_end;
> 	struct rx_mpdu_start_qcn9274_compact mpdu_start;
> 	u8 msdu_payload[];
> } __packed;
> 
> 
> // For WCN7850 full metadata:
> 
> struct rx_msdu_end_wcn7850 {
> 	__le16 info0;
> 	__le16 phy_ppdu_id;
> 	__le16 ip_hdr_cksum;
> 	__le16 info1;
> ...
> };
> 
> struct hal_rx_desc_wcn7850 {
> 	__le64 msdu_end_tag;
> 	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;
> 	struct rx_pkt_hdr_tlv	 pkt_hdr_tlv;
> 	u8 msdu_payload[];
> };
> 
> 
> Which gets then used as a generic rx desc
> 
> struct hal_rx_desc {
> 	union {
> 		struct hal_rx_desc_qcn9274 qcn9274;
> 		struct hal_rx_desc_qcn9274_compact qcn9274_compact;
> 		struct hal_rx_desc_wcn7850 wcn7850;
> 	} u;
> } __packed;
> 
> 
> 
> So it seems without the quadword registration, even though qcn9274 & wcn7850
> appears to have similar layout, the __le64 msdu_end_tag is present in the struct
> hal_rx_desc_wcn7850 but not in struct hal_rx_desc_qcn9274. That seems weird.
> Would this mean that firmware always sends 8bytes of msdu_end_tag in wcn7850 but
> not on qcn9274 ?
> 

yes, qcn9274 not packed with __le64 tag.
wcn7850 expect the __le64 tag packed in the msdu_end and mpdu_start.


> It seems unlikely as when using the compact layout on qcn9274 (the default now)
> we use QCN9274_MSDU_END_WMASK with QCN9274_MSDU_END_SELECT_MSDU_END_TAG (bit(0))
> which seems to indicate that this 8bytes tag is part of the optional data that
> we can select (or not) to be received from the firmware.
> 
> Am I missing something ? Did that even worked before switching to compact layout
> for qcn9274 ?

Yes, tag tlv is configurable in the compact word subscription mode.

Before compact mode, qcn9274 full mode is worked without any issue.

> 
> And if it's not working as intended, what's the layout style we actually want to
> use for struct hal_rx_desc_XXX, do we want the __le64 msdu_end_tag inside the
> struct hal_rx_desc_XXX or inside the struct rx_msdu_end_XXX ?
> 

Either way, __le64 msdu_end_tag is part of the msdu_end because 
ath12k_hw_wcn7850_rx_desc_get_msdu_end_offset() tells the 
offsetof(struct hal_rx_desc_wcn7850, msdu_end_tag) not the 
offsetof(struct rx_msdu_end_wcn7850 msdu_end) in the 
ath12k_dp_rxdma_ring_sel_config_wcn7850() configuration.

-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி