[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

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