Re: [Intel-wired-lan] [PATCH net v4 3/4] iavf: send MAC change request synchronously
From: 'Simon Horman'
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
iavf: send MAC change request synchronously
This commit updates the MAC address setting in the iavf driver to be
synchronous. It sends the virtchnl message directly and polls the admin
queue for the response, rather than relying on the watchdog task, to avoid
deadlocks when holding the netdev instance lock.
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 67aa14350b1b01..bc5994bf2cd99d 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
[ ... ]
> @@ -1067,25 +1109,20 @@ static int iavf_set_mac(struct net_device *netdev,
> void *p)
> return -EADDRNOTAVAIL;
>
> ret = iavf_replace_primary_mac(adapter, addr->sa_data);
> -
> if (ret)
> return ret;
>
> - ret = wait_event_interruptible_timeout(adapter->vc_waitqueue,
> -iavf_is_mac_set_handled(netdev,
> addr->sa_data),
> -msecs_to_jiffies(2500));
> -
> - /* If ret < 0 then it means wait was interrupted.
> - * If ret == 0 then it means we got a timeout.
> - * else it means we got response for set MAC from PF,
> - * check if netdev MAC was updated to requested MAC,
> - * if yes then set MAC succeeded otherwise it failed return -EACCES
> - */
> - if (ret < 0)
> + ret = iavf_set_mac_sync(adapter, addr->sa_data);
> + if (ret) {
> + /* Rollback for local failures (timeout, send error, -EBUSY).
> + * Note: If PF rejects the request (sends error response),
> + * iavf_virtchnl_completion() automatically calls
> + * iavf_mac_add_reject(), ret=0, and this is not executed.
> + * Only local failures (no PF response received) need manual
> rollback.
> + */
> + iavf_mac_add_reject(adapter);
> return ret;
> -
> - if (!ret)
> - return -EAGAIN;
> + }
Does the manual rollback in iavf_set_mac() need to restore adapter->hw.mac.addr?
Before calling iavf_set_mac_sync(), iavf_replace_primary_mac() optimistically
updates adapter->hw.mac.addr to the new MAC address and unsets the is_primary
flag on the old MAC filter. If iavf_set_mac_sync() fails locally,
iavf_mac_add_reject() deletes the pending new MAC filter but doesn't restore
adapter->hw.mac.addr back to netdev->dev_addr.
By contrast, the PF-rejection path in iavf_virtchnl_completion() explicitly
restores it:
iavf_mac_add_reject(adapter);
/* restore administratively set MAC address */
ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
If the local failure was a timeout and a delayed success response arrives from
the PF later, could iavf_virtchnl_completion() blindly copy the corrupted
adapter->hw.mac.addr into netdev->dev_addr? This seems like it would silently
change the interface MAC to an address already deleted from the internal list.
Additionally, on subsequent MAC change attempts, wouldn't
iavf_replace_primary_mac() search for the old filter using the corrupted
adapter->hw.mac.addr, fail to find it, and permanently leak the old filter in
the PF's hardware tables?
[ ... ]
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index a52c100dcbc56d..d1afb8261c2412 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
[ ... ]
> @@ -2956,3 +2966,76 @@ void iavf_virtchnl_completion(struct iavf_adapter
> *adapter,
> } /* switch v_opcode */
> adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> }
> +
> +/**
> + * iavf_poll_virtchnl_response - Poll admin queue for virtchnl response
> + * @adapter: adapter structure
> + * @condition: callback to check if desired response received
> + * @cond_data: context data passed to condition callback
> + * @timeout_ms: maximum time to wait in milliseconds
> + *
> + * Polls the admin queue and processes all incoming virtchnl messages.
> + * After processing each valid message, calls the condition callback to check
> + * if the expected response has been received. The callback receives the
> opcode
> + * of the processed message to identify which response was received.
> Continues
> + * polling until the callback returns true or timeout expires.
> + * Clear current_op on timeout to prevent permanent -EBUSY state.
> + * Caller must hold netdev_lock. This can sleep for up to timeout_ms while
> + * polling hardware.
> + *
> + * Return: 0 on success (condition met), -EAGAIN on timeout, or error code
> + **/
> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
> + bool
Re: [Intel-wired-lan] [PATCH net v4 3/4] iavf: send MAC change request synchronously
Hello Przemek, Thank you again for your comments. I will try to include the new ones too in the next version. Indeed kdoc "Return:" was already commented by you, I will try to address all the occurences now. Thanks Best regards Jose Ignacio
Re: [Intel-wired-lan] [PATCH net v4 3/4] iavf: send MAC change request synchronously
On 4/23/26 15:04, Jose Ignacio Tornos Martinez wrote:
After commit ad7c7b2172c3 ("net: hold netdev instance lock during sysfs
operations"), iavf_set_mac() is called with the netdev instance lock
already held.
The function queues a MAC address change request via
iavf_replace_primary_mac() and then waits for completion. However, in
the current flow, the actual virtchnl message is sent by the watchdog
task, which also needs to acquire the netdev lock to run. Additionally,
the adminq_task which processes virtchnl responses also needs the netdev
lock.
This creates a deadlock scenario:
1. iavf_set_mac() holds netdev lock and waits for MAC change
2. Watchdog needs netdev lock to send the request -> blocked
3. Even if request is sent, adminq_task needs netdev lock to process
PF response -> blocked
4. MAC change times out after 2.5 seconds
5. iavf_set_mac() returns -EAGAIN
This particularly affects VFs during bonding setup when multiple VFs are
enslaved in quick succession.
Fix by implementing a synchronous MAC change operation similar to the
approach used in commit fdadbf6e84c4 ("iavf: fix incorrect reset handling
in callbacks").
The solution:
1. Send the virtchnl ADD_ETH_ADDR message directly (not via watchdog)
2. Poll the admin queue hardware directly for responses
3. Process all received messages (including non-MAC messages)
4. Return when MAC change completes or times out
A new generic function iavf_poll_virtchnl_response() is introduced that
can be reused for any future synchronous virtchnl operations. It takes a
callback to check completion, allowing flexible condition checking.
This allows the operation to complete synchronously while holding
netdev_lock, without relying on watchdog or adminq_task. The function
can sleep for up to 2.5 seconds polling hardware, but this is acceptable
since netdev_lock is per-device and only serializes operations on the
same interface.
To support this, change iavf_add_ether_addrs() to return an error code
instead of void, allowing callers to detect failures. Additionally,
export iavf_mac_add_reject() to enable proper rollback on local failures
(timeouts, send errors) - PF rejections are already handled automatically
by iavf_virtchnl_completion().
Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations")
cc: [email protected]
Signed-off-by: Jose Ignacio Tornos Martinez
---
v4: Complete with Przemek Kitszel comments:
- Remove vc_waitqueue entirely (not needed any more)
nit: I would add a short note to commit message too
thanks a lot for the rest of changes
I have a few last nits, please find below
- Add named parameters to callback function pointer declaration for
clarity
- Simplify callback signature: add v_op parameter so callback
receives the opcode from the processed message to identify which
response was received
- Optimize polling loop to single condition check per iteration
instead of checking both before and after message processing
Address AI review (sashiko.dev) from Simon Horman:
- Complete iavf_add_ether_addrs() error handling
- Skip non-virtchnl hardware events (received_op=VIRTCHNL_OP_UNKNOWN),
these can cause false completion detection
- Complete rollback for local failures (not PF rejection) reusing
iavf_mac_add_reject() to restore the old primary filter
v3: https://lore.kernel.org/netdev/[email protected]/
drivers/net/ethernet/intel/iavf/iavf.h| 10 +-
drivers/net/ethernet/intel/iavf/iavf_main.c | 70 +
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 99 +--
3 files changed, 151 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
b/drivers/net/ethernet/intel/iavf/iavf.h
index e9fb0a0919e3..78fa3df06e11 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -260,7 +260,6 @@ struct iavf_adapter {
struct work_struct adminq_task;
struct work_struct finish_config;
wait_queue_head_t down_waitqueue;
- wait_queue_head_t vc_waitqueue;
struct iavf_q_vector *q_vectors;
struct list_head vlan_filter_list;
int num_vlan_filters;
@@ -589,8 +588,9 @@ void iavf_configure_queues(struct iavf_adapter *adapter);
void iavf_enable_queues(struct iavf_adapter *adapter);
void iavf_disable_queues(struct iavf_adapter *adapter);
void iavf_map_queues(struct iavf_adapter *adapter);
-void iavf_add_ether_addrs(struct iavf_adapter *adapter);
+int iavf_add_ether_addrs(struct iavf_adapter *adapter);
void iavf_del_ether_addrs(struct iavf_adapter *adapter);
+void iavf_mac_add_reject(struct iavf_adapter *adapter);
void iavf_add_vlans(struct iavf_adapter *adapter);
void iavf_del_vlans(struct iavf_adapter *adapter);
void iavf_set_promiscuous(struct iavf_adapter *adapter);
@@ -607,6 +607,12 @@ void iavf_disable_vlan_stripping(struct iavf_adapter
*adapter);
void
Re: [Intel-wired-lan] [PATCH net v4 3/4] iavf: send MAC change request synchronously
> -Original Message- > From: Jose Ignacio Tornos Martinez > Sent: Thursday, April 23, 2026 3:04 PM > To: [email protected] > Cc: [email protected]; Kitszel, Przemyslaw > ; Loktionov, Aleksandr > ; Keller, Jacob E > ; [email protected]; > [email protected]; Nguyen, Anthony L > ; [email protected]; > [email protected]; [email protected]; [email protected]; Jose Ignacio > Tornos Martinez ; [email protected] > Subject: [PATCH net v4 3/4] iavf: send MAC change request > synchronously > > After commit ad7c7b2172c3 ("net: hold netdev instance lock during > sysfs operations"), iavf_set_mac() is called with the netdev instance > lock already held. > > The function queues a MAC address change request via > iavf_replace_primary_mac() and then waits for completion. However, in > the current flow, the actual virtchnl message is sent by the watchdog > task, which also needs to acquire the netdev lock to run. > Additionally, the adminq_task which processes virtchnl responses also > needs the netdev lock. > > This creates a deadlock scenario: > 1. iavf_set_mac() holds netdev lock and waits for MAC change 2. > Watchdog needs netdev lock to send the request -> blocked 3. Even if > request is sent, adminq_task needs netdev lock to process >PF response -> blocked > 4. MAC change times out after 2.5 seconds 5. iavf_set_mac() returns - > EAGAIN > > This particularly affects VFs during bonding setup when multiple VFs > are enslaved in quick succession. > > Fix by implementing a synchronous MAC change operation similar to the > approach used in commit fdadbf6e84c4 ("iavf: fix incorrect reset > handling in callbacks"). > > The solution: > 1. Send the virtchnl ADD_ETH_ADDR message directly (not via watchdog) > 2. Poll the admin queue hardware directly for responses 3. Process all > received messages (including non-MAC messages) 4. Return when MAC > change completes or times out > > A new generic function iavf_poll_virtchnl_response() is introduced > that can be reused for any future synchronous virtchnl operations. It > takes a callback to check completion, allowing flexible condition > checking. > > This allows the operation to complete synchronously while holding > netdev_lock, without relying on watchdog or adminq_task. The function > can sleep for up to 2.5 seconds polling hardware, but this is > acceptable since netdev_lock is per-device and only serializes > operations on the same interface. > > To support this, change iavf_add_ether_addrs() to return an error code > instead of void, allowing callers to detect failures. Additionally, > export iavf_mac_add_reject() to enable proper rollback on local > failures (timeouts, send errors) - PF rejections are already handled > automatically by iavf_virtchnl_completion(). > > Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs > operations") > cc: [email protected] > Signed-off-by: Jose Ignacio Tornos Martinez > --- > v4: Complete with Przemek Kitszel comments: > - Remove vc_waitqueue entirely (not needed any more) > - Add named parameters to callback function pointer declaration > for > clarity > - Simplify callback signature: add v_op parameter so callback > receives the opcode from the processed message to identify which > response was received > - Optimize polling loop to single condition check per iteration > instead of checking both before and after message processing > Address AI review (sashiko.dev) from Simon Horman: > - Complete iavf_add_ether_addrs() error handling > - Skip non-virtchnl hardware events > (received_op=VIRTCHNL_OP_UNKNOWN), > these can cause false completion detection > - Complete rollback for local failures (not PF rejection) reusing > iavf_mac_add_reject() to restore the old primary filter > v3: https://lore.kernel.org/netdev/20260414110006.124286-4- > [email protected]/ > > drivers/net/ethernet/intel/iavf/iavf.h| 10 +- > drivers/net/ethernet/intel/iavf/iavf_main.c | 70 + > .../net/ethernet/intel/iavf/iavf_virtchnl.c | 99 +- > - > 3 files changed, 151 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf.h > b/drivers/net/ethernet/intel/iavf/iavf.h > index e9fb0a0919e3..78fa3df06e11 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf.h > +++ b/drivers/net/ethernet/intel/iavf/iavf.h > @@ -260,7 +260,6 @@ struct iavf_adapter { > struct work_struct adminq_task; > struct work_struct finish_config; > wait_queue_head_t down_waitqueue; ... > -- > 2.53.0 Reviewed-by: Aleksandr Loktionov
