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

Ferruh Yigit ferruh.yigit at intel.com
Mon Sep 16 15:22:08 CEST 2019


On 9/13/2019 8:57 PM, Andrew Rybchenko wrote:
> On 9/13/19 7:34 PM, Ferruh Yigit wrote:
>> 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?
> 
> Not sure that I understand the question, but tap_ioctl() does not use it.
> So, it is safe to move just before tap_flow_implicit_create().

I think 'dev->data->promiscuous' shouldn't be set be PMD dev_ops, and API
already does it. Is there a specific reason in tap to set this in dev_ops? If
not can we remove setting 'dev->data->promiscuous' from dev_ops?

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



More information about the dev mailing list