Re: [PATCH net-next v4 08/10] enic: add MBOX VF handlers for capability, register and link state

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
enic: add MBOX VF handlers for capability, register and link state

This commit implements VF-side mailbox message processing for SR-IOV V2
admin channel communication, including handlers for capability checks,
registration, and link state notifications.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index f5784624ebbdf..b5ed31450ee76 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c

[ ... ]

> @@ -280,6 +291,136 @@ static void enic_mbox_pf_process_msg(struct enic *enic,
>  			    hdr->msg_type, vf_id, err);
>  }
>  
> +static void enic_mbox_vf_handle_capability_reply(struct enic *enic,
> +						 void *payload)
> +{
> +	struct enic_mbox_vf_capability_reply_msg *reply = payload;
> +
> +	if (le16_to_cpu(reply->reply.ret_major) == 0)
> +		enic->pf_cap_version = le32_to_cpu(reply->version);
> +	complete(&enic->mbox_comp);
> +}

Does this code handle delayed replies safely? 

If a capability request times out and the VF proceeds to send a register
request, could a delayed capability reply call complete() here and prematurely
wake up the register request? 

Since there is only a single shared completion and no validation of the
currently expected reply type, the new request might wake up, evaluate unmatched
state, and fail incorrectly.

[ ... ]

> +static void enic_mbox_vf_handle_unregister_reply(struct enic *enic,
> +						 void *payload)
> +{
> +	struct enic_mbox_vf_register_reply_msg *reply = payload;

This isn't a bug, but is it intentional to use the register_reply_msg structure
here for an unregister reply? While they currently have the same layout, this
could be brittle if the structures diverge in the future.

> +
> +	if (le16_to_cpu(reply->reply.ret_major)) {
> +		netdev_warn(enic->netdev,
> +			    "MBOX: VF unregister rejected by PF: %u/%u\n",
> +			    le16_to_cpu(reply->reply.ret_major),
> +			    le16_to_cpu(reply->reply.ret_minor));
> +	} else {
> +		enic->vf_registered = false;
> +	}
> +	complete(&enic->mbox_comp);
> +}

[ ... ]

> +	case ENIC_MBOX_VF_UNREGISTER_REPLY: {
> +		size_t exp = sizeof(struct enic_mbox_vf_register_reply_msg);

Similarly here, the register_reply_msg size is being used for the unregister
reply validation check.

> +
> +		if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
> +					     payload_len, exp))
> +			return;
> +		enic_mbox_vf_handle_unregister_reply(enic, payload);
> +		break;
> +	}

[ ... ]

> +int enic_mbox_vf_unregister(struct enic *enic)
> +{
> +	int err;
> +
> +	if (!enic->vf_registered)
> +		return 0;
> +
> +	reinit_completion(&enic->mbox_comp);
> +
> +	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_UNREGISTER_REQUEST,
> +				 ENIC_MBOX_DST_PF, NULL, 0);
> +	if (err)
> +		return err;
> +
> +	err = enic_mbox_wait_reply(enic, 3000);
> +
> +	return enic->vf_registered ? -ETIMEDOUT : 0;
>  }

Is the err value from enic_mbox_wait_reply() intentionally ignored here?

If the PF successfully receives the request but explicitly rejects it,
enic_mbox_wait_reply() returns 0, but enic->vf_registered remains true. This
causes a protocol-level rejection to be conflated with a channel timeout by
returning -ETIMEDOUT.