Re: [dpdk-dev] [PATCH v5 04/14] drivers/net/ipn3ke: add IPN3KE representor of PMD driver

["Xu, Rosen" <rosen.xu@intel.com>]

Hi,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, April 05, 2019 3:02
> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org
> Cc: Zhang, Tianfei <tianfei.zhang@intel.com>; Wei, Dan
> <dan.wei@intel.com>; Pei, Andy <andy.pei@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>; Chen,
> Santos <santos.chen@intel.com>; Zhang, Zhang <zhang.zhang@intel.com>;
> Lomartire, David <david.lomartire@intel.com>
> Subject: Re: [PATCH v5 04/14] drivers/net/ipn3ke: add IPN3KE representor of
> PMD driver
> 
> On 4/3/2019 12:47 PM, Rosen Xu wrote:
> > Add Intel FPGA Acceleration NIC IPN3KE representor of PMD driver.
> >
> > Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> > Signed-off-by: Andy Pei <andy.pei@intel.com>
> > Signed-off-by: Dan Wei <dan.wei@intel.com>
> 
> <...>
> 
> > +static void
> > +ipn3ke_rpst_dev_stop(__rte_unused struct rte_eth_dev *dev) {
> 
> 'dev' is used in this function, can drop '__rte_unused'

Removed in v6.

> 
> > +	struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev);
> > +	struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev);
> > +	uint32_t val;
> > +
> > +	if (hw->retimer.mac_type ==
> IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) {
> > +	} else {
> 
> Why having empty 'if' condition and put everything into 'else', why not revert
> the check in if?
> Similar usage in many places.

Currently 25G MAC is auto configurable in FPGA, this condition is just to identify
10G MAC and 25G MAC configuration.
In v6, all the condition is exactly modified to' IFPGA_RAWDEV_RETIMER_MAC_TYPE_10GE_XFI'.

> > +	/* Disable the TX path */
> 
> Fix indentation please.
> 
> > +		val = 1;

Fixed in v6.

> I assume this is the value to write to say disable Tx path, I wonder if there is a
> define MACRO for it? If not does it make sense to have one?

Yep, added new inline functions for them.
ipn3ke_xmac_tx_enable()
ipn3ke_xmac_tx_disable()
ipn3ke_xmac_rx_enable()
ipn3ke_xmac_rx_disable()
ipn3ke_xmac_smac_override_disable()
ipn3ke_xmac_tx_clear_statistics()
ipn3ke_xmac_rx_clear_statistics()

> <...>
> 
> > +/*
> > + * Reset PF device only to re-initialize resources in PMD layer  */
> > +static int ipn3ke_rpst_dev_reset(struct rte_eth_dev *dev) {
> > +	struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev);
> > +	struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev);
> > +	uint32_t val;
> > +
> > +	if (hw->retimer.mac_type ==
> IFPGA_RAWDEV_RETIMER_MAC_TYPE_25GE_25GAUI) {
> > +	} else {
> > +		/* Disable the TX path */
> > +		val = 1;
> > +		val &= IPN3KE_MAC_TX_PACKET_CONTROL_MASK;
> > +		(*hw->f_mac_write)(hw,
> > +				val,
> > +				IPN3KE_MAC_TX_PACKET_CONTROL,
> > +				rpst->port_id,
> > +				0);
> > +
> > +		/* Disable the RX path */
> > +		val = 1;
> > +		val &= IPN3KE_MAC_RX_TRANSFER_CONTROL_MASK;
> > +		(*hw->f_mac_write)(hw,
> > +				val,
> > +				IPN3KE_MAC_RX_TRANSFER_CONTROL,
> > +				rpst->port_id,
> > +				0);
> > +	}
> 
> Same exact block repeated third time now, and there is one more copy as
> 'val = 0', what do you think converting this into function which gets
> enable/disable as argument so that 'val' also can be detail of that function.

I agree with you, and fixed it in v6.
Added some inline functions in v6.
Pls see my reply before.

> <...>
> 
> > +static int
> > +ipn3ke_rpst_xstats_get(__rte_unused struct rte_eth_dev *dev,
> > +	__rte_unused struct rte_eth_xstat *xstats,
> > +	__rte_unused unsigned int n)
> > +{
> > +	return 0;
> > +}
> > +static int ipn3ke_rpst_xstats_get_names(__rte_unused struct
> rte_eth_dev *dev,
> > +		__rte_unused struct rte_eth_xstat_name *xstats_names,
> > +		__rte_unused unsigned int limit)
> > +{
> > +	return 0;
> > +}
> 
> Can you please apply same syntax to this function as others:
> static int
> ....

Yep, checked and fixed in all code.

> Also can you please put a empty line between previous and this function.
> 
> <...>
> 

Yep, checked and fixed in all code.

