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

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 16 September 2013 23:30, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@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@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.

Oh, ok. It would be good to document that in the commit log as well.

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

I know it's redundant, but this is just to improve readability.

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

Sure, the structures are very good here. But like above, having small
comments improve readability. Think of it as a "title" or something like
that.

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

Same here :)

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k