From: 'Simon Horman' <[email protected]>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
iavf: refactor virtchnl polling into single function
This commit consolidates the init-time and runtime virtchnl message polling
logic into a single enhanced iavf_poll_virtchnl_msg() function.
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
[ ... ]
> +int iavf_poll_virtchnl_msg(struct iavf_hw *hw, struct iavf_arq_event_info
> *event,
> + enum virtchnl_ops op_to_poll, unsigned int
> timeout_ms,
> + bool (*condition)(struct iavf_adapter *, const void
> *),
> + const void *cond_data)
> {
[ ... ]
> + do {
> + if (timeout_ms && iavf_virtchnl_completion_done(adapter,
> condition,
> + cond_data,
> op_to_poll))
> + return 0;
>
> + status = iavf_clean_arq_element(hw, event, &pending);
> + if (status == IAVF_SUCCESS) {
> + received_op = (enum
> virtchnl_ops)le32_to_cpu(event->desc.cookie_high);
>
> + /* Handle reset events specially */
> + if (received_op == VIRTCHNL_OP_EVENT) {
> + struct virtchnl_pf_event *vpe =
> + (struct virtchnl_pf_event
> *)event->msg_buf;
>
> + if (vpe->event !=
> VIRTCHNL_EVENT_RESET_IMPENDING)
> + continue;
If received_op is VIRTCHNL_OP_EVENT but not VIRTCHNL_EVENT_RESET_IMPENDING,
the loop executes continue. Does this mean that during runtime polling (when
timeout_ms > 0), other asynchronous events like VIRTCHNL_EVENT_LINK_CHANGE
bypass iavf_virtchnl_completion() entirely and get discarded?
> +
> + dev_info(&adapter->pdev->dev,
> + "Reset indication received from the
> PF\n");
> + if (!(adapter->flags & IAVF_FLAG_RESET_PENDING))
> + iavf_schedule_reset(adapter,
> +
> IAVF_FLAG_RESET_PENDING);
> +
> + return -EIO;
> + }
> +
> + v_retval = le32_to_cpu(event->desc.cookie_low);
> +
> + if (!timeout_ms) {
> + if (received_op == op_to_poll)
> + return virtchnl_status_to_errno((enum
> virtchnl_status_code)
> + v_retval);
> + } else {
> + iavf_virtchnl_completion(adapter, received_op,
> + (enum
> iavf_status)v_retval,
> + event->msg_buf,
> event->msg_len);
> + }
> +
> + if (pending)
> + continue;
When pending is true, the loop proceeds to the next iteration before executing
the memset buffer clearing code below. Might this leave stale data in
event->msg_buf for the next iavf_clean_arq_element() call if the subsequent
message is smaller than the previous one? The original implementation zeroed
the buffer before checking pending.
> + } else if (!timeout_ms) {
> + return iavf_status_to_errno(status);
> + }
>
> + if (timeout_ms) {
> + memset(event->msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);
The size passed to memset is hardcoded to IAVF_MAX_AQ_BUF_SIZE. If a caller
ever provides a timeout but passes a smaller buffer in the event structure,
could this write out of bounds?
For example, other callers like iavf_get_vf_supported_rxdids() use an 8-byte
stack variable for event->msg_buf with event->buf_len set to 8. Would it be
safer to use event->buf_len here instead of the hardcoded macro?
> + usleep_range(50, 75);
> }
>
> + } while (!timeout_ms || time_before(jiffies, timeout));
