Re: [PATCH net 3/4] 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 converts iavf_set_mac() from queueing the ADD_ETH_ADDR request for the
watchdog and waiting on vc_waitqueue into sending the virtchnl message
directly and polling the admin receive queue for the reply. A new generic
helper iavf_poll_virtchnl_response() is added, iavf_add_ether_addrs() now
returns an error code, iavf_mac_add_reject() is exported for rollback, and
vc_waitqueue is removed.

A few questions about the new synchronous path below.

First, about the changelog text:

>     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().

[Severity: Low]

Is the "timeouts" part accurate? iavf_poll_virtchnl_response() returns
-EAGAIN on timeout, and iavf_set_mac() explicitly skips the rollback for
that value:

	if (ret != -EAGAIN) {
		iavf_mac_add_reject(adapter);
		ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
	}

So on a timeout nothing is rolled back, which is the opposite of what the
changelog describes (and matches the in-code comment). Could the changelog
be adjusted to say send errors only?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index dc31202b2a94..8c45536fd502 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h

[ ... ]

> @@ -588,8 +587,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);

[Severity: Low]

Now that iavf_add_ether_addrs() reports -EBUSY, -ENOMEM and send failures,
should the watchdog side caller consume it too? iavf_process_aq_command()
still discards it and returns 0 unconditionally:

	if (adapter->aq_required & IAVF_FLAG_AQ_ADD_MAC_FILTER) {
		iavf_add_ether_addrs(adapter);
		return 0;
	}

which also disagrees with that function's own kernel-doc ("Returns error
code if no command was sent or error code if the command failed").

[ ... ]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 630388e9d28c..3fa288e3798a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1029,6 +1029,60 @@ 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.
> + *
> + * Return: true if MAC change completed, false otherwise
> + */
> +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);
> +}

[Severity: Medium]

iavf_mac_change_done() ignores v_op and looks only at filter state, while
iavf_poll_virtchnl_response() calls the predicate after every message it
processes.

Consider setting an address that already has a filter whose earlier add
succeeded, so add_handled is true and is_new_mac is false.
iavf_replace_primary_mac() sets add = true but does not reset add_handled,
and iavf_add_ether_addrs() clears add again while building the batch:

iavf_is_mac_set_handled():
	f = iavf_find_filter(adapter, macaddr);

	if (!f || (!f->add && f->add_handled))
		ret = true;

Can an unrelated message processed by the poll loop, for example a
VIRTCHNL_OP_EVENT link change, then make this return true before the
ADD_ETH_ADDR reply arrives? netdev->dev_addr is only updated from
iavf_virtchnl_completion(), so iavf_set_mac() would return -EACCES for a
request that is still in flight and later applied.

The old code only re-evaluated the condition on
wake_up(&adapter->vc_waitqueue), which was issued exclusively from the
VIRTCHNL_OP_ADD_ETH_ADDR completion paths.

> +
> +/**
> + * iavf_set_mac_sync - Synchronously change MAC address
> + * @adapter: board private structure
> + * @addr: MAC address to set
> + *
> + * Send MAC change request to PF and poll admin queue for response.
> + * Caller must hold netdev_lock. This can sleep for up to 2.5 seconds.
> + * Event buffer is allocated before sending to avoid state mismatch if
> + * allocation fails after message is sent to PF.
> + *
> + * Return: 0 on success, negative on failure
> + */
> +static int iavf_set_mac_sync(struct iavf_adapter *adapter, const u8 *addr)
> +{
> +	struct iavf_arq_event_info event;
> +	int ret;
> +
> +	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;
> +
> +	ret = iavf_add_ether_addrs(adapter);
> +	if (ret)
> +		goto out;
> +
> +	ret = iavf_poll_virtchnl_response(adapter, &event,
> +					  iavf_mac_change_done, addr, 2500);

[Severity: Medium]

Only one batch is sent here. iavf_add_ether_addrs() truncates the request
when it does not fit the admin queue buffer:

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

The remaining filters keep f->add == true and IAVF_FLAG_AQ_ADD_MAC_FILTER
stays set, and iavf_add_filter() appends new filters to the tail of
mac_filter_list. If the just requested primary address ends up outside the
first batch, can iavf_mac_change_done() ever become true, given
iavf_is_mac_set_handled() requires !f->add? The poll loop has no way to send
the next batch, so this would burn the full 2500 ms and return -EAGAIN to
userspace while the watchdog applies the change afterwards.

> +
> +out:
> +	kfree(event.msg_buf);
> +	return ret;
> +}
> +
>  /**
>   * iavf_set_mac - NDO callback to set port MAC address
>   * @netdev: network interface device structure
> @@ -1049,25 +1103,23 @@ 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 only if send failed (message never reached PF).
> +		 * Don't rollback on timeout (-EAGAIN) because the message was
> +		 * sent and PF will eventually respond. When the response arrives,
> +		 * iavf_virtchnl_completion() will handle rollback (on PF error)
> +		 * or acceptance (on PF success) automatically.
> +		 */
> +		if (ret != -EAGAIN) {
> +			iavf_mac_add_reject(adapter);
> +			ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
> +		}

