Re: [dpdk-dev] [PATCH v6 12/19] net/ngbe: add Rx queue setup and release

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On July 6, 2021 4:06 PM, Andrew Rybchenko wrote:
> On 7/6/21 10:53 AM, Jiawen Wu wrote:
> > On July 5, 2021 5:08 PM, Andrew Rybchenko wrote:
> >> On 7/5/21 11:36 AM, Jiawen Wu wrote:
> >>> On July 3, 2021 12:36 AM, Andrew Rybchenko wrote:
> >>>> On 6/17/21 1:59 PM, Jiawen Wu wrote:
> >>>>> Setup device Rx queue and release Rx queue.
> >>>>>
> >>>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> >>>>> ---
> >>>>>  drivers/net/ngbe/meson.build   |   1 +
> >>>>>  drivers/net/ngbe/ngbe_ethdev.c |  37 +++-
> >>>>> drivers/net/ngbe/ngbe_ethdev.h |  16 ++
> >>>>>  drivers/net/ngbe/ngbe_rxtx.c   | 308
> +++++++++++++++++++++++++++++++++
> >>>>>  drivers/net/ngbe/ngbe_rxtx.h   |  96 ++++++++++
> >>>>>  5 files changed, 457 insertions(+), 1 deletion(-)  create mode
> >>>>> 100644 drivers/net/ngbe/ngbe_rxtx.c  create mode 100644
> >>>>> drivers/net/ngbe/ngbe_rxtx.h
> >>>>>
> >>>>> a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
> >>>>> index c952023e8b..e73606c5f3 100644
> >>>>> --- a/drivers/net/ngbe/ngbe_ethdev.c
> >>>>> +++ b/drivers/net/ngbe/ngbe_ethdev.c
> >>>>> @@ -37,6 +38,12 @@ static const struct rte_pci_id pci_id_ngbe_map[]
> = {
> >>>>>  	{ .vendor_id = 0, /* sentinel */ },  };
> >>>>>
> >>>>> +static const struct rte_eth_desc_lim rx_desc_lim = {
> >>>>> +	.nb_max = NGBE_RING_DESC_MAX,
> >>>>> +	.nb_min = NGBE_RING_DESC_MIN,
> >>>>> +	.nb_align = NGBE_RXD_ALIGN,
> >>>>> +};
> >>>>> +
> >>>>>  static const struct eth_dev_ops ngbe_eth_dev_ops;
> >>>>>
> >>>>>  static inline void
> >>>>> @@ -266,11 +280,30 @@ ngbe_dev_close(struct rte_eth_dev *dev)
> >>>>> static int  ngbe_dev_info_get(struct rte_eth_dev *dev, struct
> >>>>> rte_eth_dev_info *dev_info)  {
> >>>>> -	RTE_SET_USED(dev);
> >>>>> +	struct ngbe_hw *hw = ngbe_dev_hw(dev);
> >>>>> +
> >>>>> +	dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
> >>>>> +
> >>>>> +	dev_info->default_rxconf = (struct rte_eth_rxconf) {
> >>>>> +		.rx_thresh = {
> >>>>> +			.pthresh = NGBE_DEFAULT_RX_PTHRESH,
> >>>>> +			.hthresh = NGBE_DEFAULT_RX_HTHRESH,
> >>>>> +			.wthresh = NGBE_DEFAULT_RX_WTHRESH,
> >>>>> +		},
> >>>>> +		.rx_free_thresh = NGBE_DEFAULT_RX_FREE_THRESH,
> >>>>> +		.rx_drop_en = 0,
> >>>>> +		.offloads = 0,
> >>>>> +	};
> >>>>> +
> >>>>> +	dev_info->rx_desc_lim = rx_desc_lim;
> >>>>>
> > <...>
> >>>>> +int __rte_cold
> >>>>> +ngbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
> >>>>> +			 uint16_t queue_idx,
> >>>>> +			 uint16_t nb_desc,
> >>>>> +			 unsigned int socket_id,
> >>>>> +			 const struct rte_eth_rxconf *rx_conf,
> >>>>> +			 struct rte_mempool *mp)
> >>>>> +{
> >>>>> +	const struct rte_memzone *rz;
> >>>>> +	struct ngbe_rx_queue *rxq;
> >>>>> +	struct ngbe_hw     *hw;
> >>>>> +	uint16_t len;
> >>>>> +	struct ngbe_adapter *adapter = ngbe_dev_adapter(dev);
> >>>>> +
> >>>>> +	PMD_INIT_FUNC_TRACE();
> >>>>> +	hw = ngbe_dev_hw(dev);
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * Validate number of receive descriptors.
> >>>>> +	 * It must not exceed hardware maximum, and must be multiple
> >>>>> +	 * of NGBE_ALIGN.
> >>>>> +	 */
> >>>>> +	if (nb_desc % NGBE_RXD_ALIGN != 0 ||
> >>>>> +			nb_desc > NGBE_RING_DESC_MAX ||
> >>>>> +			nb_desc < NGBE_RING_DESC_MIN) {
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>
> >>>> rte_eth_rx_queue_setup cares about it
> >>>>
> >>>
> >>> I don't quite understand.
> >>
> >> ethdev does the check based dev_info provided by the driver:
> >>
> >> if (nb_rx_desc > dev_info.rx_desc_lim.nb_max ||
> >>     nb_rx_desc < dev_info.rx_desc_lim.nb_min ||
> >>     nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) {
> >>         RTE_ETHDEV_LOG(ERR,
> >>             "Invalid value for nb_rx_desc(=%hu), should be: <= %hu,
> >> >= %hu, and a product of %hu\n",
> >>             nb_rx_desc, dev_info.rx_desc_lim.nb_max,
> >>             dev_info.rx_desc_lim.nb_min,
> >>             dev_info.rx_desc_lim.nb_align);
> >>        return -EINVAL;
> >> }
> >
> > 'dev_info.rx_desc_lim' was set up in function 'ngbe_dev_info_get' with the
> struct 'rx_desc_lim' defined.
> > So I think it is appropriate to check with macros. However, the log
> > can be
> Sorry, but I don't understand why do you want to duplicate check which is
> already done in a generic code.

This check can be omitted, thanks.
NGBE driver code was ported from some earlier code, there may some redundant code left.