Re: [PATCH net] 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] 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