[net-next PATCH 0/2] Provide common means for device address sync

[net-next PATCH 0/2] Provide common means for device address sync

[Alexander Duyck]

The following series implements a means for synchronizing both unicast and
multicast addresses on a device interface.  The code is based on the original
implementation of dev_uc_sync that was available for syncing a VLAN to the
lower dev.

The original reason for coming up for this patch is a driver that is still in
the early stages of development.  The nearest driver I could find that
appeared to have the same limitations as the driver I was working on was the
Cisco enic driver.  For this reason I chose it as the first driver to make use
of this interface publicly.

However, I do not have a Cisco enic interface so I have only been able to
compile test any changes made to the driver.  I tried to keep this change as
simple as possible to avoid any issues.  Any help with testing would be
greatly appreciated.

---

Alexander Duyck (2):
      net: Add support for device specific address syncing
      enic: Update driver to use __dev_uc/mc_sync/unsync calls


 drivers/net/ethernet/cisco/enic/enic.h      |    2 
 drivers/net/ethernet/cisco/enic/enic_dev.c  |    4 -
 drivers/net/ethernet/cisco/enic/enic_dev.h  |    4 -
 drivers/net/ethernet/cisco/enic/enic_main.c |  173 ++++++++++-----------------
 drivers/net/ethernet/cisco/enic/vnic_dev.c  |    4 -
 drivers/net/ethernet/cisco/enic/vnic_dev.h  |    4 -
 include/linux/netdevice.h                   |   73 +++++++++++
 net/core/dev_addr_lists.c                   |   85 +++++++++++++
 8 files changed, 231 insertions(+), 118 deletions(-)

--

[net-next PATCH 1/2] net: Add support for device specific address syncing

[Alexander Duyck]

This change provides a function to be used in order to break the
ndo_set_rx_mode call into a set of address add and remove calls.  The code
is based on the implementation of dev_uc_sync/dev_mc_sync.  Since they
essentially do the same thing but with only one dev I simply named my
functions __dev_uc_sync/__dev_mc_sync.

I also implemented an unsync version of the functions as well to allow for
cleanup on close.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 include/linux/netdevice.h |   73 +++++++++++++++++++++++++++++++++++++++
 net/core/dev_addr_lists.c |   85 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2db1610..774e539 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3003,6 +3003,15 @@ int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
 		   struct netdev_hw_addr_list *from_list, int addr_len);
 void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
 		      struct netdev_hw_addr_list *from_list, int addr_len);
+int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
+		       struct net_device *dev,
+		       int (*sync)(struct net_device *, const unsigned char *),
+		       int (*unsync)(struct net_device *,
+				     const unsigned char *));
+void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list,
+			  struct net_device *dev,
+			  int (*unsync)(struct net_device *,
+					const unsigned char *));
 void __hw_addr_init(struct netdev_hw_addr_list *list);
 
 /* Functions used for device addresses handling */
@@ -3023,6 +3032,38 @@ void dev_uc_unsync(struct net_device *to, struct net_device *from);
 void dev_uc_flush(struct net_device *dev);
 void dev_uc_init(struct net_device *dev);
 
+/**
+ *  __dev_uc_sync - Synchonize device's unicast list
+ *  @dev:  device to sync
+ *  @sync: function to call if address should be added
+ *  @unsync: function to call if address should be removed
+ *
+ *  Add newly added addresses to the interface, and release
+ *  addresses that have been deleted.
+ **/
+static inline int __dev_uc_sync(struct net_device *dev,
+				int (*sync)(struct net_device *,
+					    const unsigned char *),
+				int (*unsync)(struct net_device *,
+					      const unsigned char *))
+{
+	return __hw_addr_sync_dev(&dev->uc, dev, sync, unsync);
+}
+
+/**
+ *  __dev_uc_unsync - Remove synchonized addresses from device
+ *  @dev:  device to sync
+ *  @unsync: function to call if address should be removed
+ *
+ *  Remove all addresses that were added to the device by dev_uc_sync().
+ **/
+static inline void __dev_uc_unsync(struct net_device *dev,
+				   int (*unsync)(struct net_device *,
+						 const unsigned char *))
+{
+	__hw_addr_unsync_dev(&dev->uc, dev, unsync);
+}
+
 /* Functions used for multicast addresses handling */
 int dev_mc_add(struct net_device *dev, const unsigned char *addr);
 int dev_mc_add_global(struct net_device *dev, const unsigned char *addr);
@@ -3035,6 +3076,38 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from);
 void dev_mc_flush(struct net_device *dev);
 void dev_mc_init(struct net_device *dev);
 
