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

[Yanling Song <songyl@ramaxel.com>]

On Wed, 19 Jan 2022 17:05:14 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> 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?
> 
It is a mistake. Will be removed in the next version.
> 
> <...>
> 
> > +#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.

OK. 'SPNIC_RELEASE' will be removed. 

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

Ok. Will be changed in the next version.