> -----Original Message-----
> From: Jakub Kicinski <[email protected]>
> Sent: Monday, August 24, 2020 1:42 PM
> To: Nguyen, Anthony L <[email protected]>
> Cc: [email protected]; Michael, Alice <[email protected]>;
> [email protected]; [email protected]; [email protected];
> Kirsher, Jeffrey T <[email protected]>; Brady, Alan
> <[email protected]>; Burra, Phani R <[email protected]>; Hay,
> Joshua A <[email protected]>; Chittim, Madhu
> <[email protected]>; Linga, Pavan Kumar
> <[email protected]>; Skidmore, Donald C
> <[email protected]>; Brandeburg, Jesse
> <[email protected]>; Samudrala, Sridhar
> <[email protected]>
> 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