+/**
+ *  __dev_mc_sync - Synchonize device's multicast list
+ *  @dev:  device to sync
+ *  @sync: function to call if address should be added
+ *  @unsync: function to call if address should be removed
+ *
+ *  Add newly added addresses to the interface, and release
+ *  addresses that have been deleted.
+ **/
+static inline int __dev_mc_sync(struct net_device *dev,
+				int (*sync)(struct net_device *,
+					    const unsigned char *),
+				int (*unsync)(struct net_device *,
+					      const unsigned char *))
+{
+	return __hw_addr_sync_dev(&dev->mc, dev, sync, unsync);
+}
+
+/**
+ *  __dev_mc_unsync - Remove synchonized addresses from device
+ *  @dev:  device to sync
+ *  @unsync: function to call if address should be removed
+ *
+ *  Remove all addresses that were added to the device by dev_mc_sync().
+ **/
+static inline void __dev_mc_unsync(struct net_device *dev,
+				   int (*unsync)(struct net_device *,
+						 const unsigned char *))
+{
+	__hw_addr_unsync_dev(&dev->mc, dev, unsync);
+}
+
 /* Functions used for secondary unicast and multicast support */
 void dev_set_rx_mode(struct net_device *dev);
 void __dev_set_rx_mode(struct net_device *dev);
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 329d579..b6b2306 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -225,6 +225,91 @@ void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
 }
 EXPORT_SYMBOL(__hw_addr_unsync);
 
