Re: [dpdk-dev] [PATCH 5/5] net/bnxt: Support for rx/tx_queue_start/stop ops

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

On 1/22/2018 6:20 AM, Ajit Khaparde wrote:
> Currently this is implemented entirely in the PMD as there is no explicit
> support in the HW. Re-program the RSS Table without this queue on stop
> and add it back to the table on start.
> 
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

<...>

> @@ -1,4 +1,4 @@
> -/*-
> +/*
>   *   BSD LICENSE
>   *
>   *   Copyright(c) Broadcom Limited.

Unrelated but since I saw this, do you plan switching to the SPDX tags?

<...>

> @@ -321,8 +334,7 @@ static int bnxt_init_chip(struct bnxt *bp)
>  	     !RTE_ETH_DEV_SRIOV(bp->eth_dev).active) &&
>  	    bp->eth_dev->data->dev_conf.intr_conf.rxq != 0) {
>  		intr_vector = bp->eth_dev->data->nb_rx_queues;
> -		PMD_DRV_LOG(INFO, "%s(): intr_vector = %d\n", __func__,
> -			intr_vector);
> +		PMD_DRV_LOG(INFO, "intr_vector = %d\n", intr_vector);

With new logging macro "__func__" is duplicate, why not fix them all in the
patch 2/5 that introduces new macro?

<...>

> @@ -2969,6 +2981,49 @@ bnxt_set_eeprom_op(struct rte_eth_dev *dev,
>  	return 0;
>  }
>  
> +int bnxt_rx_queue_start(struct rte_eth_dev *dev, uint16_t rx_queue_id)
> +{
> +	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> +	struct rte_eth_conf *dev_conf = &bp->eth_dev->data->dev_conf;
> +	struct bnxt_rx_queue *rxq = bp->rx_queues[rx_queue_id];
> +	struct bnxt_vnic_info *vnic = NULL;
> +
> +	dev->data->rx_queue_state[rx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
> +	rxq->rx_deferred_start = false;
> +	PMD_DRV_LOG(INFO, "Rx queue started %d\n", rx_queue_id);
> +	if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) {
> +		vnic = rxq->vnic;
> +		if (vnic->fw_grp_ids[rx_queue_id] != INVALID_HW_RING_ID)
> +			return 0;
> +		PMD_DRV_LOG(DEBUG, "vnic = %p fw_grp_id = %d\n",
> +			vnic, bp->grp_info[rx_queue_id + 1].fw_grp_id);
> +		vnic->fw_grp_ids[rx_queue_id] =
> +					bp->grp_info[rx_queue_id + 1].fw_grp_id;
> +		return bnxt_vnic_rss_configure(bp, vnic);
> +	}
> +
> +	return 0;
> +}
> +
> +int bnxt_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id)
> +{
> +	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> +	struct rte_eth_conf *dev_conf = &bp->eth_dev->data->dev_conf;
> +	struct bnxt_rx_queue *rxq = bp->rx_queues[rx_queue_id];
> +	struct bnxt_vnic_info *vnic = NULL;
> +
> +	dev->data->rx_queue_state[rx_queue_id] = RTE_ETH_QUEUE_STATE_STOPPED;
> +	rxq->rx_deferred_start = true;
> +	PMD_DRV_LOG(DEBUG, "Rx queue stopped\n");
> +
> +	if (dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) {
> +		vnic = rxq->vnic;
> +		vnic->fw_grp_ids[rx_queue_id] = INVALID_HW_RING_ID;
> +		return bnxt_vnic_rss_configure(bp, vnic);
> +	}
> +	return 0;
> +}

There is already a source file "bnxt_rxq.c", which seems for Rxq related
functions, why not add new functions there?

<...>

> @@ -364,3 +369,30 @@ uint16_t bnxt_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  
>  	return nb_tx_pkts;
>  }
> +
> +int bnxt_tx_queue_start(struct rte_eth_dev *dev, uint16_t tx_queue_id)
> +{
> +	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> +	struct bnxt_tx_queue *txq = bp->tx_queues[tx_queue_id];
> +
> +	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STARTED;
> +	txq->tx_deferred_start = false;
> +	PMD_DRV_LOG(DEBUG, "Tx queue started\n");
> +
> +	return 0;
> +}
> +
> +int bnxt_tx_queue_stop(struct rte_eth_dev *dev, uint16_t tx_queue_id)
> +{
> +	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
> +	struct bnxt_tx_queue *txq = bp->tx_queues[tx_queue_id];
> +
> +	/* Handle TX completions */
> +	bnxt_handle_tx_cp(txq);
> +
> +	dev->data->tx_queue_state[tx_queue_id] = RTE_ETH_QUEUE_STATE_STOPPED;
> +	txq->tx_deferred_start = true;
> +	PMD_DRV_LOG(DEBUG, "Tx queue stopped\n");
> +
> +	return 0;
> +}

Similar question for these functions, they seem implemented in txr (Tx Ring ?)
source file, bnxt_txq.c seems better fit, what do you think?

<...>