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

[Ferruh Yigit]

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 at intel.com>
> Signed-off-by: Andy Pei <andy.pei at intel.com>
> Signed-off-by: Dan Wei <dan.wei at 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'

> +	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.

> +	/* Disable the TX path */

Fix indentation please.

> +		val = 1;

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?

<...>

> +/*
> + * 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.

<...>

> +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
....

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

<...>

> +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.

<...>

> +
> +struct eth_dev_ops ipn3ke_rpst_dev_ops = {

This can be "static const"

> +	.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.

<...>

> +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.