Hello Zhijun,
On Sat, May 14, 2022 at 8:24 AM Zhijun You <hujy...@gmail.com> wrote:
> Glad to see there's someone else working on encap offload.
> Can confirm it works on my NETGEAR R7800 (QCA9984) with
> ath10k + firmware 10.4-3.9.0.2-00159
> Feel free to add my tested by tag.
>
> Tested-by: Zhijun You <hujy...@gmail.com>
Thank you! Will keep this tag in subsequent formal patch submission.
> I have a few questions though.
>
> In patch 081
>
>> +- ieee80211_tx_status(htt->ar->hw, msdu);
>> ++ memset(&status, 0, sizeof(status));
>>
>
> Instead of using memset maybe we could just init status like this?
> struct ieee80211_tx_status status = {};
Yep, this is possible. But what will this change? The initialization
on declaration will make the code one line shorter, but we will lose
the grouping. When the buffer is initialized just before use, it
becomes clear that it does not carry any additional information. Only
what is filled right now. So, I prefer to keep memset() at its
location until there are some performance issues.
> In the original patch [1] proposed by QCA guys
Thanks for pointing to the earlier work. I was not aware of it, and
did all the work from scratch using ath11k as a reference :)
> there are extra checks in ath10k_htt_tx_32
>
>> - if ((ieee80211_is_action(hdr->frame_control) ||
>> - ieee80211_is_deauth(hdr->frame_control) ||
>> - ieee80211_is_disassoc(hdr->frame_control)) &&
>> - ieee80211_has_protected(hdr->frame_control)) {
>> - skb_put(msdu, IEEE80211_CCMP_MIC_LEN);
>> - } else if (!(skb_cb->flags & ATH10K_SKB_F_NO_HWCRYPT) &&
>> - txmode == ATH10K_HW_TXRX_RAW &&
>> - ieee80211_has_protected(hdr->frame_control)) {
>> - skb_put(msdu, IEEE80211_CCMP_MIC_LEN);
>> + if (!(info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP)) {
>> + if ((ieee80211_is_action(hdr->frame_control) ||
>> + ieee80211_is_deauth(hdr->frame_control) ||
>> + ieee80211_is_disassoc(hdr->frame_control)) &&
>> + ieee80211_has_protected(hdr->frame_control)) {
>> + skb_put(msdu, IEEE80211_CCMP_MIC_LEN);
>> + } else if (!(skb_cb->flags & ATH10K_SKB_F_NO_HWCRYPT) &&
>> + txmode == ATH10K_HW_TXRX_RAW &&
>> + ieee80211_has_protected(hdr->frame_control)) {
>> + skb_put(msdu, IEEE80211_CCMP_MIC_LEN);
>> + }
>>
Nice catch! This code should already support the Ethernet
encapsulated frames according to the switch below this code block. So
this should probably be fixed with a separate patch. I will add it to
the series.
> and in ath10k_offchan_tx_work
>
>> + info = IEEE80211_SKB_CB(skb);
>> + if (info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) {
>> + peer_addr = skb->data;
>> + } else {
>> + hdr = (struct ieee80211_hdr *)skb->data;
>> + peer_addr = ieee80211_get_DA(hdr);
>>
mac80211 will not send offchannel frames with Ethernet encapsulation.
So this check is odd.
> Maybe we should add them back since both are checking for ethernet frames?
>
> 1.
> https://patchwork.kernel.org/project/linux-wireless/patch/20191216092207.31032-1-j...@phrozen.org/
--
Sergey
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
