| Message ID | 1294906496-14950-1-git-send-email-tklauser@distanz.ch |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *
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(-)