[PATCH net-next 0/3] Mellanox driver updates 2013-08-19

[PATCH net-next 0/3] Mellanox driver updates 2013-08-19

[Amir Vadai]

Hi Dave,

This small patchset contains 2 fixes to bad handling of error flows in TX
datapath, and a fix to a bug in mlx4_en_dcbnl_ieee_setpfc() where global pause
frames were enabled whenever PFC was configured.

Thanks,
Amir

Amir Vadai (2):
  net/mlx4_en: Notify user when TX ring in error state
  net/mlx4_en: Fix handling of dma_map failure

Eugenia Emantayev (1):
  net/mlx4_en: Disable global flow control when PFC enabled

 drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c |   3 +-
 drivers/net/ethernet/mellanox/mlx4/en_tx.c     | 102 ++++++++++++++++---------
 2 files changed, 69 insertions(+), 36 deletions(-)

-- 
1.8.3.4

[PATCH net-next 1/3] net/mlx4_en: Disable global flow control when PFC enabled

[Amir Vadai]

From: Eugenia Emantayev <eugenia@mellanox.com>

Fix a bug when FC and PFC are enabled/disabled at the same time.
According to ConnectX-3 Programmer Manual these two features are mutial
exclusive.  So make sure when enabling PFC to turn off global FC and
vise versa.  Otherwise it hurts the performance.


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

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c b/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c
index 9d4a1ea..fcef764 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_dcb_nl.c
@@ -169,7 +169,8 @@ static int mlx4_en_dcbnl_ieee_setpfc(struct net_device *dev,
 			pfc->mbc,
 			pfc->delay);
 
-	priv->prof->rx_pause = priv->prof->tx_pause = !!pfc->pfc_en;
+	priv->prof->rx_pause = !pfc->pfc_en;
+	priv->prof->tx_pause = !pfc->pfc_en;
 	priv->prof->rx_ppp = priv->prof->tx_ppp = pfc->pfc_en;
 
 	err = mlx4_SET_PORT_general(mdev->dev, priv->port,
-- 
1.8.3.4

[PATCH net-next 2/3] net/mlx4_en: Notify user when TX ring in error state

[Amir Vadai]

When hardware gets into error state, must notify user about it.
When QP in error state no traffic will be tx'ed from the attached
tx_ring.

Driver should know how to recover from this unexpected state. I will send later
on the recovery flow, but having the print shouldn't be delayed.

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

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 6dcca98..157bcd1 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -362,6 +362,14 @@ static void mlx4_en_process_tx_cq(struct net_device *dev, struct mlx4_en_cq *cq)
 		 */
 		rmb();
 
+		if (unlikely((cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) ==
+			     MLX4_CQE_OPCODE_ERROR)) {
+			struct mlx4_err_cqe *cqe_err = (struct mlx4_err_cqe *)cqe;
+			en_err(priv, "CQE error - vendor syndrom: 0x%x syndrom: 0x%x\n",
+			       cqe_err->vendor_err_syndrome,
+			       cqe_err->syndrome);
+		}
+
 		/* Skip over last polled CQE */
 		new_index = be16_to_cpu(cqe->wqe_index) & size_mask;
 
-- 
1.8.3.4

[PATCH net-next 3/3] net/mlx4_en: Fix handling of dma_map failure

[Amir Vadai]

Result of skb_frag_dma_map() and dma_map_single() wasn't checked.
Added a check and proper handling in case of failure.
Moved the mapping to the beginning of mlx4_en_xmit(), before updating
the ring data structure to make error handling easier.


Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 94 +++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 157bcd1..92d7097 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -673,6 +673,64 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	tx_info->skb = skb;
 	tx_info->nr_txbb = nr_txbb;
 
+	if (lso_header_size)
+		data = ((void *)&tx_desc->lso + ALIGN(lso_header_size + 4,
+						      DS_SIZE));
+	else
+		data = &tx_desc->data;
+
+	/* valid only for none inline segments */
+	tx_info->data_offset = (void *)data - (void *)tx_desc;
+
+	tx_info->linear = (lso_header_size < skb_headlen(skb) &&
+			   !is_inline(skb, NULL)) ? 1 : 0;
+
+	data += skb_shinfo(skb)->nr_frags + tx_info->linear - 1;
+
+	if (is_inline(skb, &fragptr)) {
+		tx_info->inl = 1;
+	} else {
+		/* Map fragments */
+		for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
+			frag = &skb_shinfo(skb)->frags[i];
+			dma = skb_frag_dma_map(priv->ddev, frag,
+					       0, skb_frag_size(frag),
+					       DMA_TO_DEVICE);
+			if (dma_mapping_error(priv->ddev, dma)) {
+				for (i++; i < skb_shinfo(skb)->nr_frags; i++) {
+					frag = &skb_shinfo(skb)->frags[i];
+					en_err(priv, "DMA mapping error\n");
+					dma_unmap_page(priv->ddev,
+						       (dma_addr_t) be64_to_cpu(data[i].addr),
+						       skb_frag_size(frag), PCI_DMA_TODEVICE);
+				}
+				goto tx_drop;
+			}
+
+			data->addr = cpu_to_be64(dma);
+			data->lkey = cpu_to_be32(mdev->mr.key);
+			wmb();
+			data->byte_count = cpu_to_be32(skb_frag_size(frag));
+			--data;
+		}
+
+		/* Map linear part */
+		if (tx_info->linear) {
+			u32 byte_count = skb_headlen(skb) - lso_header_size;
+			dma = dma_map_single(priv->ddev, skb->data +
+					     lso_header_size, byte_count,
+					     PCI_DMA_TODEVICE);
+			if (dma_mapping_error(priv->ddev, dma))
+				goto tx_drop;
+
+			data->addr = cpu_to_be64(dma);
+			data->lkey = cpu_to_be32(mdev->mr.key);
+			wmb();
+			data->byte_count = cpu_to_be32(byte_count);
+		}
+		tx_info->inl = 0;
+	}
+
 	/*
 	 * For timestamping add flag to skb_shinfo and
 	 * set flag for further reference
@@ -719,8 +777,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		/* Copy headers;
 		 * note that we already verified that it is linear */
 		memcpy(tx_desc->lso.header, skb->data, lso_header_size);
