netdev - RE: [net-next v5 08/15] iecm: Implement vector allocation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Aug 2020 17:28:29 +0000
From:   "Brady, Alan" <alan.brady@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "Michael, Alice" <alice.michael@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "Burra, Phani R" <phani.r.burra@...el.com>,
        "Hay, Joshua A" <joshua.a.hay@...el.com>,
        "Chittim, Madhu" <madhu.chittim@...el.com>,
        "Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>,
        "Skidmore, Donald C" <donald.c.skidmore@...el.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [net-next v5 08/15] iecm: Implement vector allocation

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, August 24, 2020 1:42 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>
> Cc: davem@...emloft.net; Michael, Alice <alice.michael@...el.com>;
> netdev@...r.kernel.org; nhorman@...hat.com; sassmann@...hat.com;
> Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; Brady, Alan
> <alan.brady@...el.com>; Burra, Phani R <phani.r.burra@...el.com>; Hay,
> Joshua A <joshua.a.hay@...el.com>; Chittim, Madhu
> <madhu.chittim@...el.com>; Linga, Pavan Kumar
> <pavan.kumar.linga@...el.com>; Skidmore, Donald C
> <donald.c.skidmore@...el.com>; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; Samudrala, Sridhar
> <sridhar.samudrala@...el.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