Re: [PATCH net 01/10] iavf: return EBUSY if reset in progress or not ready during MAC change
Jose Ignacio Tornos Martinez <[email protected]>
| Newsgroups | org.kernel.vger.stable,org.kernel.vger.netdev |
|---|---|
| Message-ID | <[email protected]> |
All the comments below fall into pre-existing issues, concerns already
addressed in previous versions, out-of-scope items, or extreme edge
cases. No code changes are considered necessary for a new version.
Details for each comment:
> [Severity: Low]
> Is the placement of the new gate ahead of is_valid_ether_addr()
> intentional?
>
> With this ordering, a request carrying an address that can never be
> accepted (all-zero, multicast, other non-unicast MAC) now reports -EBUSY
> rather than -EADDRNOTAVAIL whenever iavf_is_reset_in_progress(adapter) is
> true or adapter->state < __IAVF_DOWN.
>
> The core does not validate the address itself, so the driver check is the
> only one on the rtnetlink/ioctl/sysfs paths:
>
> net/core/dev.c:netif_set_mac_address() {
> if (!ops->ndo_set_mac_address)
> return -EOPNOTSUPP;
> if (ss->ss_family != dev->type)
> return -EINVAL;
> if (!netif_device_present(dev))
> return -ENODEV;
> ...
> err = ops->ndo_set_mac_address(dev, ss);
> }
>
> Since -EBUSY advertises a transient condition, callers that retry on EBUSY
> would loop on input that can never succeed. Would moving the state gate
> below the is_valid_ether_addr() check work equally well for the stated
> goal?
>
> The ordering is still present at the end of the series, and the commit
> message does not mention the change in returned errno for invalid
> addresses. Could that be noted, or the checks reordered?
The scenario requires passing an invalid MAC address (multicast, all-zero)
while the VF is resetting. -EBUSY is still a correct response, the VF
cannot perform any operation during reset regardless of the address.
If a caller retries on -EBUSY, the reset completes quickly (milliseconds
to seconds) and the next attempt returns -EADDRNOTAVAIL as expected.
This is a transient errno difference, not a functional issue.