diff mbox series

[2/7] net/failsafe: check code of allmulticast mode switch

Message ID 1568031190-16510-3-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: change allmulticast controls to return status |

Checks

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

Commit Message

Andrew Rybchenko Sept. 9, 2019, 12:13 p.m. UTC 
  From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>

rte_eth_allmulticast_enable()/rte_eth_allmulticast_disable() return
value was changed from void to int, so this patch modify usage
of these functions across net/failsafe according to new return type.

Try to keep all-multicast mode consistent across all active
devices in the case of failure.

Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/failsafe/failsafe_ether.c |  8 +++--
 drivers/net/failsafe/failsafe_ops.c   | 44 ++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 6 deletions(-)

Comments

Gaëtan Rivet Sept. 9, 2019, 12:56 p.m. UTC | #1 
On Mon, Sep 09, 2019 at 01:13:04PM +0100, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> 
> rte_eth_allmulticast_enable()/rte_eth_allmulticast_disable() return
> value was changed from void to int, so this patch modify usage
> of these functions across net/failsafe according to new return type.
> 
> Try to keep all-multicast mode consistent across all active
> devices in the case of failure.
> 
> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

A small nit below, but otherwise
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  drivers/net/failsafe/failsafe_ether.c |  8 +++--
>  drivers/net/failsafe/failsafe_ops.c   | 44 ++++++++++++++++++++++++---
>  2 files changed, 46 insertions(+), 6 deletions(-)
> 
[...]
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index fee783ad07..b90fa8676c 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -723,10 +723,28 @@ fs_allmulticast_enable(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
>  	uint8_t i;
> +	int ret = 0;
>  
>  	fs_lock(dev, 0);
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> -		rte_eth_allmulticast_enable(PORT_ID(sdev));
> +	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +		ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
> +		ret = fs_err(sdev, ret);
> +		if (ret != 0) {
> +			ERROR("All-multicast mode enable failed for subdevice %d",
> +				PORT_ID(sdev));
> +			break;
> +		}
> +	}
> +	if (ret != 0) {
> +		/* Rollback in the case of failure */
> +		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +			ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
> +			ret = fs_err(sdev, ret);
> +			if (ret != 0)
> +				ERROR("All-multicast mode disable to rollback failed for subdevice %d",

I'd formulate the error as such:

"All-multicast mode disable during rollback failed for subdevice %d"

That may not be the best either, but the current format seems a little
off? I could be wrong.

> +					PORT_ID(sdev));
> +		}
> +	}
>  	fs_unlock(dev, 0);
>  }
>  
> @@ -735,10 +753,28 @@ fs_allmulticast_disable(struct rte_eth_dev *dev)
>  {
>  	struct sub_device *sdev;
>  	uint8_t i;
> +	int ret = 0;
>  
>  	fs_lock(dev, 0);
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> -		rte_eth_allmulticast_disable(PORT_ID(sdev));
> +	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +		ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
> +		ret = fs_err(sdev, ret);
> +		if (ret != 0) {
> +			ERROR("All-multicast mode disable failed for subdevice %d",
> +				PORT_ID(sdev));
> +			break;
> +		}
> +	}
> +	if (ret != 0) {
> +		/* Rollback in the case of failure */
> +		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +			ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
> +			ret = fs_err(sdev, ret);
> +			if (ret != 0)
> +				ERROR("All-multicast mode enable to rollback failed for subdevice %d",

Same as above.

> +					PORT_ID(sdev));
> +		}
> +	}
>  	fs_unlock(dev, 0);
>  }
>  
> -- 
> 2.17.1
>
Andrew Rybchenko Sept. 13, 2019, 9:04 p.m. UTC | #2 
On 9/9/19 3:56 PM, Gaëtan Rivet wrote:
> On Mon, Sep 09, 2019 at 01:13:04PM +0100, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>>
>> rte_eth_allmulticast_enable()/rte_eth_allmulticast_disable() return
>> value was changed from void to int, so this patch modify usage
>> of these functions across net/failsafe according to new return type.
>>
>> Try to keep all-multicast mode consistent across all active
>> devices in the case of failure.
>>
>> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> A small nit below, but otherwise
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Thanks, will fix it in the next version and similar changes in
promiscuous mode patches (to have no identical logs).
diff mbox series

Patch

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index bd38f1c1e4..93deacd134 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -140,9 +140,13 @@  fs_eth_dev_conf_apply(struct rte_eth_dev *dev,
 	if (dev->data->all_multicast != edev->data->all_multicast) {
 		DEBUG("Configuring all_multicast");
 		if (dev->data->all_multicast)
-			rte_eth_allmulticast_enable(PORT_ID(sdev));
+			ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
 		else
-			rte_eth_allmulticast_disable(PORT_ID(sdev));
+			ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
+		if (ret != 0) {
+			ERROR("Failed to apply allmulticast mode");
+			return ret;
+		}
 	} else {
 		DEBUG("all_multicast already set");
 	}
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index fee783ad07..b90fa8676c 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -723,10 +723,28 @@  fs_allmulticast_enable(struct rte_eth_dev *dev)
 {
 	struct sub_device *sdev;
 	uint8_t i;
+	int ret = 0;
 
 	fs_lock(dev, 0);
-	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
-		rte_eth_allmulticast_enable(PORT_ID(sdev));
+	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+		ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
+		ret = fs_err(sdev, ret);
+		if (ret != 0) {
+			ERROR("All-multicast mode enable failed for subdevice %d",
+				PORT_ID(sdev));
+			break;
+		}
+	}
+	if (ret != 0) {
+		/* Rollback in the case of failure */
+		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+			ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
+			ret = fs_err(sdev, ret);
+			if (ret != 0)
+				ERROR("All-multicast mode disable to rollback failed for subdevice %d",
+					PORT_ID(sdev));
+		}
+	}
 	fs_unlock(dev, 0);
 }
 
@@ -735,10 +753,28 @@  fs_allmulticast_disable(struct rte_eth_dev *dev)
 {
 	struct sub_device *sdev;
 	uint8_t i;
+	int ret = 0;
 
 	fs_lock(dev, 0);
-	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
-		rte_eth_allmulticast_disable(PORT_ID(sdev));
+	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+		ret = rte_eth_allmulticast_disable(PORT_ID(sdev));
+		ret = fs_err(sdev, ret);
+		if (ret != 0) {
+			ERROR("All-multicast mode disable failed for subdevice %d",
+				PORT_ID(sdev));
+			break;
+		}
+	}
+	if (ret != 0) {
+		/* Rollback in the case of failure */
+		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
+			ret = rte_eth_allmulticast_enable(PORT_ID(sdev));
+			ret = fs_err(sdev, ret);
+			if (ret != 0)
+				ERROR("All-multicast mode enable to rollback failed for subdevice %d",
+					PORT_ID(sdev));
+		}
+	}
 	fs_unlock(dev, 0);
 }