Re: [dpdk-dev] [EXT] Re: [PATCH v2 11/37] net/mvpp2: save initial configuration

[Liron Himi <lironh@marvell.com>]

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Wednesday, 27 January 2021 00:58
To: Liron Himi <lironh@marvell.com>; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Cc: dev@dpdk.org; Yuri Chipchev <yuric@marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH v2 11/37] net/mvpp2: save initial configuration

External Email

----------------------------------------------------------------------
On 1/22/2021 7:18 PM, lironh@marvell.com wrote:
> From: Yuri Chipchev <yuric@marvell.com>
> 
> Save configuration that was done prior 'start' as only then the ppio 
> is being configured.
> 

Can you please give more details on what is saved and why?
[L.H.] Only in the 'start' stage the driver initialized the HW.
User is allowed to call the below function between the configure and start stage.
Until now it either got error or the required operation was not saved.
This patch "record" the final request of each functionality and apply this configuration at the 'start' stage.

> Signed-off-by: Yuri Chipchev <yuric@marvell.com>
> Reviewed-by: Liron Himi <lironh@marvell.com>
> ---
>   drivers/net/mvpp2/mrvl_ethdev.c | 107 +++++++++++++++++++++++++++-----
>   1 file changed, 92 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/mvpp2/mrvl_ethdev.c 
> b/drivers/net/mvpp2/mrvl_ethdev.c index 47b3aa28f..3891313cf 100644
> --- a/drivers/net/mvpp2/mrvl_ethdev.c
> +++ b/drivers/net/mvpp2/mrvl_ethdev.c
> @@ -1,6 +1,6 @@
>   /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2017 Marvell International Ltd.
> - * Copyright(c) 2017 Semihalf.
> + * Copyright(c) 2017-2021 Marvell International Ltd.
> + * Copyright(c) 2017-2021 Semihalf.
>    * All rights reserved.
>    */
>   
> @@ -146,6 +146,15 @@ static int rte_pmd_mrvl_remove(struct rte_vdev_device *vdev);
>   static void mrvl_deinit_pp2(void);
>   static void mrvl_deinit_hifs(void);
>   
> +static int
> +mrvl_mac_addr_add(struct rte_eth_dev *dev, struct rte_ether_addr *mac_addr,
> +		  uint32_t index, uint32_t vmdq __rte_unused); static int 
> +mrvl_mac_addr_set(struct rte_eth_dev *dev, struct rte_ether_addr 
> +*mac_addr); static int mrvl_vlan_filter_set(struct rte_eth_dev *dev, 
> +uint16_t vlan_id, int on); static int mrvl_promiscuous_enable(struct 
> +rte_eth_dev *dev); static int mrvl_allmulticast_enable(struct 
> +rte_eth_dev *dev);
>   
>   #define MRVL_XSTATS_TBL_ENTRY(name) { \
>   	#name, offsetof(struct pp2_ppio_statistics, name),	\
> @@ -404,8 +413,12 @@ mrvl_dev_configure(struct rte_eth_dev *dev)
>   		return 0;
>   	}
>   
> -	return mrvl_configure_rss(priv,
> -				  &dev->data->dev_conf.rx_adv_conf.rss_conf);
> +	ret = mrvl_configure_rss(priv,
> +				 &dev->data->dev_conf.rx_adv_conf.rss_conf);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

What is the difference? As far I can see 'mrvl_configure_rss()' return negative on error and 0 on success.

Is this refactoring part of saving config before start?
[L.H.] correct, this can be removed from this patch. Will do in v3