> > +static int
> > +ipn3ke_rpst_link_check(struct ipn3ke_rpst *rpst) {
> > +	struct ipn3ke_hw *hw;
> > +	struct rte_rawdev *rawdev;
> > +	struct rte_eth_link link;
> > +	struct rte_eth_dev *pf;
> > +
> > +	if (rpst == NULL)
> > +		return -1;
> > +
> > +	hw = rpst->hw;
> > +
> > +	memset(&link, 0, sizeof(link));
> > +
> > +	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> > +	link.link_autoneg = !(rpst->ethdev->data->dev_conf.link_speeds &
> > +				ETH_LINK_SPEED_FIXED);
> > +
> > +	rawdev = hw->rawdev;
> > +	ipn3ke_update_link(rawdev, rpst->port_id, &link);
> > +
> > +	if (!rpst->ori_linfo.link_status &&
> > +		link.link_status) {
> 
> Better to put one more tab here, to make it easy to pick the condition part
> from the indendent code.

Okay, has put one more tab here in v6.

> <...>
> 
> > +
> > +struct eth_dev_ops ipn3ke_rpst_dev_ops = {
> 
> This can be "static const"

Yep, fixed in v6.

> > +	.dev_infos_get        = ipn3ke_rpst_dev_infos_get,
> > +
> > +	.dev_configure        = ipn3ke_rpst_dev_configure,
> > +	.dev_start            = ipn3ke_rpst_dev_start,
> > +	.dev_stop             = ipn3ke_rpst_dev_stop,
> > +	.dev_close            = ipn3ke_rpst_dev_close,
> > +	.dev_reset            = ipn3ke_rpst_dev_reset,
> > +
> > +	.stats_get            = ipn3ke_rpst_stats_get,
> > +	.xstats_get           = ipn3ke_rpst_xstats_get,
> > +	.xstats_get_names     = ipn3ke_rpst_xstats_get_names,
> > +	.stats_reset          = ipn3ke_rpst_stats_reset,
> > +	.xstats_reset         = ipn3ke_rpst_stats_reset,
> > +
> > +	.filter_ctrl          = ipn3ke_afu_filter_ctrl,
> > +
> > +	.rx_queue_start       = ipn3ke_rpst_rx_queue_start,
> > +	.rx_queue_stop        = ipn3ke_rpst_rx_queue_stop,
> > +	.tx_queue_start       = ipn3ke_rpst_tx_queue_start,
> > +	.tx_queue_stop        = ipn3ke_rpst_tx_queue_stop,
> > +	.rx_queue_setup       = ipn3ke_rpst_rx_queue_setup,
> > +	.rx_queue_release     = ipn3ke_rpst_rx_queue_release,
> > +	.tx_queue_setup       = ipn3ke_rpst_tx_queue_setup,
> > +	.tx_queue_release     = ipn3ke_rpst_tx_queue_release,
> > +
> > +	.dev_set_link_up      = ipn3ke_rpst_dev_set_link_up,
> > +	.dev_set_link_down    = ipn3ke_rpst_dev_set_link_down,
> > +	.link_update          = ipn3ke_rpst_link_update,
> > +
> > +	.promiscuous_enable   = ipn3ke_rpst_promiscuous_enable,
> > +	.promiscuous_disable  = ipn3ke_rpst_promiscuous_disable,
> > +	.allmulticast_enable  = ipn3ke_rpst_allmulticast_enable,
> > +	.allmulticast_disable = ipn3ke_rpst_allmulticast_disable,
> > +	.mac_addr_set         = ipn3ke_rpst_mac_addr_set,
> > +	.mtu_set              = ipn3ke_rpst_mtu_set,
> > +
> > +	.tm_ops_get           = NULL,
> 
> No need to set NULL ones.

Fixed in v6.

> <...>
> 
> > +int
> > +ipn3ke_rpst_init(struct rte_eth_dev *ethdev, void *init_params) {
> > +	struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(ethdev);
> > +	struct ipn3ke_rpst *representor_param =
> > +			(struct ipn3ke_rpst *)init_params;
> > +
> > +	if (representor_param->port_id >= representor_param->hw-
> >port_num)
> > +		return -ENODEV;
> > +
> > +	rpst->ethdev = ethdev;
> > +	rpst->switch_domain_id = representor_param->switch_domain_id;
> > +	rpst->port_id = representor_param->port_id;
> > +	rpst->hw = representor_param->hw;
> > +	rpst->i40e_pf_eth = NULL;
> > +	rpst->i40e_pf_eth_port_id = 0xFFFF;
> > +
> > +	ethdev->data->mac_addrs = rte_zmalloc("ipn3ke",
> ETHER_ADDR_LEN, 0);
> > +	if (!ethdev->data->mac_addrs) {
> > +		IPN3KE_AFU_PMD_ERR("Failed to "
> > +			"allocated memory for storing mac address");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/** representor shares the same driver as it's PF device */
> > +	/**
> > +	 * ethdev->device->driver = rpst->hw->eth_dev->device->driver;
> > +	 */
> 
> Please remove commented code.

Okay, fixed in v6.