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

[Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>]

> -----Original Message-----
> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Thursday, August 6, 2026 3:44 AM
> 
> 
> [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?

Agree with comments. The focus of the current effort is to not to leak any skb buffers once 
fatal error is seen.We don't accept new packets for transmission. Hence "disable_traffic" flag. 
Shutting down the driver cleanly in a recoverable way would require some level of design change. 

> 
> 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():
> 


Same answer as above. Would require some level of design change if we need to fix this issue.

--
> This is an AI-generated review.