netdev - Re: [PATCH net-next] be2net: fix INTx ISR for interrupt behaviour on BE2

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]

Message-ID: <1354134328.2768.16.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Wed, 28 Nov 2012 20:25:28 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Sathya Perla <sathya.perla@...lex.com>
CC:	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] be2net: fix INTx ISR for interrupt behaviour
 on BE2

On Wed, 2012-11-28 at 20:20 +0000, Ben Hutchings wrote:
> On Wed, 2012-11-28 at 11:20 +0530, Sathya Perla wrote:
> > On BE2 chip, an interrupt may be raised even when EQ is in un-armed state.
> > As a result be_intx()::events_get() and be_poll:events_get() can race and
> > notify an EQ wrongly.
> > 
> > Fix this by counting events only in be_poll(). Commit 0b545a629 fixes
> > the same issue in the MSI-x path.
> > 
> > But, on Lancer, INTx can be de-asserted only by notifying num evts. This
> > is not an issue as the above BE2 behavior doesn't exist/has never been
> > seen on Lancer.
> [...]
> > @@ -2014,15 +1996,23 @@ static int be_rx_cqs_create(struct be_adapter *adapter)
> >  
> >  static irqreturn_t be_intx(int irq, void *dev)
> >  {
> > -	struct be_adapter *adapter = dev;
> > -	int num_evts;
> > +	struct be_eq_obj *eqo = dev;
> > +	struct be_adapter *adapter = eqo->adapter;
> > +	int num_evts = 0;
> >  
> > -	/* With INTx only one EQ is used */
> > -	num_evts = event_handle(&adapter->eq_obj[0]);
> > -	if (num_evts)
> > -		return IRQ_HANDLED;
> > -	else
> > -		return IRQ_NONE;
> > +	/* On Lancer, clear-intr bit of the EQ DB does not work.
> > +	 * INTx is de-asserted only on notifying num evts.
> > +	 */
> > +	if (lancer_chip(adapter))
> > +		num_evts = events_get(eqo);
> > +
> > +	/* The EQ-notify may not de-assert INTx rightaway, causing
> > +	 * the ISR to be invoked again. So, return HANDLED even when
> > +	 * num_evts is zero.
> > +	 */
> > +	be_eq_notify(adapter, eqo->q.id, false, true, num_evts);
> > +	napi_schedule(&eqo->napi);
> > +	return IRQ_HANDLED;
> >  }
> [...]
> 
> You shouldn't unconditionally return IRQ_HANDLED.  This prevents
> interrupt storm detection from working, not just for your device but for
> anything else sharing its IRQ.
> 
> I understand there is a real problem to be fixed (PCIe write completions
> overtaking INTx deassertion, and maybe a specific hardware bug).
[...]

I was thinking of read completions; there are no write completions to
wait for so you're pretty much guaranteed to get called a second time.
Maybe you should add an MMIO read after calling be_eq_notify().

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html