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

[Jose Ignacio Tornos Martinez <jtornosm@redhat.com>]

All the comments below fall into pre-existing issues, concerns already
addressed in previous versions and comments, out-of-scope items, or extreme
edge cases. No code changes are considered necessary for a new version.
Details for each comment:

> [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?

Yes, that is the intended split. This patch is a preparatory improvement
that adds a best-effort early bail-out, but it does not remove the wait
under the netdev instance lock. Patch 3/4 ("iavf: send MAC change
request synchronously") is the one that actually eliminates the
problematic wait and carries the Fixes: ad7c7b2172c3 tag and cc: stable.

Adding Fixes: here would be misleading since this patch alone does not
fix the problem — it only reduces the likelihood of hitting it.

> [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)?

Yes, this is a best-effort guard to inform the user as soon as possible
instead of blocking with the netdev instance lock held. If a reset starts
immediately after the check, the behavior falls back to the pre-patch
code path: blocked for up to 2.5s, then -EAGAIN. No new failure mode is
introduced.

The flags/state access patterns (plain reads, non-atomic RMW on
adapter->flags) are pre-existing throughout the iavf driver — the
watchdog, tx_timeout, and adminq paths all use the same style.
Introducing READ_ONCE / proper serialization here alone would be
inconsistent and incomplete. That is a broader driver-wide cleanup,
out of scope for this fix.