[PATCH net-next 0/9] Mellanox driver update 2014-05-12

[PATCH net-next 0/9] Mellanox driver update 2014-05-12

[Amir Vadai]

Hi,

This patchset introduce some small bug fixes:
Eyal fixed some compilation and syntactic checkers warnings. Ido fixed a
coruption in user priority mapping when changing number of channels. Noa fixed
a bug in flow steering table when using a bond device. Shani fixed some other
problems when modifying MAC address. Yuval fixed a problem when changing IRQ
affinity during high traffic - IRQ changes got effective only after the first
pause in traffic.

This patchset was tested and applied over commit 93dccc5: "mdio_bus: fix
devm_mdiobus_alloc_size export"

Thanks,
Amir

Eyal Perry (4):
  net/mlx4_core: Fix smatch error - possible access to a null variable
  net/mlx4_core: Removed unnecessary bit operation condition
  net/mlx4_en: Using positive error value for unsigned
  net/mlx4_core: Fix inaccurate return value of mlx4_flow_attach()

Ido Shamay (1):
  net/mlx4_en: User prio mapping gets corrupted when changing number of
    channels

Noa Osherovich (1):
  net/mlx4_en: Fix mac_hash database inconsistency

Shani Michaelli (2):
  net/mlx4_en: Fix errors in MAC address changing when port is down
  net/mlx4_en: Protect MAC address modification with the state_lock
    mutex

Yuval Atias (1):
  net/mlx4_core: Enforce irq affinity changes immediatly

 drivers/net/ethernet/mellanox/mlx4/cmd.c        | 14 ++++++
 drivers/net/ethernet/mellanox/mlx4/cq.c         |  3 ++
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  3 +-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  | 29 +++++++-----
 drivers/net/ethernet/mellanox/mlx4/en_rx.c      | 11 ++++-
 drivers/net/ethernet/mellanox/mlx4/en_tx.c      |  6 +++
 drivers/net/ethernet/mellanox/mlx4/eq.c         | 62 +++++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx4/main.c       |  9 ++--
 drivers/net/ethernet/mellanox/mlx4/mcg.c        |  2 +-
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  2 +-
 include/linux/mlx4/device.h                     |  2 +
 11 files changed, 121 insertions(+), 22 deletions(-)

-- 
1.8.3.4

[PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly

[Amir Vadai]

From: Yuval Atias <yuvala@mellanox.com>

During heavy traffic, napi is constatntly polling the complition queue
and no interrupt is fired. Because of that, changes to irq affinity are
ignored until traffic is stopped and resumed.

By registering to the irq notifier mechanism, and forcing interrupt when
affinity is changed, irq affinity changes will be immediatly enforced.

Signed-off-by: Yuval Atias <yuvala@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/cq.c    |  3 ++
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 11 +++++-
 drivers/net/ethernet/mellanox/mlx4/en_tx.c |  6 +++
 drivers/net/ethernet/mellanox/mlx4/eq.c    | 62 ++++++++++++++++++++++++++++++
 include/linux/mlx4/device.h                |  2 +
 5 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cq.c b/drivers/net/ethernet/mellanox/mlx4/cq.c
index 0487121..ed08f76 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cq.c
@@ -293,6 +293,9 @@ int mlx4_cq_alloc(struct mlx4_dev *dev, int nent,
 	atomic_set(&cq->refcount, 1);
 	init_completion(&cq->free);
 
+	cq->irq = priv->eq_table.eq[cq->vector].irq;
+	cq->irq_affinity_change = 0;
+
 	return 0;
 
 err_radix:
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index a151245..265ac03 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -895,10 +895,17 @@ int mlx4_en_poll_rx_cq(struct napi_struct *napi, int budget)
 	mlx4_en_cq_unlock_napi(cq);
 
 	/* If we used up all the quota - we're probably not done yet... */
-	if (done == budget)
+	if (done == budget) {
 		INC_PERF_COUNTER(priv->pstats.napi_quota);
-	else {
+		if (unlikely(cq->mcq.irq_affinity_change)) {
+			cq->mcq.irq_affinity_change = 0;
+			napi_complete(napi);
+			mlx4_en_arm_cq(priv, cq);
+			return 0;
+		}
+	} else {
 		/* Done for now */
+		cq->mcq.irq_affinity_change = 0;
 		napi_complete(napi);
 		mlx4_en_arm_cq(priv, cq);
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 89585c6..81b7571 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -474,9 +474,15 @@ int mlx4_en_poll_tx_cq(struct napi_struct *napi, int budget)
 	/* If we used up all the quota - we're probably not done yet... */
 	if (done < budget) {
 		/* Done for now */
+		cq->mcq.irq_affinity_change = 0;
 		napi_complete(napi);
 		mlx4_en_arm_cq(priv, cq);
 		return done;
+	} else if (unlikely(cq->mcq.irq_affinity_change)) {
+		cq->mcq.irq_affinity_change = 0;
+		napi_complete(napi);
+		mlx4_en_arm_cq(priv, cq);
+		return 0;
 	}
 	return budget;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 6c088bc..947364d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -53,6 +53,11 @@ enum {
 	MLX4_EQ_ENTRY_SIZE	= 0x20
 };
 
+struct mlx4_irq_notify {
+	void *arg;
+	struct irq_affinity_notify notify;
+};
+
 #define MLX4_EQ_STATUS_OK	   ( 0 << 28)
 #define MLX4_EQ_STATUS_WRITE_FAIL  (10 << 28)
 #define MLX4_EQ_OWNER_SW	   ( 0 << 24)
@@ -1083,6 +1088,57 @@ static void mlx4_unmap_clr_int(struct mlx4_dev *dev)
 	iounmap(priv->clr_base);
 }
 
+static void mlx4_irq_notifier_notify(struct irq_affinity_notify *notify,
+				     const cpumask_t *mask)
+{
+	struct mlx4_irq_notify *n = container_of(notify,
+						 struct mlx4_irq_notify,
+						 notify);
+	struct mlx4_priv *priv = (struct mlx4_priv *)n->arg;
+	struct radix_tree_iter iter;
+	void **slot;
+
+	radix_tree_for_each_slot(slot, &priv->cq_table.tree, &iter, 0) {
+		struct mlx4_cq *cq = (struct mlx4_cq *)(*slot);
+
+		if (cq->irq == notify->irq)
+			cq->irq_affinity_change = 1;
+	}
+}
+
+static void mlx4_release_irq_notifier(struct kref *ref)
+{
+	struct mlx4_irq_notify *n = container_of(ref, struct mlx4_irq_notify,
+						 notify.kref);
+	kfree(n);
+}
+
+static void mlx4_assign_irq_notifier(struct mlx4_priv *priv,
+				     struct mlx4_dev *dev, int irq)
+{
+	struct mlx4_irq_notify *irq_notifier = NULL;
+	int err = 0;
+
+	irq_notifier = kzalloc(sizeof(*irq_notifier), GFP_KERNEL);
+	if (!irq_notifier) {
+		mlx4_warn(dev, "Failed to allocate irq notifier. irq %d\n",
+			  irq);
+		return;
+	}
+
+	irq_notifier->notify.irq = irq;
+	irq_notifier->notify.notify = mlx4_irq_notifier_notify;
+	irq_notifier->notify.release = mlx4_release_irq_notifier;
+	irq_notifier->arg = priv;
+	err = irq_set_affinity_notifier(irq, &irq_notifier->notify);
+	if (err) {
+		kfree(irq_notifier);
+		irq_notifier = NULL;
+		mlx4_warn(dev, "Failed to set irq notifier. irq %d\n", irq);
+	}
+}
+
+
 int mlx4_alloc_eq_table(struct mlx4_dev *dev)
 {
 	struct mlx4_priv *priv = mlx4_priv(dev);
@@ -1353,6 +1409,9 @@ int mlx4_assign_eq(struct mlx4_dev *dev, char *name, struct cpu_rmap *rmap,
 				continue;
 				/*we dont want to break here*/
 			}
+			mlx4_assign_irq_notifier(priv, dev,
+						 priv->eq_table.eq[vec].irq);
+
 			eq_set_ci(&priv->eq_table.eq[vec], 1);
 		}
 	}
@@ -1379,6 +1438,9 @@ void mlx4_release_eq(struct mlx4_dev *dev, int vec)
 		  Belonging to a legacy EQ*/
 		mutex_lock(&priv->msix_ctl.pool_lock);
 		if (priv->msix_ctl.pool_bm & 1ULL << i) {
+			irq_set_affinity_notifier(
+				priv->eq_table.eq[vec].irq,
+				NULL);
 			free_irq(priv->eq_table.eq[vec].irq,
 				 &priv->eq_table.eq[vec]);
 			priv->msix_ctl.pool_bm &= ~(1ULL << i);
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index ba87bd2..74f5aa8 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -586,6 +586,8 @@ struct mlx4_cq {
 
 	atomic_t		refcount;
 	struct completion	free;
+	u16                     irq;
+	bool                    irq_affinity_change;
 };
 
 struct mlx4_qp {
-- 
1.8.3.4

[PATCH net-next 2/9] net/mlx4_en: User prio mapping gets corrupted when changing number of channels

[Amir Vadai]

From: Ido Shamay <idos@mellanox.com>

When using ethtool set_channels, mlx4_en_setup_tc is always called, even
when it was not configured. Fixed code to call mlx4_en_setup_tc() only if
needed.

Signed-off-by: Ido Shamay <idos@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index c373604..a72d99f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1151,7 +1151,8 @@ static int mlx4_en_set_channels(struct net_device *dev,
 	netif_set_real_num_tx_queues(dev, priv->tx_ring_num);
 	netif_set_real_num_rx_queues(dev, priv->rx_ring_num);
 
-	mlx4_en_setup_tc(dev, MLX4_EN_NUM_UP);
+	if (dev->num_tc)
+		mlx4_en_setup_tc(dev, MLX4_EN_NUM_UP);
 
 	en_warn(priv, "Using %d TX rings\n", priv->tx_ring_num);
 	en_warn(priv, "Using %d RX rings\n", priv->rx_ring_num);
-- 
1.8.3.4

[PATCH net-next 3/9] net/mlx4_en: Fix errors in MAC address changing when port is down

[Amir Vadai]

From: Shani Michaelli <shanim@mellanox.com>

This patch fix an issue that happen when changing the MAC address when
the port is down, described as follows:
1. Set the port down.
2. Change the MAC address - mlx4_en_set_mac() will change dev->dev_addr.
3. Set the port up - will result in mlx4_en_do_uc_filter that will
   remove the prev_mac entry from the mac_hash db.
4. Changing the MAC address again will eventually trigger the call to
   mlx4_en_replace_mac() in order to replace prev_mac with dev_addr but
   the prev_mac entry is already not exist in the mac_hash db therefore
   the operation fails.

The fix is to set the prev_mac with the new MAC address so in step 3
above, after setting the port up mlx4_en_get_qp() is updating the
mac_hash with the entry of dev_addr which is equal to prev_mac.
Therefore in step 4, when calling mlx4_en_replace_mac, the entry related
to prev_mac exist in mac_hash and the replace operation succeed.


Reviewed-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Shani Michaeli <shanim@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index fba3c8e..eea7868 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -770,11 +770,12 @@ static int mlx4_en_do_set_mac(struct mlx4_en_priv *priv)
 					  priv->dev->dev_addr, priv->prev_mac);
 		if (err)
 			en_err(priv, "Failed changing HW MAC address\n");
-		memcpy(priv->prev_mac, priv->dev->dev_addr,
-		       sizeof(priv->prev_mac));
 	} else
 		en_dbg(HW, priv, "Port is down while registering mac, exiting...\n");
 
+	memcpy(priv->prev_mac, priv->dev->dev_addr,
+	       sizeof(priv->prev_mac));
+
 	return err;
 }
 
-- 
1.8.3.4

[PATCH net-next 4/9] net/mlx4_core: Fix smatch error - possible access to a null variable

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

Fix the "error: we previously assumed 'out_param' could be null" found
by smatch semantic checker on:
drivers/net/ethernet/mellanox/mlx4/cmd.c:506 mlx4_cmd_poll()
drivers/net/ethernet/mellanox/mlx4/cmd.c:578 mlx4_cmd_wait()


Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/cmd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
index 2420103..357dcb0 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
@@ -473,6 +473,13 @@ static int mlx4_cmd_poll(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
 		goto out;
 	}
 
+	if (out_is_imm && !out_param) {
+		mlx4_err(dev, "response expected while output mailbox is NULL for command 0x%x\n",
+			 op);
+		err = -EINVAL;
+		goto out;
+	}
+
 	err = mlx4_cmd_post(dev, in_param, out_param ? *out_param : 0,
 			    in_modifier, op_modifier, op, CMD_POLL_TOKEN, 0);
 	if (err)
@@ -551,6 +558,13 @@ static int mlx4_cmd_wait(struct mlx4_dev *dev, u64 in_param, u64 *out_param,
 	cmd->free_head = context->next;
 	spin_unlock(&cmd->context_lock);
 
+	if (out_is_imm && !out_param) {
+		mlx4_err(dev, "response expected while output mailbox is NULL for command 0x%x\n",
+			 op);
+		err = -EINVAL;
+		goto out;
+	}
+
 	init_completion(&context->done);
 
 	mlx4_cmd_post(dev, in_param, out_param ? *out_param : 0,
-- 
1.8.3.4

[PATCH net-next 5/9] net/mlx4_core: Removed unnecessary bit operation condition

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

Fix the "warn: suspicious bitop condition" made by the smatch semantic
checker on:
drivers/net/ethernet/mellanox/mlx4/main.c:509 mlx4_slave_cap()


Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index df2c1fb..e74847d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -104,8 +104,6 @@ module_param(enable_64b_cqe_eqe, bool, 0444);
 MODULE_PARM_DESC(enable_64b_cqe_eqe,
 		 "Enable 64 byte CQEs/EQEs when the FW supports this (default: True)");
 
-#define HCA_GLOBAL_CAP_MASK            0
-
 #define PF_CONTEXT_BEHAVIOUR_MASK	MLX4_FUNC_CAP_64B_EQE_CQE
 
 static char mlx4_version[] =
@@ -582,9 +580,10 @@ static int mlx4_slave_cap(struct mlx4_dev *dev)
 		return err;
 	}
 
-	/*fail if the hca has an unknown capability */
-	if ((hca_param.global_caps | HCA_GLOBAL_CAP_MASK) !=
-	    HCA_GLOBAL_CAP_MASK) {
+	/* fail if the hca has an unknown global capability
+	 * at this time global_caps should be always zeroed
+	 */
+	if (hca_param.global_caps) {
 		mlx4_err(dev, "Unknown hca global capabilities\n");
 		return -ENOSYS;
 	}
-- 
1.8.3.4

[PATCH net-next 6/9] net/mlx4_en: Protect MAC address modification with the state_lock mutex

[Amir Vadai]

From: Shani Michaelli <shanim@mellanox.com>

This Patches solves an issue that could raise when modifying the
device's MAC. It occurs due to a simultaneous access to priv->mac_hash
from two contexts. The buggy scenario described below:
Context 1: copy the new mac address to the dev->dev_addr field.
Context 2: mlx4_en_do_uc_filter removes prev_mac entry from the mac_hash
           db since it is not in dev->uc and not equal to dev->dev_addr.
Context 1: mlx4_en_do_set_mac() calls mlx4_en_replace_mac() to replace
           prev_mac with dev_addr but it fails to update the mac_hash db
           since it no longer contains prev_mac, therefore it returns
           with an error.

The fix is to prevent mlx4_en_do_uc_filter from being executed by both
of the context 1 calls described above, This is done by putting them
both under the mdev->state_lock lock, it will solve this issue since
mlx4_en_do_uc_filter is already protected by the mdev->state_lock.


Reviewed-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Shani Michaeli <shanim@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index eea7868..9090643 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -789,9 +789,8 @@ static int mlx4_en_set_mac(struct net_device *dev, void *addr)
 	if (!is_valid_ether_addr(saddr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	memcpy(dev->dev_addr, saddr->sa_data, ETH_ALEN);
-
 	mutex_lock(&mdev->state_lock);
+	memcpy(dev->dev_addr, saddr->sa_data, ETH_ALEN);
 	err = mlx4_en_do_set_mac(priv);
 	mutex_unlock(&mdev->state_lock);
 
-- 
1.8.3.4

[PATCH net-next 7/9] net/mlx4_en: Fix mac_hash database inconsistency

[Amir Vadai]

From: Noa Osherovich <noaos@mellanox.com>

When modifying dev->dev_addr by external kernel code (e.g. bonding
driver in ALB mode) The following issue may raise:
1) Kernel module updates dev->addr to a new MAC address
2) mlx4_en_do_set_rx_mode workqueue runs which calls -
   mlx4_en_do_uc_filter. mlx4_en_do_uc_filter goes through the mac_hash
   and removes the previous MAC address from the mac_hash.
3) Then mlx4_en_set_mac runs and calls mlx4_en_replace_mac in order to
   replace previous mac with the new one, but since the mac_hash does
   not contains the entry related to the prev_mac this operation fail.

The solution is to use a private variable to store and compare current
MAC address as dev->dev_addr can be accessed directly by any other
kernel code. This way a direct access to dev->dev_addr would not result
in mac_hash database inconsistency.
- priv->prev_mac was renamed to priv->current_mac.
- comparison in mlx4_en_do_uc_filter is to current_mac instead
  of dev->dev_addr.
- added a new_mac parameter to mlx4_en_do_set_mac, and use it when
  when updating mac_hash database.


Reviewed-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Noa Osherovich <noaos@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 23 ++++++++++++++---------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   |  2 +-
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 9090643..4d38a2a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -760,21 +760,22 @@ static int mlx4_en_replace_mac(struct mlx4_en_priv *priv, int qpn,
 	return __mlx4_replace_mac(dev, priv->port, qpn, new_mac_u64);
 }
 
-static int mlx4_en_do_set_mac(struct mlx4_en_priv *priv)
+static int mlx4_en_do_set_mac(struct mlx4_en_priv *priv,
+			      unsigned char new_mac[ETH_ALEN + 2])
 {
 	int err = 0;
 
 	if (priv->port_up) {
 		/* Remove old MAC and insert the new one */
 		err = mlx4_en_replace_mac(priv, priv->base_qpn,
-					  priv->dev->dev_addr, priv->prev_mac);
+					  new_mac, priv->current_mac);
 		if (err)
 			en_err(priv, "Failed changing HW MAC address\n");
 	} else
 		en_dbg(HW, priv, "Port is down while registering mac, exiting...\n");
 
-	memcpy(priv->prev_mac, priv->dev->dev_addr,
-	       sizeof(priv->prev_mac));
+	if (!err)
+		memcpy(priv->current_mac, new_mac, sizeof(priv->current_mac));
 
 	return err;
 }
@@ -784,14 +785,17 @@ static int mlx4_en_set_mac(struct net_device *dev, void *addr)
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	struct mlx4_en_dev *mdev = priv->mdev;
 	struct sockaddr *saddr = addr;
+	unsigned char new_mac[ETH_ALEN + 2];
 	int err;
 
 	if (!is_valid_ether_addr(saddr->sa_data))
 		return -EADDRNOTAVAIL;
 
 	mutex_lock(&mdev->state_lock);
-	memcpy(dev->dev_addr, saddr->sa_data, ETH_ALEN);
-	err = mlx4_en_do_set_mac(priv);
+	memcpy(new_mac, saddr->sa_data, ETH_ALEN);
+	err = mlx4_en_do_set_mac(priv, new_mac);
+	if (!err)
+		memcpy(dev->dev_addr, saddr->sa_data, ETH_ALEN);
 	mutex_unlock(&mdev->state_lock);
 
 	return err;
@@ -1166,7 +1170,8 @@ static void mlx4_en_do_uc_filter(struct mlx4_en_priv *priv,
 			}
 
 			/* MAC address of the port is not in uc list */
-			if (ether_addr_equal_64bits(entry->mac, dev->dev_addr))
+			if (ether_addr_equal_64bits(entry->mac,
+						    priv->current_mac))
 				found = true;
 
 			if (!found) {
@@ -1476,7 +1481,7 @@ static void mlx4_en_do_get_stats(struct work_struct *work)
 		queue_delayed_work(mdev->workqueue, &priv->stats_task, STATS_DELAY);
 	}
 	if (mdev->mac_removed[MLX4_MAX_PORTS + 1 - priv->port]) {
-		mlx4_en_do_set_mac(priv);
+		mlx4_en_do_set_mac(priv, priv->current_mac);
 		mdev->mac_removed[MLX4_MAX_PORTS + 1 - priv->port] = 0;
 	}
 	mutex_unlock(&mdev->state_lock);
@@ -2501,7 +2506,7 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 		}
 	}
 
