diff mbox

[dpdk-dev,v2] net/mlx4: fix port start fail after device removal

Message ID 1516357009-15463-1-git-send-email-motih@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Shahaf Shuler
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Moti Haimovsky Jan. 19, 2018, 10:16 a.m. UTC 
  In failsafe device start can be called for ports/devices that
had been plugged out.
This patch fixes mlx4 port start failure when called by the failsafe
PMD for removed devices.

Fixes: a6e8b01c3c26 ("net/mlx4: compact interrupt functions")
Cc: stable@dpdk.org

Signed-off-by: Moti Haimovsky <motih@mellanox.com>
---
V2:
Fixed commit message.
---
 drivers/net/mlx4/mlx4.c      | 10 ++++++++--
 drivers/net/mlx4/mlx4.h      |  2 ++
 drivers/net/mlx4/mlx4_intr.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 5 deletions(-)

Comments

Shahaf Shuler Jan. 28, 2018, 1:54 p.m. UTC | #1 
Hi Moti,


Friday, January 19, 2018 12:17 PM, Moti Haimovsky
> In failsafe device start can be called for ports/devices that had been plugged
> out.
> This patch fixes mlx4 port start failure when called by the failsafe PMD for
> removed devices.

I think the commit log not fully describes the issue you try to fix.
If I understand correctly, the issue is that the interrupt handler is canceled when the port is stopped, causing link removal events not to arrive to the failsafe PMD. 
To fix that, you precede the installation of the interrupt handler to device configuration, and toggle only the rxq interrupt on start/stop.

Is that correct? 
More below. 

> 
> Fixes: a6e8b01c3c26 ("net/mlx4: compact interrupt functions")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
> V2:
> Fixed commit message.
> ---
>  drivers/net/mlx4/mlx4.c      | 10 ++++++++--
>  drivers/net/mlx4/mlx4.h      |  2 ++
>  drivers/net/mlx4/mlx4_intr.c | 43
> ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c index
> 61c5bf4..c67b221 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -108,7 +108,13 @@ struct mlx4_conf {
>  		      " flow error type %d, cause %p, message: %s",
>  		      -ret, strerror(-ret), error.type, error.cause,
>  		      error.message ? error.message : "(unspecified)");
> +		goto exit;

This fix is related to the patch? 

>  	}
> +	ret = mlx4_intr_install(priv);
> +	if (ret)
> +		ERROR("%p: interrupt handler installation failed",
> +		     (void *)dev);

Indentation is wrong.

> +exit:
>  	return ret;
>  }
> 
> @@ -141,7 +147,7 @@ struct mlx4_conf {
>  		      (void *)dev, strerror(-ret));
>  		goto err;
>  	}
> -	ret = mlx4_intr_install(priv);
> +	ret = mlx4_intr_enable(priv);

Maybe mlx4_rxq_intr_enable? 

>  	if (ret) {
>  		ERROR("%p: interrupt handler installation failed",
>  		     (void *)dev);
> @@ -187,7 +193,7 @@ struct mlx4_conf {
>  	dev->rx_pkt_burst = mlx4_rx_burst_removed;
>  	rte_wmb();
>  	mlx4_flow_sync(priv, NULL);
> -	mlx4_intr_uninstall(priv);
> +	mlx4_intr_disable(priv);

Maybe mlx4_rxq_intr_disable?

>  	mlx4_rss_deinit(priv);
>  }
> 
> diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index
> 99dc335..ab4f396 100644
> --- a/drivers/net/mlx4/mlx4.h
> +++ b/drivers/net/mlx4/mlx4.h
> @@ -176,6 +176,8 @@ int mlx4_flow_ctrl_set(struct rte_eth_dev *dev,
> 
>  int mlx4_intr_uninstall(struct priv *priv);  int mlx4_intr_install(struct priv
> *priv);
> +int mlx4_intr_enable(struct priv *priv); void mlx4_intr_disable(struct
> +priv *priv);
>  int mlx4_rx_intr_disable(struct rte_eth_dev *dev, uint16_t idx);  int
> mlx4_rx_intr_enable(struct rte_eth_dev *dev, uint16_t idx);
> 
> diff --git a/drivers/net/mlx4/mlx4_intr.c b/drivers/net/mlx4/mlx4_intr.c
> index 9becee4..eeab625 100644
> --- a/drivers/net/mlx4/mlx4_intr.c
> +++ b/drivers/net/mlx4/mlx4_intr.c
> @@ -73,6 +73,8 @@
>  {
>  	struct rte_intr_handle *intr_handle = &priv->intr_handle;
> 
> +	if (intr_handle == NULL || intr_handle->intr_vec == NULL)
> +		return;

Is the above code related to this patch? 

>  	rte_intr_free_epoll_fd(intr_handle);
>  	free(intr_handle->intr_vec);
>  	intr_handle->nb_efd = 0;
> @@ -291,7 +293,7 @@
>  	}
>  	rte_eal_alarm_cancel((void (*)(void *))mlx4_link_status_alarm,
> priv);
>  	priv->intr_alarm = 0;
> -	mlx4_rx_intr_vec_disable(priv);
> +	mlx4_intr_disable(priv);
>  	rte_errno = err;
>  	return 0;
>  }
> @@ -313,8 +315,6 @@
>  	int rc;
> 
>  	mlx4_intr_uninstall(priv);
> -	if (intr_conf->rxq && mlx4_rx_intr_vec_enable(priv) < 0)
> -		goto error;
>  	if (intr_conf->lsc | intr_conf->rmv) {
>  		priv->intr_handle.fd = priv->ctx->async_fd;
>  		rc = rte_intr_callback_register(&priv->intr_handle,
> @@ -395,3 +395,40 @@
>  	}
>  	return -ret;
>  }
> +
> +/**
> + * Enable datapath interrupts.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + *
> + * @return
> + *   0 on success, negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx4_intr_enable(struct priv *priv)
> +{
> +	const struct rte_intr_conf *const intr_conf =
> +		&priv->dev->data->dev_conf.intr_conf;
> +
> +	if (intr_conf->rxq && mlx4_rx_intr_vec_enable(priv) < 0)
> +		goto error;
> +	return 0;
> +error:
> +	return -rte_errno;
> +}
> +
> +/**
> + * Disable datapath interrupts, keeping other interrupts intact.
> + *
> + * @param priv
> + *   Pointer to private structure.
> + */
> +void
> +mlx4_intr_disable(struct priv *priv)
> +{
> +	int err = rte_errno; /* Make sure rte_errno remains unchanged. */
> +
> +	mlx4_rx_intr_vec_disable(priv);
> +	rte_errno = err;
> +}
> --
> 1.8.3.1
diff mbox

Patch

diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
index 61c5bf4..c67b221 100644
--- a/drivers/net/mlx4/mlx4.c
+++ b/drivers/net/mlx4/mlx4.c
@@ -108,7 +108,13 @@  struct mlx4_conf {
 		      " flow error type %d, cause %p, message: %s",
 		      -ret, strerror(-ret), error.type, error.cause,
 		      error.message ? error.message : "(unspecified)");
+		goto exit;
 	}
