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 patch reworks iavf_set_mac() to send the ADD_ETH_ADDR virtchnl
message directly and poll the admin queue for the response, instead of
relying on the watchdog and adminq tasks (which now block on the netdev
instance lock held by iavf_set_mac()). It also exports
iavf_mac_add_reject() so the new path can roll back local failures, and
removes vc_waitqueue.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 78c59a58e0b2..ed790dc3de6b 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,
>  	return ret;
>  }
>  

[ ... ]

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

[Medium]
Can this path mishandle the multi-batch case in iavf_add_ether_addrs()?

When the number of filters with f->add=true exceeds what fits in one
IAVF_MAX_AQ_BUF_SIZE message, iavf_add_ether_addrs() trims the batch and
sets more=true:

	len = virtchnl_struct_size(veal, list, count);
	if (len > IAVF_MAX_AQ_BUF_SIZE) {
		dev_warn(&adapter->pdev->dev, "Too many add MAC changes in one request\n");
		while (len > IAVF_MAX_AQ_BUF_SIZE)
			len = virtchnl_struct_size(veal, list, --count);
		more = true;
	}
	...
	if (!more)
		adapter->aq_required &= ~IAVF_FLAG_AQ_ADD_MAC_FILTER;

Only the first batch is sent synchronously here; the remainder is left
for the watchdog. iavf_replace_primary_mac() calls iavf_add_filter()
which uses list_add_tail(), so the new primary MAC is at the end of
mac_filter_list and ends up in the last batch.

In that scenario, iavf_poll_virtchnl_response() polls
iavf_is_mac_set_handled() for the new MAC, which never completes within
2.5 seconds because the message that would change it has not been sent
yet. iavf_set_mac() then returns -EAGAIN and the rollback below removes
the new filter via iavf_mac_add_reject() because is_new_mac is true.

Pre-patch the wait_event() on vc_waitqueue would eventually be satisfied
because the watchdog drained subsequent batches and the PF response for
the new MAC arrived. Should iavf_set_mac_sync() loop while
IAVF_FLAG_AQ_ADD_MAC_FILTER (or more) is still set, so the new MAC is
actually transmitted before the timeout?

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

[Medium]
Is iavf_mac_add_reject() safe to call when the AQ message never reached
the PF?

iavf_add_ether_addrs() clears f->add=false on every filter that it packs
into the batch, before iavf_send_pf_msg() is invoked:

	list_for_each_entry(f, &adapter->mac_filter_list, list) {
		if (f->add) {
			ether_addr_copy(veal->list[i].addr, f->macaddr);
			iavf_set_mac_addr_type(&veal->list[i], f);
			i++;
			f->add = false;
			...
		}
	}
	...
	ret = iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);

iavf_mac_add_reject() then walks the entire mac_filter_list and marks
every filter with !f->add && !f->add_handled as add_handled=true:

	list_for_each_entry_safe(f, ftmp, &adapter->mac_filter_list, list) {
		...
		if (!f->add && !f->add_handled)
			f->add_handled = true;
		...
	}

On a local failure (iavf_send_pf_msg() error, IAVF_FLAG_PF_COMMS_FAILED,
-EBUSY, -ENOMEM or polling timeout) any unrelated filters that were in
the batch (e.g., entries added by a prior ndo_set_rx_mode() that the
watchdog had not yet drained) get add_handled=true even though the PF
never received them. They are not retried since f->add was cleared.

Pre-patch iavf_mac_add_reject() was only called from
iavf_virtchnl_completion() after the PF actually rejected an
ADD_ETH_ADDR, so marking these filters as handled was correct because
the PF had seen them. Should the new local-failure rollback be scoped to
only the new primary MAC filter, rather than reusing the PF-reject
helper unchanged?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 4f2defd2331b..cd5211b9a798 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -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.
> - **/
> -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;

[ ... ]