Re: [PATCH net-next v6 10/11] net: wangxun: improve flow control setting

[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: improve flow control setting

Save the current mode of flow control, check the pause frame statistics
in watchdog to clear WX_HANG_CHECK_ARMED. The received pause frames are
divided into XON and XOFF to be counted, and Tx XON pause is disabled by
default for SP devices.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 2f71dc24306d4..4e0195f7df39e 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
[ ... ]
> @@ -2831,6 +2842,28 @@ int wx_fc_enable(struct wx *wx, bool tx_pause, bool rx_pause)
>  }
>  EXPORT_SYMBOL(wx_fc_enable);
>  
> +static void wx_update_xoff_rx_lfc(struct wx *wx)
> +{
> +	struct wx_hw_stats *hwstats = &wx->stats;
> +	u64 data;
> +	int i;
> +
> +	if (wx->fc.mode != wx_fc_full &&
> +	    wx->fc.mode != wx_fc_rx_pause)
> +		return;
> +
> +	data = rd64(wx, WX_MAC_LXOFFRXC);

Is it possible that rd64() is reading a 32-bit register here?

Looking at wx_clear_hw_cntrs(), WX_MAC_LXOFFRXC is cleared using rd32().

If rd64() reads an adjacent unmapped address, could it populate the upper 32
bits of data with garbage?

> +	hwstats->lxoffrxc += data;
> +
> +	/* refill credits (no tx hang) if we received xoff */
> +	if (!data)
> +		return;
> +
> +	for (i = 0; i < wx->num_tx_queues; i++)
> +		clear_bit(WX_HANG_CHECK_ARMED,
> +			  wx->tx_ring[i]->state);
> +}

If data is populated with garbage from the upper 32 bits, would this bypass
the check for zero and unconditionally clear the WX_HANG_CHECK_ARMED bit on
all Tx queues, preventing the Tx hang recovery watchdog from operating?

>  /**
>   * wx_update_stats - Update the board statistics counters.
>   * @wx: board private structure
> @@ -2844,6 +2877,10 @@ void wx_update_stats(struct wx *wx)
>  	u64 restart_queue = 0, tx_busy = 0;
>  	u32 i;
>  
> +	if (!netif_running(wx->netdev) ||
> +	    test_bit(WX_STATE_RESETTING, wx->state))
> +		return;
> +

Does this early return cause the final hardware statistics to be lost on
interface teardown?

Since the explicit wx_update_stats() calls were removed from
ngbe_disable_device() and txgbe_disable_device(), and the interface is
marked down before hardware disablement completes, it seems any statistics
accumulated since the last periodic service task will be lost when the
registers are cleared on reset.

[ ... ]
> @@ -2893,7 +2932,10 @@ void wx_update_stats(struct wx *wx)
>  	hwstats->mptc += rd64(wx, WX_TX_MC_FRAMES_GOOD_L);
>  	hwstats->roc += rd32(wx, WX_RX_OVERSIZE_FRAMES_GOOD);
>  	hwstats->ruc += rd32(wx, WX_RX_UNDERSIZE_FRAMES_GOOD);
> -	hwstats->lxonoffrxc += rd32(wx, WX_MAC_LXONOFFRXC);
> +	if (wx->mac.type >= wx_mac_aml)
> +		hwstats->lxonrxc = rd32(wx, WX_MAC_LXONRXC_AML);

Will this 64-bit software statistic wrap back to zero when the hardware
counter overflows?

As seen in wx_clear_hw_cntrs(), WX_MAC_LXONRXC_AML is cleared by writing 0:

    wr32(wx, WX_MAC_LXONRXC_AML, 0);

This suggests it is a continuous 32-bit hardware counter rather than
clear-on-read. Direct assignment might break the monotonic property expected
by ethtool.

> +	else
> +		hwstats->lxonrxc += rd32(wx, WX_MAC_LXONRXC);
>  	hwstats->lxontxc += rd32(wx, WX_RDB_LXONTXC);
>  	hwstats->lxofftxc += rd32(wx, WX_RDB_LXOFFTXC);
>  	hwstats->o2bgptc += rd32(wx, WX_TDM_OS2BMC_CNT);
[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index b8db772c37c5c..080310a51ef86 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -150,6 +150,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);
>  
>  	wx_service_event_complete(wx);
>  }

Can this introduce data races with userspace ethtool callbacks?

The service task executes asynchronously without holding rtnl_lock, while
ethtool callbacks like wx_get_pause_stats() hold rtnl_lock() and also call
wx_update_stats().

This might lead to double-reads of clear-on-read hardware registers and
lost software counter increments.

Also, could this cause a concurrency issue during interface teardown?

During reconfiguration, ngbe_close() deletes the service timer but does not
call cancel_work_sync(&wx->service_task).

If the service task runs concurrently with wx_clear_interrupt_scheme(),
it might bypass early return checks and access wx->tx_ring[i] while it
is being freed and set to NULL, potentially causing a use-after-free or
NULL pointer dereference.

[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index d4f87fbb95655..e9439e4ebe61e 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -132,6 +132,7 @@ static void txgbe_service_task(struct work_struct *work)
>  	wx_handle_errors_subtask(wx);
>  	txgbe_module_detection_subtask(wx);
>  	txgbe_link_config_subtask(wx);
> +	wx_update_stats(wx);
>  
>  	wx_service_event_complete(wx);
>  }

(Same questions apply to txgbe_service_task regarding data races and
teardown concurrency.)

[ ... ]