[PATCH net-next-2.6 0/4] net: change the way mc_list is accessed

[PATCH net-next-2.6 0/4] net: change the way mc_list is accessed

[Jiri Pirko]

In a struct net_device, multicast addresses are stored using a self-made linked
list. To convert this to list_head list there would be needed to do the change
in all (literally all) network device drivers at once.

To solve this situation and also to make device drivers' code prettier I'm
introducing several multicast list helpers which can (and in the future they
should) be used to access mc list. Once all drivers will use these helpers,
we can easily convert to list_head.

The part of this patchset are also 3 examples of a usage of the helpers.

Kindly asking for review.

Thanks,

Jirka

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[PATCH net-next-2.6 1/4] net: introduce mc list helpers

[Jiri Pirko]

This helpers should be used by network drivers to access to netdev
multicast lists.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 include/linux/netdevice.h |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8380009..7edc4a6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -921,6 +921,28 @@ struct net_device
 
 #define	NETDEV_ALIGN		32
 
+static inline int netdev_mc_count(struct net_device *dev)
+{
+	return dev->mc_count;
+}
+
+static inline bool netdev_mc_empty(struct net_device *dev)
+{
+	return netdev_mc_count(dev) == 0;
+}
+
+static inline void netdev_mc_walk(struct net_device *dev,
+				  void (*func)(void *, unsigned char *),
+				  void *data)
+{
+	struct dev_addr_list *mclist;
+	int i;
+
+	for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
+	     i++, mclist = mclist->next)
+		func(data, mclist->dmi_addr);
+}
+
 static inline
 struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
 					 unsigned int index)
-- 
1.6.2.5

Re: [PATCH net-next-2.6 1/4] net: introduce mc list helpers

[Ben Hutchings]

On Thu, 2009-10-22 at 15:52 +0200, Jiri Pirko wrote:
> This helpers should be used by network drivers to access to netdev
> multicast lists.
[...]
> +static inline void netdev_mc_walk(struct net_device *dev,
> +				  void (*func)(void *, unsigned char *),
> +				  void *data)
> +{
> +	struct dev_addr_list *mclist;
> +	int i;
> +
> +	for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
> +	     i++, mclist = mclist->next)
> +		func(data, mclist->dmi_addr);
> +}
[...]

We usually implement iteration as macros so that any context doesn't
have to be squeezed through a single untyped (void *) variable.  A macro
for this would look something like:

#define netdev_for_each_mc_addr(dev, addr)						\
	for (addr = (dev)->mc_list ? (dev)->mc_list->dmi_addr : NULL;			\
	     addr;									\
	     addr = (container_of(addr, struct dev_addr_list, dmi_addr)->next ?		\
		     container_of(addr, struct dev_addr_list, dmi_addr)->next->dmi_addr : \
		     NULL))

Once you change the list type this can presumably be made less ugly.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 1/4] net: introduce mc list helpers

[Jiri Pirko]

Thu, Oct 22, 2009 at 04:18:32PM CEST, bhutchings@solarflare.com wrote:
>On Thu, 2009-10-22 at 15:52 +0200, Jiri Pirko wrote:
>> This helpers should be used by network drivers to access to netdev
>> multicast lists.
>[...]
>> +static inline void netdev_mc_walk(struct net_device *dev,
>> +				  void (*func)(void *, unsigned char *),
>> +				  void *data)
>> +{
>> +	struct dev_addr_list *mclist;
>> +	int i;
>> +
>> +	for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
>> +	     i++, mclist = mclist->next)
>> +		func(data, mclist->dmi_addr);
>> +}
>[...]
>
>We usually implement iteration as macros so that any context doesn't
>have to be squeezed through a single untyped (void *) variable.  A macro
>for this would look something like:
>
>#define netdev_for_each_mc_addr(dev, addr)						\
>	for (addr = (dev)->mc_list ? (dev)->mc_list->dmi_addr : NULL;			\
>	     addr;									\
>	     addr = (container_of(addr, struct dev_addr_list, dmi_addr)->next ?		\
>		     container_of(addr, struct dev_addr_list, dmi_addr)->next->dmi_addr : \
>		     NULL))

I admit this would look better. Going to change this and then repost.

Thanks Ben

>
>Once you change the list type this can presumably be made less ugly.
>
>Ben.
>
>-- 
>Ben Hutchings, Senior Software Engineer, Solarflare Communications
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>

Re: [PATCH net-next-2.6 1/4] net: introduce mc list helpers

[Jiri Pirko]

