[PATCH net] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[PATCH net] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

adin1110_net_stop() programs the RX-ready interrupt mask of the wrong
port: it uses ADIN2111_RX_RDY_IRQ (the second port's bit) when stopping
port 0 and ADIN1110_RX_RDY_IRQ (the first port's bit) when stopping
port 1, while adin1110_port_rx_ready() uses the opposite mapping, port 0
-> ADIN1110_RX_RDY and port 1 -> ADIN2111_P2_RX_RDY.

The port being stopped therefore keeps its own RX interrupt enabled
while the peer's is masked.  On ADIN2111 that stalls RX on the peer port;
on ADIN1110 the RX interrupt is never masked at all.  As the interrupt is
level triggered and adin1110_read_frames() skips the FIFO while
netif_oper_up() is false, the stopped port also keeps re-raising its
interrupt with nobody draining the FIFO.

Swap the two bits so the port being stopped masks its own interrupt.

Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>
---
 drivers/net/ethernet/adi/adin1110.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
index 53fac3d..abdc160 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
 	u32 mask;
 	int ret;
 
-	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
+	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
 
 	/* Disable RX RDY IRQs */
 	mutex_lock(&priv->lock);
-- 
2.43.0

Re: [PATCH net] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

Hi,

Here is the information missing from the changelog:

- How the issue was found: during a code review of the driver that was
  assisted by an AI coding assistant (an LLM).  The assistant compared
  adin1110_net_stop() against the bit definitions and the mapping used by
  adin1110_port_rx_ready(); I then checked the finding against the register
  semantics used by adin1110_net_open() (IMASK1: 1 = masked).

- Triggered or theoretical: theoretical.  I have not triggered it and there
  is no reproducer.  Expected symptoms if it is hit: on ADIN2111, bringing
  one port down masks the peer port's RX-ready interrupt so the peer stops
  receiving, while the port that was brought down keeps its own RX interrupt
  enabled; since the interrupt is level triggered and
  adin1110_read_frames() skips the FIFO while netif_oper_up() is false,
  pending frames keep re-asserting it.  On the single-port ADIN1110 the RX
  interrupt is never masked at all.

- Hardware: none.  The change was not tested on real hardware; it was
  compile-tested only (CONFIG_ADIN1110=m, x86_64) and passes
  scripts/checkpatch.pl --strict.

Per Documentation/process/generated-content.rst, the tool assistance is
noted above.  I have reviewed the analysis and the change and take
responsibility for them.

If another revision is needed, I will fold this information into the
commit message.

Thanks,
Yuehong Wu

[PATCH net v2] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

adin1110_net_stop() programs the RX-ready interrupt mask of the wrong
port: it uses ADIN2111_RX_RDY_IRQ (the second port's bit) when stopping
port 0 and ADIN1110_RX_RDY_IRQ (the first port's bit) when stopping
port 1, while adin1110_port_rx_ready() uses the opposite mapping, port 0
-> ADIN1110_RX_RDY and port 1 -> ADIN2111_P2_RX_RDY.

The port being stopped therefore keeps its own RX interrupt enabled
while the peer's is masked.  On ADIN2111 that stalls RX on the peer port;
on ADIN1110 the RX interrupt is never masked at all.  As the interrupt is
level triggered and adin1110_read_frames() skips the FIFO while
netif_oper_up() is false, the stopped port also keeps re-raising its
interrupt with nobody draining the FIFO.

Swap the two bits so the port being stopped masks its own interrupt.

Found by code review while comparing the bit mapping against
adin1110_port_rx_ready().  I do not have access to ADIN1110 or ADIN2111
hardware, so this was only compile-tested (CONFIG_ADIN1110=m, x86_64),
not exercised on a device.

Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>
---
v2:
- Cc the netdev maintainers, as flagged by the cc_maintainers CI check on v1.
- Explain how the issue was found and that the change was only compile-tested,
  no hardware access (netdev guideline for patches found by code review).
v1: https://lore.kernel.org/all/20261103-adin1110-net-stop-1-yuehongwu@cmss.chinamobile.com/

 drivers/net/ethernet/adi/adin1110.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
index 53fac3d..abdc160 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
 	u32 mask;
 	int ret;
 
-	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
+	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
 
 	/* Disable RX RDY IRQs */
 	mutex_lock(&priv->lock);
-- 
2.43.0

Re: [PATCH net v2] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Andrew Lunn]

> v2:
> - Cc the netdev maintainers, as flagged by the cc_maintainers CI check on v1.

./scripts/get_maintainer.pl drivers/net/ethernet/adi/adin1110.c 
Andrew Lunn <andrew+netdev@lunn.ch> (maintainer:NETWORKING DRIVERS)
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@kernel.org> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS,commit_signer:1/3=33%)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS,commit_signer:1/3=33%)
"Nuno Sá" <nuno.sa@analog.com> (commit_signer:1/3=33%)
Heiner Kallweit <hkallweit1@gmail.com> (commit_signer:1/3=33%,authored:1/3=33%,added_lines:1/5=20%,removed_lines:1/2=50%)
Chen Ni <nichen@iscas.ac.cn> (commit_signer:1/3=33%,authored:1/3=33%,added_lines:3/5=60%)
netdev@vger.kernel.org (open list:NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

Since this is an analogue devices device, don't you think Cc:ing
"Nuno Sá" <nuno.sa@analog.com> would be good?

    Andrew

---
pw-bot: cr

Re: [PATCH net v2] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 692 bytes --]

> Since this is an analogue devices device, don't you think Cc:ing
> "Nuno Sá" <nuno.sa@analog.com> would be good?

You are right, sorry about that.  I ran get_maintainer.pl in a tree
without git history, so the commit-signers did not show up in my Cc list
at all and I only picked the maintainers and the open lists.

Adding Nuno, Heiner and Chen here.  There is no code change, so the v2
currently in patchwork is still the version to review:

https://patchwork.kernel.org/project/netdevbpf/patch/20261104-adin1110-net-stop-v2-1-yuehongwu@cmss.chinamobile.com/

I can resend it as a v3 with the Cc list fixed if you would rather have
the patch in everyone's inbox -- just say the word.

[PATCH net v3] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

adin1110_net_stop() programs the RX-ready interrupt mask of the wrong
port: it uses ADIN2111_RX_RDY_IRQ (the second port's bit) when stopping
port 0 and ADIN1110_RX_RDY_IRQ (the first port's bit) when stopping
port 1, while adin1110_port_rx_ready() uses the opposite mapping, port 0
-> ADIN1110_RX_RDY and port 1 -> ADIN2111_P2_RX_RDY.

The port being stopped therefore keeps its own RX interrupt enabled
while the peer's is masked.  On ADIN2111 that stalls RX on the peer port;
on ADIN1110 the RX interrupt is never masked at all.  As the interrupt is
level triggered and adin1110_read_frames() skips the FIFO while
netif_oper_up() is false, the stopped port also keeps re-raising its
interrupt with nobody draining the FIFO.

Swap the two bits so the port being stopped masks its own interrupt.

Found by code review while comparing the bit mapping against
adin1110_port_rx_ready().  I do not have access to ADIN1110 or ADIN2111
hardware, so this was only compile-tested (CONFIG_ADIN1110=m, x86_64),
not exercised on a device.

Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>
---
v2 -> v3: no code change.  Cc the ADI commit-signers that
get_maintainer.pl reports once a git history is available (Nuno Sa,
Heiner Kallweit, Chen Ni).  Thanks to Andrew Lunn for pointing out that
Nuno was missing.

v2:
- Cc the netdev maintainers, as flagged by the cc_maintainers CI check on v1.
- Explain how the issue was found and that the change was only compile-tested,
  no hardware access (netdev guideline for patches found by code review).
v1: https://lore.kernel.org/all/20261103-adin1110-net-stop-1-yuehongwu@cmss.chinamobile.com/

 drivers/net/ethernet/adi/adin1110.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
index 53fac3d..abdc160 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
 	u32 mask;
 	int ret;
 
-	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
+	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
 
 	/* Disable RX RDY IRQs */
 	mutex_lock(&priv->lock);
-- 
2.43.0

Re: [PATCH net v3] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Nuno Sá]

On Fri, Nov 06, 2026 at 10:02:51AM +0800, Yuehong Wu wrote:
> adin1110_net_stop() programs the RX-ready interrupt mask of the wrong
> port: it uses ADIN2111_RX_RDY_IRQ (the second port's bit) when stopping
> port 0 and ADIN1110_RX_RDY_IRQ (the first port's bit) when stopping
> port 1, while adin1110_port_rx_ready() uses the opposite mapping, port 0
> -> ADIN1110_RX_RDY and port 1 -> ADIN2111_P2_RX_RDY.
> 
> The port being stopped therefore keeps its own RX interrupt enabled
> while the peer's is masked.  On ADIN2111 that stalls RX on the peer port;
> on ADIN1110 the RX interrupt is never masked at all.  As the interrupt is
> level triggered and adin1110_read_frames() skips the FIFO while
> netif_oper_up() is false, the stopped port also keeps re-raising its
> interrupt with nobody draining the FIFO.
> 
> Swap the two bits so the port being stopped masks its own interrupt.
> 
> Found by code review while comparing the bit mapping against
> adin1110_port_rx_ready().  I do not have access to ADIN1110 or ADIN2111
> hardware, so this was only compile-tested (CONFIG_ADIN1110=m, x86_64),
> not exercised on a device.
> 
> Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
> Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>
> ---

LGTM. But looking at the commit message, I suspect you're also missing
an Assisted-by tag :). Anyways:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> v2 -> v3: no code change.  Cc the ADI commit-signers that
> get_maintainer.pl reports once a git history is available (Nuno Sa,
> Heiner Kallweit, Chen Ni).  Thanks to Andrew Lunn for pointing out that
> Nuno was missing.
> 
> v2:
> - Cc the netdev maintainers, as flagged by the cc_maintainers CI check on v1.
> - Explain how the issue was found and that the change was only compile-tested,
>   no hardware access (netdev guideline for patches found by code review).
> v1: https://lore.kernel.org/all/20261103-adin1110-net-stop-1-yuehongwu@cmss.chinamobile.com/
> 
>  drivers/net/ethernet/adi/adin1110.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 53fac3d..abdc160 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c
> @@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
>  	u32 mask;
>  	int ret;
>  
> -	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
> +	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
>  
>  	/* Disable RX RDY IRQs */
>  	mutex_lock(&priv->lock);
> -- 
> 2.43.0
> 
> 
> 

Re: [PATCH net] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete: the peer port's ndo_open can re-arm the
  RX-ready interrupt of a port that is still down.

--

net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

This patch swaps the two RX_RDY bits in adin1110_net_stop(). Each port
now masks its own RX-ready interrupt in IMASK1 when it is stopped, which
matches the bit mapping in adin1110_port_rx_ready().

> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 53fac3dc8be62..abdc16089c9e3 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c
> @@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
>  	u32 mask;
>  	int ret;
>  
> -	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
> +	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
>  
>  	/* Disable RX RDY IRQs */
>  	mutex_lock(&priv->lock);

[Severity: High]
Is this fix complete? The peer port's open path looks like it can unmask
this bit again while this port is still down.

adin1110_net_stop() masks the bit with a read-modify-write.
adin1110_net_open() does not. It rebuilds IMASK1 from scratch and
unmasks RX_RDY for both ports:

drivers/net/ethernet/adi/adin1110.c:adin1110_net_open() {
    ...
	val = ADIN1110_TX_RDY_IRQ | ADIN1110_RX_RDY_IRQ | ADIN1110_SPI_ERR_IRQ;
	if (priv->cfg->id == ADIN2111_MAC)
		val |= ADIN2111_RX_RDY_IRQ;

	priv->irq_mask = val;
	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~val);
    ...
}

On ADIN2111, take this sequence:

  ip link set eth1 down    -> adin1110_net_stop() masks ADIN2111_RX_RDY_IRQ
  ip link set eth0 down
  ip link set eth0 up      -> adin1110_net_open() clears that mask again

eth1 is still down at this point, but its RX_RDY interrupt is enabled
again. A port that was never opened also gets its RX_RDY unmasked as soon
as the other port opens.

For the down port, adin1110_port_rx_ready() returns false:

	if (!netif_oper_up(port_priv->netdev))
		return false;

So adin1110_irq() never drains that port's FIFO. It only writes STATUS1
with priv->irq_mask and returns IRQ_HANDLED.

The interrupt is requested with IRQF_TRIGGER_LOW. Frames can be left in
the FIFO, for example frames that arrive between the IMASK1 write and
phy_stop(). The commit message describes this exact state: "the stopped
port also keeps re-raising its interrupt with nobody draining the FIFO".
Wouldn't that state still be reachable after this patch?

The handler always returns IRQ_HANDLED, so the spurious IRQ detector
would not disable the line. The threaded handler would keep running SPI
transactions under priv->lock.

Also, priv->irq_mask is overwritten in adin1110_net_open() and never
updated in adin1110_net_stop(). The driver therefore does not track which
ports have RX_RDY masked.

Should adin1110_net_open() unmask only the opening port's RX_RDY bit with
a per-port read-modify-write, like adin1110_net_stop() does? Should the
stopped port's RX FIFO also be drained or flushed on stop?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103-adin1110-net-stop-1-yuehongwu%40cmss.chinamobile.com

[PATCH net v4] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

adin1110_net_stop() programs the RX-ready interrupt mask of the wrong
port: it uses ADIN2111_RX_RDY_IRQ (the second port's bit) when stopping
port 0 and ADIN1110_RX_RDY_IRQ (the first port's bit) when stopping
port 1, while adin1110_port_rx_ready() uses the opposite mapping, port 0
-> ADIN1110_RX_RDY and port 1 -> ADIN2111_P2_RX_RDY.

The port being stopped therefore keeps its own RX interrupt enabled while
the peer's is masked.  On ADIN2111 that stalls RX on the peer port; on
ADIN1110 the RX interrupt is never masked at all.  As the interrupt is
level triggered and adin1110_read_frames() skips the FIFO while
netif_oper_up() is false, the stopped port also keeps re-raising its
interrupt with nobody draining the FIFO.

Swap the two bits so that the port being stopped masks its own interrupt.

adin1110_net_open() rewrites the whole IMASK1 register and, on ADIN2111,
always includes ADIN2111_RX_RDY_IRQ, so opening one port also re-enables
the RX-ready interrupt of a peer port that is still down and undoes the
mask just programmed by its stop.  Only keep the peer port's bit enabled
while that port is running; the write stays absolute, so the bit is still
included when the peer is up.

A port's RX-ready IRQ is now enabled only while that port is open: the
peer's bit is never re-enabled for a stopped port.  IMASK1 is written in
exactly two places, adin1110_net_open() and adin1110_net_stop(), both
under priv->lock, and both compute the mask from the state of the two
ports.  priv->irq_mask keeps its previous value and meaning (the STATUS1
write-one-to-clear mask), so the status clearing is unchanged.
Single-port ADIN1110 is unchanged.

Found by code review while comparing the bit mapping against
adin1110_port_rx_ready().  I do not have access to ADIN1110 or ADIN2111
hardware, so this was only compile-tested (CONFIG_ADIN1110=m and =y, gcc,
W=1, full kernel builds), not exercised on a device.

Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Yuehong Wu <wuyuehong@cmss.chinamobile.com>
---
v4:
- Also keep a stopped peer port's RX-ready IRQ masked in
  adin1110_net_open(), whose absolute IMASK1 write re-enabled the
  interrupt of a port that is still down and undid the stop mask.
  Reported by Sashiko (netdev bot) as an incomplete fix.
- Did not carry the Reviewed-by from v3, as adin1110_net_open() is
  changed as well; a Tested-by from ADI on real hardware would be
  welcome.
v3: no code change.  Cc the ADI commit-signers that get_maintainer.pl
reports once a git history is available (Nuno Sa, Heiner Kallweit,
Chen Ni).  Thanks to Andrew Lunn for pointing out that Nuno was missing.
v2:
- Cc the netdev maintainers, as flagged by cc_maintainers on v1.
- Explain how the issue was found and that it was only compile-tested.
v1: https://lore.kernel.org/all/20261103-adin1110-net-stop-1-yuehongwu@cmss.chinamobile.com/

 drivers/net/ethernet/adi/adin1110.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -885,6 +885,7 @@
 {
 	struct adin1110_port_priv *port_priv = netdev_priv(net_dev);
 	struct adin1110_priv *priv = port_priv->priv;
+	u32 common, own, peer, irq_mask;
 	u32 val;
 	int ret;
 
@@ -895,12 +896,24 @@
 	if (ret < 0)
 		goto out;
 
-	val = ADIN1110_TX_RDY_IRQ | ADIN1110_RX_RDY_IRQ | ADIN1110_SPI_ERR_IRQ;
-	if (priv->cfg->id == ADIN2111_MAC)
-		val |= ADIN2111_RX_RDY_IRQ;
+	common = ADIN1110_TX_RDY_IRQ | ADIN1110_SPI_ERR_IRQ;
+	own = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
+	peer = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
+
+	/*
+	 * The write below is absolute, so only keep the peer port's
+	 * RX-ready IRQ enabled while that port is running; a stopped peer
+	 * must stay masked until it is opened again.
+	 */
+	irq_mask = common | own;
+	priv->irq_mask = common | ADIN1110_RX_RDY_IRQ;
+	if (priv->cfg->id == ADIN2111_MAC) {
+		priv->irq_mask |= ADIN2111_RX_RDY_IRQ;
+		if (netif_running(priv->ports[!port_priv->nr]->netdev))
+			irq_mask |= peer;
+	}
 
-	priv->irq_mask = val;
-	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~val);
+	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~irq_mask);
 	if (ret < 0) {
 		netdev_err(net_dev, "Failed to enable chip IRQs: %d\n", ret);
 		goto out;
@@ -945,7 +958,7 @@
 	u32 mask;
 	int ret;
 
-	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
+	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
 
 	/* Disable RX RDY IRQs */
 	mutex_lock(&priv->lock);

Re: [PATCH net] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[Yuehong Wu]

Hi Sashiko,

Thanks, the finding is correct: adin1110_net_open() writes IMASK1 with an
absolute value that includes ADIN2111_RX_RDY_IRQ whenever the chip is an
ADIN2111, so opening either port re-enables the RX-ready interrupt of a
peer port that is still down and undoes the mask from adin1110_net_stop().

v4 keeps a stopped peer port masked: the peer's bit is only added to the
value written to IMASK1 when netif_running() is true for that port.  The
write stays absolute, so the bit is still enabled while the peer is
running, and netif_running() is __LINK_STATE_START, with ndo_open() and
ndo_stop() serialised by RTNL.  priv->irq_mask keeps its previous value
and meaning (the STATUS1 write-one-to-clear mask), so the clearing
behaviour is unchanged.  IMASK1 is written in exactly two places, both
under priv->lock.  Single-port ADIN1110 writes exactly the same values as
before.

On the FIFO question: I do not think it needs to be drained on stop.
While the port is down its RX-ready bit is masked, so it cannot raise an
interrupt, and whatever is left in its FIFO is drained by
adin1110_read_frames() when the port is opened again.  Draining in the
stop path would mean reading the port FIFO from a second context while
the peer may be running.

The patch only changes the mask programming; the pre-existing probe-time
mask state, the TX/SPI interrupt policy and the operstate gating in
adin1110_port_rx_ready() are untouched.

Testing: no ADIN1110 or ADIN2111 hardware; compile-tested with gcc, W=1,
CONFIG_ADIN1110=m and =y, full kernel builds.

Thanks,
Yuehong

pw-bot: cr

Re: [PATCH net v2] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On ADIN2111, the patch's fix in adin1110_net_stop() stops
  working as soon as the other port is opened.

--

net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

This commit swaps the RX-ready mask bits in adin1110_net_stop(), so the
port being stopped now masks its own RX interrupt. This matches the
mapping in adin1110_port_rx_ready(). The goal is to keep a stopped port
from re-raising its level-triggered interrupt when nothing drains its
FIFO.

> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 53fac3dc8be62..abdc16089c9e3 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c
> @@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
>  	u32 mask;
>  	int ret;
>  
> -	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
> +	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;
>  
>  	/* Disable RX RDY IRQs */
>  	mutex_lock(&priv->lock);

[Severity: High]
On ADIN2111, does this mask still hold once the peer port is opened?

adin1110_net_open() does not check which port is opening. It rewrites
the whole IMASK1 register and clears the RX_RDY mask bits of both ports:

drivers/net/ethernet/adi/adin1110.c:adin1110_net_open() {
    ...
	val = ADIN1110_TX_RDY_IRQ | ADIN1110_RX_RDY_IRQ | ADIN1110_SPI_ERR_IRQ;
	if (priv->cfg->id == ADIN2111_MAC)
		val |= ADIN2111_RX_RDY_IRQ;

	priv->irq_mask = val;
	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~val);
    ...
}

Take this sequence:

  ip link set eth1 down  -> adin1110_net_stop() sets ADIN2111_RX_RDY_IRQ
  ip link set eth0 down
  ip link set eth0 up    -> adin1110_net_open() clears it again

After the last step, port 1's RX_RDY interrupt is unmasked while eth1 is
still down.

eth1's RX FIFO may still hold frames at that point. Frames can arrive
after the IMASK1 write but before phy_stop() in adin1110_net_stop(), and
frames can also be left over when the ADIN1110_MAX_FRAMES_READ budget
runs out. The handler skips them:

adin1110_irq()
    adin1110_port_rx_ready(priv->ports[1], status1)
        if (!netif_oper_up(port_priv->netdev))
            return false;
    adin1110_write_reg(priv, ADIN1110_STATUS1, priv->irq_mask);
    -> IRQ_HANDLED

The IRQ is requested with IRQF_TRIGGER_LOW | IRQF_ONESHOT. Wouldn't the
threaded handler then run again and again until eth1 is brought back
up? This assumes RX_RDY stays asserted while the FIFO is not empty. The
commit message relies on the same assumption.

This looks like the same "stopped port also keeps re-raising its
interrupt with nobody draining the FIFO" case the commit message
describes. The only difference is that the peer port's ndo_open
triggers it.

Should adin1110_net_open() also change so that it unmasks only the
opening port's RX_RDY bit, plus the shared TX_RDY and SPI_ERR bits? And
should priv->irq_mask include only ports that are up?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-adin1110-net-stop-v2-1-yuehongwu%40cmss.chinamobile.com

Re: [PATCH net v3] net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch fixes the bit mapping in adin1110_net_stop(), but on
  ADIN2111 a stopped port's RX-ready interrupt can still end up unmasked.

--

net: ethernet: adi: adin1110: mask the RX IRQ of the port being stopped

This commit swaps the RX-ready interrupt bits used in adin1110_net_stop().
With the swap, the port being stopped masks its own RX interrupt in
ADIN1110_IMASK1 and no longer masks its peer's. The commit message says
the old mapping left the stopped port's interrupt unmasked, so it kept
firing with nobody draining the FIFO.

> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 53fac3dc8be62..abdc16089c9e3 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c
> @@ -945,7 +945,7 @@ static int adin1110_net_stop(struct net_device *net_dev)
>  	u32 mask;
>  	int ret;
>  
> -	mask = !port_priv->nr ? ADIN2111_RX_RDY_IRQ : ADIN1110_RX_RDY_IRQ;
> +	mask = !port_priv->nr ? ADIN1110_RX_RDY_IRQ : ADIN2111_RX_RDY_IRQ;

[Severity: Medium]
On ADIN2111, can a stopped port's RX-ready interrupt still be unmasked
again when the other port is opened?

The bit set here is only kept in hardware. Nothing records it per port,
and priv->irq_mask is not updated. adin1110_net_open() does not do a
per-port read-modify-write. It rewrites the whole register with both
ports' RX bits:

drivers/net/ethernet/adi/adin1110.c:adin1110_net_open() {
    ...
	val = ADIN1110_TX_RDY_IRQ | ADIN1110_RX_RDY_IRQ | ADIN1110_SPI_ERR_IRQ;
	if (priv->cfg->id == ADIN2111_MAC)
		val |= ADIN2111_RX_RDY_IRQ;

	priv->irq_mask = val;
	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~val);
    ...
}

Here is one way to reach this:

  ip link set port0 down          (net_stop masks ADIN1110_RX_RDY_IRQ)
  ip link set port1 down
  ip link set port1 up            (net_open clears it again)

Port 0 is still down at the end, but its RX-ready source is unmasked.

Frames can reach port 0's RX FIFO between the IMASK1 write in
adin1110_net_stop() and phy_stop(). Once the port is no longer oper-up,
nothing drains those frames:

drivers/net/ethernet/adi/adin1110.c:adin1110_port_rx_ready() {
	if (!netif_oper_up(port_priv->netdev))
		return false;
    ...
}

After port 1's open unmasks the bit, the interrupt is requested with
IRQF_TRIGGER_LOW | IRQF_ONESHOT. Would adin1110_irq() then run over and
over? It skips port 0 and only acknowledges the status:

	adin1110_write_reg(priv, ADIN1110_STATUS1, priv->irq_mask);

This assumes STATUS1.RX_RDY stays asserted while the FIFO holds data,
which is how adin1110_read_frames() treats it.

If so, this is still reachable on ADIN2111 through the peer's open:

  "the stopped port also keeps re-raising its interrupt with nobody
  draining the FIFO"

Should adin1110_net_open() unmask only its own port's RX bit with a
read-modify-write, and keep priv->irq_mask consistent per port? On
ADIN1110 there is only one port, so the change looks complete there.

>  
>  	/* Disable RX RDY IRQs */
>  	mutex_lock(&priv->lock);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-adin1110-net-stop-v3-1-yuehongwu%40cmss.chinamobile.com