>   }
>   
>   /**
> @@ -492,8 +505,10 @@ mrvl_dev_set_link_up(struct rte_eth_dev *dev)
>   	struct mrvl_priv *priv = dev->data->dev_private;
>   	int ret;
>   
> -	if (!priv->ppio)
> -		return -EPERM;
> +	if (!priv->ppio) {
> +		dev->data->dev_link.link_status = ETH_LINK_UP;
> +		return 0;
> +	}
>   
>   	ret = pp2_ppio_enable(priv->ppio);
>   	if (ret)
> @@ -507,10 +522,13 @@ mrvl_dev_set_link_up(struct rte_eth_dev *dev)
>   	 * Set mtu to default DPDK value here.
>   	 */
>   	ret = mrvl_mtu_set(dev, dev->data->mtu);
> -	if (ret)
> +	if (ret) {
>   		pp2_ppio_disable(priv->ppio);
> +		return ret;
> +	}
>   
> -	return ret;
> +	dev->data->dev_link.link_status = ETH_LINK_UP;
> +	return 0;
>   }
>   
>   /**
> @@ -526,11 +544,18 @@ static int
>   mrvl_dev_set_link_down(struct rte_eth_dev *dev)
>   {
>   	struct mrvl_priv *priv = dev->data->dev_private;
> +	int ret;
>   
> -	if (!priv->ppio)
> -		return -EPERM;
> +	if (!priv->ppio) {
> +		dev->data->dev_link.link_status = ETH_LINK_DOWN;
> +		return 0;
> +	}
> +	ret = pp2_ppio_disable(priv->ppio);
> +	if (ret)
> +		return ret;
>   
> -	return pp2_ppio_disable(priv->ppio);
> +	dev->data->dev_link.link_status = ETH_LINK_DOWN;
> +	return 0;
>   }

Similarly, are these changes on the link up/down, related to what described in the commit log?
[L.H.] yes. As we record the link up/down request and on the 'start' apply the final value.

>   
>   /**
> @@ -612,6 +637,9 @@ mrvl_dev_start(struct rte_eth_dev *dev)
>   	struct mrvl_priv *priv = dev->data->dev_private;
>   	char match[MRVL_MATCH_LEN];
>   	int ret = 0, i, def_init_size;
> +	uint32_t j;
> +	struct rte_vlan_filter_conf *vfc;
> +	struct rte_ether_addr *mac_addr;
>   
>   	if (priv->ppio)
>   		return mrvl_dev_set_link_up(dev);
> @@ -677,6 +705,47 @@ mrvl_dev_start(struct rte_eth_dev *dev)
>   	if (ret)
>   		MRVL_LOG(ERR, "Failed to set MTU to %d", dev->data->mtu);
>   
> +	if (!rte_is_zero_ether_addr(&dev->data->mac_addrs[0]))
> +		mrvl_mac_addr_set(dev, &dev->data->mac_addrs[0]);
> +
> +	for (i = 1; i < MRVL_MAC_ADDRS_MAX; i++) {
> +		mac_addr = &dev->data->mac_addrs[i];
> +
> +		/* skip zero address */
> +		if (rte_is_zero_ether_addr(mac_addr))
> +			continue;
> +
> +		mrvl_mac_addr_add(dev, mac_addr, i, 0);
> +	}
> +
> +	if (dev->data->all_multicast == 1)
> +		mrvl_allmulticast_enable(dev);
> +
> +	vfc = &dev->data->vlan_filter_conf;
> +	for (j = 0; j < RTE_DIM(vfc->ids); j++) {
> +		uint64_t vlan;
> +		uint64_t vbit;
> +		uint64_t ids = vfc->ids[j];
> +
> +		if (ids == 0)
> +			continue;
> +
> +		while (ids) {
> +			vlan = 64 * j;
> +			/* count trailing zeroes */
> +			vbit = ~ids & (ids - 1);
> +			/* clear least significant bit set */
> +			ids ^= (ids ^ (ids - 1)) ^ vbit;
> +			for (; vbit; vlan++)
> +				vbit >>= 1;
> +			ret = mrvl_vlan_filter_set(dev, vlan, 1);
> +			if (ret) {
> +				MRVL_LOG(ERR, "Failed to setup VLAN filter\n");
> +				goto out;
> +			}
> +		}
> +	}
> +
>   	/* For default QoS config, don't start classifier. */
>   	if (mrvl_qos_cfg  &&
>   	    mrvl_qos_cfg->port[dev->data->port_id].use_global_defaults == 
> 0) { @@ -687,10 +756,16 @@ mrvl_dev_start(struct rte_eth_dev *dev)
>   		}
>   	}
>   
> -	ret = mrvl_dev_set_link_up(dev);
> -	if (ret) {
> -		MRVL_LOG(ERR, "Failed to set link up");
> -		goto out;
> +	if (dev->data->promiscuous == 1)
> +		mrvl_promiscuous_enable(dev);
> +
> +	if (dev->data->dev_link.link_status == ETH_LINK_UP) {
> +		ret = mrvl_dev_set_link_up(dev);
> +		if (ret) {
> +			MRVL_LOG(ERR, "Failed to set link up");
> +			dev->data->dev_link.link_status = ETH_LINK_DOWN;
> +			goto out;
> +		}
>   	}
>   
>   	/* start tx queues */
> @@ -2936,6 +3011,8 @@ mrvl_eth_dev_create(struct rte_vdev_device *vdev, const char *name)
>   	eth_dev->dev_ops = &mrvl_ops;
>   	eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>   
> +	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
> +
>   	rte_eth_dev_probing_finish(eth_dev);
>   	return 0;
>   out_free:
>