Re: [dpdk-dev] [PATCH 2/5] fm10k: enable Rx queue interrupts for PF and VF

["Chen, Jing D" <jing.d.chen@intel.com>]

Hi,

Best Regards,
Mark


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shaopeng He
> Sent: Friday, September 25, 2015 1:37 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/5] fm10k: enable Rx queue interrupts for PF
> and VF
> 
> The patch does below things for fm10k PF and VF:
> - Setup NIC to generate MSI-X interrupts
> - Set the RXINT register to map interrupt causes to vectors
> - Implement interrupt enable/disable functions

The description is too simple, can you help to extend?
Besides that, there are complicated changes in this patch. 
Can you help you split it to several smaller ones for better understanding?

> 
> Signed-off-by: Shaopeng He <shaopeng.he@intel.com>
> ---
>  drivers/net/fm10k/fm10k_ethdev.c | 147
> +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 140 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> b/drivers/net/fm10k/fm10k_ethdev.c
> index a82cd59..6648934 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> 
>  static int
> +fm10k_dev_rx_queue_intr_enable(struct rte_eth_dev *dev, uint16_t
> queue_id)
> +{
> +	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +
> +	/* Enable ITR */
> +	if (hw->mac.type == fm10k_mac_pf)
> +		FM10K_WRITE_REG(hw, FM10K_ITR(Q2V(dev, queue_id)),
> +			FM10K_ITR_AUTOMASK |
> FM10K_ITR_MASK_CLEAR);
> +	else
> +		FM10K_WRITE_REG(hw, FM10K_VFITR(Q2V(dev, queue_id)),
> +			FM10K_ITR_AUTOMASK |
> FM10K_ITR_MASK_CLEAR);
> +	rte_intr_enable(&dev->pci_dev->intr_handle);
> +	return 0;
> +}
> +
> +static int
> +fm10k_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t
> queue_id)
> +{
> +	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +
> +	/* Disable ITR */
> +	if (hw->mac.type == fm10k_mac_pf)
> +		FM10K_WRITE_REG(hw, FM10K_ITR(Q2V(dev, queue_id)),
> +			FM10K_ITR_MASK_SET);
> +	else
> +		FM10K_WRITE_REG(hw, FM10K_VFITR(Q2V(dev, queue_id)),
> +			FM10K_ITR_MASK_SET);

In previous enable function, you'll use rte_intr_enable() to enable interrupt, but 
You needn't disable it in this function? 

> +	return 0;
> +}
> +
> +static int
> +fm10k_dev_rxq_interrupt_setup(struct rte_eth_dev *dev)
> +{
> +	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> +	struct rte_intr_handle *intr_handle = &dev->pci_dev->intr_handle;
> +	uint32_t intr_vector, vec;
> +	uint16_t queue_id;
> +	int result = 0;
> +
> +	/* fm10k needs interrupt for mailbox
> +	 * so igb_uio is not supported for rx interrupt
> +	 */

I guess you'll support both igb_uio and VFIO, RX interrupt mode only enabled in case VFIO is used.
I suggest you add more comments here for better understanding.

> +	if (!rte_intr_cap_multiple(intr_handle) ||
> +			dev->data->dev_conf.intr_conf.rxq == 0)
> +		return result;
> +
> +	intr_vector = dev->data->nb_rx_queues;
> +
> +	/* disable interrupt first */
> +	rte_intr_disable(&dev->pci_dev->intr_handle);
> +	if (hw->mac.type == fm10k_mac_pf)
> +		fm10k_dev_disable_intr_pf(dev);
> +	else
> +		fm10k_dev_disable_intr_vf(dev);
> +
> +	if (rte_intr_efd_enable(intr_handle, intr_vector)) {
> +		PMD_INIT_LOG(ERR, "Failed to init event fd");
> +		result = -EIO;
> +	}
> +
> +	if (rte_intr_dp_is_en(intr_handle) && !result) {
> +		intr_handle->intr_vec =	rte_zmalloc("intr_vec",
> +			dev->data->nb_rx_queues * sizeof(int), 0);
> +		if (intr_handle->intr_vec) {
> +			for (queue_id = 0, vec = RX_VEC_START;
> +					queue_id < dev->data-
> >nb_rx_queues;
> +					queue_id++) {
> +				intr_handle->intr_vec[queue_id] = vec;
> +				if (vec < intr_handle->nb_efd - 1 +
> RX_VEC_START)
> +					vec++;

No "else" to handle exceptional case?

> +			}
> +		} else {
> +			PMD_INIT_LOG(ERR, "Failed to allocate %d
> rx_queues"
> +				" intr_vec", dev->data->nb_rx_queues);
> +			result = -ENOMEM;
> +		}
> +	}
> +
> +	if (hw->mac.type == fm10k_mac_pf)
> +		fm10k_dev_enable_intr_pf(dev);
> +	else
> +		fm10k_dev_enable_intr_vf(dev);
> +	rte_intr_enable(&dev->pci_dev->intr_handle);
> +	hw->mac.ops.update_int_moderator(hw);
> +	return result;
> +}
> +
> +static int
>  fm10k_dev_handle_fault(struct fm10k_hw *hw, uint32_t eicr)
>  {
>  	struct fm10k_fault fault;
> @@ -2050,6 +2181,8 @@ static const struct eth_dev_ops
> fm10k_eth_dev_ops = {
>  	.tx_queue_setup		= fm10k_tx_queue_setup,
>  	.tx_queue_release	= fm10k_tx_queue_release,
>  	.rx_descriptor_done	= fm10k_dev_rx_descriptor_done,
> +	.rx_queue_intr_enable	= fm10k_dev_rx_queue_intr_enable,
> +	.rx_queue_intr_disable	= fm10k_dev_rx_queue_intr_disable,
>  	.reta_update		= fm10k_reta_update,
>  	.reta_query		= fm10k_reta_query,
>  	.rss_hash_update	= fm10k_rss_hash_update,
> @@ -2060,7 +2193,7 @@ static int
>  eth_fm10k_dev_init(struct rte_eth_dev *dev)
>  {
>  	struct fm10k_hw *hw = FM10K_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> -	int diag;
> +	int diag, i;
>  	struct fm10k_macvlan_filter_info *macvlan;
> 
>  	PMD_INIT_FUNC_TRACE();
> @@ -2177,7 +2310,7 @@ eth_fm10k_dev_init(struct rte_eth_dev *dev)
>  		fm10k_dev_enable_intr_vf(dev);
>  	}
> 
> -	/* Enable uio intr after callback registered */
> +	/* Enable intr after callback registered */
>  	rte_intr_enable(&(dev->pci_dev->intr_handle));
> 
>  	hw->mac.ops.update_int_moderator(hw);
> @@ -2185,7 +2318,6 @@ eth_fm10k_dev_init(struct rte_eth_dev *dev)
>  	/* Make sure Switch Manager is ready before going forward. */
>  	if (hw->mac.type == fm10k_mac_pf) {
>  		int switch_ready = 0;
> -		int i;
> 
>  		for (i = 0; i < MAX_QUERY_SWITCH_STATE_TIMES; i++) {
>  			fm10k_mbx_lock(hw);
> @@ -2291,7 +2423,8 @@ static struct eth_driver rte_pmd_fm10k = {
>  	.pci_drv = {
>  		.name = "rte_pmd_fm10k",
>  		.id_table = pci_id_fm10k_map,
> -		.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> RTE_PCI_DRV_DETACHABLE,
> +		.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> RTE_PCI_DRV_INTR_LSC |
> +			RTE_PCI_DRV_DETACHABLE,
>  	},
>  	.eth_dev_init = eth_fm10k_dev_init,
>  	.eth_dev_uninit = eth_fm10k_dev_uninit,
> --
> 1.9.3