Re: [PATCH net-next v2 1/6] net: ngbe: implement libwx reset ops

[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: ngbe: implement libwx reset ops

This commit implements the wx->do_reset() operation for the ngbe driver,
allowing the library module to trigger a reset by bringing the interface
down and back up.

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index d8e3827a8b1f2..bd905e2675758 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c

[ ... ]

> +static void ngbe_reinit_locked(struct wx *wx)
> +{
> +	netif_trans_update(wx->netdev);
> +
> +	mutex_lock(&wx->reset_lock);
> +	set_bit(WX_STATE_RESETTING, wx->state);
> +
> +	ngbe_down(wx);
> +	ngbe_up(wx);
> +
> +	clear_bit(WX_STATE_RESETTING, wx->state);

Does this introduce a regression by using WX_STATE_RESETTING as an ad-hoc
lock here?

While this pattern appears to exist elsewhere in the driver, adding it
to ngbe_reinit_locked() relies on a design where background readers might
check test_bit(WX_STATE_RESETTING, wx->state) without holding reset_lock.

Since the flag check and subsequent accesses are not atomic, a reader could
test the flag just before it is set and then proceed to access shared state
concurrently with the ngbe_down() and ngbe_up() teardown path.

Could this result in torn or corrupted reads, and would it be safer to use a
proper synchronization mechanism like RCU or a shared lock for readers?

> +	mutex_unlock(&wx->reset_lock);
> +}
> +
> +void ngbe_do_reset(struct net_device *netdev)

[ ... ]