On Thu, 2016-01-21 at 10:30 +0100, Janusz Dziedzic wrote:
>
> +static inline unsigned int
> +__ieee80211_hdrlen(struct ieee80211_hw *hw, __le16 fc) {
coding style - should have that brace on the next line
perhaps this really ought to be called
ieee80211_padded_hdrlen() or so instead of just the __.
> + unsigned int hdrlen;
> +
> + hdrlen = ieee80211_hdrlen(fc);
> + if (ieee80211_hw_check(hw, NEEDS_ALIGNED4_SKBS))
> + hdrlen += hdrlen & 3;
This needs a a comment, it only works because the hdrlen is guaranteed
to be a multiple of 2 already. Perhaps it should be & 2 ;-)
> - u8 hdr[30 + 2 + IEEE80211_FAST_XMIT_MAX_IV +
> - sizeof(rfc1042_header)];
> + u8 hdr[round_up(30 + 2 + IEEE80211_FAST_XMIT_MAX_IV +
> + sizeof(rfc1042_header), 4)];
I'm still not sure this is right, given the position of the padding. It
probably works since MAX_IV and sizeof() are divisible by 4, but
shouldn't it really be
round_up(30 + 2, 4) + MAX_IV + sizeof()?
> + /* Remove padding if was added */
> + if (ieee80211_hw_check(&local->hw, NEEDS_ALIGNED4_SKBS)) {
> + hdrlen = ieee80211_hdrlen(hdr->frame_control);
> + padsize = hdrlen & 3;
same as above
> + /* Check if HW require skb to be aligned */
> + if (ieee80211_hw_check(&sdata->local->hw,
> NEEDS_ALIGNED4_SKBS))
> + padsize = hdrlen & 3;
>
ditto
Perhaps also extract this if (...) padsize=... into a helper? Although
then the "hdrlen += 0" would remain for !ALIGNED4 drivers.
> + if (padsize)
> + skb_push(skb, padsize);
You should initialize the memory, imho, just in case it goes out
anywhere by accident.
> + /* Check if aligned skb required */
> + if (ieee80211_hw_check(&local->hw, NEEDS_ALIGNED4_SKBS))
> + build.hdr_len += build.hdr_len & 3;
As above.
You also need to clarify - IIRC the PN/IV fields are considered part of
the MAC header even if we don't really take them into account in
hdrlen(), so you should clearly document that the padding is before
those.
johannes
--
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