Thu, Oct 22, 2009 at 04:18:32PM CEST, bhutchings@solarflare.com wrote:
>On Thu, 2009-10-22 at 15:52 +0200, Jiri Pirko wrote:
>> This helpers should be used by network drivers to access to netdev
>> multicast lists.
>[...]
>> +static inline void netdev_mc_walk(struct net_device *dev,
>> +				  void (*func)(void *, unsigned char *),
>> +				  void *data)
>> +{
>> +	struct dev_addr_list *mclist;
>> +	int i;
>> +
>> +	for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
>> +	     i++, mclist = mclist->next)
>> +		func(data, mclist->dmi_addr);
>> +}
>[...]
>
>We usually implement iteration as macros so that any context doesn't
>have to be squeezed through a single untyped (void *) variable.  A macro
>for this would look something like:
>
>#define netdev_for_each_mc_addr(dev, addr)						\
>	for (addr = (dev)->mc_list ? (dev)->mc_list->dmi_addr : NULL;			\
>	     addr;									\
>	     addr = (container_of(addr, struct dev_addr_list, dmi_addr)->next ?		\
>		     container_of(addr, struct dev_addr_list, dmi_addr)->next->dmi_addr : \
>		     NULL))
>
>Once you change the list type this can presumably be made less ugly.

Looking at this, I'm not sure how to deal with this macro once we need to
convert it to work with list_head. I see two options:

1) traverse through the list by hand in this macro (ugly)
2) introduce something like "list_for_each_struct_entry" which takes pointer of
   the structure member as a cursor. Then netdev_for_each_mc_addr would be just
   wrap-up of this.

What do you think?

Thanks

Jirka
>
>Ben.
>
>-- 
>Ben Hutchings, Senior Software Engineer, Solarflare Communications
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel

[PATCH net-next-2.6 2/4] 8139too: use mc helpers to access multicast list

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/8139too.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index 7e333f7..f0c3670 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -2501,6 +2501,15 @@ static struct net_device_stats *rtl8139_get_stats (struct net_device *dev)
 	return &dev->stats;
 }
 
+static void mc_walker(void *data, unsigned char *addr)
+{
+	u32 *mc_filter = data;
+	int bit_nr;
+
+	bit_nr = ether_crc(ETH_ALEN, addr) >> 26;
+	mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 31);
+}
+
 /* Set or clear the multicast filter for this adaptor.
    This routine is not state sensitive and need not be SMP locked. */
 
@@ -2509,7 +2518,7 @@ static void __set_rx_mode (struct net_device *dev)
 	struct rtl8139_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
 	u32 mc_filter[2];	/* Multicast hash filter */
-	int i, rx_mode;
+	int rx_mode;
 	u32 tmp;
 
 	pr_debug("%s:   rtl8139_set_rx_mode(%4.4x) done -- Rx config %8.8lx.\n",
@@ -2521,22 +2530,17 @@ static void __set_rx_mode (struct net_device *dev)
 		    AcceptBroadcast | AcceptMulticast | AcceptMyPhys |
 		    AcceptAllPhys;
 		mc_filter[1] = mc_filter[0] = 0xffffffff;
-	} else if ((dev->mc_count > multicast_filter_limit)
+	} else if ((netdev_mc_count(dev) > multicast_filter_limit)
 		   || (dev->flags & IFF_ALLMULTI)) {
 		/* Too many to filter perfectly -- accept all multicasts. */
 		rx_mode = AcceptBroadcast | AcceptMulticast | AcceptMyPhys;
 		mc_filter[1] = mc_filter[0] = 0xffffffff;
 	} else {
-		struct dev_mc_list *mclist;
 		rx_mode = AcceptBroadcast | AcceptMyPhys;
-		mc_filter[1] = mc_filter[0] = 0;
-		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
-		     i++, mclist = mclist->next) {
-			int bit_nr = ether_crc(ETH_ALEN, mclist->dmi_addr) >> 26;
-
-			mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 31);
+		if (!netdev_mc_empty(dev))
 			rx_mode |= AcceptMulticast;
-		}
+		mc_filter[1] = mc_filter[0] = 0;
+		netdev_mc_walk(dev, mc_walker, mc_filter);
 	}
 
 	/* We can safely update without stopping the chip. */
-- 
1.6.2.5


------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

[PATCH net-next-2.6 3/4] e1000e: use mc helpers to access multicast list

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/e1000e/netdev.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 3769248..97cd106 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2529,6 +2529,17 @@ static void e1000_update_mc_addr_list(struct e1000_hw *hw, u8 *mc_addr_list,
 }
 
 /**
+ * e1000_mc_walker - helper function
+ **/
+static void e1000_mc_walker(void *data, unsigned char *addr)
+{
+	u8 **mta_list_i = data;
+
+	memcpy(*mta_list_i, addr, ETH_ALEN);
+	*mta_list_i += ETH_ALEN;
+}
+
+/**
  * e1000_set_multi - Multicast and Promiscuous mode set
  * @netdev: network interface device structure
  *
@@ -2542,10 +2553,9 @@ static void e1000_set_multi(struct net_device *netdev)
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
 	struct e1000_mac_info *mac = &hw->mac;
-	struct dev_mc_list *mc_ptr;
-	u8  *mta_list;
+	u8  *mta_list, *mta_list_i;
 	u32 rctl;
-	int i;
+	int mc_count;
 
 	/* Check for Promiscuous and All Multicast modes */
 
