Re: [PATCH net 5/8] iavf: send MAC change request synchronously

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
iavf: send MAC change request synchronously

This commit implements a synchronous MAC address change operation that sends
the ADD_ETH_ADDR message directly and polls for the response. This avoids
deadlocks involving the netdev lock, watchdog task, and adminq task.

cc: stable@vger.kernel.org

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 78c59a58e0b2b..ed790dc3de6b7 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
[ ... ]
> @@ -1029,6 +1029,48 @@ static bool iavf_is_mac_set_handled(struct net_device *netdev,
[ ... ]
> +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;

[Severity: Medium]
Will this reliably fail with -EBUSY if a background operation (such as
VIRTCHNL_OP_GET_STATS sent by the watchdog) is already in flight?

If adapter->current_op != VIRTCHNL_OP_UNKNOWN, iavf_add_ether_addrs()
instantly returns -EBUSY. Because iavf_set_mac() holds the netdev_lock,
the adminq_task cannot run to process the pending response and clear
current_op, resulting in a spurious failure rather than waiting for the
background operation to complete.

> +
> +	return iavf_poll_virtchnl_response(adapter, iavf_mac_change_done,
> +					   addr, 2500);
> +}
[ ... ]
> @@ -1049,25 +1091,21 @@ 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);
> +		ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
>  		return ret;
> -
> -	if (!ret)
> -		return -EAGAIN;

[Severity: High]
Does rolling back on local failure here cause a permanent state mismatch
between the PF and VF?

If the MAC change request was already sent to the PF by
iavf_add_ether_addrs(), but iavf_poll_virtchnl_response() subsequently
fails (e.g., memory allocation failure for event.msg_buf, or a timeout
occurs), this local rollback deletes the new MAC filter and restores the
old MAC.

However, the PF might still asynchronously process the request and
configure the new MAC. Later, the VF's watchdog will send a pending
deletion for the old MAC, leaving the PF configured with the new MAC
while the VF uses the old MAC, causing a complete loss of network
connectivity.

Should the buffer allocation in iavf_poll_virtchnl_response() be done
before sending the request to the PF, and can timeouts be handled without
rolling back in-flight requests?