Re: [PATCH net-next v2 2/6] net: wangxun: add Tx timeout process

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wangxun: add Tx timeout process

Implement .ndo_tx_timeout to handle Tx side timeout events, triggering
a driver reset process. It also adds logic to track potential hangs and
clear false detections during 802.3 pause frames.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> new file mode 100644
> index 0000000000000..ba5f23cefc0f9
> --- /dev/null
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> @@ -0,0 +1,126 @@

[ ... ]

> +static void wx_reset_subtask(struct wx *wx)
> +{
> +	if (!test_bit(WX_FLAG_NEED_PF_RESET, wx->flags))
> +		return;
> +
> +	rtnl_lock();
> +
> +	if (!netif_running(wx->netdev) ||
> +	    test_bit(WX_STATE_RESETTING, wx->state))
> +		goto out;

If the interface is being brought down or a reset is in progress, does
this leave WX_FLAG_NEED_PF_RESET set indefinitely? When the interface is
brought back up, could this stale flag trigger an immediate, redundant
hardware reset?

Also, could this rtnl_lock() call cause an AB-BA deadlock? If the network
interface is administratively brought down, the networking core acquires
rtnl_lock() and invokes the driver's .ndo_stop routine.

If the teardown path calls cancel_work_sync(&wx->service_task), the close
path holding rtnl_lock() will wait for service_task to finish, while this
service_task waits indefinitely to acquire rtnl_lock().

> +
> +	wx_warn(wx, "Reset adapter.\n");
> +
> +	if (test_and_clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags)) {
> +		if (wx->do_reset)
> +			wx->do_reset(wx->netdev);
> +	}
> +
> +out:
> +	rtnl_unlock();
> +}

[ ... ]

> +void wx_tx_timeout(struct net_device *netdev, unsigned int txqueue)
> +{
> +	struct wx *wx = netdev_priv(netdev);
> +	u32 head, tail;
> +	int i;
> +
> +	for (i = 0; i < wx->num_tx_queues; i++) {
> +		struct wx_ring *tx_ring = wx->tx_ring[i];
> +
> +		if (test_bit(WX_TX_DETECT_HANG, tx_ring->state) &&
> +		    wx_check_tx_hang(tx_ring))

Since wx_tx_timeout() can execute concurrently with NAPI's wx_clean_tx_irq()
on healthy queues, is it safe to call wx_check_tx_hang() here?

wx_check_tx_hang() mutates per-queue NAPI state like
ring->tx_stats.tx_done_old and ring->state without synchronization. Modifying
this state concurrently could corrupt the hang detection state machine.

> +			wx_warn(wx, "Real tx hang detected on queue %d\n", i);
> +
> +		head = rd32(wx, WX_PX_TR_RP(tx_ring->reg_idx));
> +		tail = rd32(wx, WX_PX_TR_WP(tx_ring->reg_idx));
> +		wx_warn(wx,
> +			"tx ring %d next_to_use is %d, next_to_clean is %d\n",
> +			i, tx_ring->next_to_use,
> +			tx_ring->next_to_clean);
> +		wx_warn(wx, "tx ring %d hw rp is 0x%x, wp is 0x%x\n",
> +			i, head, tail);
> +	}
> +
> +	wx_tx_timeout_reset(wx);
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index bd905e2675758..e9561996b970f 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -147,6 +148,7 @@ static void ngbe_service_task(struct work_struct *work)
>  {
>  	struct wx *wx = container_of(work, struct wx, service_task);
>  
> +	wx_handle_errors_subtask(wx);
>  	wx_update_stats(wx);

Is there a race condition here that could cause spurious Tx hang resets?

wx_handle_errors_subtask() sets the WX_TX_DETECT_HANG bit.
wx_update_stats() clears the WX_HANG_CHECK_ARMED bit when XOFF pause
frames are received.

If a hardware interrupt schedules NAPI between these two calls,
wx_clean_tx_irq() will execute wx_check_tx_hang() and might test the
WX_HANG_CHECK_ARMED bit from the previous interval (since wx_update_stats()
hasn't cleared it yet).

Would calling wx_update_stats() before wx_handle_errors_subtask() prevent
this false hang detection?

>  
>  	wx_service_event_complete(wx);

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index 8b7c3753bb6ac..5793da5b7bab4 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -128,6 +129,7 @@ static void txgbe_service_task(struct work_struct *work)
>  {
>  	struct wx *wx = container_of(work, struct wx, service_task);
>  
> +	wx_handle_errors_subtask(wx);
>  	txgbe_module_detection_subtask(wx);
>  	txgbe_link_config_subtask(wx);
>  	wx_update_stats(wx);

Similar to ngbe_service_task(), could NAPI execution between
wx_handle_errors_subtask() and wx_update_stats() trigger a false Tx hang
here as well?