| Message ID | 1294824713-10644-1-git-send-email-tklauser@distanz.ch |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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!
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. -- 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 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? -- 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 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. -- 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 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. -- 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/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);
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(-)