Re: [PATCH v6 02/26] net/spnic: initialize the HW interface

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/30/2021 6:08 AM, Yanling Song wrote:
> Add HW interface registers and initialize the HW
> interface.
> 
> Signed-off-by: Yanling Song <songyl@ramaxel.com>

<...>

> diff --git a/drivers/net/spnic/base/spnic_hwdev.h b/drivers/net/spnic/base/spnic_hwdev.h
> new file mode 100644
> index 0000000000..c89a4fa840
> --- /dev/null
> +++ b/drivers/net/spnic/base/spnic_hwdev.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Ramaxel Memory Technology, Ltd
> + */
> +
> +#ifndef _SPNIC_HWDEV_H_
> +#define _SPNIC_HWDEV_H_
> +
> +#include <rte_ether.h>
> +

Why is this header required in this file?


<...>

> +#ifdef SPNIC_RELEASE
> +static int wait_until_doorbell_flush_states(struct spnic_hwif *hwif,
> +					    enum spnic_doorbell_ctrl states)
> +{
> +	enum spnic_doorbell_ctrl db_ctrl;
> +	u32 cnt = 0;
> +
> +	if (!hwif)
> +		return -EINVAL;
> +
> +	while (cnt < SPNIC_WAIT_DOORBELL_AND_OUTBOUND_TIMEOUT) {
> +		db_ctrl = spnic_get_doorbell_ctrl_status(hwif);
> +		if (db_ctrl == states)
> +			return 0;
> +
> +		rte_delay_ms(1);
> +		cnt++;
> +	}
> +
> +	return -EFAULT;
> +}
> +#endif

What is this 'SPNIC_RELEASE' macro and why it exists?

Please get rid of all all compile time macros, if the code is not required you
can delete it while upstreaming.

<...>

>   struct spnic_nic_dev {
> +	struct spnic_hwdev *hwdev; /* Hardware device */
> +
> +	struct spnic_txq **txqs;
> +	struct spnic_rxq **rxqs;
> +	struct rte_mempool *cpy_mpool;
> +
> +	u16 num_sqs;
> +	u16 num_rqs;
> +	u16 max_sqs;
> +	u16 max_rqs;
> +
> +	u16 rx_buff_len;
> +	u16 mtu_size;
> +
> +	u16 rss_state;
> +	u8 num_rss;
> +	u8 rsvd0;
> +
> +	u32 rx_mode;
> +	u8 rx_queue_list[SPNIC_MAX_QUEUE_NUM];
> +	rte_spinlock_t queue_list_lock;
> +	pthread_mutex_t rx_mode_mutex;
> +
> +	u32 default_cos;
> +	u32 rx_csum_en;
> +
>   	u32 dev_status;
> +
> +	bool pause_set;
> +	pthread_mutex_t pause_mutuex;
> +
> +	struct rte_ether_addr default_addr;
> +	struct rte_ether_addr *mc_list;
> +
>   	char dev_name[SPNIC_DEV_NAME_LEN];
> +	u32 vfta[SPNIC_VFTA_SIZE]; /* VLAN bitmap */
>   };


Most of these additions to the struct is not used at all, can you please add them
when they are used?