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

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

[-Jesse, he moved to another company a while ago]

> v6: Address edge cases found by AI review (Jakub Kicinski):
>      Although unlikely in practice, v6 adds robustness for corner cases:
>      - Allocation failure after message sent: allocate event buffer BEFORE
>        sending to PF (theoretical - allocation rarely fails for small buffers)
>      - Multi-batch scenario: add loop to send all batches when >200 MACs pending
>        (rare - most configurations have far fewer MACs)
>      - Timeout rollback: only rollback on send failure (ret != -EAGAIN), not on
>        timeout where PF response handler will sync state (transient inconsistency
>        during timeout is acceptable and will be resolved by response)
> v5: https://lore.kernel.org/all/20260429102426.210750-4-jtornosm@redhat.com/
> 
>   drivers/net/ethernet/intel/iavf/iavf.h        | 11 ++-
>   drivers/net/ethernet/intel/iavf/iavf_main.c   | 91 +++++++++++++----
>   .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 99 +++++++++++++++++--
>   3 files changed, 171 insertions(+), 30 deletions(-)
> 

[...]

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

[...]

> +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;
> +
> +	while (adapter->aq_required & IAVF_FLAG_AQ_ADD_MAC_FILTER) {
> +		ret = iavf_add_ether_addrs(adapter);

I believe that this change (made in v6) is wrong.
(just an observation: AI review made this series worse vs v5).

the second step onward would fail a check:
"if (adapter->current_op != VIRTCHNL_OP_UNKNOWN)" and thus return
-EBUSY

watchdog would not kick the VC/AQ queue since we hold the netdev lock
here, there is need to manually ensure forward progress by calling
iavf_poll_virtchnl_response() within the loop

I think it should be fine to stop when the "iavf_mac_change_done"
condition is met, this will simply leave the rest of the changes
for watchdog (as we do now).

> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = iavf_poll_virtchnl_response(adapter, &event,
> +					  iavf_mac_change_done, addr, 2500);
> +
> +out:
> +	kfree(event.msg_buf);
> +	return ret;
> +}