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

[PATCH net v5] 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>
---
v5:
- Repost as a new thread, as requested by Jakub Kicinski: the netdev CI
  could not parse the v4 mail.  No code change.
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
index 53fac3d..c41fad7 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -885,6 +885,7 @@ static int adin1110_net_open(struct net_device *net_dev)
 {
 	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 @@ static int adin1110_net_open(struct net_device *net_dev)
 	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;
 
-	priv->irq_mask = val;
-	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~val);
+	/*
+	 * 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;
+	}
+
+	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 @@ 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);

Re: [PATCH net v5] 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] When adin1110_net_open() fails after it has written IMASK1,
  the port's own RX-ready interrupt stays enabled even though the port…

--

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

This patch changes adin1110_net_stop() so that it masks the RX-ready
interrupt bit of the port being stopped instead of the peer's bit. It
also changes adin1110_net_open() so that, on ADIN2111, the peer's
RX-ready bit is unmasked in IMASK1 only when the peer is running.

> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 53fac3dc8be62..c41fad7835d07 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c

[ ... ]

> @@ -895,12 +896,24 @@ static int adin1110_net_open(struct net_device *net_dev)
>  	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;
>  
> -	priv->irq_mask = val;
> -	ret = adin1110_write_reg(priv, ADIN1110_IMASK1, ~val);
> +	/*
> +	 * 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;
> +	}
> +
> +	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;

[Severity: Medium]
The commit message says "A port's RX-ready IRQ is now enabled only while
that port is open". Is that still true if adin1110_net_open() fails after
this IMASK1 write?

This write unmasks the port's own RX-ready bit. Three later steps in
adin1110_net_open() can still fail, and each one jumps to out without
masking that bit again:

	ret = adin1110_read_reg(priv, ADIN1110_TX_SPACE, &val);
	if (ret < 0) {
		...
		goto out;
	}
	...
	ret = adin1110_set_mac_address(net_dev, net_dev->dev_addr);
	if (ret < 0) {
		...
		goto out;
	}

	ret = adin1110_set_bits(priv, ADIN1110_CONFIG1, ADIN1110_CONFIG1_SYNC,
				ADIN1110_CONFIG1_SYNC);

out:
	mutex_unlock(&priv->lock);

	if (ret < 0)
		return ret;

When ndo_open fails, __dev_open() only clears __LINK_STATE_START. It does
not call ndo_stop, so adin1110_net_stop() never masks the bit either.

The port then ends up in the state the commit message describes as the
bug. Its RX-ready interrupt is unmasked, but the handler skips the port
because netif_oper_up() is false:

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

Any frames left in that port's FIFO keep the level-triggered interrupt
asserted, so the threaded handler keeps running and drains nothing.

On ADIN1110 this lasts until the next successful open. On ADIN2111 it
lasts until the peer port is opened. The peer's open sees netif_running()
as false for this port and masks the bit again.

Could the open error path mask the own RX-ready bit in IMASK1 again while
priv->lock is still held?

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