Re: [net-next,v4,09/10] net: wangxun: add Tx timeout process

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: wangxun: add Tx timeout process

This commit implements .ndo_tx_timeout to handle Tx side timeout events.
When a Tx timeout occurs, it triggers a driver reset process.

> diff --git a/drivers/net/ethernet/wangxun/libwx/Makefile b/drivers/net/ethernet/wangxun/libwx/Makefile
> index a71b0ad77de3..c8724bb129aa 100644
> --- a/drivers/net/ethernet/wangxun/libwx/Makefile
> +++ b/drivers/net/ethernet/wangxun/libwx/Makefile
> @@ -4,5 +4,5 @@
>
>  obj-$(CONFIG_LIBWX) += libwx.o
>
> -libwx-objs := wx_hw.o wx_lib.o wx_ethtool.o wx_ptp.o wx_mbx.o wx_sriov.o
> +libwx-objs := wx_hw.o wx_lib.o wx_ethtool.o wx_ptp.o wx_mbx.o wx_sriov.o wx_err.o
>  libwx-objs += wx_vf.o wx_vf_lib.o wx_vf_common.o
> 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 000000000000..bb77bdce69d2
> --- /dev/null
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index c65240ca3c28..5a21478a592b 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c

[ ... ]

> @@ -742,6 +743,48 @@ static struct netdev_queue *wx_txring_txq(const struct wx_ring *ring)
>  	return netdev_get_tx_queue(ring->netdev, ring->queue_index);
>  }
>
> +static u64 wx_get_tx_pending(struct wx_ring *ring)
> +{
> +	unsigned int head, tail;
> +
> +	head = ring->next_to_clean;
> +	tail = ring->next_to_use;
> +
> +	return ((head <= tail) ? tail : tail + ring->count) - head;
> +}
> +
> +bool wx_check_tx_hang(struct wx_ring *ring)
> +{
> +	u64 tx_done_old = ring->tx_stats.tx_done_old;
> +	u64 tx_pending = wx_get_tx_pending(ring);
> +	u64 tx_done = ring->stats.packets;
> +
> +	clear_bit(WX_TX_DETECT_HANG, ring->state);
> +
> +	/*
> +	 * Check for a hung queue, but be thorough. This verifies
> +	 * that a transmit has been completed since the previous
> +	 * check AND there is at least one packet pending. The
> +	 * ARMED bit is set to indicate a potential hang. The
> +	 * bit is cleared if a pause frame is received to remove
> +	 * false hang detection due to PFC or 802.3x frames. By
> +	 * requiring this to fail twice we avoid races with
> +	 * pfc clearing the ARMED bit and conditions where we
> +	 * run the check_tx_hang logic with a transmit completion
> +	 * pending but without time to complete it yet.
> +	 */

The comment here mentions that "the bit is cleared if a pause frame is
received to remove false hang detection due to PFC or 802.3x frames."
However, I don't see any code in the wangxun driver that clears
WX_HANG_CHECK_ARMED when pause frames are received.

Looking at where WX_HANG_CHECK_ARMED is used in the driver:

drivers/net/ethernet/wangxun/libwx/wx_lib.c:wx_check_tx_hang() {
    ...
    clear_bit(WX_TX_DETECT_HANG, ring->state);
    ...
    if (tx_done_old == tx_done && tx_pending)
        return test_and_set_bit(WX_HANG_CHECK_ARMED, ring->state);
    ...
    clear_bit(WX_HANG_CHECK_ARMED, ring->state);
    return false;
}

The bit is only cleared within wx_check_tx_hang() itself when packets
have completed or when there are no pending packets. The driver does
support flow control (both ngbe and txgbe implement get_pauseparam and
set_pauseparam and advertise MAC_SYM_PAUSE/MAC_ASYM_PAUSE capabilities).

Won't legitimate TX stalls caused by pause frames be misidentified as
hangs after two service timer intervals, triggering unnecessary adapter
resets? The ixgbe driver this appears to be based on has
ixgbe_update_xoff_received() which clears __IXGBE_HANG_CHECK_ARMED when
flow control pause frames are received.

Should the wangxun driver implement similar pause frame handling to
prevent false hang detections, or should the comment be updated to
reflect the current behavior?

> +	if (tx_done_old == tx_done && tx_pending)
> +		/* make sure it is true for two checks in a row */
> +		return test_and_set_bit(WX_HANG_CHECK_ARMED, ring->state);
> +
> +	/* update completed stats and continue */
> +	ring->tx_stats.tx_done_old = tx_done;
> +	/* reset the countdown */
> +	clear_bit(WX_HANG_CHECK_ARMED, ring->state);
> +
> +	return false;
> +}

[ ... ]