Re: [PATCH net-next v1 2/5] net: wangxun: add Tx timeout process

[Paolo Abeni <pabeni@redhat.com>]

On 4/28/26 4:11 AM, Jiawen Wu wrote:
> +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))
> +		return;

Sashiko says:

Does this early return path leak the rtnl_lock?
If the interface is brought down concurrently while a reset is scheduled,
it appears this would return without calling rtnl_unlock(). Since all
network
configuration operations require the RTNL lock, could this lead to a
system-wide deadlock in the networking subsystem?

> +
> +	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);
> +	}
> +
> +	rtnl_unlock();
> +}
> +
> +/*
> + * wx_check_tx_hang_subtask - check for hung queues and dropped interrupts
> + * @wx - pointer to the device wx structure
> + *
> + * This function serves two purposes.  First it strobes the interrupt lines
> + * in order to make certain interrupts are occurring.  Secondly it sets the
> + * bits needed to check for TX hangs.  As a result we should immediately
> + * determine if a hang has occurred.
> + */
> +static void wx_check_tx_hang_subtask(struct wx *wx)
> +{
> +	int i;
> +
> +	/* If we're down or resetting, just bail */
> +	if (!netif_running(wx->netdev) ||
> +	    test_bit(WX_STATE_RESETTING, wx->state))
> +		return;
> +
> +	/* Force detection of hung controller */
> +	if (netif_carrier_ok(wx->netdev)) {
> +		for (i = 0; i < wx->num_tx_queues; i++)
> +			set_bit(WX_TX_DETECT_HANG, wx->tx_ring[i]->state);
> +	}
> +}
> +
> +void wx_handle_errors_subtask(struct wx *wx)
> +{
> +	wx_reset_subtask(wx);
> +	wx_check_tx_hang_subtask(wx);
> +}
> +EXPORT_SYMBOL(wx_handle_errors_subtask);
> +
> +static void wx_tx_timeout_reset(struct wx *wx)
> +{
> +	if (!netif_running(wx->netdev))
> +		return;
> +
> +	set_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
> +	wx_warn(wx, "initiating reset due to tx timeout\n");
> +	wx_service_event_schedule(wx);
> +}
> +
> +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))
> +			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);
> +}
> +EXPORT_SYMBOL(wx_tx_timeout);
> +
> +void wx_handle_tx_hang(struct wx_ring *tx_ring, unsigned int next)
> +{
> +	struct wx *wx = netdev_priv(tx_ring->netdev);
> +
> +	wx_warn(wx, "Detected Tx Unit Hang\n"
> +		"  Tx Queue             <%d>\n"
> +		"  TDH, TDT             <%x>, <%x>\n"
> +		"  next_to_use          <%x>\n"
> +		"  next_to_clean        <%x>\n"
> +		"tx_buffer_info[next_to_clean]\n"
> +		"  time_stamp           <%lx>\n"
> +		"  jiffies              <%lx>\n",

It's better to use a single string for the whole message, even if it
would exceed the 80 chars limit

> +		tx_ring->queue_index,
> +		rd32(wx, WX_PX_TR_RP(tx_ring->reg_idx)),
> +		rd32(wx, WX_PX_TR_WP(tx_ring->reg_idx)),
> +		tx_ring->next_to_use, next,
> +		tx_ring->tx_buffer_info[next].time_stamp, jiffies);
> +
> +	netif_stop_subqueue(tx_ring->netdev, tx_ring->queue_index);
> +
> +	wx_warn(wx, "tx hang detected on queue %d, resetting adapter\n",
> +		tx_ring->queue_index);

Possibly two warn messages for the same cause is a bit too verbose (same
in wx_tx_timeout()).

> +bool wx_check_tx_hang(struct wx_ring *ring)
> +{
> +	u32 tx_done_old = ring->tx_stats.tx_done_old;
> +	u32 tx_pending = wx_get_tx_pending(ring);
> +	u32 tx_done = ring->stats.packets;
> +
> +	clear_bit(WX_TX_DETECT_HANG, ring->state);

It looks like every caller checks WX_TX_DETECT_HANG, it would be
probably better to use test_and_clear_bit() here, and drop the test from
the caller.

/P