lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZCMSzzw1MTIc9dZW@nimitz> Date: Tue, 28 Mar 2023 18:16:15 +0200 From: Piotr Raczynski <piotr.raczynski@...el.com> To: Simon Horman <simon.horman@...igine.com> CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>, <michal.swiatkowski@...el.com>, <shiraz.saleem@...el.com>, <jacob.e.keller@...el.com>, <sridhar.samudrala@...el.com>, <jesse.brandeburg@...el.com>, <aleksander.lobakin@...el.com>, <lukasz.czapnik@...el.com>, Michal Swiatkowski <michal.swiatkowski@...ux.intel.com> Subject: Re: [PATCH net-next v3 6/8] ice: add individual interrupt allocation On Sun, Mar 26, 2023 at 03:18:03PM +0200, Simon Horman wrote: > 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@...el.com> > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com> > > Signed-off-by: Piotr Raczynski <piotr.raczynski@...el.com> > > Reviewed-by: Simon Horman <simon.horman@...igine.com> > > I've made a few minor observations inline. > I don't think there is a need to respin for any of them. > Thanks. > > --- > > 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. Thanks will take a look. > > > + } > > + } > > + > > + 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). Will also take a look. > > > + > > + 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; > > +}
Powered by blists - more mailing lists