[PATCH net-next 0/4] mlx4 misc for 4.16

[PATCH net-next 0/4] mlx4 misc for 4.16

[Tariq Toukan]

Hi Dave,

This patchset contains misc cleanups and improvements
to the mlx4 Core and Eth drivers.

In patches 1 and 2 I reduce and reorder the branches in the RX csum flow.
In patch 3 I align the FMR unmapping flow with the device spec, to allow
  a remapping afterwards.
Patch 4 by Moni changes the default QoS settings so that a pause
  frame stops all traffic regardless of its prio.

Series generated against net-next commit:
836df24a7062 net: hns3: hns3_get_channels() can be static

Thanks,
Tariq.


Moni Shoua (1):
  net/mlx4_en: Change default QoS settings

Tariq Toukan (3):
  net/mlx4_en: RX csum, remove redundant branches and checks
  net/mlx4_en: RX csum, reorder branches
  net/mlx4_core: Cleanup FMR unmapping flow

 drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c |  5 +++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |  7 ++++
 drivers/net/ethernet/mellanox/mlx4/en_rx.c     | 56 +++++++++++++-------------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   |  1 +
 drivers/net/ethernet/mellanox/mlx4/mr.c        | 40 +++++++++---------
 5 files changed, 63 insertions(+), 46 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/4] net/mlx4_en: RX csum, remove redundant branches and checks

[Tariq Toukan]

Do not check IPv6 bit in cqe status if CONFIG_IPV6 is not enabled.
Function check_csum() is reached only with IPv4 or IPv6 set (if enabled),
if IPv6 is not set (or is not enabled) it is redundant to test the
IPv4 bit.

Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 85e28efcda33..680bd3fbaa60 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -617,6 +617,10 @@ static int get_fixed_ipv6_csum(__wsum hw_checksum, struct sk_buff *skb,
 	return 0;
 }
 #endif
+
+/* We reach this function only after checking that any of
+ * the (IPv4 | IPv6) bits are set in cqe->status.
+ */
 static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
 		      netdev_features_t dev_features)
 {
@@ -632,13 +636,11 @@ static int check_csum(struct mlx4_cqe *cqe, struct sk_buff *skb, void *va,
 		hdr += sizeof(struct vlan_hdr);
 	}
 
-	if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4))
-		return get_fixed_ipv4_csum(hw_checksum, skb, hdr);
 #if IS_ENABLED(CONFIG_IPV6)
 	if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV6))
 		return get_fixed_ipv6_csum(hw_checksum, skb, hdr);
 #endif
-	return 0;
+	return get_fixed_ipv4_csum(hw_checksum, skb, hdr);
 }
 
 int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int budget)
@@ -830,7 +832,11 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 			} else {
 				if (priv->flags & MLX4_EN_FLAG_RX_CSUM_NON_TCP_UDP &&
 				    (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4 |
+#if IS_ENABLED(CONFIG_IPV6)
 							       MLX4_CQE_STATUS_IPV6))) {
+#else
+							       0))) {
+#endif
 					if (check_csum(cqe, skb, va, dev->features)) {
 						goto csum_none;
 					} else {
-- 
1.8.3.1

[PATCH net-next 2/4] net/mlx4_en: RX csum, reorder branches

[Tariq Toukan]

Use early goto commands, and save else branches.
This uses less indentations and brackets, making the code
more readable.

Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 46 ++++++++++++++----------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 680bd3fbaa60..5f9dbc9a7f5b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -816,37 +816,33 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 		if (likely(dev->features & NETIF_F_RXCSUM)) {
 			if (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_TCP |
 						      MLX4_CQE_STATUS_UDP)) {
-				if ((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
-				    cqe->checksum == cpu_to_be16(0xffff)) {
-					bool l2_tunnel = (dev->hw_enc_features & NETIF_F_RXCSUM) &&
-						(cqe->vlan_my_qpn & cpu_to_be32(MLX4_CQE_L2_TUNNEL));
-
-					ip_summed = CHECKSUM_UNNECESSARY;
-					hash_type = PKT_HASH_TYPE_L4;
-					if (l2_tunnel)
-						skb->csum_level = 1;
-					ring->csum_ok++;
-				} else {
+				bool l2_tunnel;
+
+				if (!((cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPOK)) &&
+				      cqe->checksum == cpu_to_be16(0xffff)))
 					goto csum_none;
-				}
+
+				l2_tunnel = (dev->hw_enc_features & NETIF_F_RXCSUM) &&
+					(cqe->vlan_my_qpn & cpu_to_be32(MLX4_CQE_L2_TUNNEL));
+				ip_summed = CHECKSUM_UNNECESSARY;
+				hash_type = PKT_HASH_TYPE_L4;
+				if (l2_tunnel)
+					skb->csum_level = 1;
+				ring->csum_ok++;
 			} else {
-				if (priv->flags & MLX4_EN_FLAG_RX_CSUM_NON_TCP_UDP &&
-				    (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4 |
+				if (!(priv->flags & MLX4_EN_FLAG_RX_CSUM_NON_TCP_UDP &&
+				      (cqe->status & cpu_to_be16(MLX4_CQE_STATUS_IPV4 |
 #if IS_ENABLED(CONFIG_IPV6)
-							       MLX4_CQE_STATUS_IPV6))) {
+								 MLX4_CQE_STATUS_IPV6))))
 #else
-							       0))) {
+								 0))))
 #endif