@@ -2567,23 +2577,17 @@ static void e1000_set_multi(struct net_device *netdev)
 
 	ew32(RCTL, rctl);
 
-	if (netdev->mc_count) {
-		mta_list = kmalloc(netdev->mc_count * 6, GFP_ATOMIC);
+	mc_count = netdev_mc_count(netdev);
+	if (mc_count) {
+		mta_list = kmalloc(mc_count * ETH_ALEN, GFP_ATOMIC);
 		if (!mta_list)
 			return;
 
 		/* prepare a packed array of only addresses. */
-		mc_ptr = netdev->mc_list;
-
-		for (i = 0; i < netdev->mc_count; i++) {
-			if (!mc_ptr)
-				break;
-			memcpy(mta_list + (i*ETH_ALEN), mc_ptr->dmi_addr,
-			       ETH_ALEN);
-			mc_ptr = mc_ptr->next;
-		}
+		mta_list_i = mta_list;
+		netdev_mc_walk(netdev, e1000_mc_walker, &mta_list_i);
 
-		e1000_update_mc_addr_list(hw, mta_list, i, 1,
+		e1000_update_mc_addr_list(hw, mta_list, mc_count, 1,
 					  mac->rar_entry_count);
 		kfree(mta_list);
 	} else {
-- 
1.6.2.5


------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

Re: [PATCH net-next-2.6 0/4] net: change the way mc_list is accessed

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/media/dvb/dvb-core/dvb_net.c |   22 +++++++---------------
 1 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/media/dvb/dvb-core/dvb_net.c b/drivers/media/dvb/dvb-core/dvb_net.c
index 8c9ae0a..eb50fb0 100644
--- a/drivers/media/dvb/dvb-core/dvb_net.c
+++ b/drivers/media/dvb/dvb-core/dvb_net.c
@@ -1110,17 +1110,16 @@ static int dvb_net_feed_stop(struct net_device *dev)
 }
 
 
-static int dvb_set_mc_filter (struct net_device *dev, struct dev_mc_list *mc)
+static void dvb_set_mc_filter(void *data, unsigned char *addr)
 {
-	struct dvb_net_priv *priv = netdev_priv(dev);
+	struct dvb_net_priv *priv = data;
 
 	if (priv->multi_num == DVB_NET_MULTICAST_MAX)
-		return -ENOMEM;
+		return;
 
-	memcpy(priv->multi_macs[priv->multi_num], mc->dmi_addr, 6);
+	memcpy(priv->multi_macs[priv->multi_num], addr, ETH_ALEN);
 
 	priv->multi_num++;
-	return 0;
 }
 
 
@@ -1140,21 +1139,14 @@ static void wq_set_multicast_list (struct work_struct *work)
 	} else if ((dev->flags & IFF_ALLMULTI)) {
 		dprintk("%s: allmulti mode\n", dev->name);
 		priv->rx_mode = RX_MODE_ALL_MULTI;
-	} else if (dev->mc_count) {
-		int mci;
-		struct dev_mc_list *mc;
-
+	} else if (netdev_mc_count(dev)) {
 		dprintk("%s: set_mc_list, %d entries\n",
-			dev->name, dev->mc_count);
+			dev->name, netdev_mc_count(dev));
 
 		priv->rx_mode = RX_MODE_MULTI;
 		priv->multi_num = 0;
 
-		for (mci = 0, mc=dev->mc_list;
-		     mci < dev->mc_count;
-		     mc = mc->next, mci++) {
-			dvb_set_mc_filter(dev, mc);
-		}
+		netdev_mc_walk(dev, dvb_set_mc_filter, priv);
 	}
 
 	netif_addr_unlock_bh(dev);
-- 
1.6.2.5


------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference

Re: [PATCH net-next-2.6 0/4] net: change the way mc_list is accessed

[Jiri Pirko]

wrong subject... reposting...

