Re: [PATCH net-next v3 6/8] ice: add individual interrupt allocation

[Simon Horman <simon.horman@corigine.com>]

On Thu, Mar 23, 2023 at 01:24:38PM +0100, Piotr Raczynski wrote:
> Currently interrupt allocations, depending on a feature are distributed
> in batches. Also, after allocation there is a series of operations that
> distributes per irq settings through that batch of interrupts.
> 
> Although driver does not yet support dynamic interrupt allocation, keep
> allocated interrupts in a pool and add allocation abstraction logic to
> make code more flexible. Keep per interrupt information in the
> ice_q_vector structure, which yields ice_vsi::base_vector redundant.
> Also, as a result there are a few functions that can be removed.
> 
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Signed-off-by: Piotr Raczynski <piotr.raczynski@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

I've made a few minor observations inline.
I don't think there is a need to respin for any of them.

> ---
>  drivers/net/ethernet/intel/ice/ice.h         |  11 +-
>  drivers/net/ethernet/intel/ice/ice_arfs.c    |   5 +-
>  drivers/net/ethernet/intel/ice/ice_base.c    |  36 ++-
>  drivers/net/ethernet/intel/ice/ice_ethtool.c |   2 +-
>  drivers/net/ethernet/intel/ice/ice_idc.c     |  45 ++--
>  drivers/net/ethernet/intel/ice/ice_irq.c     |  46 +++-
>  drivers/net/ethernet/intel/ice/ice_irq.h     |   3 +
>  drivers/net/ethernet/intel/ice/ice_lib.c     | 225 ++-----------------

Nice code removal from ice_lib.c :)

>  drivers/net/ethernet/intel/ice/ice_lib.h     |   4 +-
>  drivers/net/ethernet/intel/ice/ice_main.c    |  44 ++--
>  drivers/net/ethernet/intel/ice/ice_ptp.c     |   2 +-
>  drivers/net/ethernet/intel/ice/ice_sriov.c   |   2 +-
>  drivers/net/ethernet/intel/ice/ice_xsk.c     |   5 +-
>  13 files changed, 154 insertions(+), 276 deletions(-)

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> index 1911d644dfa8..e5db23eaa3f4 100644
> --- a/drivers/net/ethernet/intel/ice/ice_base.c
> +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> @@ -118,9 +118,31 @@ static int ice_vsi_alloc_q_vector(struct ice_vsi *vsi, u16 v_idx)
>  	q_vector->rx.itr_mode = ITR_DYNAMIC;
>  	q_vector->tx.type = ICE_TX_CONTAINER;
>  	q_vector->rx.type = ICE_RX_CONTAINER;
> +	q_vector->irq.index = -ENOENT;
>  
> -	if (vsi->type == ICE_VSI_VF)
> +	if (vsi->type == ICE_VSI_VF) {
> +		q_vector->reg_idx = ice_calc_vf_reg_idx(vsi->vf, q_vector);
>  		goto out;
> +	} else if (vsi->type == ICE_VSI_CTRL && vsi->vf) {
> +		struct ice_vsi *ctrl_vsi = ice_get_vf_ctrl_vsi(pf, vsi);
> +
> +		if (ctrl_vsi) {
> +			if (unlikely(!ctrl_vsi->q_vectors))
> +				return -ENOENT;
> +			q_vector->irq = ctrl_vsi->q_vectors[0]->irq;
> +			goto skip_alloc;

nit: I think goto for error paths is very much the norm.
     But, FWIIW, I would have avoided using goto here.

> +		}
> +	}
> +
> +	q_vector->irq = ice_alloc_irq(pf);
> +	if (q_vector->irq.index < 0) {
> +		kfree(q_vector);
> +		return -ENOMEM;
> +	}
> +
> +skip_alloc:
> +	q_vector->reg_idx = q_vector->irq.index;
> +
>  	/* only set affinity_mask if the CPU is online */
>  	if (cpu_online(v_idx))
>  		cpumask_set_cpu(v_idx, &q_vector->affinity_mask);
> @@ -168,6 +190,18 @@ static void ice_free_q_vector(struct ice_vsi *vsi, int v_idx)
>  	if (vsi->netdev)
>  		netif_napi_del(&q_vector->napi);
>  
> +	/* release MSIX interrupt if q_vector had interrupt allocated */
> +	if (q_vector->irq.index < 0)
> +		goto free_q_vector;
> +
> +	/* only free last VF ctrl vsi interrupt */
> +	if (vsi->type == ICE_VSI_CTRL && vsi->vf &&
> +	    ice_get_vf_ctrl_vsi(pf, vsi))
> +		goto free_q_vector;

Ditto (x2).

> +
> +	ice_free_irq(pf, q_vector->irq);
> +
> +free_q_vector:
>  	devm_kfree(dev, q_vector);
>  	vsi->q_vectors[v_idx] = NULL;
>  }

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c
> index f61be5d76373..ca1a1de26766 100644
> --- a/drivers/net/ethernet/intel/ice/ice_irq.c
> +++ b/drivers/net/ethernet/intel/ice/ice_irq.c
> @@ -194,9 +194,53 @@ int ice_init_interrupt_scheme(struct ice_pf *pf)
>  	}
>  
>  	/* populate SW interrupts pool with number of OS granted IRQs. */
> -	pf->num_avail_sw_msix = (u16)vectors;
>  	pf->irq_tracker->num_entries = (u16)vectors;
>  	pf->irq_tracker->end = pf->irq_tracker->num_entries;
>  
>  	return 0;
>  }
> +
> +/**
> + * ice_alloc_irq - Allocate new interrupt vector
> + * @pf: board private structure
> + *
> + * Allocate new interrupt vector for a given owner id.
> + * return struct msi_map with interrupt details and track
> + * allocated interrupt appropriately.
> + *
> + * This function mimics individual interrupt allocation,
> + * even interrupts are actually already allocated with
> + * pci_alloc_irq_vectors. Individual allocation helps
> + * to track interrupts and simplifies interrupt related
> + * handling.
> + *
> + * On failure, return map with negative .index. The caller
> + * is expected to check returned map index.
> + *
> + */
> +struct msi_map ice_alloc_irq(struct ice_pf *pf)
> +{
> +	struct msi_map map = { .index = -ENOENT };
> +	int entry;
> +
> +	entry = ice_get_res(pf, pf->irq_tracker);
> +	if (entry < 0)

nit: map.index could be initialised here.

> +		return map;
> +
> +	map.index = entry;
> +	map.virq = pci_irq_vector(pf->pdev, map.index);
> +
> +	return map;
> +}