* [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 13:57 ` Eric Dumazet
2014-05-17 20:33 ` Ben Hutchings
2014-05-12 7:43 ` [PATCH net-next 2/9] net/mlx4_en: User prio mapping gets corrupted when changing number of channels Amir Vadai
` (7 subsequent siblings)
8 siblings, 2 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Yuval Atias
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly
2014-05-12 7:43 ` [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly Amir Vadai
@ 2014-05-12 13:57 ` Eric Dumazet
2014-05-13 10:17 ` Amir Vadai
2014-05-17 20:33 ` Ben Hutchings
1 sibling, 1 reply; 15+ messages in thread
From: Eric Dumazet @ 2014-05-12 13:57 UTC (permalink / raw)
To: Amir Vadai
Cc: David S. Miller, netdev, Yevgeny Petrilin, Or Gerlitz,
Yuval Atias
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly
2014-05-12 13:57 ` Eric Dumazet
@ 2014-05-13 10:17 ` Amir Vadai
0 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-13 10:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, netdev, Yevgeny Petrilin, Or Gerlitz,
Yuval Atias
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
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly
2014-05-12 7:43 ` [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly Amir Vadai
2014-05-12 13:57 ` Eric Dumazet
@ 2014-05-17 20:33 ` Ben Hutchings
2014-05-19 11:33 ` Amir Vadai
1 sibling, 1 reply; 15+ messages in thread
From: Ben Hutchings @ 2014-05-17 20:33 UTC (permalink / raw)
To: Amir Vadai
Cc: David S. Miller, netdev, Yevgeny Petrilin, Or Gerlitz,
Yuval Atias
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly
2014-05-17 20:33 ` Ben Hutchings
@ 2014-05-19 11:33 ` Amir Vadai
2014-05-26 17:20 ` Ben Hutchings
0 siblings, 1 reply; 15+ messages in thread
From: Amir Vadai @ 2014-05-19 11:33 UTC (permalink / raw)
To: Ben Hutchings
Cc: David S. Miller, netdev, Yevgeny Petrilin, Or Gerlitz,
Yuval Atias
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.
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly
2014-05-19 11:33 ` Amir Vadai
@ 2014-05-26 17:20 ` Ben Hutchings
0 siblings, 0 replies; 15+ messages in thread
From: Ben Hutchings @ 2014-05-26 17:20 UTC (permalink / raw)
To: Amir Vadai
Cc: David S. Miller, netdev, Yevgeny Petrilin, Or Gerlitz,
Yuval Atias
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next 2/9] net/mlx4_en: User prio mapping gets corrupted when changing number of channels
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 3/9] net/mlx4_en: Fix errors in MAC address changing when port is down Amir Vadai
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Ido Shamay
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 3/9] net/mlx4_en: Fix errors in MAC address changing when port is down
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 1/9] net/mlx4_core: Enforce irq affinity changes immediatly Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 2/9] net/mlx4_en: User prio mapping gets corrupted when changing number of channels Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 4/9] net/mlx4_core: Fix smatch error - possible access to a null variable Amir Vadai
` (5 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Shani Michaelli
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 4/9] net/mlx4_core: Fix smatch error - possible access to a null variable
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
` (2 preceding siblings ...)
2014-05-12 7:43 ` [PATCH net-next 3/9] net/mlx4_en: Fix errors in MAC address changing when port is down Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 5/9] net/mlx4_core: Removed unnecessary bit operation condition Amir Vadai
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Eyal Perry
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 5/9] net/mlx4_core: Removed unnecessary bit operation condition
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
` (3 preceding siblings ...)
2014-05-12 7:43 ` [PATCH net-next 4/9] net/mlx4_core: Fix smatch error - possible access to a null variable Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 6/9] net/mlx4_en: Protect MAC address modification with the state_lock mutex Amir Vadai
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Eyal Perry
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 6/9] net/mlx4_en: Protect MAC address modification with the state_lock mutex
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
` (4 preceding siblings ...)
2014-05-12 7:43 ` [PATCH net-next 5/9] net/mlx4_core: Removed unnecessary bit operation condition Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 7/9] net/mlx4_en: Fix mac_hash database inconsistency Amir Vadai
` (2 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Shani Michaelli
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 7/9] net/mlx4_en: Fix mac_hash database inconsistency
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
` (5 preceding siblings ...)
2014-05-12 7:43 ` [PATCH net-next 6/9] net/mlx4_en: Protect MAC address modification with the state_lock mutex Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 8/9] net/mlx4_en: Using positive error value for unsigned Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 9/9] net/mlx4_core: Fix inaccurate return value of mlx4_flow_attach() Amir Vadai
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Noa Osherovich
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 8/9] net/mlx4_en: Using positive error value for unsigned
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
` (6 preceding siblings ...)
2014-05-12 7:43 ` [PATCH net-next 7/9] net/mlx4_en: Fix mac_hash database inconsistency Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
2014-05-12 7:43 ` [PATCH net-next 9/9] net/mlx4_core: Fix inaccurate return value of mlx4_flow_attach() Amir Vadai
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Eyal Perry
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH net-next 9/9] net/mlx4_core: Fix inaccurate return value of mlx4_flow_attach()
2014-05-12 7:43 [PATCH net-next 0/9] Mellanox driver update 2014-05-12 Amir Vadai
` (7 preceding siblings ...)
2014-05-12 7:43 ` [PATCH net-next 8/9] net/mlx4_en: Using positive error value for unsigned Amir Vadai
@ 2014-05-12 7:43 ` Amir Vadai
8 siblings, 0 replies; 15+ messages in thread
From: Amir Vadai @ 2014-05-12 7:43 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Amir Vadai, Yevgeny Petrilin, Or Gerlitz, Eyal Perry
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
^ permalink raw reply related [flat|nested] 15+ messages in thread