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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

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;
> +}