Re: [PATCH v12 15/15] net/enetc4: add WRR Tx scheduler devarg for VF rings

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 21 Aug 2026 11:26:43 +0530
Gagandeep Singh <g.singh@nxp.com> wrote:

> @@ -71,7 +72,46 @@ parse_txq_prior(const char *key __rte_unused, const char *value, void *opaque)
>  
>  	str = strtok(input_str, "|");
>  	while (str != NULL && i < hw->max_tx_queues) {
> -		hw->txq_prior[i++] = (uint32_t)atoi(str);
> +		hw->txq_prior[i++] = atoi(str) & ENETC_TBMR_PRIO_MASK;
> +		str = strtok(NULL, "|");
> +	}
> +
> +	free(input_str);
> +	return 0;
> +}

The arg parsing in this driver has lots of usage of functions that are on
the naughty list like: atoi, atof, and strtok.

Suggest reworking this to use strtok_r and strtoul and strtod.

Not a fan of so many nerd knobs either. These kind of queue configurations really
need to be under ethdev but that is more work.

Something like this?

diff --git a/drivers/net/enetc/enetc4_ethdev.c b/drivers/net/enetc/enetc4_ethdev.c
index bb579a0b90..3e13542302 100644
--- a/drivers/net/enetc/enetc4_ethdev.c
+++ b/drivers/net/enetc/enetc4_ethdev.c
@@ -2,7 +2,9 @@
  * Copyright 2024-2026 NXP
  */
 
+#include <errno.h>
 #include <stdbool.h>
+#include <stdlib.h>
 #include <rte_kvargs.h>
 #include <rte_random.h>
 #include <dpaax_iova_table.h>
@@ -49,85 +51,172 @@ static uint64_t dev_tx_offloads_sup =
 
 #define ENETC4_NC_MEMORY	"nc"
 
+/* Tx ring WRR weight range; ENETC_TBMR_WRR() encodes it as (weight - 1). */
+#define ENETC4_TXQ_WRR_MIN	1
+#define ENETC4_TXQ_WRR_MAX	8
+
+static const char * const enetc4_valid_args[] = {
+	ENETC4_TXQ_PRIORITIES,
+	ENETC4_TXQ_WRR,
+	ENETC4_NC_MEMORY,
+	NULL,
+};
+
+/*
+ * Parse one unsigned decimal value, rejecting empty strings, signs, trailing
+ * garbage and values outside [min, max]. atoi() reports none of these: it
+ * returns 0 for any non-numeric string and is undefined on overflow.
+ */
 static int
