Re: [PATCH v6 06/15] net/xsc: initialize xsc representors

["WanRenyong" <wanry@yunsilicon.com>]

On 2025/1/21 3:06, Stephen Hemminger wrote:
> On Mon, 20 Jan 2025 19:14:44 +0800
> "WanRenyong" <wanry@yunsilicon.com> wrote:
>
>> +static int
>> +xsc_ethdev_init_representors(struct rte_eth_dev *eth_dev)
>> +{
>> +	struct xsc_ethdev_priv *priv = TO_XSC_ETHDEV_PRIV(eth_dev);
>> +	struct rte_eth_devargs eth_da = { .nb_representor_ports = 0 };
>> +	struct rte_device *dev;
>> +	struct xsc_dev *xdev;
>> +	struct xsc_repr_port *repr_port;
>> +	char name[RTE_ETH_NAME_MAX_LEN];
>> +	int i;
>> +	int ret;
>> +
>> +	PMD_INIT_FUNC_TRACE();
>> +
>> +	dev = &priv->pci_dev->device;
>> +	if (dev->devargs != NULL) {
>> +		ret = rte_eth_devargs_parse(dev->devargs->args, &eth_da, 1);
>> +		if (ret < 0) {
>> +			PMD_DRV_LOG(ERR, "Failed to parse device arguments: %s",
>> +				    dev->devargs->args);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	xdev = priv->xdev;
>> +	ret = xsc_dev_repr_ports_probe(xdev, eth_da.nb_representor_ports, RTE_MAX_ETHPORTS);
>> +	if (ret != 0) {
>> +		PMD_DRV_LOG(ERR, "Failed to probe %d xsc device representors",
>> +			    eth_da.nb_representor_ports);
>> +		return ret;
>> +	}
>> +
>> +	/* PF rep init */
>> +	repr_port = &xdev->repr_ports[xdev->num_repr_ports - 1];
>> +	ret = xsc_ethdev_init_one_representor(eth_dev, repr_port);
>> +	if (ret != 0) {
>> +		PMD_DRV_LOG(ERR, "Failed to init backing representor");
>> +		return ret;
>> +	}
>> +
>> +	/* VF rep init */
>> +	for (i = 0; i < eth_da.nb_representor_ports; i++) {
>> +		repr_port = &xdev->repr_ports[i];
>> +		snprintf(name, sizeof(name), "%s_rep_%d",
>> +			 xdev->name, repr_port->info.repr_id);
>
> If I enable format-truncation warnings then there is this message.
> The warning is suppressed by drivers/meson.build, but would like in the future
> to not disable warnings there.
>
> ../drivers/net/xsc/xsc_ethdev.c: In function ‘xsc_ethdev_init’:
> ../drivers/net/xsc/xsc_ethdev.c:787:49: warning: ‘_rep_’ directive output may be truncated writing 5 bytes into a region of size between 1 and 64 [-Wformat-truncation=]
>    787 |                 snprintf(name, sizeof(name), "%s_rep_%d",
>        |                                                 ^~~~~
> In function ‘xsc_ethdev_init_representors’,
>      inlined from ‘xsc_ethdev_init’ at ../drivers/net/xsc/xsc_ethdev.c:834:8:
> ../drivers/net/xsc/xsc_ethdev.c:787:17: note: ‘snprintf’ output between 7 and 80 bytes into a destination of size 64
>    787 |                 snprintf(name, sizeof(name), "%s_rep_%d",
>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    788 |                          xdev->name, repr_port->info.repr_id);
>        |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This is because local variable name is of size RTE_ETH_NAME_MAX_LEN which is 64
> and xdev->name is defined as RTE_ETH_NAME_MAX_LEN as well.
> The warning is correct but impossible to happen since xdev->name is only filled
> in with PCI information. A simple way to get rid of it (and save some space)
> would be to define name to be for PCI address only.
>
> Would this work?
>
> diff --git a/drivers/net/xsc/xsc_dev.h b/drivers/net/xsc/xsc_dev.h
> index d661523e13..cc5fa8aad8 100644
> --- a/drivers/net/xsc/xsc_dev.h
> +++ b/drivers/net/xsc/xsc_dev.h
> @@ -121,7 +121,7 @@ struct xsc_dev {
>          int ifindex;
>          int port_id; /* Probe dev */
>          void *dev_priv;
> -       char name[RTE_ETH_NAME_MAX_LEN];
> +       char name[PCI_PRI_STR_SIZE];
>          void *bar_addr;
>          void *jumbo_buffer_pa;
>          void *jumbo_buffer_va;

Hello Stephen Hemminger,

Thanks for your review.

Your suggestion is correct. In fact, xdev->name is initialized to the 
PCI address , it will work if size of xdev->name defined as 
PCI_PRI_STR_SIZE. I will  modify it in the next version.




-- 
Best regards,
WanRenyong