Re: [dpdk-dev] [PATCH 02/13] net/failsafe: check code of promiscuous mode switch

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/5/19 7:25 PM, Gaëtan Rivet wrote:
> Hi,
>
> On Thu, Sep 05, 2019 at 05:10:40PM +0100, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>>
>> rte_eth_promiscuous_enable()/rte_eth_promiscuous_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 promiscuous 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(-)
>>
>> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
>> index 504c76edb0..bd38f1c1e4 100644
>> --- a/drivers/net/failsafe/failsafe_ether.c
>> +++ b/drivers/net/failsafe/failsafe_ether.c
>> @@ -126,9 +126,13 @@ fs_eth_dev_conf_apply(struct rte_eth_dev *dev,
>>   	if (dev->data->promiscuous != edev->data->promiscuous) {
>>   		DEBUG("Configuring promiscuous");
>>   		if (dev->data->promiscuous)
>> -			rte_eth_promiscuous_enable(PORT_ID(sdev));
>> +			ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
>>   		else
>> -			rte_eth_promiscuous_disable(PORT_ID(sdev));
>> +			ret = rte_eth_promiscuous_disable(PORT_ID(sdev));
>> +		if (ret != 0) {
>> +			ERROR("Failed to apply promiscuous mode");
>> +			return ret;
>> +		}
>>   	} else {
>>   		DEBUG("promiscuous already set");
>>   	}
>> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
>> index cc14bc2bcc..cbf143fb3c 100644
>> --- a/drivers/net/failsafe/failsafe_ops.c
>> +++ b/drivers/net/failsafe/failsafe_ops.c
>> @@ -659,11 +659,29 @@ fs_promiscuous_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_promiscuous_enable(PORT_ID(sdev));
>> +	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
>> +		ret = rte_eth_promiscuous_enable(PORT_ID(sdev));
>> +		if (ret != 0) {
>> +			ERROR("Promiscuous 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_promiscuous_disable(PORT_ID(sdev));
>> +			if (ret != 0)
>> +				ERROR("Promiscuous mode disable failed for subdevice %d",
>> +					PORT_ID(sdev));
>> +		}
>> +	}
>>   	fs_unlock(dev, 0);
>> +
>> +	return;
> This patch should be applied after the ethdev change to avoid breaking
> the build, I think?

As far as I tested it does not break the build. Sorry for confusing
return at the end of function without return value.
The function is still void and it is updated to forward ret when
callback has return value.

> You should be able to change the ethdev API, leaving the fail-safe
> internals ignore the return value, then apply this patch and fix it.
> This way the patchset should not break the build mid-series.
>
> Otherwise good implementation with the rollback.

Thanks.