Re: [dpdk-dev] [PATCH 4/7] ethdev: change allmulticast callbacks to return status

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/24/19 11:27 AM, Ferruh Yigit wrote:
> On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>>
>> Enabling/disabling of allmulticast 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: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> <...>
>
>> @@ -1227,23 +1227,27 @@ atl_dev_promiscuous_disable(struct rte_eth_dev *dev)
>>   	return 0;
>>   }
>>   
>> -static void
>> +static int
>>   atl_dev_allmulticast_enable(struct rte_eth_dev *dev)
>>   {
>>   	struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>   
>>   	hw_atl_rpfl2_accept_all_mc_packets_set(hw, true);
>> +
>> +	return 0;
>>   }
>>   
>> -static void
>> +static int
>>   atl_dev_allmulticast_disable(struct rte_eth_dev *dev)
>>   {
>>   	struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>   
>>   	if (dev->data->promiscuous == 1)
>> -		return; /* must remain in all_multicast mode */
>> +		return 0; /* must remain in all_multicast mode */
> What about making this change in the API level, so all dev_ops not need to do
> the similar check.
> Indeed I can see you are already adding this to API in following patches, but we
> can document and guarantee this behavior in API level, so driver developers can
> know and rely on this behavior, what do you think?
>
> And this can be a general guideline for all enable/disable APIs. If the feature
> already enabled, detect in the API and don't call underlying enable dev_ops,
> same for the disable. This saves doing the checks by multiple drivers.

I'm a bit confused.

It is all-multicast disable callback which check promiscuous mode and
it relies on fact that promiscuous mode dominates. However, a driver
could still have own settings which are used on promiscuous
enable/disable to configure HW consistently.

If we're talking about all-multicast mode check here, in general I agree,
but there is rte_eth_dev_config_restore() which calls ops function to
replay current settings. If driver cares about it on start itself,
the check still makes sense.

I would prefer to require it from drivers to apply correct settings
on startup and remove rte_eth_dev_config_restore() function completely.
It would allow to remove such checks from driver callbacks.
However, I understand that current solution is generic and still makes
sense.

> <...>
>
>> @@ -58,10 +58,10 @@ typedef int (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev);
>>   typedef int (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev);
>>   /**< @internal Function used to disable the RX promiscuous mode of an Ethernet device. */
>>   
>> -typedef void (*eth_allmulticast_enable_t)(struct rte_eth_dev *dev);
>> +typedef int (*eth_allmulticast_enable_t)(struct rte_eth_dev *dev);
>>   /**< @internal Enable the receipt of all multicast packets by an Ethernet device. */
>>   
>> -typedef void (*eth_allmulticast_disable_t)(struct rte_eth_dev *dev);
>> +typedef int (*eth_allmulticast_disable_t)(struct rte_eth_dev *dev);
>>   /**< @internal Disable the receipt of all multicast packets by an Ethernet device. */
> Can you please document what a driver should return? As done in other patches.

Yes, will do in v2.