Re: [PATCH net 1/4] iavf: return EBUSY if reset in progress or not ready during MAC change

[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.
---
iavf: return EBUSY if reset in progress or not ready during MAC change

This adds an early bail-out to iavf_set_mac() so that a MAC address change
returns -EBUSY when a reset is in progress or when the VF has not yet
reached __IAVF_DOWN.  The stated motivation is to avoid waiting inside the
ndo callback while the netdev instance lock is held.

No Link: tags present in the commit header.

A couple of questions below, one about the commit message and one about the
new check itself.

>     After commit ad7c7b2172c3 ("net: hold netdev instance lock
>     during sysfs operations"), MAC changes are called with the netdev lock
>     held, so we should not wait with the lock held during reset or
>     initialization.

[Severity: Low]
This paragraph describes a user-visible behaviour change caused by
ad7c7b2172c3, but there is no Fixes: tag on this commit.  Was that
intentional?

Worth noting that this commit does not actually remove the wait under the
netdev instance lock: after this patch iavf_set_mac() still calls

	ret = wait_event_interruptible_timeout(adapter->vc_waitqueue,
					       iavf_is_mac_set_handled(netdev, addr->sa_data),
					       msecs_to_jiffies(2500));

so a Fixes: ad7c7b2172c3 here would arguably be misleading.  The later
patch in the same series, "iavf: send MAC change request synchronously",
does carry Fixes: ad7c7b2172c3 and cc: stable, which looks like the right
place for the provenance.  Is that the intended split?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 29b8403a066b..630388e9d28c 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1042,6 +1042,9 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>  	struct sockaddr *addr = p;
>  	int ret;
>  
> +	if (iavf_is_reset_in_progress(adapter) || adapter->state < __IAVF_DOWN)
> +		return -EBUSY;
> +

[Severity: Low]
Is this check-then-act sequence racy against the reset scheduling path?

iavf_is_reset_in_progress() does plain reads of adapter->state and
adapter->flags:

	static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter)
	{
		if (adapter->state == __IAVF_RESETTING ||
		    adapter->flags & (IAVF_FLAG_RESET_PENDING |
				      IAVF_FLAG_RESET_NEEDED))
			return true;

		return false;
	}

IAVF_FLAG_RESET_NEEDED can be set from a context that does not hold the
netdev instance lock:

dev_watchdog()
  netif_freeze_queues(dev);
  dev->netdev_ops->ndo_tx_timeout(dev, i);    /* timer context */
    iavf_tx_timeout()
      iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED)
        adapter->flags |= flags;              /* non-atomic RMW */

net/sched/sch_generic.c only freezes the TX queues around ndo_tx_timeout(),
so there does not appear to be anything serializing that writer against
iavf_set_mac().  If the TX timeout fires just after the new test, does
iavf_set_mac() go on to the 2.5 s wait_event_interruptible_timeout() with
the netdev instance lock held anyway, i.e. exactly the case the check is
meant to reject?

The window is small and the fallback is the pre-patch behaviour (blocked
for up to 2.5 s, then -EAGAIN), so this looks like a best-effort guard
rather than a new failure mode.  Is that the intent, or should the state
and flags reads be done under the same serialization as the reset path
(and with READ_ONCE(), given the concurrent non-atomic |= on
adapter->flags)?