Re: [PATCH] net/iavf: simplify mailbox message generation for mac

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Oct 01, 2024 at 11:15:07AM +0200, David Marchand wrote:
> Caught by code review.
> The mailbox message maximum size is larger than the biggest message
> to update mac addresses.
> Remove the double loop and add some build / assert checks.
> 
> This cleanup also fixes a (never hit) issue where multiple mac addresses
> would be marked as VIRTCHNL_ETHER_ADDR_PRIMARY if multiple messages had
> been effectively sent.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Thanks David. The overall approach looks good. While not familiar (yet)
with the iavf to pf protocol and messaging, some review comments inline
below.

/Bruce

>  drivers/net/iavf/iavf_vchnl.c | 86 ++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
> index 065ab3594c..e5113605ac 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -1397,62 +1397,44 @@ void
>  iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
>  {
>  	struct virtchnl_ether_addr_list *list;
> +	char buf[sizeof(*list) + sizeof(struct virtchnl_ether_addr) * IAVF_NUM_MACADDR_MAX];

The struct virtchnl_ether_addr_list variable includes a virtchnl_ether_addr
element in it, so I think we may have an off-by-one error in a number of
places here. For example, should the multiplication not be
sizeof(...) * (IAVF_NUM_MACADDR_MAX - 1).

The overall logic I think would be more sane if the 'list' element of the
list structure was made an undimensioned array rather than being size 1,
but that's a more significant change.

>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> -	struct rte_ether_addr *addr;
>  	struct iavf_cmd_info args;
> -	int len, err, i, j;
> -	int next_begin = 0;
> -	int begin = 0;
> -
> -	do {
> -		j = 0;
> -		len = sizeof(struct virtchnl_ether_addr_list);
> -		for (i = begin; i < IAVF_NUM_MACADDR_MAX; i++, next_begin++) {
> -			addr = &adapter->dev_data->mac_addrs[i];
> -			if (rte_is_zero_ether_addr(addr))
> -				continue;
> -			len += sizeof(struct virtchnl_ether_addr);
> -			if (len >= IAVF_AQ_BUF_SZ) {
> -				next_begin = i + 1;
> -				break;
> -			}
> -		}
> +	int len, i;
>  
> -		list = rte_zmalloc("iavf_del_mac_buffer", len, 0);
> -		if (!list) {
> -			PMD_DRV_LOG(ERR, "fail to allocate memory");
> -			return;
> -		}
> +	RTE_BUILD_BUG_ON(sizeof(buf) > IAVF_AQ_BUF_SZ);
> +	list = (struct virtchnl_ether_addr_list *)buf;

Minor nit - I'd tend towards putting the definition of "buf" first in the
function, so you could collapse this line into the definition of list on
the second line of the function. That allows the BUILD_BUG_ON to stand
alone and more visible.
>  
> -		for (i = begin; i < next_begin; i++) {
> -			addr = &adapter->dev_data->mac_addrs[i];
> -			if (rte_is_zero_ether_addr(addr))
> -				continue;
> -			rte_memcpy(list->list[j].addr, addr->addr_bytes,
> -				   sizeof(addr->addr_bytes));
> -			list->list[j].type = (j == 0 ?
> -					      VIRTCHNL_ETHER_ADDR_PRIMARY :
> -					      VIRTCHNL_ETHER_ADDR_EXTRA);
> -			PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
> -				    RTE_ETHER_ADDR_BYTES(addr));
> -			j++;
> -		}
> -		list->vsi_id = vf->vsi_res->vsi_id;
> -		list->num_elements = j;
> -		args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR :
> -			   VIRTCHNL_OP_DEL_ETH_ADDR;
> -		args.in_args = (uint8_t *)list;
> -		args.in_args_size = len;
> -		args.out_buffer = vf->aq_resp;
> -		args.out_size = IAVF_AQ_BUF_SZ;
> -		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
> -		if (err)
> -			PMD_DRV_LOG(ERR, "fail to execute command %s",
> -				    add ? "OP_ADD_ETHER_ADDRESS" :
> -				    "OP_DEL_ETHER_ADDRESS");
> -		rte_free(list);
> -		begin = next_begin;
> -	} while (begin < IAVF_NUM_MACADDR_MAX);
> +	len = sizeof(struct virtchnl_ether_addr_list);
> +	for (i = 0; i < IAVF_NUM_MACADDR_MAX; i++) {
> +		struct rte_ether_addr *addr;
> +
> +		addr = &adapter->dev_data->mac_addrs[i];
> +		if (rte_is_zero_ether_addr(addr))
> +			continue;
> +		len += sizeof(struct virtchnl_ether_addr);

Where this is the first address you find, you should not increment the
length since list[0] is already included in the initial structure size.

[I'm assuming here that the message is not meant to be terminated with a
special null, or zero address, in which case we need to zero the buffer at
the start].

Is it worthwhile splitting this main loop into two - one to find the first
element and set it to ADDR_PRIMARY, and then a second loop to add all the
other addresses, setting them to ADDR_EXTRA and increasing the length for
each one?

> +		assert(len <= IAVF_AQ_BUF_SZ);
> +
> +		rte_memcpy(list->list[i].addr, addr->addr_bytes,
> +			   sizeof(addr->addr_bytes));
> +		list->list[i].type = (i == 0 ?
> +				      VIRTCHNL_ETHER_ADDR_PRIMARY :
> +				      VIRTCHNL_ETHER_ADDR_EXTRA);
> +		PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
> +			    RTE_ETHER_ADDR_BYTES(addr));
> +	}
> +
> +	list->vsi_id = vf->vsi_res->vsi_id;
> +	list->num_elements = i;
> +	args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR : VIRTCHNL_OP_DEL_ETH_ADDR;
> +	args.in_args = (uint8_t *)list;

If you use "buf" here, you can avoid the cast, I suspect.

> +	args.in_args_size = len;
> +	args.out_buffer = vf->aq_resp;
> +	args.out_size = IAVF_AQ_BUF_SZ;
> +
> +	if (iavf_execute_vf_cmd_safe(adapter, &args, 0))
> +		PMD_DRV_LOG(ERR, "fail to execute command %s",
> +			    add ? "OP_ADD_ETHER_ADDRESS" : "OP_DEL_ETHER_ADDRESS");
>  }
>  
>  int
> -- 
> 2.46.2
>