[Severity: High]

Can this rollback destroy filters that belong to a different, still
outstanding request?

iavf_mac_add_reject() is a completion time helper that walks the whole list:

	list_for_each_entry_safe(f, ftmp, &adapter->mac_filter_list, list) {
		if (f->remove && ether_addr_equal(f->macaddr, netdev->dev_addr))
			f->remove = false;

		if (!f->add && !f->add_handled)
			f->add_handled = true;

		if (f->is_new_mac) {
			list_del(&f->list);
			kfree(f);
		}
	}

One way to reach it with ret == -EBUSY is an ADD_ETH_ADDR batch the watchdog
already sent and whose reply has not been processed yet. Those filters are
exactly in state add == false, is_new_mac == true (is_new_mac is only
cleared by iavf_mac_add_ok() on a successful reply), so they are freed here
while the PF still has the adds pending.

Newly queued unicast/multicast filters from iavf_addr_sync() are in the same
boat, since iavf_add_filter() sets:

	f->add = true;
	f->add_handled = false;
	f->is_new_mac = true;

The netdev core already considers those addresses synced, so once they are
dropped here nothing programs or retries them until a reset. Should the
rollback be scoped to the address that iavf_set_mac() itself queued?

[Severity: Medium]

Does the -EAGAIN assumption in the comment above ("the message was sent and
PF will eventually respond") hold when IAVF_FLAG_PF_COMMS_FAILED is set?
iavf_send_pf_msg() returns success without posting anything in that case:

	if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
		return 0; /* nothing to see here, move along */

iavf_add_ether_addrs() then reports 0 while current_op stays
VIRTCHNL_OP_ADD_ETH_ADDR and f->add has already been cleared, so the poll
spins for the full 2.5 s under rtnl_lock plus the netdev instance lock,
returns -EAGAIN, and no rollback runs. adapter->hw.mac.addr and the filter
bookkeeping then diverge from netdev->dev_addr, and current_op stays pending
so later virtchnl requests hit "Cannot add filters, command %d pending".

iavf_disable_vf() sets IAVF_FLAG_PF_COMMS_FAILED, clears
IAVF_FLAG_RESET_PENDING and moves the state to __IAVF_DOWN, so the guard at
the top of iavf_set_mac() still passes in that window:

	if (iavf_is_reset_in_progress(adapter) || adapter->state < __IAVF_DOWN)
		return -EBUSY;

>  		return ret;
> -
> -	if (!ret)
> -		return -EAGAIN;
> +	}
>  
>  	if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
>  		return -EACCES;

[ ... ]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index ec234cc8bd9d..e6b7e8f82c7c 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)
> + *
> + * Return: 0 on success, negative on failure
> + */
> +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;
>  	}

[Severity: Medium]

Is this -EBUSY reachable as a hard failure from the new caller?

iavf_set_mac() runs with the netdev instance lock held, and the only context
that clears current_op is iavf_virtchnl_completion() as called from
iavf_adminq_task(), which starts with netdev_lock(netdev). The watchdog does
the same. So while iavf_set_mac() holds the lock, an outstanding command's
reply cannot be consumed and current_op cannot go back to
VIRTCHNL_OP_UNKNOWN.

iavf_set_mac_sync() sends once and gives up:

	ret = iavf_add_ether_addrs(adapter);
	if (ret)
		goto out;

Before this change iavf_set_mac() only set IAVF_FLAG_AQ_ADD_MAC_FILTER and
the watchdog retried the send after the previous command completed, so the
operation succeeded. Can "ip link set dev X address ..." or bond_enslave()
now fail with -EBUSY whenever the watchdog has a command in flight, for
instance the VIRTCHNL_OP_GET_STATS request it sends every couple of
watchdog ticks?

[ ... ]