-	memcpy(priv->prev_mac, dev->dev_addr, sizeof(priv->prev_mac));
+	memcpy(priv->current_mac, dev->dev_addr, sizeof(priv->current_mac));
 
 	priv->stride = roundup_pow_of_two(sizeof(struct mlx4_en_rx_desc) +
 					  DS_SIZE * MLX4_EN_MAX_RX_FRAGS);
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index b5db1bf..88d5cf6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -531,7 +531,7 @@ struct mlx4_en_priv {
 	int registered;
 	int allocated;
 	int stride;
-	unsigned char prev_mac[ETH_ALEN + 2];
+	unsigned char current_mac[ETH_ALEN + 2];
 	int mac_index;
 	unsigned max_mtu;
 	int base_qpn;
-- 
1.8.3.4

[PATCH net-next 8/9] net/mlx4_en: Using positive error value for unsigned

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

Using a positive value for error: MLX4_NET_TRANS_RULE_NUM instead
of -EPROTONOSUPPORT, to remove compilation warning.


Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 4d38a2a..5bb7eda 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -130,7 +130,7 @@ static enum mlx4_net_trans_rule_id mlx4_ip_proto_to_trans_rule_id(u8 ip_proto)
 	case IPPROTO_TCP:
 		return MLX4_NET_TRANS_RULE_ID_TCP;
 	default:
-		return -EPROTONOSUPPORT;
+		return MLX4_NET_TRANS_RULE_NUM;
 	}
 };
 
