On 9/10/19 10:53 AM, Matan Azrad wrote:
Hi

Review for mlx5 part:

Added not very important comment below.
You can stay it as is if no new version will be created.

Acked-by: Matan Azrad <ma...@mellanox.com>

Thank you, will fix in the next version.

Thanks.

From: Andrew Rybchenko
Enabling/disabling of promiscuous mode is not always successful and
it should be taken into account to be able to handle it properly.

When correct return status is unclear from driver code, -EAGAIN is used.

  drivers/net/mlx4/mlx4.h                   |  4 +-
  drivers/net/mlx4/mlx4_ethdev.c            | 24 ++++++++---
  drivers/net/mlx5/mlx5.h                   |  4 +-
  drivers/net/mlx5/mlx5_rxmode.c            | 40 ++++++++++++++---

  static void
diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h
index 7730b530af..21517d70a2 100644
--- a/drivers/net/mlx4/mlx4.h
+++ b/drivers/net/mlx4/mlx4.h
@@ -205,8 +205,8 @@ int mlx4_mtu_get(struct mlx4_priv *priv, uint16_t
*mtu);
  int mlx4_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
  int mlx4_dev_set_link_down(struct rte_eth_dev *dev);
  int mlx4_dev_set_link_up(struct rte_eth_dev *dev);
-void mlx4_promiscuous_enable(struct rte_eth_dev *dev);
-void mlx4_promiscuous_disable(struct rte_eth_dev *dev);
+int mlx4_promiscuous_enable(struct rte_eth_dev *dev);
+int mlx4_promiscuous_disable(struct rte_eth_dev *dev);
  void mlx4_allmulticast_enable(struct rte_eth_dev *dev);
  void mlx4_allmulticast_disable(struct rte_eth_dev *dev);
  void mlx4_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
diff --git a/drivers/net/mlx4/mlx4_ethdev.c
b/drivers/net/mlx4/mlx4_ethdev.c
index 623ebd88cb..c8a73bc1f4 100644
--- a/drivers/net/mlx4/mlx4_ethdev.c
+++ b/drivers/net/mlx4/mlx4_ethdev.c
@@ -341,13 +341,17 @@ enum rxmode_toggle {
   *   Pointer to Ethernet device structure.
   * @param toggle
   *   Toggle to set.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
   */
-static void
+static int
  mlx4_rxmode_toggle(struct rte_eth_dev *dev, enum rxmode_toggle
toggle)
  {
        struct mlx4_priv *priv = dev->data->dev_private;
        const char *mode;
        struct rte_flow_error error;
+       int ret;

        switch (toggle) {
        case RXMODE_TOGGLE_PROMISC_OFF:
@@ -363,12 +367,16 @@ mlx4_rxmode_toggle(struct rte_eth_dev *dev,
enum rxmode_toggle toggle)
        default:
                mode = "undefined";
        }
-       if (!mlx4_flow_sync(priv, &error))
-               return;
+
+       ret = mlx4_flow_sync(priv, &error);
+       if (!ret)
+               return 0;
+
        ERROR("cannot toggle %s mode (code %d, \"%s\"),"
              " flow error type %d, cause %p, message: %s",
              mode, rte_errno, strerror(rte_errno), error.type, error.cause,
              error.message ? error.message : "(unspecified)");
+       return ret;
  }

  /**
@@ -376,8 +384,11 @@ mlx4_rxmode_toggle(struct rte_eth_dev *dev,
enum rxmode_toggle toggle)
   *
   * @param dev
   *   Pointer to Ethernet device structure.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
   */
-void
+int
  mlx4_promiscuous_enable(struct rte_eth_dev *dev)
  {
        mlx4_rxmode_toggle(dev, RXMODE_TOGGLE_PROMISC_ON);
@@ -388,8 +399,11 @@ mlx4_promiscuous_enable(struct rte_eth_dev
*dev)
   *
   * @param dev
   *   Pointer to Ethernet device structure.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
   */
-void
+int
  mlx4_promiscuous_disable(struct rte_eth_dev *dev)
  {
        mlx4_rxmode_toggle(dev, RXMODE_TOGGLE_PROMISC_OFF);
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 8ddbb7a17c..11d540c3a5 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -752,8 +752,8 @@ int mlx5_dev_rss_reta_update(struct rte_eth_dev
*dev,

  /* mlx5_rxmode.c */

-void mlx5_promiscuous_enable(struct rte_eth_dev *dev);
-void mlx5_promiscuous_disable(struct rte_eth_dev *dev);
+int mlx5_promiscuous_enable(struct rte_eth_dev *dev);
+int mlx5_promiscuous_disable(struct rte_eth_dev *dev);
  void mlx5_allmulticast_enable(struct rte_eth_dev *dev);
  void mlx5_allmulticast_disable(struct rte_eth_dev *dev);

diff --git a/drivers/net/mlx5/mlx5_rxmode.c
b/drivers/net/mlx5/mlx5_rxmode.c
index d5077db0db..c862fc9520 100644
--- a/drivers/net/mlx5/mlx5_rxmode.c
+++ b/drivers/net/mlx5/mlx5_rxmode.c
@@ -28,8 +28,11 @@
   *
   * @param dev
   *   Pointer to Ethernet device structure.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
   */
-void
+int
  mlx5_promiscuous_enable(struct rte_eth_dev *dev)
  {
        struct mlx5_priv *priv = dev->data->dev_private;
@@ -41,14 +44,24 @@ mlx5_promiscuous_enable(struct rte_eth_dev *dev)
                        "port %u cannot enable promiscuous mode"
                        " in flow isolation mode",
                        dev->data->port_id);
-               return;
+               return 0;
+       }
+       if (priv->config.vf) {
+               ret = mlx5_nl_promisc(dev, 1);
+               if (ret)
+                       goto error;
No need the jump, just return ret here and below.

        }
-       if (priv->config.vf)
-               mlx5_nl_promisc(dev, 1);
        ret = mlx5_traffic_restart(dev);
        if (ret)
                DRV_LOG(ERR, "port %u cannot enable promiscuous mode:
%s",
                        dev->data->port_id, strerror(rte_errno));
+
+error:
+       /*
+        * rte_eth_dev_promiscuous_enable() rollback
+        * dev->data->promiscuous in the case of failure.
+        */
+       return ret;
  }

  /**
@@ -56,20 +69,33 @@ mlx5_promiscuous_enable(struct rte_eth_dev *dev)
   *
   * @param dev
   *   Pointer to Ethernet device structure.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
   */
-void
+int
  mlx5_promiscuous_disable(struct rte_eth_dev *dev)
  {
        struct mlx5_priv *priv = dev->data->dev_private;
        int ret;

        dev->data->promiscuous = 0;
-       if (priv->config.vf)
-               mlx5_nl_promisc(dev, 0);
+       if (priv->config.vf) {
+               ret = mlx5_nl_promisc(dev, 0);
+               if (ret)
+                       goto error;
Same here.

+       }
        ret = mlx5_traffic_restart(dev);
        if (ret)
                DRV_LOG(ERR, "port %u cannot disable promiscuous mode:
%s",
                        dev->data->port_id, strerror(rte_errno));
+
+error:
+       /*
+        * rte_eth_dev_promiscuous_disable() rollback
+        * dev->data->promiscuous in the case of failure.
+        */
+       return ret;
  }

  /**

Reply via email to