Re: [PATCH net v4 3/4] iavf: send MAC change request synchronously

Przemek Kitszel
2026-04-27T09:23:26Z

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: stable@vger.kernel.org
> Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
> ---
> 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/20260414110006.124286-4-jtornosm@redhat.com/
>
> 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 iavf_virtchnl_completion(struct iavf_adapter *adapter,
> enum virtchnl_ops v_opcode,
> enum iavf_status v_retval, u8 *msg, u16 msglen);
> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
> + bool (*condition)(struct iavf_adapter *adapter,
> + const void *data,
> + enum virtchnl_ops v_op),
> + const void *cond_data,
> + unsigned int timeout_ms);
> int iavf_config_rss(struct iavf_adapter *adapter);
> void iavf_cfg_queues_bw(struct iavf_adapter *adapter);
> void iavf_cfg_queues_quanta_size(struct iavf_adapter *adapter);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 67aa14350b1b..bc5994bf2cd9 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1047,6 +1047,48 @@ static bool iavf_is_mac_set_handled(struct net_device *netdev,
> return ret;
> }
>
> +/**
> + * iavf_mac_change_done - Check if MAC change completed
> + * @adapter: board private structure
> + * @data: MAC address being checked (as const void *)
> + * @v_op: virtchnl opcode from processed message
> + *
> + * Callback for iavf_poll_virtchnl_response() to check if MAC change completed.
> + *
> + * Returns true if MAC change completed, false otherwise

I'm not a fan of kdoc, but would rather write kdoc-compilant comments:
Return: ...

> + */
> +static bool iavf_mac_change_done(struct iavf_adapter *adapter,
> + const void *data, enum virtchnl_ops v_op)
> +{
> + const u8 *addr = data;
> +
> + return iavf_is_mac_set_handled(adapter->netdev, addr);
> +}
> +
> +/**
> + * iavf_set_mac_sync - Synchronously change MAC address
> + * @adapter: board private structure
> + * @addr: MAC address to set
> + *
> + * Sends MAC change request to PF and polls admin queue for response.
> + * Caller must hold netdev_lock. This can sleep for up to 2.5 seconds.
> + *
> + * Returns 0 on success, negative on failure

ditto kdoc "Return:"

> + */
> +static int iavf_set_mac_sync(struct iavf_adapter *adapter, const u8 *addr)
> +{
> + int ret;
> +
> + netdev_assert_locked(adapter->netdev);
> +
> + ret = iavf_add_ether_addrs(adapter);
> + if (ret)
> + return ret;
> +
> + return iavf_poll_virtchnl_response(adapter, iavf_mac_change_done,
> + addr, 2500);
> +}
> +
> /**
> * iavf_set_mac - NDO callback to set port MAC address
> * @netdev: network interface device structure
> @@ -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;
> + }
>
> if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
> return -EACCES;
> @@ -5415,9 +5452,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> /* Setup the wait queue for indicating transition to down status */
> init_waitqueue_head(&adapter->down_waitqueue);
>
> - /* Setup the wait queue for indicating virtchannel events */
> - init_waitqueue_head(&adapter->vc_waitqueue);
> -
> INIT_LIST_HEAD(&adapter->ptp.aq_cmds);
> init_waitqueue_head(&adapter->ptp.phc_time_waitqueue);
> mutex_init(&adapter->ptp.aq_cmd_lock);
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index a52c100dcbc5..d1afb8261c24 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -2,6 +2,7 @@
> /* Copyright(c) 2013 - 2018 Intel Corporation. */
>
> #include <linux/net/intel/libie/rx.h>
> +#include <net/netdev_lock.h>
>
> #include "iavf.h"
> #include "iavf_ptp.h"
> @@ -555,20 +556,23 @@ iavf_set_mac_addr_type(struct virtchnl_ether_addr *virtchnl_ether_addr,
> * @adapter: adapter structure
> *
> * Request that the PF add one or more addresses to our filters.
> + *
> + * Return: 0 on success, negative on failure
> **/

thank you for also changing the kdoc when touching the function :)