-					if (check_csum(cqe, skb, va, dev->features)) {
-						goto csum_none;
-					} else {
-						ip_summed = CHECKSUM_COMPLETE;
-						hash_type = PKT_HASH_TYPE_L3;
-						ring->csum_complete++;
-					}
-				} else {
 					goto csum_none;
-				}
+				if (check_csum(cqe, skb, va, dev->features))
+					goto csum_none;
+				ip_summed = CHECKSUM_COMPLETE;
+				hash_type = PKT_HASH_TYPE_L3;
+				ring->csum_complete++;
 			}
 		} else {
 csum_none:
-- 
1.8.3.1

[PATCH net-next 3/4] net/mlx4_core: Cleanup FMR unmapping flow

[Tariq Toukan]

Remove redundant and not essential operations in fmr unmap/free.
According to device spec, in FMR unmap it is sufficient to set
ownership bit to SW. This allows remapping afterwards.

Fixes: 8ad11fb6b073 ("IB/mlx4: Implement FMRs")
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Signed-off-by: Moshe Shemesh <moshe@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/mr.c | 40 +++++++++++++++++----------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/mr.c b/drivers/net/ethernet/mellanox/mlx4/mr.c
index c7c0764991c9..2e84f10f59ba 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mr.c
+++ b/drivers/net/ethernet/mellanox/mlx4/mr.c
@@ -1103,30 +1103,16 @@ int mlx4_fmr_enable(struct mlx4_dev *dev, struct mlx4_fmr *fmr)
 void mlx4_fmr_unmap(struct mlx4_dev *dev, struct mlx4_fmr *fmr,
 		    u32 *lkey, u32 *rkey)
 {
-	struct mlx4_cmd_mailbox *mailbox;
-	int err;
-
 	if (!fmr->maps)
 		return;
 
-	fmr->maps = 0;
+	/* To unmap: it is sufficient to take back ownership from HW */
+	*(u8 *)fmr->mpt = MLX4_MPT_STATUS_SW;
 
-	mailbox = mlx4_alloc_cmd_mailbox(dev);
-	if (IS_ERR(mailbox)) {
-		err = PTR_ERR(mailbox);
-		pr_warn("mlx4_ib: mlx4_alloc_cmd_mailbox failed (%d)\n", err);
-		return;
-	}
+	/* Make sure MPT status is visible */
+	wmb();
 
-	err = mlx4_HW2SW_MPT(dev, NULL,
-			     key_to_hw_index(fmr->mr.key) &
-			     (dev->caps.num_mpts - 1));
-	mlx4_free_cmd_mailbox(dev, mailbox);
-	if (err) {
-		pr_warn("mlx4_ib: mlx4_HW2SW_MPT failed (%d)\n", err);
-		return;
-	}
-	fmr->mr.enabled = MLX4_MPT_EN_SW;
+	fmr->maps = 0;
 }
 EXPORT_SYMBOL_GPL(mlx4_fmr_unmap);
 
@@ -1136,6 +1122,22 @@ int mlx4_fmr_free(struct mlx4_dev *dev, struct mlx4_fmr *fmr)
 
 	if (fmr->maps)
 		return -EBUSY;
+	if (fmr->mr.enabled == MLX4_MPT_EN_HW) {
+		/* In case of FMR was enabled and unmapped
+		 * make sure to give ownership of MPT back to HW
+		 * so HW2SW_MPT command will success.
+		 */
+		*(u8 *)fmr->mpt = MLX4_MPT_STATUS_SW;
+		/* Make sure MPT status is visible before changing MPT fields */
+		wmb();
+		fmr->mpt->length = 0;
+		fmr->mpt->start  = 0;
+		/* Make sure MPT data is visible after changing MPT status */
+		wmb();
+		*(u8 *)fmr->mpt = MLX4_MPT_STATUS_HW;
+		/* make sure MPT status is visible */
+		wmb();
+	}
 
 	ret = mlx4_mr_free(dev, &fmr->mr);
 	if (ret)
-- 
1.8.3.1

[PATCH net-next 4/4] net/mlx4_en: Change default QoS settings

[Tariq Toukan]

From: Moni Shoua <monis@mellanox.com>

Change the default mapping between TC and TCG as follows:

Prio     |             TC/TCG
         |      from             to
         |    (set by FW)      (set by SW)
---------+-----------------------------------
0        |      0/0              0/7
1        |      1/0              0/6
2        |      2/0              0/5
3        |      3/0              0/4
4        |      4/0              0/3
5        |      5/0              0/2
6        |      6/0              0/1
7        |      7/0              0/0

These new settings cause that a pause frame for any prio stops
traffic for all prios.

Fixes: 564c274c3df0 ("net/mlx4_en: DCB QoS support")
Signed-off-by: Moni Shoua <monis@mellanox.com>
Signed-off-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c | 5 +++++
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 7 +++++++
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   | 1 +
 3 files changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c b/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c
index 5f41dc92aa68..1a0c3bf86ead 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c
@@ -310,6 +310,7 @@ static int mlx4_en_ets_validate(struct mlx4_en_priv *priv, struct ieee_ets *ets)
 		}
 
 		switch (ets->tc_tsa[i]) {
+		case IEEE_8021QAZ_TSA_VENDOR:
 		case IEEE_8021QAZ_TSA_STRICT:
 			break;
 		case IEEE_8021QAZ_TSA_ETS:
@@ -347,6 +348,10 @@ static int mlx4_en_config_port_scheduler(struct mlx4_en_priv *priv,
 	/* higher TC means higher priority => lower pg */
 	for (i = IEEE_8021QAZ_MAX_TCS - 1; i >= 0; i--) {
 		switch (ets->tc_tsa[i]) {
+		case IEEE_8021QAZ_TSA_VENDOR:
+			pg[i] = MLX4_EN_TC_VENDOR;
+			tc_tx_bw[i] = MLX4_EN_BW_MAX;
+			break;
 		case IEEE_8021QAZ_TSA_STRICT:
 			pg[i] = num_strict++;
 			tc_tx_bw[i] = MLX4_EN_BW_MAX;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 99051a294fa6..21bc17fa3854 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -3336,6 +3336,13 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 	priv->msg_enable = MLX4_EN_MSG_LEVEL;
 #ifdef CONFIG_MLX4_EN_DCB
 	if (!mlx4_is_slave(priv->mdev->dev)) {
+		u8 prio;
+
+		for (prio = 0; prio < IEEE_8021QAZ_MAX_TCS; ++prio) {
+			priv->ets.prio_tc[prio] = prio;
+			priv->ets.tc_tsa[prio]  = IEEE_8021QAZ_TSA_VENDOR;
+		}
+
 		priv->dcbx_cap = DCB_CAP_DCBX_VER_CEE | DCB_CAP_DCBX_HOST |
 			DCB_CAP_DCBX_VER_IEEE;
 		priv->flags |= MLX4_EN_DCB_ENABLED;
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index 2b72677eccd4..7db3d0d9bfce 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -479,6 +479,7 @@ struct mlx4_en_frag_info {
 #define MLX4_EN_BW_MIN 1
 #define MLX4_EN_BW_MAX 100 /* Utilize 100% of the line */
 
+#define MLX4_EN_TC_VENDOR 0
 #define MLX4_EN_TC_ETS 7
 
 enum dcb_pfc_type {
-- 
1.8.3.1

Re: [PATCH net-next 0/4] mlx4 misc for 4.16

[David Miller]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Thu, 28 Dec 2017 16:26:07 +0200

> This patchset contains misc cleanups and improvements
> to the mlx4 Core and Eth drivers.
> 
> In patches 1 and 2 I reduce and reorder the branches in the RX csum flow.
> In patch 3 I align the FMR unmapping flow with the device spec, to allow
>   a remapping afterwards.
> Patch 4 by Moni changes the default QoS settings so that a pause
>   frame stops all traffic regardless of its prio.
> 
> Series generated against net-next commit:
> 836df24a7062 net: hns3: hns3_get_channels() can be static

Series applied, thanks Tariq.

I can't say that that ipv6 ifdef you added in patch #1 is the nicest.

It's one thing to ifdef control entire blocks of code, or individual
values.

It's another thing to partially include pieces of code structure
such as closing parenthesis, arithmetic operators, and curly braces.
Two of those were included in the ifdef section.

For example, if we have:

	if (x & (IPV4_THING | IPV6_THING)) {

I don't want to see:

	if (x & (IPV4_THING |
#ifdef IPV6_TEST
		 IPV6_THING)) {
#else
		 0)) {
#endif

Those closing parenthesis and the openning curly brace are there
regardless of the CPP test, and duplicating them in both arms of
the CPP test only makes the code more confusing to read.

I can say I would prefer:

	if (x & (IPV4_THING |
#ifdef IPV6_TEST
		 IPV6_THING
#else
		 0
#endif
		 )) {

which is better.  However, best would be:

#ifdef IPV6_TEST
#define THING_MASK	IPV4_THING | IPV6_THING
#else
#define THING_MASK	IPV4_THING
#endif

	if (x & THING_MASK) {

is the best.

Re: [PATCH net-next 0/4] mlx4 misc for 4.16

[Tariq Toukan]

On 28/12/2017 7:28 PM, David Miller wrote:
> From: Tariq Toukan <tariqt@mellanox.com>
> Date: Thu, 28 Dec 2017 16:26:07 +0200
> 
>> This patchset contains misc cleanups and improvements
>> to the mlx4 Core and Eth drivers.
>>
>> In patches 1 and 2 I reduce and reorder the branches in the RX csum flow.
>> In patch 3 I align the FMR unmapping flow with the device spec, to allow
>>    a remapping afterwards.
>> Patch 4 by Moni changes the default QoS settings so that a pause
>>    frame stops all traffic regardless of its prio.
>>
>> Series generated against net-next commit:
>> 836df24a7062 net: hns3: hns3_get_channels() can be static
> 
> Series applied, thanks Tariq.
> 
> I can't say that that ipv6 ifdef you added in patch #1 is the nicest.
> 
> It's one thing to ifdef control entire blocks of code, or individual
> values.
> 
> It's another thing to partially include pieces of code structure
> such as closing parenthesis, arithmetic operators, and curly braces.
> Two of those were included in the ifdef section.
> 
> For example, if we have:
> 
> 	if (x & (IPV4_THING | IPV6_THING)) {
> 
> I don't want to see:
> 
> 	if (x & (IPV4_THING |
> #ifdef IPV6_TEST
> 		 IPV6_THING)) {
> #else
> 		 0)) {
> #endif
> 
> Those closing parenthesis and the openning curly brace are there
> regardless of the CPP test, and duplicating them in both arms of
> the CPP test only makes the code more confusing to read.
> 
> I can say I would prefer:
> 
> 	if (x & (IPV4_THING |
> #ifdef IPV6_TEST
> 		 IPV6_THING
> #else
> 		 0
> #endif
> 		 )) {
> 
> which is better.  However, best would be:
> 
> #ifdef IPV6_TEST
> #define THING_MASK	IPV4_THING | IPV6_THING
> #else
> #define THING_MASK	IPV4_THING
> #endif
> 
> 	if (x & THING_MASK) {
> 
> is the best.
> 
Hi Dave,
Thanks for accepting the series.
Your suggestions look better indeed, I will prepare a followup patch and 
submit soon.

Regards,
Tariq