Re: [PATCH net-next v6 04/11] 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() callback for the ngbe driver.
It provides a routine to bring the device down and back up when a reset
is requested while the device is running.

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 8c9d505721b17..50a13dcf4b3c8 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c

[ ... ]

> @@ -579,6 +586,32 @@ int ngbe_setup_tc(struct net_device *dev, u8 tc)
>  	return 0;
>  }
>  
> +static void ngbe_reinit_locked(struct wx *wx)
> +{
> +	int err = 0;
> +
> +	netif_trans_update(wx->netdev);
> +
> +	err = wx_set_state_reset(wx);
> +	if (err) {
> +		wx_err(wx, "wait device reset timeout\n");
> +		return;
> +	}
> +
> +	ngbe_down(wx);
> +	ngbe_up(wx);
> +
> +	clear_bit(WX_STATE_RESETTING, wx->state);
> +}

Is it possible to use a standard kernel mutex here instead of a bit flag for
synchronization?

Looking at the underlying implementation of wx_set_state_reset(), it relies
on an open-coded polling loop:

static inline int wx_set_state_reset(struct wx *wx)
{
        u8 timeout = 50;

        while (test_and_set_bit(WX_STATE_RESETTING, wx->state)) {
                timeout--;
                if (!timeout)
                        return -EBUSY;

                usleep_range(1000, 2000);
        }

        return 0;
}

Using a bit flag and a sleep-polling loop to guard a teardown and bringup
section acts as an ad-hoc lock. This approach bypasses standard kernel
synchronization guarantees and prevents lockdep from analyzing the lock
ordering.

Since this context is allowed to sleep, could this section be protected by
a standard mutex instead?
-- 
pw-bot: cr