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

[netdev-bot+sashiko@kernel.org]

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