-----Original Message-----
From: Jerin Jacob <jerinjac...@gmail.com>
Sent: Monday, 11 January 2021 16:57
To: Liron Himi <lir...@marvell.com>
Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; dpdk-dev <dev@dpdk.org>;
Yuri Chipchev <yu...@marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH v1 11/38] net/mvpp2: align checking order
External Email
----------------------------------------------------------------------
On Wed, Dec 2, 2020 at 3:46 PM <lir...@marvell.com> wrote:
>
> From: Yuri Chipchev <yu...@marvell.com>
>
> first check for 'isolated' and then for '!ppio'
>
> Signed-off-by: Yuri Chipchev <yu...@marvell.com>
> Reviewed-by: Liron Himi <lir...@marvell.com>
> ---
> drivers/net/mvpp2/mrvl_ethdev.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/mvpp2/mrvl_ethdev.c
> b/drivers/net/mvpp2/mrvl_ethdev.c index 130f5221d..76847b303 100644
> --- a/drivers/net/mvpp2/mrvl_ethdev.c
> +++ b/drivers/net/mvpp2/mrvl_ethdev.c
> @@ -1007,10 +1007,10 @@ mrvl_promiscuous_enable(struct rte_eth_dev *dev)
> struct mrvl_priv *priv = dev->data->dev_private;
> int ret;
>
> - if (!priv->ppio)
> + if (priv->isolated)
> return 0;
Rather than duplicating these checks in every function.
Please move sanity check to a function and reuse it.
[L.H.] yes, currently it looks like they are connected and should be in a
separate function.
but, the check for the 'ppio' will have more body, so I don't want to put them
together.
>
> - if (priv->isolated)
> + if (!priv->ppio)
> return 0;
>
> ret = pp2_ppio_set_promisc(priv->ppio, 1); @@ -1037,10
> +1037,10 @@ mrvl_allmulticast_enable(struct rte_eth_dev *dev)
> struct mrvl_priv *priv = dev->data->dev_private;
> int ret;
>
> - if (!priv->ppio)
> + if (priv->isolated)
> return 0;
>
> - if (priv->isolated)
> + if (!priv->ppio)
> return 0;
>
> ret = pp2_ppio_set_mc_promisc(priv->ppio, 1); @@ -1067,6
> +1067,9 @@ mrvl_promiscuous_disable(struct rte_eth_dev *dev)
> struct mrvl_priv *priv = dev->data->dev_private;
> int ret;
>
> + if (priv->isolated)
> + return 0;
> +
> if (!priv->ppio)
> return 0;
>
> @@ -1094,6 +1097,9 @@ mrvl_allmulticast_disable(struct rte_eth_dev *dev)
> struct mrvl_priv *priv = dev->data->dev_private;
> int ret;
>
> + if (priv->isolated)
> + return 0;
> +
> if (!priv->ppio)
> return 0;
>
> @@ -1121,10 +1127,10 @@ mrvl_mac_addr_remove(struct rte_eth_dev *dev,
> uint32_t index)
> char buf[RTE_ETHER_ADDR_FMT_SIZE];
> int ret;
>
> - if (!priv->ppio)
> + if (priv->isolated)
> return;
>
> - if (priv->isolated)
> + if (!priv->ppio)
> return;
>
> ret = pp2_ppio_remove_mac_addr(priv->ppio,
> @@ -1209,12 +1215,12 @@ mrvl_mac_addr_set(struct rte_eth_dev *dev, struct
> rte_ether_addr *mac_addr)
> struct mrvl_priv *priv = dev->data->dev_private;
> int ret;
>
> - if (!priv->ppio)
> - return 0;
> -
> if (priv->isolated)
> return -ENOTSUP;
>
> + if (!priv->ppio)
> + return 0;
> +
> ret = pp2_ppio_set_mac_addr(priv->ppio, mac_addr->addr_bytes);
> if (ret) {
> char buf[RTE_ETHER_ADDR_FMT_SIZE]; @@ -1590,12
> +1596,12 @@ mrvl_vlan_filter_set(struct rte_eth_dev *dev, uint16_t
> vlan_id, int on) {
> struct mrvl_priv *priv = dev->data->dev_private;
>
> - if (!priv->ppio)
> - return -EPERM;
> -
> if (priv->isolated)
> return -ENOTSUP;
>
> + if (!priv->ppio)
> + return 0;
> +
> return on ? pp2_ppio_add_vlan(priv->ppio, vlan_id) :
> pp2_ppio_remove_vlan(priv->ppio, vlan_id); }
> --
> 2.28.0
>
