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

[Ferruh Yigit]

On 9/13/2019 5:05 PM, Andrew Rybchenko wrote:
> 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.

I see, but do we need it enabled earlier?

> 
>>> +		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;
>>>   }
> 
>