-		data = ((void *) &tx_desc->lso +
-			ALIGN(lso_header_size + 4, DS_SIZE));
 
 		priv->port_stats.tso_packets++;
 		i = ((skb->len - lso_header_size) / skb_shinfo(skb)->gso_size) +
@@ -732,7 +788,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		op_own = cpu_to_be32(MLX4_OPCODE_SEND) |
 			((ring->prod & ring->size) ?
 			 cpu_to_be32(MLX4_EN_BIT_DESC_OWN) : 0);
-		data = &tx_desc->data;
 		tx_info->nr_bytes = max_t(unsigned int, skb->len, ETH_ZLEN);
 		ring->packets++;
 
@@ -741,38 +796,7 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
 	AVG_PERF_COUNTER(priv->pstats.tx_pktsz_avg, skb->len);
 
-
-	/* valid only for none inline segments */
-	tx_info->data_offset = (void *) data - (void *) tx_desc;
-
-	tx_info->linear = (lso_header_size < skb_headlen(skb) && !is_inline(skb, NULL)) ? 1 : 0;
-	data += skb_shinfo(skb)->nr_frags + tx_info->linear - 1;
-
-	if (!is_inline(skb, &fragptr)) {
-		/* Map fragments */
-		for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
-			frag = &skb_shinfo(skb)->frags[i];
-			dma = skb_frag_dma_map(priv->ddev, frag,
-					       0, skb_frag_size(frag),
-					       DMA_TO_DEVICE);
-			data->addr = cpu_to_be64(dma);
-			data->lkey = cpu_to_be32(mdev->mr.key);
-			wmb();
-			data->byte_count = cpu_to_be32(skb_frag_size(frag));
-			--data;
-		}
-
-		/* Map linear part */
-		if (tx_info->linear) {
-			dma = dma_map_single(priv->ddev, skb->data + lso_header_size,
-					     skb_headlen(skb) - lso_header_size, PCI_DMA_TODEVICE);
-			data->addr = cpu_to_be64(dma);
-			data->lkey = cpu_to_be32(mdev->mr.key);
-			wmb();
-			data->byte_count = cpu_to_be32(skb_headlen(skb) - lso_header_size);
-		}
-		tx_info->inl = 0;
-	} else {
+	if (tx_info->inl) {
 		build_inline_wqe(tx_desc, skb, real_size, &vlan_tag, tx_ind, fragptr);
 		tx_info->inl = 1;
 	}
-- 
1.8.3.4

Re: [PATCH net-next 2/3] net/mlx4_en: Notify user when TX ring in error state

[Sergei Shtylyov]

Hello.

On 08/19/2013 10:42 AM, Amir Vadai wrote:

> When hardware gets into error state, must notify user about it.
> When QP in error state no traffic will be tx'ed from the attached
> tx_ring.

> Driver should know how to recover from this unexpected state. I will send later
> on the recovery flow, but having the print shouldn't be delayed.

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

> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index 6dcca98..157bcd1 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -362,6 +362,14 @@ static void mlx4_en_process_tx_cq(struct net_device *dev, struct mlx4_en_cq *cq)
>   		 */
>   		rmb();
>
> +		if (unlikely((cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) ==
> +			     MLX4_CQE_OPCODE_ERROR)) {
> +			struct mlx4_err_cqe *cqe_err = (struct mlx4_err_cqe *)cqe;

    Empty line wouldn't hurt here, after declaration.

> +			en_err(priv, "CQE error - vendor syndrom: 0x%x syndrom: 0x%x\n",

    s/syndrom/syndrome/ as below?

> +			       cqe_err->vendor_err_syndrome,
> +			       cqe_err->syndrome);
> +		}
> +

WBR, Sergei

Re: [PATCH net-next 3/3] net/mlx4_en: Fix handling of dma_map failure

[Francois Romieu]

Amir Vadai <amirv@mellanox.com> :
[...]
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index 157bcd1..92d7097 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -673,6 +673,64 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>  	tx_info->skb = skb;
>  	tx_info->nr_txbb = nr_txbb;
>  
> +	if (lso_header_size)
> +		data = ((void *)&tx_desc->lso + ALIGN(lso_header_size + 4,
> +						      DS_SIZE));
> +	else
> +		data = &tx_desc->data;
> +
> +	/* valid only for none inline segments */
> +	tx_info->data_offset = (void *)data - (void *)tx_desc;
> +
> +	tx_info->linear = (lso_header_size < skb_headlen(skb) &&
> +			   !is_inline(skb, NULL)) ? 1 : 0;
> +
> +	data += skb_shinfo(skb)->nr_frags + tx_info->linear - 1;
> +
> +	if (is_inline(skb, &fragptr)) {
> +		tx_info->inl = 1;
> +	} else {
> +		/* Map fragments */
> +		for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
> +			frag = &skb_shinfo(skb)->frags[i];
> +			dma = skb_frag_dma_map(priv->ddev, frag,
> +					       0, skb_frag_size(frag),
> +					       DMA_TO_DEVICE);
> +			if (dma_mapping_error(priv->ddev, dma)) {

				goto err_unmap_frags;

You have a lot of huge scope variables. At least use these to hide
the 80 cols problems.

> +				for (i++; i < skb_shinfo(skb)->nr_frags; i++) {
> +					frag = &skb_shinfo(skb)->frags[i];
> +					en_err(priv, "DMA mapping error\n");
> +					dma_unmap_page(priv->ddev,
> +						       (dma_addr_t) be64_to_cpu(data[i].addr),
> +						       skb_frag_size(frag), PCI_DMA_TODEVICE);
> +				}
> +				goto tx_drop;
> +			}
> +
> +			data->addr = cpu_to_be64(dma);
> +			data->lkey = cpu_to_be32(mdev->mr.key);
> +			wmb();
> +			data->byte_count = cpu_to_be32(skb_frag_size(frag));
> +			--data;
> +		}
> +
> +		/* Map linear part */
> +		if (tx_info->linear) {
> +			u32 byte_count = skb_headlen(skb) - lso_header_size;
> +			dma = dma_map_single(priv->ddev, skb->data +
> +					     lso_header_size, byte_count,
> +					     PCI_DMA_TODEVICE);
> +			if (dma_mapping_error(priv->ddev, dma))
> +				goto tx_drop;

(frags dma leak)
				goto err_unmap_frags;

You may consider a local variable for 'priv->ddev' btw.

-- 
Ueimor

Re: [PATCH net-next 2/3] net/mlx4_en: Notify user when TX ring in error state

[Amir Vadai]

On 19/08/2013 21:49, Sergei Shtylyov wrote:
> Hello.
> 
> On 08/19/2013 10:42 AM, Amir Vadai wrote:
> 
>> When hardware gets into error state, must notify user about it.
>> When QP in error state no traffic will be tx'ed from the attached
>> tx_ring.
> 
>> Driver should know how to recover from this unexpected state. I will
>> send later
>> on the recovery flow, but having the print shouldn't be delayed.
> 
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_tx.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index 6dcca98..157bcd1 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -362,6 +362,14 @@ static void mlx4_en_process_tx_cq(struct
>> net_device *dev, struct mlx4_en_cq *cq)
>>            */
>>           rmb();
>>
>> +        if (unlikely((cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) ==
>> +                 MLX4_CQE_OPCODE_ERROR)) {
>> +            struct mlx4_err_cqe *cqe_err = (struct mlx4_err_cqe *)cqe;
> 
>    Empty line wouldn't hurt here, after declaration.
> 
>> +            en_err(priv, "CQE error - vendor syndrom: 0x%x syndrom:
>> 0x%x\n",
> 
>    s/syndrom/syndrome/ as below?
> 
>> +                   cqe_err->vendor_err_syndrome,
>> +                   cqe_err->syndrome);
>> +        }
>> +
> 
> WBR, Sergei
> 
> 

Thanks - will be fixed for V1

Amir

Re: [PATCH net-next 3/3] net/mlx4_en: Fix handling of dma_map failure

[Amir Vadai]

On 19/08/2013 23:42, Francois Romieu wrote:
> Amir Vadai <amirv@mellanox.com> :
> [...]
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index 157bcd1..92d7097 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -673,6 +673,64 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	tx_info->skb = skb;
>>  	tx_info->nr_txbb = nr_txbb;
>>  
>> +	if (lso_header_size)
>> +		data = ((void *)&tx_desc->lso + ALIGN(lso_header_size + 4,
>> +						      DS_SIZE));
>> +	else
>> +		data = &tx_desc->data;
>> +
>> +	/* valid only for none inline segments */
>> +	tx_info->data_offset = (void *)data - (void *)tx_desc;
>> +
>> +	tx_info->linear = (lso_header_size < skb_headlen(skb) &&
>> +			   !is_inline(skb, NULL)) ? 1 : 0;
>> +
>> +	data += skb_shinfo(skb)->nr_frags + tx_info->linear - 1;
>> +
>> +	if (is_inline(skb, &fragptr)) {
>> +		tx_info->inl = 1;
>> +	} else {
>> +		/* Map fragments */
>> +		for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
>> +			frag = &skb_shinfo(skb)->frags[i];
>> +			dma = skb_frag_dma_map(priv->ddev, frag,
>> +					       0, skb_frag_size(frag),
>> +					       DMA_TO_DEVICE);
>> +			if (dma_mapping_error(priv->ddev, dma)) {
> 
> 				goto err_unmap_frags;
> 
> You have a lot of huge scope variables. At least use these to hide
> the 80 cols problems.
Will try to re-arrange a bit this code - I think I will add another patch.
Split this one into:
1. Code re-arrange
2. Move and handle dma_map_failure

> 
>> +				for (i++; i < skb_shinfo(skb)->nr_frags; i++) {
>> +					frag = &skb_shinfo(skb)->frags[i];
>> +					en_err(priv, "DMA mapping error\n");
>> +					dma_unmap_page(priv->ddev,
>> +						       (dma_addr_t) be64_to_cpu(data[i].addr),
>> +						       skb_frag_size(frag), PCI_DMA_TODEVICE);
>> +				}
>> +				goto tx_drop;
>> +			}
>> +
>> +			data->addr = cpu_to_be64(dma);
>> +			data->lkey = cpu_to_be32(mdev->mr.key);
>> +			wmb();
>> +			data->byte_count = cpu_to_be32(skb_frag_size(frag));
>> +			--data;
>> +		}
>> +
>> +		/* Map linear part */
>> +		if (tx_info->linear) {
>> +			u32 byte_count = skb_headlen(skb) - lso_header_size;
>> +			dma = dma_map_single(priv->ddev, skb->data +
>> +					     lso_header_size, byte_count,
>> +					     PCI_DMA_TODEVICE);
>> +			if (dma_mapping_error(priv->ddev, dma))
>> +				goto tx_drop;
> 
> (frags dma leak)
Thanks for the catch.

> 				goto err_unmap_frags;
> 
> You may consider a local variable for 'priv->ddev' btw.
Will do
> 

Amir.