Re: [B.A.T.M.A.N.] [PATCH] staging: batman-adv: Use linux/etherdevice.h address helper functions

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: Text/Plain, Size: 2036 bytes --]

On Wednesday 03 November 2010 11:25:12 Tobias Klauser wrote:
> On 2010-11-03 at 11:12:12 +0100, Sven Eckelmann <sven.eckelmann@gmx.de> 
wrote:
> > On Wednesday 03 November 2010 10:59:02 you wrote:
> > > Replace custom ethernet address check functions by calls to the helpers
> > > in linux/etherdevice.h
> > 
> > Have you proven that the addresses are always two bytes aligned in
> > memory? Afaik this function needs it.
> 
> I don't think they need to be two bytes aligned, but I might be wrong.

compare_ether_addr uses a two byte pointer to access 3x two bytes. This makes 
it necessary to have all those 3 bytes aligned to 2 byte boundaries. Otherwise 
the compiler has to generate special instructions on architectures which don't 
support loads on non-aligned addresses. Usually he doesn't do it unless he has 
some indications that it is necessary (__attribute__ ((packed)) for example).

There is also documentation available on that topic in 
Documentation/unaligned-memory-access.txt

And maybe it is good to use is_broadcast_ether_addr, but leave 
compare_ether_addr part open (or prove that we always have those two operands 
correctly aligned).

> > > In one case where the address was tested for broadcast and multicast
> > > address, the broadcast address check can be omitted as broadcast is
> > > also a multicast address.
> > 
> > We need to distinguish between these two types for different
> > optimizations (research is currently done for multicast over mesh - but
> > currently not part of batman-adv).
> 
> I was refering to the following part of the patch:
> 
> -       if (is_bcast(ethhdr->h_dest) || is_mcast(ethhdr->h_dest)) {
> +       if (is_multicast_ether_addr(ethhdr->h_dest)) {
> 
> I think this change should be legitimate as the same branch is done for
> multicast and broadcast addresses.

Yes, in that situation it is correct. I was talking more about the complete 
bcast vs. multicast in batman-adv and didn't look at your patch.

Best regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]