[dpdk-dev] [PATCH v2 04/13] ethdev: change promiscuous callbacks to return status

[Andrew Rybchenko]

On 9/13/19 6:39 PM, Ferruh Yigit wrote:
> On 9/9/2019 12:58 PM, Andrew Rybchenko wrote:
>> 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.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko at solarflare.com>
> <...>
>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index f85458c3cd..41612ce838 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -1100,28 +1100,60 @@ tap_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
>>   	return 0;
>>   }
>>   
>> -static void
>> +static int
>>   tap_promisc_enable(struct rte_eth_dev *dev)
>>   {
>>   	struct pmd_internals *pmd = dev->data->dev_private;
>>   	struct ifreq ifr = { .ifr_flags = IFF_PROMISC };
>> +	int ret;
>>   
>> -	dev->data->promiscuous = 1;
>> -	tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>> -	if (pmd->remote_if_index && !pmd->flow_isolate)
>> -		tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC);
>> +	ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	if (pmd->remote_if_index && !pmd->flow_isolate) {
>> +		dev->data->promiscuous = 1;
> I think PMD shouldn't be setting this variable, it is already set by the API.
> I quickly checked if an internal function requires this but it looks like it has
> been set by mistake, I think we can remove this.

It is set after callback in the case of enable.

>> +		ret = tap_flow_implicit_create(pmd, TAP_REMOTE_PROMISC);
>> +		if (ret != 0) {
>> +			/* Rollback promisc flag */
>> +			tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>> +			/*
>> +			 * rte_eth_dev_promiscuous_enable() rollback
>> +			 * dev->data->promiscuous in the case of failure.
>> +			 */
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>> -static void
>> +static int
>>   tap_promisc_disable(struct rte_eth_dev *dev)
>>   {
>>   	struct pmd_internals *pmd = dev->data->dev_private;
>>   	struct ifreq ifr = { .ifr_flags = IFF_PROMISC };
>> +	int ret;
>>   
>> -	dev->data->promiscuous = 0;
>> -	tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>> -	if (pmd->remote_if_index && !pmd->flow_isolate)
>> -		tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC);
>> +	ret = tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 0, LOCAL_AND_REMOTE);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	if (pmd->remote_if_index && !pmd->flow_isolate) {
>> +		dev->data->promiscuous = 0;
> Ditto
>
>> +		ret = tap_flow_implicit_destroy(pmd, TAP_REMOTE_PROMISC);
>> +		if (ret != 0) {
>> +			/* Rollback promisc flag */
>> +			tap_ioctl(pmd, SIOCSIFFLAGS, &ifr, 1, LOCAL_AND_REMOTE);
>> +			/*
>> +			 * rte_eth_dev_promiscuous_disable() rollback
>> +			 * dev->data->promiscuous in the case of failure.
>> +			 */
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>>   }