Re: [PATCH 4/7] mac80211: make ieee80211_get_hdrlen_from_skb return unsigned

[Harvey Harrison <harvey.harrison@gmail.com>]

On Fri, 2008-06-06 at 20:24 +0200, Michael Buesch wrote:
> On Friday 06 June 2008 19:51:12 Harvey Harrison wrote:
> > -int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
> > +unsigned int ieee80211_get_hdrlen_from_skb(const struct sk_buff *skb)
> >  {
> >  	const struct ieee80211_hdr *hdr = (const struct ieee80211_hdr *) skb->data;
> > -	int hdrlen;
> > +	unsigned int hdrlen;
> >  
> > -	if (unlikely(skb->len < 10))
> > +	if (skb->len < 10)
> 
> Why are you removing the unlikely()? This should actually be pretty unlikely.
> Sane firmware will drop short packets, so the kernel won't ever see them.
> 

When all that is in the if-block is a return, there's not much point.
It's a trivial test/return and gcc does a fine job with this.

> >  		return 0;
> > -	hdrlen = ieee80211_get_hdrlen(le16_to_cpu(hdr->frame_control));
> > -	if (unlikely(hdrlen > skb->len))
> > +	hdrlen = ieee80211_hdrlen(hdr->frame_control);
> > +	if (hdrlen > skb->len)
> 
> I think this also is unlikely, as it can only come from corrupted packets.
> 

unlikely() isn't some magic make-me-faster function, it just moves code
to the end of the function to get it out of the icache and jumps to it
in the unlikely case it is taken.  When all there is is a return, I
don't think it even makes any difference.  In both these cases a comment
is probably more appropriate.

Harvey