Michal Kazior <michal.kazior@xxxxxxxxx> writes: > On 16 September 2013 23:30, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote: >> Michal Kazior <michal.kazior@xxxxxxxxx> 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@xxxxxxxxx> >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
