[RFC 3/4] ath10k: cleanup RX decap handling

[Michal Kazior]

On 16 September 2013 23:30, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior at tieto.com> writes:
>
>> Native Wifi frames are always decapped as non-QoS
>> data frames meaning mac80211 couldn't set sk_buff
>> priority properly. The patch fixes this by using
>> the original 802.11 header.
>>
>> Signed-off-by: Michal Kazior <michal.kazior at tieto.com>
>
> The patch title doesn't seem to match with the content. Unless I'm
> mistaken it looks like you are adding native wifi frame format support
> and doing some cleanup at the same time. They should be in separate
> patches.

You're right. I'll split it up.

Nwifi was supported, however QoS Data frames were reported as Data
frames though.


>> @@ -652,6 +652,19 @@ static void ath10k_htt_rx_amsdu(struct ath10k_htt *htt,
>>                       skb_trim(skb, skb->len - FCS_LEN);
>>                       break;
>>               case RX_MSDU_DECAP_NATIVE_WIFI:
>> +                     hdr = (struct ieee80211_hdr *)skb->data;
>> +                     hdr_len = ieee80211_hdrlen(hdr->frame_control);
>> +                     memcpy(addr, ieee80211_get_DA(hdr), ETH_ALEN);
>> +                     skb_pull(skb, hdr_len);
>> +
>> +                     hdr = (struct ieee80211_hdr *)hdr_buf;
>> +                     hdr_len = ieee80211_hdrlen(hdr->frame_control);
>> +                     memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
>> +
>> +                     hdr = (struct ieee80211_hdr *)skb->data;
>> +                     qos = ieee80211_get_qos_ctl(hdr);
>> +                     qos[0] &= ~IEEE80211_QOS_CTL_A_MSDU_PRESENT;
>> +                     memcpy(ieee80211_get_DA(hdr), addr, ETH_ALEN);
>>                       break;
>
> It would be good to have small comments what each part is doing("remove
> the native wifi header", "copy the real 802.11 header", "copy correct
> destination address" etc), makes it a lot faster to skim the code. Also
> document why IEEE80211_QOS_CTL_A_MSDU_PRESENT needs to be cleared.

Okay.


>>       case RX_MSDU_DECAP_RAW:
>> -             /* remove trailing FCS */
>> -             skb_trim(skb, skb->len - 4);
>> +             skb_trim(skb, skb->len - FCS_LEN);
>>               break;
>
> Please keep the comment still

Why? The point of the comment was to explain the literal "4". Using
define makes the comment redundant.


>
>>       case RX_MSDU_DECAP_NATIVE_WIFI:
>> -             /* nothing to do here */
>> +             hdr = (struct ieee80211_hdr *)skb->data;
>> +             hdr_len = ieee80211_hdrlen(hdr->frame_control);
>> +             memcpy(addr, ieee80211_get_DA(hdr), ETH_ALEN);
>> +             skb_pull(skb, hdr_len);
>> +
>> +             hdr = (struct ieee80211_hdr *)rxd->rx_hdr_status;
>> +             hdr_len = ieee80211_hdrlen(hdr->frame_control);
>> +             memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
>> +
>> +             hdr = (struct ieee80211_hdr *)skb->data;
>> +             memcpy(ieee80211_get_DA(hdr), addr, ETH_ALEN);
>>               break;
>
> Again add small comments describing how we are modifying the headers.

Okay.


>>       case RX_MSDU_DECAP_ETHERNET2_DIX:
>> -             /* macaddr[6] + macaddr[6] + ethertype[2] */
>> -             skb_pull(skb, 6 + 6 + 2);
>> +             rfc1042 = hdr;
>> +             rfc1042 += roundup(hdr_len, 4);
>> +             rfc1042 += roundup(ath10k_htt_rx_crypto_param_len(enctype), 4);
>> +
>> +             skb_pull(skb, sizeof(struct ethhdr));
>> +             memcpy(skb_push(skb, sizeof(struct rfc1042_hdr)),
>> +                    rfc1042, sizeof(struct rfc1042_hdr));
>> +             memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
>>               break;
>
> Ditto.

Comment was supposed to explain where those numbers come from. Using
structures explains it now.


>>       case RX_MSDU_DECAP_8023_SNAP_LLC:
>> -             /* macaddr[6] + macaddr[6] + len[2] */
>> -             /* we don't need this for non-A-MSDU */
>> -             skb_pull(skb, 6 + 6 + 2);
>> +             skb_pull(skb, sizeof(struct amsdu_subframe_hdr));
>> +             memcpy(skb_push(skb, hdr_len), hdr, hdr_len);
>>               break;
>
> And here.

Ditto.


Michał.