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

Przemek Kitszel
2026-04-17T13:05:44Z

[-Jesse]

Thank you very much for working on this!
I see that we are going in good direction.
Please find some feedback inline.

> @@ -1067,26 +1107,13 @@ 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,

this was the only waiter on this waitqueue, please remove it entriely

> - 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)
> return ret;
>
> - if (!ret)
> - return -EAGAIN;
> -
> if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
> return -EACCES;
>

[..]

> +/**
> + * iavf_virtchnl_done - Check if virtchnl operation completed
> + * @adapter: board private structure
> + * @condition: optional callback for custom completion check
> + * (takes priority)
> + * @cond_data: context data for callback
> + * @v_opcode: virtchnl opcode value we're waiting for if no condition
> + * configured (typically VIRTCHNL_OP_UNKNOWN), if condition not used
> + *
> + * Checks completion status. Callback takes priority if provided. Otherwise
> + * waits for current_op to reach v_opcode (typically VIRTCHNL_OP_UNKNOWN
> + * after completion).
> + *
> + * Return: true if operation completed
> + */
> +static inline bool iavf_virtchnl_done(struct iavf_adapter *adapter,
> + bool (*condition)(struct iavf_adapter *, const void *),
> + const void *cond_data,
> + enum virtchnl_ops v_opcode)
> +{
> + if (condition)
> + return condition(adapter, cond_data);
> +
> + return adapter->current_op == v_opcode;
> +}

after seeing this and patch 5, I think that the changes to combine the
two polling functions together are too big for "a preparation for fix"
type of change - so I agree with others that this should be scoped out
off this series

that stands for iavf_virtchnl_done() too - there is no caller that wants
"some opcode" in patches 1-4

and it will be possible to just pass "wanted_opcode" as the current
param "const void *" of condition()

> +
> +/**
> + * iavf_poll_virtchnl_response - Poll admin queue for virtchnl response
> + * @adapter: board private structure
> + * @condition: optional callback to check if desired response received
> + * (takes priority)
> + * @cond_data: context data passed to condition callback
> + * @v_opcode: virtchnl opcode value to wait for if no condition configured
> + * (typically VIRTCHNL_OP_UNKNOWN), if condition, not used
> + * @timeout_ms: maximum time to wait in milliseconds
> + *
> + * Polls admin queue and processes all messages until condition returns true
> + * or timeout expires. If condition is NULL, waits for current_op to become
> + * v_opcode (typically VIRTCHNL_OP_UNKNOWN after operation completes).
> + * 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
> + */
> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
> + bool (*condition)(struct iavf_adapter *, const void *),

please add v_op from below as a param

nit: I would also name the params instead of using plain types, not sure
how easy it will be for kdoc... (so no pressure for that)

> + const void *cond_data,
> + enum virtchnl_ops v_opcode,
> + unsigned int timeout_ms)
> +{
> + struct iavf_hw *hw = &adapter->hw;
> + struct iavf_arq_event_info event;
> + enum virtchnl_ops v_op;
> + enum iavf_status v_ret;
> + unsigned long timeout;
> + u16 pending;
> + 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;
> +
> + timeout = jiffies + msecs_to_jiffies(timeout_ms);
> + do {
> + if (iavf_virtchnl_done(adapter, condition, cond_data, v_opcode)) {
> + ret = 0;
> + goto out;
> + }
> +
> + ret = iavf_clean_arq_element(hw, &event, &pending);
> + if (!ret) {
> + v_op = (enum virtchnl_ops)le32_to_cpu(event.desc.cookie_high);

comment about condition() signature:
I believe that condition() should take this @v_op

sidenote for patch5:
...@v_op instead of looking at adapter->current_op

> + v_ret = (enum iavf_status)le32_to_cpu(event.desc.cookie_low);
> +
> + iavf_virtchnl_completion(adapter, v_op, v_ret,
> + event.msg_buf, event.msg_len);
> +
> + memset(event.msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);
> +
> + if (pending)
> + continue;

please incorporate the condition() check with iavf_clean_arq_element()
response (to avoid processing all subsequent VC messages if condition()
was met already)

it's fine to pass 0 as "v_op" to condition() when there is no VC msg yet

> + }
> +
> + usleep_range(50, 75);
> + } while (time_before(jiffies, timeout));
> +
> + if (iavf_virtchnl_done(adapter, condition, cond_data, v_opcode))
> + ret = 0;
> + else
> + ret = -EAGAIN;

please change into just one call to condition(), and don't sleep between
the call and time_before() check (that will resolve my v2 concern)

> +
> +out:
> + kfree(event.msg_buf);
> + return ret;
> +}