Weird layout of struct hal_rx_desc_qcn9274

Weird layout of struct hal_rx_desc_qcn9274

[Nicolas Escande]

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 ?

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 ?

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 ?

Thanks

Re: 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
--
கார்த்திகேயன் பெரியசாமி

Re: Weird layout of struct hal_rx_desc_qcn9274

[Nicolas Escande]

On 2024-09-05 4:53, Karthikeyan Periyasamy wrote:

[...]

> > 
> > 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.

So in the full form, we just do not have the _le64 tag on qcn9274 but not in
wcn7850, right ? 

> 
> 
> > 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.
>

So that means that setting all bits in the tlv subscription mode *is not* the
same as not using the subscription and receiving the full desciptor. Setting all
bits will get you the __le64 tag, but not using the registration at all will not
get it right ? 

> > 
> > 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.
> 
Yep I saw this part, for me it accentuates the fact that the msdu_end_tag is not
something that can subscribed out of in wcn7850, as it is out of the struct,
but can in qcn9274.

Re: Weird layout of struct hal_rx_desc_qcn9274

[Karthikeyan Periyasamy]

On 9/5/2024 6:36 PM, Nicolas Escande wrote:
> On 2024-09-05 4:53, Karthikeyan Periyasamy wrote:
> 
> [...]
> 
>>>
>>> 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.
> 
> So in the full form, we just do not have the _le64 tag on qcn9274 but not in
> wcn7850, right ?
> 

Yes

>>
>>
>>> 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.
>>
> 
> So that means that setting all bits in the tlv subscription mode *is not* the
> same as not using the subscription and receiving the full desciptor. Setting all
> bits will get you the __le64 tag, but not using the registration at all will not
> get it right ?
> 

yes

>>>
>>> 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.
>>
> Yep I saw this part, for me it accentuates the fact that the msdu_end_tag is not
> something that can subscribed out of in wcn7850, as it is out of the struct,
> but can in qcn9274.

Not sure about the subscription in wcn7850
-- 
Karthikeyan Periyasamy
--
கார்த்திகேயன் பெரியசாமி