-parse_txq_prior(const char *key __rte_unused, const char *value, void *opaque)
+enetc4_parse_uint(const char *str, uint32_t min, uint32_t max, uint32_t *val)
 {
-	struct rte_eth_dev *dev = (struct rte_eth_dev *)opaque;
-	struct enetc_eth_hw *hw =
-				ENETC_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	char *input_str;
-	char *str;
-	uint32_t i = 0;
+	unsigned long res;
+	char *endptr;
+
+	if (str == NULL || *str == '\0')
+		return -EINVAL;
+
+	/* strtoul() silently wraps a leading '-', so reject signs up front. */
+	if (*str == '-' || *str == '+')
+		return -EINVAL;
+
+	errno = 0;
+	res = strtoul(str, &endptr, 10);
+	if (errno != 0 || endptr == str || *endptr != '\0')
+		return -EINVAL;
+
+	if (res < min || res > max)
+		return -ERANGE;
+
+	*val = (uint32_t)res;
+	return 0;
+}
+
+/*
+ * Parse a "v0|v1|..." list into vals[], validating every field against
+ * [min, max]. Returns the number of values parsed, or a negative errno.
+ */
+static int
+enetc4_parse_uint_list(const char *key, const char *value, uint32_t min,
+		       uint32_t max, uint32_t *vals, uint32_t max_vals)
+{
+	char *input_str, *str, *saveptr;
+	uint32_t n = 0;
+	int ret;
+
+	if (value == NULL) {
+		ENETC_PMD_ERR("%s: missing value", key);
+		return -EINVAL;
+	}
 
 	input_str = strdup(value);
-	if (!input_str)
+	if (input_str == NULL)
 		return -ENOMEM;
 
-	rte_free(hw->txq_prior);
-	hw->txq_prior = rte_zmalloc(NULL, hw->max_tx_queues * sizeof(uint32_t), 0);
-	if (!hw->txq_prior) {
-		free(input_str);
-		return -ENOMEM;
+	for (str = strtok_r(input_str, "|", &saveptr); str != NULL;
+	     str = strtok_r(NULL, "|", &saveptr)) {
+		if (n == max_vals) {
+			ENETC_PMD_ERR("%s: too many values, at most %u supported",
+				      key, max_vals);
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = enetc4_parse_uint(str, min, max, &vals[n]);
+		if (ret != 0) {
+			ENETC_PMD_ERR("%s: invalid value '%s' at index %u, expected %u..%u",
+				      key, str, n, min, max);
+			goto out;
+		}
+		n++;
 	}
 
-	str = strtok(input_str, "|");
-	while (str != NULL && i < hw->max_tx_queues) {
-		hw->txq_prior[i++] = atoi(str) & ENETC_TBMR_PRIO_MASK;
-		str = strtok(NULL, "|");
+	if (n == 0) {
+		ENETC_PMD_ERR("%s: empty value list", key);
+		ret = -EINVAL;
+		goto out;
 	}
 
+	ret = n;
+out:
 	free(input_str);
+	return ret;
+}
+
+/* Parse enetc4_txq_prior="p0|p1|..." devarg; priority 0..7 per ring. */
+static int
+parse_txq_prior(const char *key, const char *value, void *opaque)
+{
+	struct rte_eth_dev *dev = opaque;
+	struct enetc_eth_hw *hw =
+				ENETC_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t *prior;
+	int ret;
+
+	prior = rte_zmalloc(NULL, hw->max_tx_queues * sizeof(uint32_t), 0);
+	if (prior == NULL)
+		return -ENOMEM;
+
+	ret = enetc4_parse_uint_list(key, value, 0, ENETC_TBMR_PRIO_MASK,
+				     prior, hw->max_tx_queues);
+	if (ret < 0) {
+		rte_free(prior);
+		return ret;
+	}
+
+	/* Only swap in the new table once the whole list is known good. */
+	rte_free(hw->txq_prior);
+	hw->txq_prior = prior;
+
 	return 0;
 }
 
 /* Parse enetc4_txq_wrr="w0|w1|..." devarg; weight 1..8 per ring. */
-static int parse_txq_wrr(const char *key __rte_unused, const char *value,
-			  void *opaque)
+static int
+parse_txq_wrr(const char *key, const char *value, void *opaque)
 {
-	struct rte_eth_dev *dev = (struct rte_eth_dev *)opaque;
+	struct rte_eth_dev *dev = opaque;
 	struct enetc_eth_hw *hw =
 			ENETC_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	char *input_str;
-	char *str;
-	uint32_t i = 0;
-	int w;
+	uint32_t *wrr;
+	int n, i;
 
-	input_str = strdup(value);
-	if (!input_str)
+	wrr = rte_zmalloc(NULL, hw->max_tx_queues * sizeof(uint32_t), 0);
+	if (wrr == NULL)
 		return -ENOMEM;
 
-	rte_free(hw->txq_wrr);
-	hw->txq_wrr = rte_zmalloc(NULL,
-			hw->max_tx_queues * sizeof(uint32_t), 0);
-	if (!hw->txq_wrr) {
-		free(input_str);
-		return -ENOMEM;
+	n = enetc4_parse_uint_list(key, value, ENETC4_TXQ_WRR_MIN,
+				   ENETC4_TXQ_WRR_MAX, wrr, hw->max_tx_queues);
+	if (n < 0) {
+		rte_free(wrr);
+		return n;
 	}
 
-	str = strtok(input_str, "|");
-	while (str != NULL && i < hw->max_tx_queues) {
-		w = atoi(str);
-		if (w < 1)
-			w = 1;
-		if (w > 8)
-			w = 8;
-		hw->txq_wrr[i++] = ENETC_TBMR_WRR(w);
-		str = strtok(NULL, "|");
-	}
+	for (i = 0; i < n; i++)
+		wrr[i] = ENETC_TBMR_WRR(wrr[i]);
+
+	/* Only swap in the new table once the whole list is known good. */
+	rte_free(hw->txq_wrr);
+	hw->txq_wrr = wrr;
 
-	free(input_str);
 	return 0;
 }
 
 static int
-parse_nc(const char *key __rte_unused, const char *value, void *extra_args)
+parse_nc(const char *key, const char *value, void *extra_args)
 {
 	struct rte_eth_dev *dev = extra_args;
 	struct enetc_eth_hw *hw =
 		ENETC_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t val;
+
+	if (enetc4_parse_uint(value, 0, 1, &val) != 0) {
+		ENETC_PMD_ERR("%s: invalid value '%s', expected 0 or 1",
+			      key, value ? value : "(null)");
+		return -EINVAL;
+	}
 
-	if (value && atoi(value) == 1)
-		hw->nc_mode = 1;
+	hw->nc_mode = val;
 
 	return 0;
 }
@@ -135,45 +224,41 @@ parse_nc(const char *key __rte_unused, const char *value, void *extra_args)
 static int
 enetc4_get_devargs(struct rte_eth_dev *dev, const char *key)
 {
+	static const struct {
+		const char *key;
+		arg_handler_t handler;
+	} handlers[] = {
+		{ ENETC4_TXQ_PRIORITIES, parse_txq_prior },
+		{ ENETC4_TXQ_WRR,	 parse_txq_wrr	 },
+		{ ENETC4_NC_MEMORY,	 parse_nc	 },
+	};
 	struct rte_devargs *devargs = dev->device->devargs;
 	struct rte_kvargs *kvlist;
+	unsigned int i;
+	int ret = 0;
 
-	if (!devargs)
+	if (devargs == NULL)
 		return 0;
 
-	kvlist = rte_kvargs_parse(devargs->args, NULL);
-	if (!kvlist)
-		return 0;
-
-	if (!rte_kvargs_count(kvlist, key)) {
-		rte_kvargs_free(kvlist);
-		return 0;
+	/* Passing the key list makes a mistyped devarg an error, not a no-op. */
+	kvlist = rte_kvargs_parse(devargs->args, enetc4_valid_args);
+	if (kvlist == NULL) {
+		ENETC_PMD_ERR("Invalid device arguments '%s'", devargs->args);
+		return -EINVAL;
 	}
 
-	if (!strcmp(key, ENETC4_TXQ_PRIORITIES)) {
-		if (rte_kvargs_process(kvlist, key,
-				       parse_txq_prior, (void *)dev) < 0) {
-			rte_kvargs_free(kvlist);
-			return 0;
-		}
-	}
-	if (!strcmp(key, ENETC4_TXQ_WRR)) {
-		if (rte_kvargs_process(kvlist, key,
-				       parse_txq_wrr, (void *)dev) < 0) {
-			rte_kvargs_free(kvlist);
-			return 0;
-		}
-	}
-	if (!strcmp(key, ENETC4_NC_MEMORY)) {
-		if (rte_kvargs_process(kvlist, key,
-				       parse_nc, (void *)dev) < 0) {
-			rte_kvargs_free(kvlist);
-			return 0;
-		}
-	}
+	if (rte_kvargs_count(kvlist, key) == 0)
+		goto out;
 
+	for (i = 0; i < RTE_DIM(handlers); i++) {
+		if (strcmp(key, handlers[i].key) != 0)
+			continue;
+		ret = rte_kvargs_process(kvlist, key, handlers[i].handler, dev);
+		break;
+	}
+out:
 	rte_kvargs_free(kvlist);
-	return 0;
+	return ret;
 }
 
 static int
@@ -1129,9 +1214,13 @@ enetc4_dev_configure(struct rte_eth_dev *dev)
 		enetc4_txbdr_wr(enetc_hw, i, ENETC_TBMR, ENETC_BMR_RESET);
 
 	hw->nc_mode = 0;
-	enetc4_get_devargs(dev, ENETC4_TXQ_PRIORITIES);
-	enetc4_get_devargs(dev, ENETC4_TXQ_WRR);
-	enetc4_get_devargs(dev, ENETC4_NC_MEMORY);
+	ret = enetc4_get_devargs(dev, ENETC4_TXQ_PRIORITIES);
+	if (ret == 0)
+		ret = enetc4_get_devargs(dev, ENETC4_TXQ_WRR);
+	if (ret == 0)
+		ret = enetc4_get_devargs(dev, ENETC4_NC_MEMORY);
+	if (ret != 0)
+		return ret;
 
 	if (dev->data->nb_rx_queues <= 1)
 		return 0;
@@ -1538,8 +1627,13 @@ enetc4_dev_init(struct rte_eth_dev *eth_dev)
 	hw->max_rx_queues = (si_cap >> 16) & ENETC_SICAPR0_BDR_MASK;
 
 	hw->nc_mode = 0;
-	enetc4_get_devargs(eth_dev, ENETC4_TXQ_PRIORITIES);
-	enetc4_get_devargs(eth_dev, ENETC4_NC_MEMORY);
+	error = enetc4_get_devargs(eth_dev, ENETC4_TXQ_PRIORITIES);
+	if (error == 0)
+		error = enetc4_get_devargs(eth_dev, ENETC4_NC_MEMORY);
+	if (error != 0) {
+		ENETC_PMD_ERR("Invalid device arguments");
+		return error;
+	}
 	if (hw->nc_mode) {
 		eth_dev->rx_pkt_burst = &enetc_recv_pkts_nc;
 		eth_dev->tx_pkt_burst = &enetc_xmit_pkts_nc;