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

Sven Eckelmann
2010-11-03T10:56:19Z

[-- 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.