On 9/24/19 11:27 AM, Ferruh Yigit wrote:
On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
From: Ivan Ilchenko <ivan.ilche...@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.ilche...@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybche...@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.
