Re: [PATCH net-next v5 11/15] ice: introduce handling of virtchnl LARGE VF opcodes

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch changes the type of struct ice_q_vector::num_ring_rx
  and num_ring_tx from u8 to u16 in drivers/net/ethernet/intel/ice/ice.h…
- [Low] The kernel-doc summary of ice_vc_validate_qv_maps() in
  drivers/net/ethernet/intel/ice/virt/queues.c (line 1257) reads…

--

ice: introduce handling of virtchnl LARGE VF opcodes

This adds PF-side handlers for VIRTCHNL_OP_ENABLE_QUEUES_V2,
VIRTCHNL_OP_DISABLE_QUEUES_V2 and VIRTCHNL_OP_MAP_QUEUE_VECTOR. With these
handlers, VFs that negotiate VIRTCHNL_VF_LARGE_NUM_QPAIRS can use more than
16 queues. VIRTCHNL_OP_GET_MAX_RSS_QREGION is only allowlisted for now.

> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 89a2bdbecf609..fc2e84136e247 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -474,8 +474,8 @@ struct ice_q_vector {
>
>  	u16 v_idx;			/* index in the vsi->q_vector array. */
>  	u16 reg_idx;			/* PF relative register index */
> -	u8 num_ring_rx;			/* total number of Rx rings in vector */
> -	u8 num_ring_tx;			/* total number of Tx rings in vector */
> +	u16 num_ring_rx;		/* total number of Rx rings in vector */
> +	u16 num_ring_tx;		/* total number of Tx rings in vector */

[Severity: Low]
This isn't a bug, but could the commit message body mention this change?

struct ice_q_vector is the PF's own interrupt vector structure, and the PF
data path uses it too. The only explanation for widening num_ring_rx and
num_ring_tx is in the v5 changelog below the '---' line:

  widen num_ring_{rx,tx} of struct ice_q_vector to u16, mapping all
  256 queues of a VF to one vector wrapped them around

That text is dropped when the patch is applied.

The reason is that ice_vc_map_q_vector_msg() increments num_ring_rx and
num_ring_tx once for each mapped queue, and up to 256 queues can be mapped
to one vector. Would a sentence saying that in the commit body be worth
adding?

>  	u8 wb_on_itr:1;			/* if true, WB on ITR is enabled */
>  	/* in usecs, need to use ice_intrl_to_usecs_reg() before writing this
>  	 * value to the device
> diff --git a/drivers/net/ethernet/intel/ice/virt/queues.c b/drivers/net/ethernet/intel/ice/virt/queues.c
> index 7f2b459dbc836..f87ccf5d370bb 100644
> --- a/drivers/net/ethernet/intel/ice/virt/queues.c
> +++ b/drivers/net/ethernet/intel/ice/virt/queues.c
> @@ -1046,3 +1046,342 @@ int ice_vc_request_qs_msg(struct ice_vf *vf, u8 *msg)

[ ... ]

> +/**
> + * ice_vc_validate_qv_maps - validate parameters sent in the qs_msg structure

[Severity: Low]
This isn't a bug, but the kernel-doc summary names the wrong structure.

ice_vc_validate_qv_maps() validates a struct virtchnl_queue_vector_maps
passed as qv_maps, and it has no qs_msg parameter. The text looks copied
from ice_vc_validate_qs_v2_msg().

Should this say "validate parameters sent in the qv_maps structure"?

> + * @vf: VF the message was received from
> + * @qv_maps: contents of the message from the VF
> + * @msglen: length of the @qv_maps

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110141534.95614-1-przemyslaw.kitszel%40intel.com