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