+/**
+ *  __hw_addr_sync_dev - Synchonize device's multicast list
+ *  @list: address list to syncronize
+ *  @dev:  device to sync
+ *  @sync: function to call if address should be added
+ *  @unsync: function to call if address should be removed
+ *
+ *  This funciton is intended to be called from the ndo_set_rx_mode
+ *  function of devices that require explicit address add/remove
+ *  notifications.  The unsync function may be NULL in which case
+ *  the addresses requiring removal will simply be removed without
+ *  any notification to the device.
+ **/
+int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
+		       struct net_device *dev,
+		       int (*sync)(struct net_device *, const unsigned char *),
+		       int (*unsync)(struct net_device *,
+				     const unsigned char *))
+{
+	struct netdev_hw_addr *ha, *tmp;
+	int err;
+
+	/* first go through and flush out any stale entries */
+	list_for_each_entry_safe(ha, tmp, &list->list, list) {
+		if (!ha->sync_cnt || ha->refcount != 1)
+			continue;
+
+		/* if unsync is defined and fails defer unsyncing address */
+		if (unsync && unsync(dev, ha->addr))
+			continue;
+
+		ha->sync_cnt--;
+		__hw_addr_del_entry(list, ha, false, false);
+	}
+
+	/* go through and sync new entries to the list */
+	list_for_each_entry_safe(ha, tmp, &list->list, list) {
+		if (ha->sync_cnt)
+			continue;
+
+		err = sync(dev, ha->addr);
+		if (err)
+			return err;
+
+		ha->sync_cnt++;
+		ha->refcount++;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(__hw_addr_sync_dev);
+
+/**
+ *  __hw_addr_unsync_dev - Remove synchonized addresses from device
+ *  @list: address list to remove syncronized addresses from
+ *  @dev:  device to sync
+ *  @unsync: function to call if address should be removed
+ *
+ *  Remove all addresses that were added to the device by __hw_addr_sync_dev().
+ *  This function is intended to be called from the ndo_stop or ndo_open
+ *  functions on devices that require explicit address add/remove
+ *  notifications.  If the unsync function pointer is NULL then this function
+ *  can be used to just reset the sync_cnt for the addresses in the list.
+ **/
+void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list,
+			  struct net_device *dev,
+			  int (*unsync)(struct net_device *,
+					const unsigned char *))
+{
+	struct netdev_hw_addr *ha, *tmp;
+
+	list_for_each_entry_safe(ha, tmp, &list->list, list) {
+		if (!ha->sync_cnt)
+			continue;
+
+		/* if unsync is defined and fails defer unsyncing address */
+		if (unsync && unsync(dev, ha->addr))
+			continue;
+
+		ha->sync_cnt--;
+		__hw_addr_del_entry(list, ha, false, false);
+	}
+}
+EXPORT_SYMBOL(__hw_addr_unsync_dev);
+
 static void __hw_addr_flush(struct netdev_hw_addr_list *list)
 {
 	struct netdev_hw_addr *ha, *tmp;

[net-next PATCH 2/2] enic: Update driver to use __dev_uc/mc_sync/unsync calls

[Alexander Duyck]

This change updates the enic driver to make use of __dev_uc_sync and
__dev_mc_sync calls.  Previously the driver was doing its own list
management by storing the mc_addr and uc_addr list in a 32 address array.
With this change the sync data is stored in the netdev_addr_list structures
and instead we just track how many addresses we have written to the device.
When we encounter 32 we stop and print a message as occurred previously with
the old approach.

Other than the core change the only other bit needed was to propagate the
constant attribute with the MAC address as there were several spots where
is twas only passed as a u8 * instead of a const u8 *.

This patch is meant to maintain the original functionality without the use
of the mc_addr and uc_addr arrays.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/cisco/enic/enic.h      |    2 
 drivers/net/ethernet/cisco/enic/enic_dev.c  |    4 -
 drivers/net/ethernet/cisco/enic/enic_dev.h  |    4 -
 drivers/net/ethernet/cisco/enic/enic_main.c |  173 ++++++++++-----------------
 drivers/net/ethernet/cisco/enic/vnic_dev.c  |    4 -
 drivers/net/ethernet/cisco/enic/vnic_dev.h  |    4 -
 6 files changed, 73 insertions(+), 118 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index f23ef32..14f465f 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -114,8 +114,6 @@ struct enic {
 	u32 msg_enable;
 	spinlock_t devcmd_lock;
 	u8 mac_addr[ETH_ALEN];
-	u8 mc_addr[ENIC_MULTICAST_PERFECT_FILTERS][ETH_ALEN];
-	u8 uc_addr[ENIC_UNICAST_PERFECT_FILTERS][ETH_ALEN];
 	unsigned int flags;
 	unsigned int priv_flags;
 	unsigned int mc_count;
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
index 4b6e569..3e27df5 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
@@ -88,7 +88,7 @@ int enic_dev_packet_filter(struct enic *enic, int directed, int multicast,
 	return err;
 }
 
-int enic_dev_add_addr(struct enic *enic, u8 *addr)
+int enic_dev_add_addr(struct enic *enic, const u8 *addr)
 {
 	int err;
 
@@ -99,7 +99,7 @@ int enic_dev_add_addr(struct enic *enic, u8 *addr)
 	return err;
 }
 
-int enic_dev_del_addr(struct enic *enic, u8 *addr)
+int enic_dev_del_addr(struct enic *enic, const u8 *addr)
 {
 	int err;
 
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.h b/drivers/net/ethernet/cisco/enic/enic_dev.h
index 129b14a..36ea1ab 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.h
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.h
@@ -45,8 +45,8 @@ int enic_dev_add_station_addr(struct enic *enic);
 int enic_dev_del_station_addr(struct enic *enic);
 int enic_dev_packet_filter(struct enic *enic, int directed, int multicast,
 	int broadcast, int promisc, int allmulti);
-int enic_dev_add_addr(struct enic *enic, u8 *addr);
-int enic_dev_del_addr(struct enic *enic, u8 *addr);
+int enic_dev_add_addr(struct enic *enic, const u8 *addr);
+int enic_dev_del_addr(struct enic *enic, const u8 *addr);
 int enic_vlan_rx_add_vid(struct net_device *netdev, __be16 proto, u16 vid);
 int enic_vlan_rx_kill_vid(struct net_device *netdev, __be16 proto, u16 vid);
 int enic_dev_notify_unset(struct enic *enic);
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 0d8995c..6439c82 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -616,8 +616,71 @@ static struct rtnl_link_stats64 *enic_get_stats(struct net_device *netdev,
 	return net_stats;
 }
 
+static int enic_mc_sync(struct net_device *netdev, const u8 *mc_addr)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	if (enic->mc_count == ENIC_MULTICAST_PERFECT_FILTERS) {
+		unsigned int mc_count = netdev_mc_count(netdev);
+
+		netdev_warn(netdev, "Registering only %d out of %d multicast addresses\n",
+			    ENIC_MULTICAST_PERFECT_FILTERS, mc_count);
+
+		return -ENOSPC;
+	}
+
+	enic_dev_add_addr(enic, mc_addr);
+	enic->mc_count++;
+
+	return 0;
+}
+
+static int enic_mc_unsync(struct net_device *netdev, const u8 *mc_addr)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	enic_dev_del_addr(enic, mc_addr);
+	enic->mc_count--;
+
+	return 0;
+}
+
+static int enic_uc_sync(struct net_device *netdev, const u8 *uc_addr)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	if (enic->uc_count == ENIC_UNICAST_PERFECT_FILTERS) {
+		unsigned int uc_count = netdev_uc_count(netdev);
+
+		netdev_warn(netdev, "Registering only %d out of %d unicast addresses\n",
+			    ENIC_UNICAST_PERFECT_FILTERS, uc_count);
+
+		return -ENOSPC;
+	}
+
+	enic_dev_add_addr(enic, uc_addr);
+	enic->uc_count++;
+
+	return 0;
+}
+
+static int enic_uc_unsync(struct net_device *netdev, const u8 *uc_addr)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	enic_dev_del_addr(enic, uc_addr);
+	enic->uc_count--;
+
+	return 0;
+}
+
 void enic_reset_addr_lists(struct enic *enic)
 {
+	struct net_device *netdev = enic->netdev;
+
+	__dev_uc_unsync(netdev, NULL);
+	__dev_mc_unsync(netdev, NULL);
+
 	enic->mc_count = 0;
 	enic->uc_count = 0;
 	enic->flags = 0;
@@ -684,112 +747,6 @@ static int enic_set_mac_address(struct net_device *netdev, void *p)
 	return enic_dev_add_station_addr(enic);
 }
 