> @@ -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;
>  }
>  
>  /**
> @@ -712,8 +724,8 @@ static void iavf_mac_add_ok(struct iavf_adapter *adapter)
>   * @adapter: adapter structure
>   *
>   * 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;

[Severity: Medium]

Is this helper able to roll back a filter whose is_new_mac is already
cleared?

Setting an address that was programmed successfully before leaves
is_new_mac == false, and iavf_replace_primary_mac() then does:

	old_f->is_primary = false;
	old_f->remove = true;
	...
	new_f->is_primary = true;
	new_f->add = true;

If iavf_set_mac_sync() fails locally before the batch is built (-EBUSY from
the current_op check, or -ENOMEM from the veal kzalloc), both branches in
iavf_mac_add_reject() skip that entry: f->add is still true, and
f->is_new_mac is false. So f->add and IAVF_FLAG_AQ_ADD_MAC_FILTER survive,
and the watchdog later sends ADD_ETH_ADDR for the address whose change was
just reported as failed. Since f->is_primary is still set,
iavf_set_mac_addr_type() marks it as primary:

	virtchnl_ether_addr->type = filter->is_primary ?
		VIRTCHNL_ETHER_ADDR_PRIMARY :
		VIRTCHNL_ETHER_ADDR_EXTRA;

Does the PF then end up with a primary MAC that differs from
netdev->dev_addr, with no filter marked primary for the address the
interface is actually using, since old_f->is_primary is never restored?

[ ... ]

> @@ -2950,3 +2960,72 @@ 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

[ ... ]

> + * Return: 0 on success (condition met), -EAGAIN on timeout, or error code
> + */
> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
> +				struct iavf_arq_event_info *event,
> +				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;
> +	enum virtchnl_ops received_op;
> +	unsigned long timeout;
> +	int ret = -EAGAIN;
> +	u16 pending = 0;
> +	u32 v_retval;
> +
> +	netdev_assert_locked(adapter->netdev);
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	do {
> +		if (!pending)
> +			usleep_range(50, 75);
> +
> +		if (iavf_clean_arq_element(hw, event, &pending) == IAVF_SUCCESS) {

[Severity: Medium]

The kernel-doc promises "or error code", but ret is only ever -EAGAIN or 0,
and the iavf_clean_arq_element() status is dropped here. The sibling helper
in this file does convert it:

iavf_poll_virtchnl_msg():
	status = iavf_clean_arq_element(hw, event, NULL);
	if (status != IAVF_SUCCESS)
		return iavf_status_to_errno(status);

For the IAVF_ERR_ADMIN_QUEUE_ERROR case, iavf_clean_arq_element() has
already consumed the descriptor, re-posted it and advanced IAVF_VF_ARQT1:

	flags = le16_to_cpu(desc->flags);
	if (flags & LIBIE_AQ_FLAG_ERR) {
		ret_code = IAVF_ERR_ADMIN_QUEUE_ERROR;

so the awaited reply no longer exists. Should that be reported instead of
spinning to the -EAGAIN timeout, which iavf_set_mac() reads as "still
outstanding, the PF will respond later" and therefore skips the rollback,
leaving current_op pending?

[Severity: High]

What happens to this loop if a VFR/EMPR lands while it is polling? The other
ARQ consumer checks for that explicitly:

iavf_adminq_task():
	if (iavf_is_reset_in_progress(adapter))
		goto freedom;

	/* check for error indications */
	val = rd32(hw, IAVF_VF_ARQLEN1);
	if (val == 0xdeadbeef || val == 0xffffffff) /* device in reset */
		goto freedom;

and it also clears the sticky ARQVFE/ARQOVFL/ARQCRIT bits. None of that
happens here, and iavf_clean_arq_element() takes the producer index straight
from the register without validating it:

	ntu = rd32(hw, IAVF_VF_ARQH1) & IAVF_VF_ARQH1_ARQH_MASK;
	if (ntu == ntc) {

With the reset sentinels, 0xdeadbeef & 0x3FF is 751 and 0xffffffff & 0x3FF
is 1023, both well above hw->aq.num_arq_entries and never equal to
next_to_clean, so IAVF_ERR_ADMIN_QUEUE_NO_WORK is not returned. Can this
then "clean" descriptors the hardware never produced, rewrite them, do
wr32(hw, IAVF_VF_ARQT1, ntc) on a disabled queue and advance
next_to_clean/next_to_use?

*pending would also come back as hundreds, so the throttle is skipped:

	if (!pending)
		usleep_range(50, 75);
	...
		if (pending)
			continue;

Does that busy-spin on MMIO for the remaining 2.5 s while holding the netdev
instance lock and the caller's rtnl_lock, which is what iavf_reset_task()
needs to recover? Note the loop itself can process
VIRTCHNL_EVENT_RESET_IMPENDING, which only schedules the reset and does not
stop the polling.

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

[Severity: Low]

Should this use event->buf_len rather than the hardcoded
IAVF_MAX_AQ_BUF_SIZE? The buffer is caller owned and its size is described
by event->buf_len, which iavf_clean_arq_element() honours:

	e->msg_len = min(datalen, e->buf_len);

Today's only caller allocates exactly IAVF_MAX_AQ_BUF_SIZE so nothing is
overwritten, but the helper is exported in iavf.h as generic, and this file
already has ARQ polling callers with small buffers, e.g.:

	event.msg_buf = (u8 *)&rxdids;
	event.buf_len = sizeof(rxdids);

Such a caller would get a 4096 byte memset of a few bytes of storage.

> +
> +			if (pending)
> +				continue;
> +		}
> +	} while (time_before(jiffies, timeout));
> +
> +	return ret;
> +}