Re: [PATCH net v5 3/4] net: ethernet: oa_tc6: Disable tx queues on fatal error

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
--- Patch [3]: [PATCH net v5 3/4] net: ethernet: oa_tc6: Disable tx queues on fatal error ---
commit e2e6581c7bb33d3713f9dc8fab5b2d869c2c7d89
Author: Selvamani Rajagopal <Selvamani.Rajagopal@xxxxxxxxxx>

net: ethernet: oa_tc6: Disable tx queues on fatal error

This adds a netif_tx_disable() call to oa_tc6_disable_traffic() so that
the TX queues are stopped when the disable_traffic flag is set, on the
grounds that there is no recovery from that state short of a driver
reload.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 421f72e6f71e3..bb644509c32b3 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -730,6 +730,10 @@ static void oa_tc6_disable_traffic(struct oa_tc6 *tc6)
>  	skb = oa_tc6_detach_waiting_tx_skb(tc6);
>  	spin_unlock_bh(&tc6->tx_skb_lock);
>  
> +	/* disable_traffic, when set, is a point of no return to working
> +	 * state. TX queues are disabled.
> +	 */
> +	netif_tx_disable(tc6->netdev);

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
The comment describes this as "a point of no return to working state", but
netif_tx_disable() only sets the reversible per-queue __QUEUE_STATE_DRV_XOFF
bit.  Is that enough to make the state stick?

The only in-tree consumer clears it again on every open, without looking at
tc6->disable_traffic:

drivers/net/ethernet/microchip/lan865x/lan865x.c:lan865x_net_open() {
	ret = lan865x_hw_enable(priv);
	...
	phy_start(netdev->phydev);

	netif_start_queue(netdev);
}

So after oa_tc6_disable_traffic() runs, an ordinary

	ip link set dev X down
	ip link set dev X up

re-enables the queues, while tc6->disable_traffic stays true (it is only
ever set, never cleared), the SPI interrupt stays disabled by
disable_irq_nosync() in the threaded IRQ handler and INT_MASK0 masks all
device interrupts.  Frames then land in the pre-existing drop branch:

drivers/net/ethernet/oa_tc6.c:oa_tc6_start_xmit() {
	if (tc6->disable_traffic) {
		spin_unlock_bh(&tc6->tx_skb_lock);
		oa_tc6_drop_tx_skb(tc6, skb);
		return NETDEV_TX_OK;
	}
}

Is an interface that looks operational but silently swallows every frame the
intended end state here?

There is also an ordering aspect: oa_tc6_init() arms the threaded IRQ with
devm_request_threaded_irq() and then calls irq_wake_thread(), which happens
before the consumer's register_netdev() in lan865x_probe().  If a fatal
error is detected during that window, the queue disable is applied to a
netdev that has not been registered yet and is then undone by the first
open.  Would netif_device_detach(), netif_carrier_off(), or having ndo_open
consult the tc6 fatal state express this more durably, or should the comment
and commit message be reworded to match what the code actually does?

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
A second question about the same call, regarding what userspace can observe.
The v4 changelog notes the carrier-off change was reverted, so the carrier
stays up while every txq has DRV_XOFF set.  With the queues stopped,
netif_xmit_stopped() is true and ndo_start_xmit is never called again, so the
per-frame accounting in the disable_traffic branch of oa_tc6_start_xmit():

	oa_tc6_drop_tx_skb(tc6, skb);	/* netdev->stats.tx_dropped++ */

becomes unreachable.  Packets are instead dropped at the qdisc, and qdisc
drops are not folded into rtnl_link_stats64.tx_dropped (only dev->stats and
dev->core_stats are; the enqueue-drop path only does qdisc_qstats_drop()).
Does that mean ip -s link reports zero errors and zero drops for a device
that is permanently dead?

lan865x_netdev_ops also registers no .ndo_tx_timeout:

drivers/net/ethernet/microchip/lan865x/lan865x.c:
static const struct net_device_ops lan865x_netdev_ops = {
	.ndo_open		= lan865x_net_open,
	.ndo_stop		= lan865x_net_close,
	.ndo_start_xmit		= lan865x_send_packet,
	...
};

so __netdev_watchdog_up() never arms dev_watchdog() and there is no
"transmit queue timed out" message either.  Link monitors and
carrier-driven aggregators such as bonding, team and bridge would still see
UP/LOWER_UP with no errors, and senders stall rather than getting drops.

Could the commit message document this loss of device-level drop accounting,
or could the dead state be made observable with netif_device_detach() /
netif_carrier_off() or another persistent error indication?
-- 
This is an AI-generated review.