-static void enic_update_multicast_addr_list(struct enic *enic)
-{
-	struct net_device *netdev = enic->netdev;
-	struct netdev_hw_addr *ha;
-	unsigned int mc_count = netdev_mc_count(netdev);
-	u8 mc_addr[ENIC_MULTICAST_PERFECT_FILTERS][ETH_ALEN];
-	unsigned int i, j;
-
-	if (mc_count > ENIC_MULTICAST_PERFECT_FILTERS) {
-		netdev_warn(netdev, "Registering only %d out of %d "
-			"multicast addresses\n",
-			ENIC_MULTICAST_PERFECT_FILTERS, mc_count);
-		mc_count = ENIC_MULTICAST_PERFECT_FILTERS;
-	}
-
-	/* Is there an easier way?  Trying to minimize to
-	 * calls to add/del multicast addrs.  We keep the
-	 * addrs from the last call in enic->mc_addr and
-	 * look for changes to add/del.
-	 */
-
-	i = 0;
-	netdev_for_each_mc_addr(ha, netdev) {
-		if (i == mc_count)
-			break;
-		memcpy(mc_addr[i++], ha->addr, ETH_ALEN);
-	}
-
-	for (i = 0; i < enic->mc_count; i++) {
-		for (j = 0; j < mc_count; j++)
-			if (ether_addr_equal(enic->mc_addr[i], mc_addr[j]))
-				break;
-		if (j == mc_count)
-			enic_dev_del_addr(enic, enic->mc_addr[i]);
-	}
-
-	for (i = 0; i < mc_count; i++) {
-		for (j = 0; j < enic->mc_count; j++)
-			if (ether_addr_equal(mc_addr[i], enic->mc_addr[j]))
-				break;
-		if (j == enic->mc_count)
-			enic_dev_add_addr(enic, mc_addr[i]);
-	}
-
-	/* Save the list to compare against next time
-	 */
-
-	for (i = 0; i < mc_count; i++)
-		memcpy(enic->mc_addr[i], mc_addr[i], ETH_ALEN);
-
-	enic->mc_count = mc_count;
-}
-
-static void enic_update_unicast_addr_list(struct enic *enic)
-{
-	struct net_device *netdev = enic->netdev;
-	struct netdev_hw_addr *ha;
-	unsigned int uc_count = netdev_uc_count(netdev);
-	u8 uc_addr[ENIC_UNICAST_PERFECT_FILTERS][ETH_ALEN];
-	unsigned int i, j;
-
-	if (uc_count > ENIC_UNICAST_PERFECT_FILTERS) {
-		netdev_warn(netdev, "Registering only %d out of %d "
-			"unicast addresses\n",
-			ENIC_UNICAST_PERFECT_FILTERS, uc_count);
-		uc_count = ENIC_UNICAST_PERFECT_FILTERS;
-	}
-
-	/* Is there an easier way?  Trying to minimize to
-	 * calls to add/del unicast addrs.  We keep the
-	 * addrs from the last call in enic->uc_addr and
-	 * look for changes to add/del.
-	 */
-
-	i = 0;
-	netdev_for_each_uc_addr(ha, netdev) {
-		if (i == uc_count)
-			break;
-		memcpy(uc_addr[i++], ha->addr, ETH_ALEN);
-	}
-
-	for (i = 0; i < enic->uc_count; i++) {
-		for (j = 0; j < uc_count; j++)
-			if (ether_addr_equal(enic->uc_addr[i], uc_addr[j]))
-				break;
-		if (j == uc_count)
-			enic_dev_del_addr(enic, enic->uc_addr[i]);
-	}
-
-	for (i = 0; i < uc_count; i++) {
-		for (j = 0; j < enic->uc_count; j++)
-			if (ether_addr_equal(uc_addr[i], enic->uc_addr[j]))
-				break;
-		if (j == enic->uc_count)
-			enic_dev_add_addr(enic, uc_addr[i]);
-	}
-
-	/* Save the list to compare against next time
-	 */
-
-	for (i = 0; i < uc_count; i++)
-		memcpy(enic->uc_addr[i], uc_addr[i], ETH_ALEN);
-
-	enic->uc_count = uc_count;
-}
-
 /* netif_tx_lock held, BHs disabled */
 static void enic_set_rx_mode(struct net_device *netdev)
 {
@@ -812,9 +769,9 @@ static void enic_set_rx_mode(struct net_device *netdev)
 	}
 
 	if (!promisc) {
-		enic_update_unicast_addr_list(enic);
+		__dev_uc_sync(netdev, enic_uc_sync, enic_uc_unsync);
 		if (!allmulti)
-			enic_update_multicast_addr_list(enic);
+			__dev_mc_sync(netdev, enic_mc_sync, enic_mc_unsync);
 	}
 }
 
diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.c b/drivers/net/ethernet/cisco/enic/vnic_dev.c
index 69dd925..e86a45c 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_dev.c
@@ -657,7 +657,7 @@ int vnic_dev_packet_filter(struct vnic_dev *vdev, int directed, int multicast,
 	return err;
 }
 
-int vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
+int vnic_dev_add_addr(struct vnic_dev *vdev, const u8 *addr)
 {
 	u64 a0 = 0, a1 = 0;
 	int wait = 1000;
@@ -674,7 +674,7 @@ int vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
 	return err;
 }
 
-int vnic_dev_del_addr(struct vnic_dev *vdev, u8 *addr)
+int vnic_dev_del_addr(struct vnic_dev *vdev, const u8 *addr)
 {
 	u64 a0 = 0, a1 = 0;
 	int wait = 1000;
diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.h b/drivers/net/ethernet/cisco/enic/vnic_dev.h
index e670029..1f3b301 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_dev.h
+++ b/drivers/net/ethernet/cisco/enic/vnic_dev.h
@@ -95,8 +95,8 @@ int vnic_dev_stats_dump(struct vnic_dev *vdev, struct vnic_stats **stats);
 int vnic_dev_hang_notify(struct vnic_dev *vdev);
 int vnic_dev_packet_filter(struct vnic_dev *vdev, int directed, int multicast,
 	int broadcast, int promisc, int allmulti);
-int vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr);
-int vnic_dev_del_addr(struct vnic_dev *vdev, u8 *addr);
+int vnic_dev_add_addr(struct vnic_dev *vdev, const u8 *addr);
+int vnic_dev_del_addr(struct vnic_dev *vdev, const u8 *addr);
 int vnic_dev_get_mac_addr(struct vnic_dev *vdev, u8 *mac_addr);
 int vnic_dev_notify_set(struct vnic_dev *vdev, u16 intr);
 int vnic_dev_notify_unset(struct vnic_dev *vdev);

Re: [net-next PATCH 0/2] Provide common means for device address sync

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Wed, 28 May 2014 18:44:41 -0700

> The following series implements a means for synchronizing both unicast and
> multicast addresses on a device interface.  The code is based on the original
> implementation of dev_uc_sync that was available for syncing a VLAN to the
> lower dev.
> 
> The original reason for coming up for this patch is a driver that is still in
> the early stages of development.  The nearest driver I could find that
> appeared to have the same limitations as the driver I was working on was the
> Cisco enic driver.  For this reason I chose it as the first driver to make use
> of this interface publicly.
> 
> However, I do not have a Cisco enic interface so I have only been able to
> compile test any changes made to the driver.  I tried to keep this change as
> simple as possible to avoid any issues.  Any help with testing would be
> greatly appreciated.

I'll apply this series once there is some testing feedback, thanks.

Re: [net-next PATCH 2/2] enic: Update driver to use __dev_uc/mc_sync/unsync calls

[Govindarajulu Varadarajan]

On Wed, 28 May 2014, Alexander Duyck wrote:

> This change updates the enic driver to make use of __dev_uc_sync and
> __dev_mc_sync calls.  Previously the driver was doing its own list
> management by storing the mc_addr and uc_addr list in a 32 address array.
> With this change the sync data is stored in the netdev_addr_list structures
> and instead we just track how many addresses we have written to the device.
> When we encounter 32 we stop and print a message as occurred previously with
> the old approach.
>
> Other than the core change the only other bit needed was to propagate the
> constant attribute with the MAC address as there were several spots where
> is twas only passed as a u8 * instead of a const u8 *.
>
> This patch is meant to maintain the original functionality without the use
> of the mc_addr and uc_addr arrays.
>