+	ret = mlx4_intr_install(priv);
+	if (ret)
+		ERROR("%p: interrupt handler installation failed",
+		     (void *)dev);
+exit:
 	return ret;
 }
 
@@ -141,7 +147,7 @@  struct mlx4_conf {
 		      (void *)dev, strerror(-ret));
 		goto err;
 	}
-	ret = mlx4_intr_install(priv);
+	ret = mlx4_intr_enable(priv);
 	if (ret) {
 		ERROR("%p: interrupt handler installation failed",
 		     (void *)dev);
@@ -187,7 +193,7 @@  struct mlx4_conf {
 	dev->rx_pkt_burst = mlx4_rx_burst_removed;
 	rte_wmb();
 	mlx4_flow_sync(priv, NULL);
-	mlx4_intr_uninstall(priv);
+	mlx4_intr_disable(priv);
 	mlx4_rss_deinit(priv);
 }
 
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index 99dc335..ab4f396 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -176,6 +176,8 @@  int mlx4_flow_ctrl_set(struct rte_eth_dev *dev,
 
 int mlx4_intr_uninstall(struct priv *priv);
 int mlx4_intr_install(struct priv *priv);
+int mlx4_intr_enable(struct priv *priv);
+void mlx4_intr_disable(struct priv *priv);
 int mlx4_rx_intr_disable(struct rte_eth_dev *dev, uint16_t idx);
 int mlx4_rx_intr_enable(struct rte_eth_dev *dev, uint16_t idx);
 
diff --git a/drivers/net/mlx4/mlx4_intr.c b/drivers/net/mlx4/mlx4_intr.c
index 9becee4..eeab625 100644
--- a/drivers/net/mlx4/mlx4_intr.c
+++ b/drivers/net/mlx4/mlx4_intr.c
@@ -73,6 +73,8 @@ 
 {
 	struct rte_intr_handle *intr_handle = &priv->intr_handle;
 
+	if (intr_handle == NULL || intr_handle->intr_vec == NULL)
+		return;
 	rte_intr_free_epoll_fd(intr_handle);
 	free(intr_handle->intr_vec);
 	intr_handle->nb_efd = 0;
@@ -291,7 +293,7 @@ 
 	}
 	rte_eal_alarm_cancel((void (*)(void *))mlx4_link_status_alarm, priv);
 	priv->intr_alarm = 0;
-	mlx4_rx_intr_vec_disable(priv);
+	mlx4_intr_disable(priv);
 	rte_errno = err;
 	return 0;
 }
@@ -313,8 +315,6 @@ 
 	int rc;
 
 	mlx4_intr_uninstall(priv);
-	if (intr_conf->rxq && mlx4_rx_intr_vec_enable(priv) < 0)
-		goto error;
 	if (intr_conf->lsc | intr_conf->rmv) {
 		priv->intr_handle.fd = priv->ctx->async_fd;
 		rc = rte_intr_callback_register(&priv->intr_handle,
@@ -395,3 +395,40 @@ 
 	}
 	return -ret;
 }
+
+/**
+ * Enable datapath interrupts.
+ *
+ * @param priv
+ *   Pointer to private structure.
+ *
+ * @return
+ *   0 on success, negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx4_intr_enable(struct priv *priv)
+{
+	const struct rte_intr_conf *const intr_conf =
+		&priv->dev->data->dev_conf.intr_conf;
+
+	if (intr_conf->rxq && mlx4_rx_intr_vec_enable(priv) < 0)
+		goto error;
+	return 0;
+error:
+	return -rte_errno;
+}
+
+/**
+ * Disable datapath interrupts, keeping other interrupts intact.
+ *
+ * @param priv
+ *   Pointer to private structure.
+ */
+void
+mlx4_intr_disable(struct priv *priv)
+{
+	int err = rte_errno; /* Make sure rte_errno remains unchanged. */
+
+	mlx4_rx_intr_vec_disable(priv);
+	rte_errno = err;
+}