> -----Original Message-----
> From: Jakub Kicinski <k...@kernel.org>
> Sent: Monday, August 24, 2020 1:42 PM
> To: Nguyen, Anthony L <anthony.l.ngu...@intel.com>
> Cc: da...@davemloft.net; Michael, Alice <alice.mich...@intel.com>;
> netdev@vger.kernel.org; nhor...@redhat.com; sassm...@redhat.com;
> Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>; Brady, Alan
> <alan.br...@intel.com>; Burra, Phani R <phani.r.bu...@intel.com>; Hay,
> Joshua A <joshua.a....@intel.com>; Chittim, Madhu
> <madhu.chit...@intel.com>; Linga, Pavan Kumar
> <pavan.kumar.li...@intel.com>; Skidmore, Donald C
> <donald.c.skidm...@intel.com>; Brandeburg, Jesse
> <jesse.brandeb...@intel.com>; Samudrala, Sridhar
> <sridhar.samudr...@intel.com>
> Subject: Re: [net-next v5 08/15] iecm: Implement vector allocation
>
> On Mon, 24 Aug 2020 10:32:59 -0700 Tony Nguyen wrote:
> > static void iecm_mb_intr_rel_irq(struct iecm_adapter *adapter) {
> > - /* stub */
> > + int irq_num;
> > +
> > + irq_num = adapter->msix_entries[0].vector;
> > + synchronize_irq(irq_num);
>
> I don't think you need to sync irq before freeing it.
>
I see other non-Intel drivers syncing before disable/free and Intel drivers
have historically done it, not that that's necessarily correct, but are you
certain?
> > + free_irq(irq_num, adapter);
>
>
> > static int iecm_mb_intr_init(struct iecm_adapter *adapter) {
> > - /* stub */
> > + int err = 0;
> > +
> > + iecm_get_mb_vec_id(adapter);
> > + adapter->dev_ops.reg_ops.mb_intr_reg_init(adapter);
> > + adapter->irq_mb_handler = iecm_mb_intr_clean;
> > + err = iecm_mb_intr_req_irq(adapter);
> > + return err;
>
> return iecm_mb_intr_req_irq(adapter);
>
> > static void iecm_vport_intr_rel_irq(struct iecm_vport *vport) {
> > - /* stub */
> > + struct iecm_adapter *adapter = vport->adapter;
> > + int vector;
> > +
> > + for (vector = 0; vector < vport->num_q_vectors; vector++) {
> > + struct iecm_q_vector *q_vector = &vport->q_vectors[vector];
> > + int irq_num, vidx;
> > +
> > + /* free only the IRQs that were actually requested */
> > + if (!q_vector)
> > + continue;
> > +
> > + vidx = vector + vport->q_vector_base;
> > + irq_num = adapter->msix_entries[vidx].vector;
> > +
> > + /* clear the affinity_mask in the IRQ descriptor */
> > + irq_set_affinity_hint(irq_num, NULL);
> > + synchronize_irq(irq_num);
>
> here as well
>
> > + free_irq(irq_num, q_vector);
> > + }
> > }
>
> > void iecm_vport_intr_dis_irq_all(struct iecm_vport *vport) {
> > - /* stub */
> > + struct iecm_q_vector *q_vector = vport->q_vectors;
> > + struct iecm_hw *hw = &vport->adapter->hw;
> > + int q_idx;
> > +
> > + for (q_idx = 0; q_idx < vport->num_q_vectors; q_idx++)
> > + writel_relaxed(0, hw->hw_addr +
> > + q_vector[q_idx].intr_reg.dyn_ctl);
>
> Why the use of _releaxed() here? is this performance-sensitive?
> There is no barrier after, which makes this code fragile.
>
_relaxed not required here, will fix.
-Alan