@@ -177,7 +177,7 @@ static void mlx4_en_filter_work(struct work_struct *work)
 	int rc;
 	__be64 mac_mask = cpu_to_be64(MLX4_MAC_MASK << 16);
 
-	if (spec_tcp_udp.id < 0) {
+	if (spec_tcp_udp.id >= MLX4_NET_TRANS_RULE_NUM) {
 		en_warn(priv, "RFS: ignoring unsupported ip protocol (%d)\n",
 			filter->ip_proto);
 		goto ignore;
-- 
1.8.3.4

[PATCH net-next 9/9] net/mlx4_core: Fix inaccurate return value of mlx4_flow_attach()

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

Adopt the "info: why not propagate 'ret' from parse_trans_rule()..."
suggestion made by the smatch semantic checker on:
drivers/net/ethernet/mellanox/mlx4/mcg.c:867 mlx4_flow_attach()


Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/mcg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/mcg.c b/drivers/net/ethernet/mellanox/mlx4/mcg.c
index 7c6eba6..4c36def 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mcg.c
+++ b/drivers/net/ethernet/mellanox/mlx4/mcg.c
@@ -897,7 +897,7 @@ int mlx4_flow_attach(struct mlx4_dev *dev,
 		ret = parse_trans_rule(dev, cur, mailbox->buf + size);
 		if (ret < 0) {
 			mlx4_free_cmd_mailbox(dev, mailbox);
-			return -EINVAL;
+			return ret;
 		}
 		size += ret;
 	}
-- 
1.8.3.4

Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly

[Eric Dumazet]

On Mon, 2014-05-12 at 10:43 +0300, Amir Vadai wrote:
> From: Yuval Atias <yuvala@mellanox.com>
> 
> During heavy traffic, napi is constatntly polling the complition queue
> and no interrupt is fired. Because of that, changes to irq affinity are
> ignored until traffic is stopped and resumed.
> 
> By registering to the irq notifier mechanism, and forcing interrupt when
> affinity is changed, irq affinity changes will be immediatly enforced.
> 
> Signed-off-by: Yuval Atias <yuvala@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---

Interesting. Its a bit sad it has to be in fast path...

> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index 89585c6..81b7571 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -474,9 +474,15 @@ int mlx4_en_poll_tx_cq(struct napi_struct *napi, int budget)
>  	/* If we used up all the quota - we're probably not done yet... */
>  	if (done < budget) {
>  		/* Done for now */
> +		cq->mcq.irq_affinity_change = 0;


>  		napi_complete(napi);
>  		mlx4_en_arm_cq(priv, cq);
>  		return done;
> +	} else if (unlikely(cq->mcq.irq_affinity_change)) {
> +		cq->mcq.irq_affinity_change = 0;
> +		napi_complete(napi);
> +		mlx4_en_arm_cq(priv, cq);
> +		return 0;
>  	}
>  	return budget;


>  			priv->msix_ctl.pool_bm &= ~(1ULL << i);
> diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
> index ba87bd2..74f5aa8 100644
> --- a/include/linux/mlx4/device.h
> +++ b/include/linux/mlx4/device.h
> @@ -586,6 +586,8 @@ struct mlx4_cq {
>  
>  	atomic_t		refcount;
>  	struct completion	free;
> +	u16                     irq;
> +	bool                    irq_affinity_change;
>  };
>  
>  struct mlx4_qp {

But could you place irq_affinity_change in another cache line ?

It seems there is a 32bit hole between cons_index and set_ci_db

(Both fields can be moved)

Thanks

Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly

[Amir Vadai]

On 5/12/2014 4:57 PM, Eric Dumazet wrote:
> On Mon, 2014-05-12 at 10:43 +0300, Amir Vadai wrote:
>> From: Yuval Atias <yuvala@mellanox.com>
>>
>> During heavy traffic, napi is constatntly polling the complition queue
>> and no interrupt is fired. Because of that, changes to irq affinity are
>> ignored until traffic is stopped and resumed.
>>
>> By registering to the irq notifier mechanism, and forcing interrupt when
>> affinity is changed, irq affinity changes will be immediatly enforced.
>>
>> Signed-off-by: Yuval Atias <yuvala@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>
> Interesting. Its a bit sad it has to be in fast path...
Right.
The other option we thought of - was to add some sort of 'restart' to 
the NAPI, to enable the notifier callback to stop any current running 
NAPI loop, and make it start again on the right CPU - this looks very 
complicated and adding one if doesn't seem to be so bad. So we chose 
that option.

>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index 89585c6..81b7571 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -474,9 +474,15 @@ int mlx4_en_poll_tx_cq(struct napi_struct *napi, int budget)
>>   	/* If we used up all the quota - we're probably not done yet... */
>>   	if (done < budget) {
>>   		/* Done for now */
>> +		cq->mcq.irq_affinity_change = 0;
>
>
>>   		napi_complete(napi);
>>   		mlx4_en_arm_cq(priv, cq);
>>   		return done;
>> +	} else if (unlikely(cq->mcq.irq_affinity_change)) {
>> +		cq->mcq.irq_affinity_change = 0;
>> +		napi_complete(napi);
>> +		mlx4_en_arm_cq(priv, cq);
>> +		return 0;
>>   	}
>>   	return budget;
>
>
>>   			priv->msix_ctl.pool_bm &= ~(1ULL << i);
>> diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
>> index ba87bd2..74f5aa8 100644
>> --- a/include/linux/mlx4/device.h
>> +++ b/include/linux/mlx4/device.h
>> @@ -586,6 +586,8 @@ struct mlx4_cq {
>>
>>   	atomic_t		refcount;
>>   	struct completion	free;
>> +	u16                     irq;
>> +	bool                    irq_affinity_change;
>>   };
>>
>>   struct mlx4_qp {
>
> But could you place irq_affinity_change in another cache line ?
>
> It seems there is a 32bit hole between cons_index and set_ci_db
>
> (Both fields can be moved)
Done in V1

Thanks,
Amir

>
> Thanks
>
>

Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On Mon, 2014-05-12 at 10:43 +0300, Amir Vadai wrote:
> From: Yuval Atias <yuvala@mellanox.com>
> 
> During heavy traffic, napi is constatntly polling the complition queue
> and no interrupt is fired. Because of that, changes to irq affinity are
> ignored until traffic is stopped and resumed.
> 
> By registering to the irq notifier mechanism, and forcing interrupt when
> affinity is changed, irq affinity changes will be immediatly enforced.
[...]

This somewhat breaks ARFS in your driver, as that depends on IRQ
affinity notification.

It is also not safe to put an IRQ affinity notification function in a
module currently, as the work item for notification doesn't carry a
module reference and could be called after the module is removed.

Ben.

-- 
Ben Hutchings
I'm not a reverse psychological virus.  Please don't copy me into your sig.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly

[Amir Vadai]

On 5/17/2014 11:33 PM, Ben Hutchings wrote:
> On Mon, 2014-05-12 at 10:43 +0300, Amir Vadai wrote:
>> From: Yuval Atias <yuvala@mellanox.com>
>>
>> During heavy traffic, napi is constatntly polling the complition queue
>> and no interrupt is fired. Because of that, changes to irq affinity are
>> ignored until traffic is stopped and resumed.
>>
>> By registering to the irq notifier mechanism, and forcing interrupt when
>> affinity is changed, irq affinity changes will be immediatly enforced.
> [...]
>
> This somewhat breaks ARFS in your driver, as that depends on IRQ
> affinity notification.
Interesting...
I missed the fact that irq_set_affinity_notifier is 'set' and not 'add'.
So, do you have objection if I change the API to have a list of 
notifiers instead of only one?

>
> It is also not safe to put an IRQ affinity notification function in a
> module currently, as the work item for notification doesn't carry a
> module reference and could be called after the module is removed.
I can get a module reference when a notifier is added and put it when 
the notifier is removed.

Thanks,
Amir

>
> Ben.
>

Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

On Mon, 2014-05-19 at 14:33 +0300, Amir Vadai wrote:
> On 5/17/2014 11:33 PM, Ben Hutchings wrote:
> > On Mon, 2014-05-12 at 10:43 +0300, Amir Vadai wrote:
> >> From: Yuval Atias <yuvala@mellanox.com>
> >>
> >> During heavy traffic, napi is constatntly polling the complition queue
> >> and no interrupt is fired. Because of that, changes to irq affinity are
> >> ignored until traffic is stopped and resumed.
> >>
> >> By registering to the irq notifier mechanism, and forcing interrupt when
> >> affinity is changed, irq affinity changes will be immediatly enforced.
> > [...]
> >
> > This somewhat breaks ARFS in your driver, as that depends on IRQ
> > affinity notification.
> Interesting...
> I missed the fact that irq_set_affinity_notifier is 'set' and not 'add'.
> So, do you have objection if I change the API to have a list of 
> notifiers instead of only one?

I don't mind.

> > It is also not safe to put an IRQ affinity notification function in a
> > module currently, as the work item for notification doesn't carry a
> > module reference and could be called after the module is removed.
> I can get a module reference when a notifier is added and put it when 
> the notifier is removed.

Maybe you can drop the reference in the affinity notifier release()
function, though that might still leave a race condition.  But the
module won't be removable until its devices are first removed or at
least brought down (depending on where you add/remove the notifier).

Anyway, I think that the issue you have found is quite general and
should be fixed in the networking core rather than your specific driver.

Ben.

-- 
Ben Hutchings
Humans are not rational beings; they are rationalising beings.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]