[PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[Tobias Klauser]

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(-)

diff --git a/drivers/net/tile/tilepro.c b/drivers/net/tile/tilepro.c
index 0e6bac5..8d8b67b 100644
--- a/drivers/net/tile/tilepro.c
+++ b/drivers/net/tile/tilepro.c
@@ -142,14 +142,6 @@
 MODULE_AUTHOR("Tilera");
 MODULE_LICENSE("GPL");
 
-
-#define IS_MULTICAST(mac_addr) \
-	(((u8 *)(mac_addr))[0] & 0x01)
-
-#define IS_BROADCAST(mac_addr) \
-	(((u16 *)(mac_addr))[0] == 0xffff)
-
-
 /*
  * Queue of incoming packets for a specific cpu and device.
  *
@@ -795,7 +787,7 @@ static bool tile_net_poll_aux(struct tile_net_cpu *info, int index)
 		/*
 		 * FIXME: Implement HW multicast filter.
 		 */
-		if (!IS_MULTICAST(buf) && !IS_BROADCAST(buf)) {
+		if (!is_multicast_ether_addr(buf)) {
 			/* Filter packets not for our address. */
 			const u8 *mine = dev->dev_addr;
 			filter = compare_ether_addr(mine, buf);
-- 
1.7.0.4

Re: [PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[Chris Metcalf]

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

Re: [PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[David Miller]

From: Chris Metcalf <cmetcalf@tilera.com>
Date: Wed, 12 Jan 2011 12:49:03 -0500

> 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!

Don't, his transformation is buggy.

You can't get rid of the broadcast check, it needs to be there.
Think about it.

Re: [PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[Tobias Klauser]

On 2011-01-13 at 03:45:01 +0100, David Miller <davem@davemloft.net> wrote:
> From: Chris Metcalf <cmetcalf@tilera.com>
> Date: Wed, 12 Jan 2011 12:49:03 -0500
> 
> > 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!
> 
> 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?

Re: [PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[David Miller]

From: Tobias Klauser <tklauser@distanz.ch>
Date: Thu, 13 Jan 2011 07:58:55 +0100

> On 2011-01-13 at 03:45:01 +0100, David Miller <davem@davemloft.net> wrote:
>> From: Chris Metcalf <cmetcalf@tilera.com>
>> Date: Wed, 12 Jan 2011 12:49:03 -0500
>> 
>> > 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!
>> 
>> 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.

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.

Re: [PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper

[Tobias Klauser]

On 2011-01-13 at 08:42:50 +0100, David Miller <davem@davemloft.net> wrote:
> From: Tobias Klauser <tklauser@distanz.ch>
> Date: Thu, 13 Jan 2011 07:58:55 +0100
> 
> > On 2011-01-13 at 03:45:01 +0100, David Miller <davem@davemloft.net> wrote:
> >> From: Chris Metcalf <cmetcalf@tilera.com>
> >> Date: Wed, 12 Jan 2011 12:49:03 -0500
> >> 
> >> > 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!
> >> 
> >> 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.
> 
> 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.

[PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[Tobias Klauser]

>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(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index bec8b82..ab68f78 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -99,6 +99,17 @@ static inline int is_broadcast_ether_addr(const u8 *addr)
 }
 
 /**
+ * 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);
+}
+
+/**
  * is_valid_ether_addr - Determine if the given Ethernet address is valid
  * @addr: Pointer to a six-byte array containing the Ethernet address
  *
-- 
1.7.0.4

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[Joe Perches]

On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
> >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 +++++++++++
[]
>  /**
> + * 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.

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[Tobias Klauser]

On 2011-01-13 at 09:24:41 +0100, Joe Perches <joe@perches.com> wrote:
> On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
> > >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 +++++++++++
> []
> >  /**
> > + * 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 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

> 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)

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[Joe Perches]

On Thu, 2011-01-13 at 09:35 +0100, Tobias Klauser wrote:
> On 2011-01-13 at 09:24:41 +0100, Joe Perches <joe@perches.com> 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
> > 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)

Adding the #define makes a bit more sense to me.

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[Eric Dumazet]

Le jeudi 13 janvier 2011 à 00:24 -0800, Joe Perches a écrit :
> On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
> > >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 +++++++++++
> []
> >  /**
> > + * 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.

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[Chris Metcalf]

On 1/13/2011 3:55 AM, Eric Dumazet wrote:
> Le jeudi 13 janvier 2011 à 00:24 -0800, Joe Perches a écrit :
>> On Thu, 2011-01-13 at 09:14 +0100, Tobias Klauser wrote:
>>> >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 +++++++++++
>> []
>>>  /**
>>> + * 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 <cmetcalf@tilera.com>

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[David Miller]

From: Chris Metcalf <cmetcalf@tilera.com>
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.

Re: [PATCH net-next-2.6] etherdevice.h: Add is_unicast_ether_addr function

[David Miller]

From: Tobias Klauser <tklauser@distanz.ch>
Date: Thu, 13 Jan 2011 09:14:56 +0100

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

Applied.

[PATCH v2 net-next-2.6] netdev: tilepro: Use is_unicast_ether_addr helper

[Tobias Klauser]

Use is_unicast_ether_addr from linux/etherdevice.h instead of custom
macros.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/net/tile/tilepro.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/drivers/net/tile/tilepro.c b/drivers/net/tile/tilepro.c
index 0e6bac5..7cb301d 100644
--- a/drivers/net/tile/tilepro.c
+++ b/drivers/net/tile/tilepro.c
@@ -142,14 +142,6 @@
 MODULE_AUTHOR("Tilera");
 MODULE_LICENSE("GPL");
 
-
-#define IS_MULTICAST(mac_addr) \
-	(((u8 *)(mac_addr))[0] & 0x01)
-
-#define IS_BROADCAST(mac_addr) \
-	(((u16 *)(mac_addr))[0] == 0xffff)
-
-
 /*
  * Queue of incoming packets for a specific cpu and device.
  *
@@ -795,7 +787,7 @@ static bool tile_net_poll_aux(struct tile_net_cpu *info, int index)
 		/*
 		 * FIXME: Implement HW multicast filter.
 		 */
-		if (!IS_MULTICAST(buf) && !IS_BROADCAST(buf)) {
+		if (is_unicast_ether_addr(buf)) {
 			/* Filter packets not for our address. */
 			const u8 *mine = dev->dev_addr;
 			filter = compare_ether_addr(mine, buf);
-- 
1.7.0.4

Re: [PATCH v2 net-next-2.6] netdev: tilepro: Use is_unicast_ether_addr helper

[David Miller]

From: Tobias Klauser <tklauser@distanz.ch>
Date: Thu, 13 Jan 2011 09:15:08 +0100

> Use is_unicast_ether_addr from linux/etherdevice.h instead of custom
> macros.
> 
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>

Applied.