Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/net/tile/tilepro.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
From: Chris Metcalf <hidden> Date: 2011-01-12 17:54:30
On 1/12/2011 4:31 AM, Tobias Klauser wrote:
Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/net/tile/tilepro.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
Thanks, I've taken this into the Tilera tree!
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
From: David Miller <davem@davemloft.net> Date: 2011-01-13 02:44:29
From: Chris Metcalf <redacted>
Date: Wed, 12 Jan 2011 12:49:03 -0500
On 1/12/2011 4:31 AM, Tobias Klauser wrote:
quoted
Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/net/tile/tilepro.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
Thanks, I've taken this into the Tilera tree!
Don't, his transformation is buggy.
You can't get rid of the broadcast check, it needs to be there.
Think about it.
On 2011-01-13 at 03:45:01 +0100, David Miller [off-list ref] wrote:
From: Chris Metcalf <redacted>
Date: Wed, 12 Jan 2011 12:49:03 -0500
quoted
On 1/12/2011 4:31 AM, Tobias Klauser wrote:
quoted
Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/net/tile/tilepro.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
Thanks, I've taken this into the Tilera tree!
Don't, his transformation is buggy.
Why?
Doesn't the code want to make sure that only unicast addresses get
filtered?
You can't get rid of the broadcast check, it needs to be there.
Think about it.
If a unicast address is in buf, is_multicast_ether_addr returns false
(and is_broadcast_addr would too) and thus it would get filtered.
If a multicast address is in buf, is_multicast_ether_addr returns
true, so the it won't get filtered (no matter what is_broadcast_addr
would tell).
If a broadcast address is in buf, is_multicast_ether_addr returns true,
so it won't get filtered (no matter what is_broadcast_addr would tell).
Are there any special addresses I missed? Or did I didn't get the above
right?
On 2011-01-13 at 03:45:01 +0100, David Miller [off-list ref] wrote:
quoted
From: Chris Metcalf <redacted>
Date: Wed, 12 Jan 2011 12:49:03 -0500
quoted
On 1/12/2011 4:31 AM, Tobias Klauser wrote:
quoted
Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/net/tile/tilepro.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
Thanks, I've taken this into the Tilera tree!
Don't, his transformation is buggy.
Why?
Doesn't the code want to make sure that only unicast addresses get
filtered?
quoted
You can't get rid of the broadcast check, it needs to be there.
Think about it.
If a unicast address is in buf, is_multicast_ether_addr returns false
(and is_broadcast_addr would too) and thus it would get filtered.
Ok, this may be correct, but it makes the code hard to read.
I think the old code, whilst redundant, is easier to understand.
Why not add a function "is_unicast_ether_addr()" and use that
here instead?
That solves all of the issues I have with your change.
On 2011-01-13 at 03:45:01 +0100, David Miller [off-list ref] wrote:
quoted
From: Chris Metcalf <redacted>
Date: Wed, 12 Jan 2011 12:49:03 -0500
quoted
On 1/12/2011 4:31 AM, Tobias Klauser wrote:
quoted
Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/net/tile/tilepro.c | 10 +---------
1 files changed, 1 insertions(+), 9 deletions(-)
Thanks, I've taken this into the Tilera tree!
Don't, his transformation is buggy.
Why?
Doesn't the code want to make sure that only unicast addresses get
filtered?
quoted
You can't get rid of the broadcast check, it needs to be there.
Think about it.
If a unicast address is in buf, is_multicast_ether_addr returns false
(and is_broadcast_addr would too) and thus it would get filtered.
Ok, this may be correct, but it makes the code hard to read.
I think the old code, whilst redundant, is easier to understand.
Agreed. Thanks for clarifying.
Why not add a function "is_unicast_ether_addr()" and use that
here instead?
I'll send a patch to add the function to linux/etherdevice.h and an
updated patch for the tilepro driver as a follow up.
From a check for !is_multicast_ether_addr it is not always obvious that
we're checking for a unicast address. So add this helper function to
make those code paths easier to read.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
include/linux/etherdevice.h | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
From: Joe Perches <joe@perches.com> Date: 2011-01-13 08:24:43
On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
quoted
From a check for !is_multicast_ether_addr it is not always obvious that
we're checking for a unicast address. So add this helper function to
make those code paths easier to read.
include/linux/etherdevice.h | 11 +++++++++++
[]
quoted hunk
/**+ * is_unicast_ether_addr - Determine if the Ethernet address is unicast+ * @addr: Pointer to a six-byte array containing the Ethernet address+ *+ * Return true if the address is a unicast address.+ */+static inline int is_unicast_ether_addr(const u8 *addr)+{+ return !is_multicast_ether_addr(addr);+}
Can't you simply use is_valid_ether_addr?
I can't think of much reason that a new function for
!multicast without the !is_zero is needed.
On 2011-01-13 at 09:24:41 +0100, Joe Perches [off-list ref] wrote:
On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
quoted
quoted
From a check for !is_multicast_ether_addr it is not always obvious that
we're checking for a unicast address. So add this helper function to
make those code paths easier to read.
include/linux/etherdevice.h | 11 +++++++++++
[]
quoted
/**+ * is_unicast_ether_addr - Determine if the Ethernet address is unicast+ * @addr: Pointer to a six-byte array containing the Ethernet address+ *+ * Return true if the address is a unicast address.+ */+static inline int is_unicast_ether_addr(const u8 *addr)+{+ return !is_multicast_ether_addr(addr);+}
From: Joe Perches <joe@perches.com> Date: 2011-01-13 08:43:18
On Thu, 2011-01-13 at 09:35 +0100, Tobias Klauser wrote:
On 2011-01-13 at 09:24:41 +0100, Joe Perches [off-list ref] wrote:
I added is_unicast_ether_addr to make the intention of checking for a
unicast address clearer (see [1] for context).
[1] http://article.gmane.org/gmane.linux.network/183615
quoted
I can't think of much reason that a new function for
!multicast without the !is_zero is needed.
Maybe I should just add something like the following?
#define is_unicast_ether_addr(addr) is_valid_ether_addr(addr)
From: Eric Dumazet <hidden> Date: 2011-01-13 08:55:22
Le jeudi 13 janvier 2011 à 00:24 -0800, Joe Perches a écrit :
On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
quoted
quoted
From a check for !is_multicast_ether_addr it is not always obvious that
we're checking for a unicast address. So add this helper function to
make those code paths easier to read.
include/linux/etherdevice.h | 11 +++++++++++
[]
quoted
/**+ * is_unicast_ether_addr - Determine if the Ethernet address is unicast+ * @addr: Pointer to a six-byte array containing the Ethernet address+ *+ * Return true if the address is a unicast address.+ */+static inline int is_unicast_ether_addr(const u8 *addr)+{+ return !is_multicast_ether_addr(addr);+}
Can't you simply use is_valid_ether_addr?
I can't think of much reason that a new function for
!multicast without the !is_zero is needed.
performance ?
is_valid_ether_addr() is used at device init time, not when receiving
packets.
I am not sure we _need_ to check for is_zero_ether_addr() each time we
receive a packet.
Either a MAC is unicast or multicast.
A zero address is not multicast for sure.
From: Chris Metcalf <hidden> Date: 2011-01-13 16:38:43
On 1/13/2011 3:55 AM, Eric Dumazet wrote:
Le jeudi 13 janvier 2011 à 00:24 -0800, Joe Perches a écrit :
quoted
On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
quoted
quoted
From a check for !is_multicast_ether_addr it is not always obvious that
we're checking for a unicast address. So add this helper function to
make those code paths easier to read.
include/linux/etherdevice.h | 11 +++++++++++
[]
quoted
/**+ * is_unicast_ether_addr - Determine if the Ethernet address is unicast+ * @addr: Pointer to a six-byte array containing the Ethernet address+ *+ * Return true if the address is a unicast address.+ */+static inline int is_unicast_ether_addr(const u8 *addr)+{+ return !is_multicast_ether_addr(addr);+}
Can't you simply use is_valid_ether_addr?
I can't think of much reason that a new function for
!multicast without the !is_zero is needed.
performance ?
is_valid_ether_addr() is used at device init time, not when receiving
packets.
I am not sure we _need_ to check for is_zero_ether_addr() each time we
receive a packet.
Either a MAC is unicast or multicast.
A zero address is not multicast for sure.
I agree - the is_zero_ether_addr() check seems pointless in the context of
the running interface.
Also, I think a static inline is better form than a #define, all things
being equal.
So, I like Tobias' reworked patches. I can take them into the Tilera tree,
but I'd prefer David Miller take them into the net tree if he is agreeable,
since it now includes changes to generic networking code. If you take the
latter approach you can include my:
Acked-by: Chris Metcalf <redacted>
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
From: David Miller <davem@davemloft.net> Date: 2011-01-13 20:35:05
From: Chris Metcalf <redacted>
Date: Thu, 13 Jan 2011 11:38:42 -0500
So, I like Tobias' reworked patches. I can take them into the Tilera tree,
but I'd prefer David Miller take them into the net tree if he is agreeable,
since it now includes changes to generic networking code. If you take the
latter approach you can include my:
I'll take them into my networking tree, which is where all network
device changes ought to go in the first place.
From a check for !is_multicast_ether_addr it is not always obvious that
we're checking for a unicast address. So add this helper function to
make those code paths easier to read.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>