Thu, Oct 22, 2009 at 03:54:47PM CEST, jpirko@redhat.com wrote:
>Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>---
> drivers/media/dvb/dvb-core/dvb_net.c |   22 +++++++---------------
> 1 files changed, 7 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/media/dvb/dvb-core/dvb_net.c b/drivers/media/dvb/dvb-core/dvb_net.c
>index 8c9ae0a..eb50fb0 100644
>--- a/drivers/media/dvb/dvb-core/dvb_net.c
>+++ b/drivers/media/dvb/dvb-core/dvb_net.c
>@@ -1110,17 +1110,16 @@ static int dvb_net_feed_stop(struct net_device *dev)
> }
> 
> 
>-static int dvb_set_mc_filter (struct net_device *dev, struct dev_mc_list *mc)
>+static void dvb_set_mc_filter(void *data, unsigned char *addr)
> {
>-	struct dvb_net_priv *priv = netdev_priv(dev);
>+	struct dvb_net_priv *priv = data;
> 
> 	if (priv->multi_num == DVB_NET_MULTICAST_MAX)
>-		return -ENOMEM;
>+		return;
> 
>-	memcpy(priv->multi_macs[priv->multi_num], mc->dmi_addr, 6);
>+	memcpy(priv->multi_macs[priv->multi_num], addr, ETH_ALEN);
> 
> 	priv->multi_num++;
>-	return 0;
> }
> 
> 
>@@ -1140,21 +1139,14 @@ static void wq_set_multicast_list (struct work_struct *work)
> 	} else if ((dev->flags & IFF_ALLMULTI)) {
> 		dprintk("%s: allmulti mode\n", dev->name);
> 		priv->rx_mode = RX_MODE_ALL_MULTI;
>-	} else if (dev->mc_count) {
>-		int mci;
>-		struct dev_mc_list *mc;
>-
>+	} else if (netdev_mc_count(dev)) {
> 		dprintk("%s: set_mc_list, %d entries\n",
>-			dev->name, dev->mc_count);
>+			dev->name, netdev_mc_count(dev));
> 
> 		priv->rx_mode = RX_MODE_MULTI;
> 		priv->multi_num = 0;
> 
>-		for (mci = 0, mc=dev->mc_list;
>-		     mci < dev->mc_count;
>-		     mc = mc->next, mci++) {
>-			dvb_set_mc_filter(dev, mc);
>-		}
>+		netdev_mc_walk(dev, dvb_set_mc_filter, priv);
> 	}
> 
> 	netif_addr_unlock_bh(dev);
>-- 
>1.6.2.5
>

[PATCH net-next-2.6 4/4] dvb: dvb_net: use mc helpers to access multicast list

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/media/dvb/dvb-core/dvb_net.c |   22 +++++++---------------
 1 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/media/dvb/dvb-core/dvb_net.c b/drivers/media/dvb/dvb-core/dvb_net.c
index 8c9ae0a..eb50fb0 100644
--- a/drivers/media/dvb/dvb-core/dvb_net.c
+++ b/drivers/media/dvb/dvb-core/dvb_net.c
@@ -1110,17 +1110,16 @@ static int dvb_net_feed_stop(struct net_device *dev)
 }
 
 
-static int dvb_set_mc_filter (struct net_device *dev, struct dev_mc_list *mc)
+static void dvb_set_mc_filter(void *data, unsigned char *addr)
 {
-	struct dvb_net_priv *priv = netdev_priv(dev);
+	struct dvb_net_priv *priv = data;
 
 	if (priv->multi_num == DVB_NET_MULTICAST_MAX)
-		return -ENOMEM;
+		return;
 
-	memcpy(priv->multi_macs[priv->multi_num], mc->dmi_addr, 6);
+	memcpy(priv->multi_macs[priv->multi_num], addr, ETH_ALEN);
 
 	priv->multi_num++;
-	return 0;
 }
 
 
@@ -1140,21 +1139,14 @@ static void wq_set_multicast_list (struct work_struct *work)
 	} else if ((dev->flags & IFF_ALLMULTI)) {
 		dprintk("%s: allmulti mode\n", dev->name);
 		priv->rx_mode = RX_MODE_ALL_MULTI;
-	} else if (dev->mc_count) {
-		int mci;
-		struct dev_mc_list *mc;
-
+	} else if (netdev_mc_count(dev)) {
 		dprintk("%s: set_mc_list, %d entries\n",
-			dev->name, dev->mc_count);
+			dev->name, netdev_mc_count(dev));
 
 		priv->rx_mode = RX_MODE_MULTI;
 		priv->multi_num = 0;
 
-		for (mci = 0, mc=dev->mc_list;
-		     mci < dev->mc_count;
-		     mc = mc->next, mci++) {
-			dvb_set_mc_filter(dev, mc);
-		}
+		netdev_mc_walk(dev, dvb_set_mc_filter, priv);
 	}
 
 	netif_addr_unlock_bh(dev);
-- 
1.6.2.5