Tested on enic hw. No problem seen. Thanks.

Acked-by: Govindarajulu Varadarajan <_govind@gmx.com>

> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> drivers/net/ethernet/cisco/enic/enic.h      |    2
> drivers/net/ethernet/cisco/enic/enic_dev.c  |    4 -
> drivers/net/ethernet/cisco/enic/enic_dev.h  |    4 -
> drivers/net/ethernet/cisco/enic/enic_main.c |  173 ++++++++++-----------------
> drivers/net/ethernet/cisco/enic/vnic_dev.c  |    4 -
> drivers/net/ethernet/cisco/enic/vnic_dev.h  |    4 -
> 6 files changed, 73 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index f23ef32..14f465f 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h
> @@ -114,8 +114,6 @@ struct enic {
> 	u32 msg_enable;
> 	spinlock_t devcmd_lock;
> 	u8 mac_addr[ETH_ALEN];
> -	u8 mc_addr[ENIC_MULTICAST_PERFECT_FILTERS][ETH_ALEN];
> -	u8 uc_addr[ENIC_UNICAST_PERFECT_FILTERS][ETH_ALEN];
> 	unsigned int flags;
> 	unsigned int priv_flags;
> 	unsigned int mc_count;
> diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
> index 4b6e569..3e27df5 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_dev.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
> @@ -88,7 +88,7 @@ int enic_dev_packet_filter(struct enic *enic, int directed, int multicast,
> 	return err;
> }
>
> -int enic_dev_add_addr(struct enic *enic, u8 *addr)
> +int enic_dev_add_addr(struct enic *enic, const u8 *addr)
> {
> 	int err;
>
> @@ -99,7 +99,7 @@ int enic_dev_add_addr(struct enic *enic, u8 *addr)
> 	return err;
> }
>
> -int enic_dev_del_addr(struct enic *enic, u8 *addr)
> +int enic_dev_del_addr(struct enic *enic, const u8 *addr)
> {
> 	int err;
>
> diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.h b/drivers/net/ethernet/cisco/enic/enic_dev.h
> index 129b14a..36ea1ab 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_dev.h
> +++ b/drivers/net/ethernet/cisco/enic/enic_dev.h
> @@ -45,8 +45,8 @@ int enic_dev_add_station_addr(struct enic *enic);
> int enic_dev_del_station_addr(struct enic *enic);
> int enic_dev_packet_filter(struct enic *enic, int directed, int multicast,
> 	int broadcast, int promisc, int allmulti);
> -int enic_dev_add_addr(struct enic *enic, u8 *addr);
> -int enic_dev_del_addr(struct enic *enic, u8 *addr);
> +int enic_dev_add_addr(struct enic *enic, const u8 *addr);
> +int enic_dev_del_addr(struct enic *enic, const u8 *addr);
> int enic_vlan_rx_add_vid(struct net_device *netdev, __be16 proto, u16 vid);
> int enic_vlan_rx_kill_vid(struct net_device *netdev, __be16 proto, u16 vid);
> int enic_dev_notify_unset(struct enic *enic);
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 0d8995c..6439c82 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
> @@ -616,8 +616,71 @@ static struct rtnl_link_stats64 *enic_get_stats(struct net_device *netdev,
> 	return net_stats;
> }
>
> +static int enic_mc_sync(struct net_device *netdev, const u8 *mc_addr)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	if (enic->mc_count == ENIC_MULTICAST_PERFECT_FILTERS) {
> +		unsigned int mc_count = netdev_mc_count(netdev);
> +
> +		netdev_warn(netdev, "Registering only %d out of %d multicast addresses\n",
> +			    ENIC_MULTICAST_PERFECT_FILTERS, mc_count);
> +
> +		return -ENOSPC;
> +	}
> +
> +	enic_dev_add_addr(enic, mc_addr);
> +	enic->mc_count++;
> +
> +	return 0;
> +}
> +
> +static int enic_mc_unsync(struct net_device *netdev, const u8 *mc_addr)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	enic_dev_del_addr(enic, mc_addr);
> +	enic->mc_count--;
> +
> +	return 0;
> +}
> +
> +static int enic_uc_sync(struct net_device *netdev, const u8 *uc_addr)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	if (enic->uc_count == ENIC_UNICAST_PERFECT_FILTERS) {
> +		unsigned int uc_count = netdev_uc_count(netdev);
> +
> +		netdev_warn(netdev, "Registering only %d out of %d unicast addresses\n",
> +			    ENIC_UNICAST_PERFECT_FILTERS, uc_count);
> +
> +		return -ENOSPC;
> +	}
> +
> +	enic_dev_add_addr(enic, uc_addr);
> +	enic->uc_count++;
> +
> +	return 0;
> +}
> +
> +static int enic_uc_unsync(struct net_device *netdev, const u8 *uc_addr)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	enic_dev_del_addr(enic, uc_addr);
> +	enic->uc_count--;
> +
> +	return 0;
> +}
> +
> void enic_reset_addr_lists(struct enic *enic)
> {
> +	struct net_device *netdev = enic->netdev;
> +
> +	__dev_uc_unsync(netdev, NULL);
> +	__dev_mc_unsync(netdev, NULL);
> +
> 	enic->mc_count = 0;
> 	enic->uc_count = 0;
> 	enic->flags = 0;
> @@ -684,112 +747,6 @@ static int enic_set_mac_address(struct net_device *netdev, void *p)
> 	return enic_dev_add_station_addr(enic);
> }
>
> -static void enic_update_multicast_addr_list(struct enic *enic)
> -{
> -	struct net_device *netdev = enic->netdev;
> -	struct netdev_hw_addr *ha;
> -	unsigned int mc_count = netdev_mc_count(netdev);
> -	u8 mc_addr[ENIC_MULTICAST_PERFECT_FILTERS][ETH_ALEN];
> -	unsigned int i, j;
> -
> -	if (mc_count > ENIC_MULTICAST_PERFECT_FILTERS) {
> -		netdev_warn(netdev, "Registering only %d out of %d "
> -			"multicast addresses\n",
> -			ENIC_MULTICAST_PERFECT_FILTERS, mc_count);
> -		mc_count = ENIC_MULTICAST_PERFECT_FILTERS;
> -	}
> -
> -	/* Is there an easier way?  Trying to minimize to
> -	 * calls to add/del multicast addrs.  We keep the
> -	 * addrs from the last call in enic->mc_addr and
> -	 * look for changes to add/del.
> -	 */
> -
> -	i = 0;
> -	netdev_for_each_mc_addr(ha, netdev) {
> -		if (i == mc_count)
> -			break;
> -		memcpy(mc_addr[i++], ha->addr, ETH_ALEN);
> -	}
> -
> -	for (i = 0; i < enic->mc_count; i++) {
> -		for (j = 0; j < mc_count; j++)
> -			if (ether_addr_equal(enic->mc_addr[i], mc_addr[j]))
> -				break;
> -		if (j == mc_count)
> -			enic_dev_del_addr(enic, enic->mc_addr[i]);
> -	}
> -
> -	for (i = 0; i < mc_count; i++) {
> -		for (j = 0; j < enic->mc_count; j++)
> -			if (ether_addr_equal(mc_addr[i], enic->mc_addr[j]))
> -				break;
> -		if (j == enic->mc_count)
> -			enic_dev_add_addr(enic, mc_addr[i]);
> -	}
> -
> -	/* Save the list to compare against next time
> -	 */
> -
> -	for (i = 0; i < mc_count; i++)
> -		memcpy(enic->mc_addr[i], mc_addr[i], ETH_ALEN);
> -
> -	enic->mc_count = mc_count;
> -}
> -
> -static void enic_update_unicast_addr_list(struct enic *enic)
> -{
> -	struct net_device *netdev = enic->netdev;
> -	struct netdev_hw_addr *ha;
> -	unsigned int uc_count = netdev_uc_count(netdev);
> -	u8 uc_addr[ENIC_UNICAST_PERFECT_FILTERS][ETH_ALEN];
> -	unsigned int i, j;
> -
> -	if (uc_count > ENIC_UNICAST_PERFECT_FILTERS) {
> -		netdev_warn(netdev, "Registering only %d out of %d "
> -			"unicast addresses\n",
> -			ENIC_UNICAST_PERFECT_FILTERS, uc_count);
> -		uc_count = ENIC_UNICAST_PERFECT_FILTERS;
> -	}
> -
> -	/* Is there an easier way?  Trying to minimize to
> -	 * calls to add/del unicast addrs.  We keep the
> -	 * addrs from the last call in enic->uc_addr and
> -	 * look for changes to add/del.
> -	 */
> -
> -	i = 0;
> -	netdev_for_each_uc_addr(ha, netdev) {
> -		if (i == uc_count)
> -			break;
> -		memcpy(uc_addr[i++], ha->addr, ETH_ALEN);
> -	}
> -
> -	for (i = 0; i < enic->uc_count; i++) {
> -		for (j = 0; j < uc_count; j++)
> -			if (ether_addr_equal(enic->uc_addr[i], uc_addr[j]))
> -				break;
> -		if (j == uc_count)
> -			enic_dev_del_addr(enic, enic->uc_addr[i]);
> -	}
> -
> -	for (i = 0; i < uc_count; i++) {
> -		for (j = 0; j < enic->uc_count; j++)
> -			if (ether_addr_equal(uc_addr[i], enic->uc_addr[j]))
> -				break;
> -		if (j == enic->uc_count)
> -			enic_dev_add_addr(enic, uc_addr[i]);
> -	}
> -
> -	/* Save the list to compare against next time
> -	 */
> -
> -	for (i = 0; i < uc_count; i++)
> -		memcpy(enic->uc_addr[i], uc_addr[i], ETH_ALEN);
> -
> -	enic->uc_count = uc_count;
> -}
> -
> /* netif_tx_lock held, BHs disabled */
> static void enic_set_rx_mode(struct net_device *netdev)
> {
> @@ -812,9 +769,9 @@ static void enic_set_rx_mode(struct net_device *netdev)
> 	}
>
> 	if (!promisc) {
> -		enic_update_unicast_addr_list(enic);
> +		__dev_uc_sync(netdev, enic_uc_sync, enic_uc_unsync);
> 		if (!allmulti)
> -			enic_update_multicast_addr_list(enic);
> +			__dev_mc_sync(netdev, enic_mc_sync, enic_mc_unsync);
> 	}
> }
>
> diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.c b/drivers/net/ethernet/cisco/enic/vnic_dev.c
> index 69dd925..e86a45c 100644
> --- a/drivers/net/ethernet/cisco/enic/vnic_dev.c
> +++ b/drivers/net/ethernet/cisco/enic/vnic_dev.c
> @@ -657,7 +657,7 @@ int vnic_dev_packet_filter(struct vnic_dev *vdev, int directed, int multicast,
> 	return err;
> }
>
> -int vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
> +int vnic_dev_add_addr(struct vnic_dev *vdev, const u8 *addr)
> {
> 	u64 a0 = 0, a1 = 0;
> 	int wait = 1000;
> @@ -674,7 +674,7 @@ int vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
> 	return err;
> }
>
> -int vnic_dev_del_addr(struct vnic_dev *vdev, u8 *addr)
> +int vnic_dev_del_addr(struct vnic_dev *vdev, const u8 *addr)
> {
> 	u64 a0 = 0, a1 = 0;
> 	int wait = 1000;
> diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.h b/drivers/net/ethernet/cisco/enic/vnic_dev.h
> index e670029..1f3b301 100644
> --- a/drivers/net/ethernet/cisco/enic/vnic_dev.h
> +++ b/drivers/net/ethernet/cisco/enic/vnic_dev.h
> @@ -95,8 +95,8 @@ int vnic_dev_stats_dump(struct vnic_dev *vdev, struct vnic_stats **stats);
> int vnic_dev_hang_notify(struct vnic_dev *vdev);
> int vnic_dev_packet_filter(struct vnic_dev *vdev, int directed, int multicast,
> 	int broadcast, int promisc, int allmulti);
> -int vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr);
> -int vnic_dev_del_addr(struct vnic_dev *vdev, u8 *addr);
> +int vnic_dev_add_addr(struct vnic_dev *vdev, const u8 *addr);
> +int vnic_dev_del_addr(struct vnic_dev *vdev, const u8 *addr);
> int vnic_dev_get_mac_addr(struct vnic_dev *vdev, u8 *mac_addr);
> int vnic_dev_notify_set(struct vnic_dev *vdev, u16 intr);
> int vnic_dev_notify_unset(struct vnic_dev *vdev);
>
>

Re: [net-next PATCH 0/2] Provide common means for device address sync

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Wed, 28 May 2014 18:44:41 -0700

> The following series implements a means for synchronizing both unicast and
> multicast addresses on a device interface.  The code is based on the original
> implementation of dev_uc_sync that was available for syncing a VLAN to the
> lower dev.
> 
> The original reason for coming up for this patch is a driver that is still in
> the early stages of development.  The nearest driver I could find that
> appeared to have the same limitations as the driver I was working on was the
> Cisco enic driver.  For this reason I chose it as the first driver to make use
> of this interface publicly.
> 
> However, I do not have a Cisco enic interface so I have only been able to
> compile test any changes made to the driver.  I tried to keep this change as
> simple as possible to avoid any issues.  Any help with testing would be
> greatly appreciated.

Series applied, thanks Alexander.