Re: [Linux-ATM-General] [RFC][PATCH 0/2] atm: br2682: Improve encapsulation checks and headroom allocation

[Philip P. <phi...@re...>]

On 8/15/11 8:39 AM, Pascal Hambourg wrote:
> chas williams - CONTRACTOR a écrit :
>> On Fri, 12 Aug 2011 22:01:10 +0200
>> Pascal Hambourg <pa...@pl...> wrote:
>>
>>> I updated and resent the patch 1/2 (v2) accordingly :
> [...]
>> this looks fine at a glance.  i do wonder if:
>>
>> *((u16 *) (skb->data + 8)) == 0
>>
>> works on all platforms.  cpu's other than x86 sometimes have issues
>> with arbitrary alignments of data.   it should probalby be a memcmp()
>> but that should probably be fixed in a seperate patch.
> 
> I copied this line from the VC-MUX part, even though I didn't like it.
> But from some readings I understood that skb data are supposed to be
> aligned on multiple of at least 2, and no one complained so far, so I
> thought it should be fine. Also I felt that memcmp() may not be the most
> efficient way to check that two consecutive bytes are 0. However I'll be
> glad to replace the line with something like
> 
> 	memcmp(skb->data + 8, pad, BR2684_PAD_LEN) == 0
> or
> 	skb->data[8] == 0 && skb->data[9] == 0
> or
> 	(skb->data[8] | skb->data[9]) == 0
> 
> Which one is fastest ? I'm afraid this is platform-dependent.
> Or is there a built-in function in the kernel which can do this
> efficiently ?

Sorry about getting to the party late, but... just to double-check what I thought I knew...

Wouldn't the proper check be:

    __be16_to_cpup(&skb->data[8]) == 0

You might need a cast there to (__be16 *)... and yes, arguably, zero is the same in both LE and BE architectures, but it's worth being consistent if it stops you from overlooking it elsewhere (and losing portability).

-Philip