> -void iavf_add_ether_addrs(struct iavf_adapter *adapter)
> +int iavf_add_ether_addrs(struct iavf_adapter *adapter)
> {
> struct virtchnl_ether_addr_list *veal;
> struct iavf_mac_filter *f;
> int i = 0, count = 0;
> bool more = false;
> size_t len;
> + int ret;
>
> if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
> /* bail because we already have a command pending */
> dev_err(&adapter->pdev->dev, "Cannot add filters, command %d pending\n",
> adapter->current_op);
> - return;
> + return -EBUSY;
> }
>
> spin_lock_bh(&adapter->mac_vlan_list_lock);
> @@ -580,7 +584,7 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
> if (!count) {
> adapter->aq_required &= ~IAVF_FLAG_AQ_ADD_MAC_FILTER;
> spin_unlock_bh(&adapter->mac_vlan_list_lock);
> - return;
> + return 0;
> }
> adapter->current_op = VIRTCHNL_OP_ADD_ETH_ADDR;
>
> @@ -594,8 +598,9 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
>
> veal = kzalloc(len, GFP_ATOMIC);
> if (!veal) {
> + adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> spin_unlock_bh(&adapter->mac_vlan_list_lock);
> - return;
> + return -ENOMEM;
> }
>
> veal->vsi_id = adapter->vsi_res->vsi_id;
> @@ -615,8 +620,15 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
>
> spin_unlock_bh(&adapter->mac_vlan_list_lock);
>
> - iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);
> + ret = iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);
> kfree(veal);
> + if (ret) {
> + dev_err(&adapter->pdev->dev,
> + "Unable to send ADD_ETH_ADDR message to PF, error %d\n", ret);
> + adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> + }
> +
> + return ret;
> }
>
> /**
> @@ -713,7 +725,7 @@ static void iavf_mac_add_ok(struct iavf_adapter *adapter)
> *
> * Remove filters from list based on PF response.
> **/
> -static void iavf_mac_add_reject(struct iavf_adapter *adapter)
> +void iavf_mac_add_reject(struct iavf_adapter *adapter)
> {
> struct net_device *netdev = adapter->netdev;
> struct iavf_mac_filter *f, *ftmp;
> @@ -2389,7 +2401,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
> iavf_mac_add_reject(adapter);
> /* restore administratively set MAC address */
> ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
> - wake_up(&adapter->vc_waitqueue);
> break;
> case VIRTCHNL_OP_DEL_VLAN:
> dev_err(&adapter->pdev->dev, "Failed to delete VLAN filter, error %s\n",
> @@ -2586,7 +2597,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
> eth_hw_addr_set(netdev, adapter->hw.mac.addr);
> netif_addr_unlock_bh(netdev);
> }
> - wake_up(&adapter->vc_waitqueue);
> break;
> case VIRTCHNL_OP_GET_STATS: {
> struct iavf_eth_stats *stats =
> @@ -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
> + **/

single star for closing coments **/ → */

> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
> + bool (*condition)(struct iavf_adapter *adapter,
> + const void *data,
> + enum virtchnl_ops v_op),
> + const void *cond_data,
> + unsigned int timeout_ms)
> +{
> + struct iavf_hw *hw = &adapter->hw;
> + struct iavf_arq_event_info event;
> + enum virtchnl_ops received_op;
> + unsigned long timeout;
> + u32 v_retval;
> + u16 pending;
> + int ret = -EAGAIN;

RCT violation - we sort lines from longest to shortest

> +
> + netdev_assert_locked(adapter->netdev);
> +
> + event.buf_len = IAVF_MAX_AQ_BUF_SIZE;
> + event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL);
> + if (!event.msg_buf)
> + return -ENOMEM;
> +
> + timeout = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + if (iavf_clean_arq_element(hw, &event, &pending) == IAVF_SUCCESS) {
> + received_op = (enum virtchnl_ops)le32_to_cpu(event.desc.cookie_high);
> + if (received_op != VIRTCHNL_OP_UNKNOWN) {
> + v_retval = le32_to_cpu(event.desc.cookie_low);
> +
> + iavf_virtchnl_completion(adapter, received_op,
> + (enum iavf_status)v_retval,
> + event.msg_buf, event.msg_len);
> +
> + if (condition(adapter, cond_data, received_op)) {
> + ret = 0;
> + break;
> + }
> + }
> +
> + memset(event.msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);
> +
> + if (pending)
> + continue;
> + }
> +
> + usleep_range(50, 75);

we got again to the "sleep then check time" situation
to resolve that, you could init @pending with 0, and sleep at the very
begining of each loop step if (!pending)

after that I will be no longer complaining on this patch,
thank you again for the work!

> + } while (time_before(jiffies, timeout));
> +
> + if (ret == -EAGAIN && adapter->current_op != VIRTCHNL_OP_UNKNOWN)
> + adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> +
> + kfree(event.msg_buf);
> + return ret;
> +}