Re: [B.A.T.M.A.N.] patch "staging: batman-adv: Use linux/etherdevice.h address helper functions" added to staging tree
Greg,
The patch is only compile-tested.
[..]
diff --git a/drivers/staging/batman-adv/main.c b/drivers/staging/batman-adv/main.c index 0587940..6ea6420 100644 --- a/drivers/staging/batman-adv/main.c +++ b/drivers/staging/batman-adv/main.c @@ -149,7 +149,7 @@ void dec_module_count(void)
int compare_orig(void *data1, void *data2) {
- return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0);
- return (compare_ether_addr(data1, data2) == 0 ? 1 : 0);
}
I'm wondering why you accepted this patch despite the raised objections regarding alignment problems. [1][2] Is there something you know that we overlooked ? We haven't heard from Thomas or you since the alignment question came on the table.
Cheers, Marek
[1]https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2010-November/003640.html [2]https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2010-November/003643.html
On Wed, Nov 10, 2010 at 11:17:03AM +0100, Marek Lindner wrote:
Greg,
The patch is only compile-tested.
[..]
diff --git a/drivers/staging/batman-adv/main.c b/drivers/staging/batman-adv/main.c index 0587940..6ea6420 100644 --- a/drivers/staging/batman-adv/main.c +++ b/drivers/staging/batman-adv/main.c @@ -149,7 +149,7 @@ void dec_module_count(void)
int compare_orig(void *data1, void *data2) {
- return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0);
- return (compare_ether_addr(data1, data2) == 0 ? 1 : 0);
}
I'm wondering why you accepted this patch despite the raised objections regarding alignment problems. [1][2]
Because at the end of that thread, it sounded like you all agreed that this patch was acceptable.
If not, then please let me know and I will revert it.
thanks,
greg k-h
On Wednesday 10 November 2010 17:04:55 Greg KH wrote:
I'm wondering why you accepted this patch despite the raised objections regarding alignment problems. [1][2]
Because at the end of that thread, it sounded like you all agreed that this patch was acceptable.
If not, then please let me know and I will revert it.
The end of the thread was that he should remove parts of the patch and resent it - or prove that all the data is 2 bytes aligned.
On Wednesday 03 November 2010 11:56:19 Sven Eckelmann wrote: [...]
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.
[correction sent later: "6x two 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).
[...]
None of that happened yet. And yes, this wasn't the last mail we sent to him to get some response - no answer till now.
Best regards, Sven
On Wed, Nov 10, 2010 at 05:46:37PM +0100, Sven Eckelmann wrote:
On Wednesday 10 November 2010 17:04:55 Greg KH wrote:
I'm wondering why you accepted this patch despite the raised objections regarding alignment problems. [1][2]
Because at the end of that thread, it sounded like you all agreed that this patch was acceptable.
If not, then please let me know and I will revert it.
The end of the thread was that he should remove parts of the patch and resent it - or prove that all the data is 2 bytes aligned.
On Wednesday 03 November 2010 11:56:19 Sven Eckelmann wrote: [...]
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.
[correction sent later: "6x two 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).
[...]
None of that happened yet. And yes, this wasn't the last mail we sent to him to get some response - no answer till now.
Oops, my fault, I've now reverted this patch, sorry about that.
thanks,
greg k-h
On Thursday 11 November 2010 02:11:37 Greg KH wrote:
None of that happened yet. And yes, this wasn't the last mail we sent to him to get some response - no answer till now.
Oops, my fault, I've now reverted this patch, sorry about that.
No problem, that can happen.
Thanks, Marek
On 2010-11-10 at 17:46:37 +0100, Sven Eckelmann sven.eckelmann@gmx.de wrote:
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.
[correction sent later: "6x two 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).
[...]
None of that happened yet. And yes, this wasn't the last mail we sent to him to get some response - no answer till now.
I'll send an updated patch, leaving out the compare_ether_addr part.
Cheers Tobias
Tobias,
I'll send an updated patch, leaving out the compare_ether_addr part.
not necessary anymore. Today I included a patch from Sven that does the job.
Thanks for bringing this up, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Greg KH -
Marek Lindner -
Sven Eckelmann -
Tobias Klauser