netdev - Re: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

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] [day] [month] [year] [list]

Message-ID: <6c9f98de-4090-4fe2-8fa3-446c1907f50b@nbd.name>
Date: Fri, 28 Jun 2024 21:57:43 +0200
From: Felix Fietkau <nbd@....name>
To: Arnd Bergmann <arnd@...db.de>, Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Netdev <netdev@...r.kernel.org>, lorenzo.bianconi83@...il.com,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Conor Dooley <conor@...nel.org>,
 linux-arm-kernel@...ts.infradead.org, Rob Herring <robh+dt@...nel.org>,
 krzysztof.kozlowski+dt@...aro.org, Conor Dooley <conor+dt@...nel.org>,
 devicetree@...r.kernel.org, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>, upstream@...oha.com,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 benjamin.larsson@...exis.eu, linux-clk@...r.kernel.org,
 Ratheesh Kannoth <rkannoth@...vell.com>, Sunil Goutham
 <sgoutham@...vell.com>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v2 net-next 2/2] net: airoha: Introduce ethernet support
 for EN7581 SoC

On 28.06.24 21:34, Arnd Bergmann wrote:
>>> > +static irqreturn_t airoha_irq_handler(int irq, void *dev_instance)
>>> > +{
>>> > +	struct airoha_eth *eth = dev_instance;
>>> > +	u32 intr[ARRAY_SIZE(eth->irqmask)];
>>> > +	int i;
>>> > +
>>> > +	for (i = 0; i < ARRAY_SIZE(eth->irqmask); i++) {
>>> > +		intr[i] = airoha_qdma_rr(eth, REG_INT_STATUS(i));
>>> > +		intr[i] &= eth->irqmask[i];
>>> > +		airoha_qdma_wr(eth, REG_INT_STATUS(i), intr[i]);
>>> > +	}
>>> 
>>> This looks like you send an interrupt Ack to each
>>> interrupt in order to re-arm it, but then you disable
>>> it again. Would it be possible to leave the interrupt enabled
>>> but defer the Ack until the napi poll function is completed?
>>
>> I guess doing so we are not using NAPIs as expected since they are
>> supposed to run with interrupt disabled. Agree?
> 
> The idea of NAPI is that you don't get the same interrupt
> again until all remaining events have been processed.
> 
> How this is achieved is device dependent, and it can either
> be done by masking the interrupt as you do here, or by
> not rearming the interrupt if it gets turned off automatically
> by the hardware. My guess is that writing to REG_INT_STATUS(i)
> is the rearming here, but the device documentation should
> clarify that. It's also possible that this is an Ack that
> is required so you don't immediately get another interrupt.

The interrupt handling of this hardware is pretty much the same as what 
many other devices do: the interrupt status is not automatically cleared 
by the hardware, so the write at the beginning of the interrupt handler 
does that explicitly.
Within the same handler, the interrupt is then masked to ensure that it 
does not fire again until the NAPI poll has completed.

Performing the status write after the poll has completed would be wrong, 
since that leaves open a small race window where events might be missed.

- Felix