diff mbox series

[v4,4/5] bus: add device arguments name parsing API

Message ID 1618064637-16413-5-git-send-email-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series eal: enable global device syntax by default | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Xueming Li April 10, 2021, 2:23 p.m. UTC
For device probe and iterator, devargs name was key information,
parsed by rte_devargs_parse. In legacy parser, devargs name was
extracted after bus name:
  bus:name,kv_arguments,,,
Example:
  pci:83:00.0,arguments,...
  vdev:pcap0,...

To be compatible with legacy parser, this patch introduces new
bus driver API devargs_parse to parse devargs and update devargs name.
If devargs_parse not implemented by bus driver, the new syntax parser
rte_devargs_layers_parse default will resolve devargs name from bus's
"name" argument.

Different bus driver might choose different keys from arguments with
unified format. The PCI bus implementation fills the devargs name with
the "addr" argument, example:
 -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
    name: 0000:03:00.0
 -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
    name:pcap0

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 drivers/bus/pci/pci_common.c               |  1 +
 drivers/bus/pci/pci_params.c               | 47 ++++++++++++++++++++++
 drivers/bus/pci/private.h                  | 14 +++++++
 lib/librte_eal/common/eal_common_devargs.c | 31 ++++++++++++++
 lib/librte_eal/include/rte_bus.h           | 18 +++++++++
 5 files changed, 111 insertions(+)

Comments

Thomas Monjalon April 12, 2021, 9:16 p.m. UTC | #1
10/04/2021 16:23, Xueming Li:
> +	/* Resolve devarg's name. */

s/devarg's name/devargs name/

> +	if (bus && bus->devargs_parse)

Please make checks explicits with != NULL

> +		ret = bus->devargs_parse(devargs);
> +	else if (layers[0].kvlist != NULL)
> +		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
[...]
> +/**
> + * Parse device arguments, setting the device name in the devargs as a result.

It should be
"
Parse bus part of the device arguments.

The field name of the struct rte_devargs will be set.
"

> + *
> + * On error rte_errno is set.

This sentence can be below  (in @return section).

> + *
> + * @param da
> + *	Pointer to the devargs to parse.
> + *	The 'bus_str' field must be set.

Why "must"?
It should be optional, so this sentence should be removed.

> + *
> + * @return
> + *	0 on successful parsing.
> + *	-EINVAL: on parsing error.
> + *	-ENODEV: if no key matching a device argument is specified.
> + *	-E2BIG: device name is too long.
> + */
> +typedef int (*rte_bus_devargs_parse_t)(struct rte_devargs *da);

[...]
> +	rte_bus_devargs_parse_t devargs_parse; /**< Parse device arguments */

Should be "Parse bus devargs"
Xueming Li April 12, 2021, 11:37 p.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 13, 2021 5:17 AM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>
> Cc: Gaetan Rivet <gaetanr@nvidia.com>; dev@dpdk.org; Asaf Penso <asafp@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v4 4/5] bus: add device arguments name parsing API
> 
> 10/04/2021 16:23, Xueming Li:
> > +	/* Resolve devarg's name. */
> 
> s/devarg's name/devargs name/
> 
> > +	if (bus && bus->devargs_parse)
> 
> Please make checks explicits with != NULL
> 
> > +		ret = bus->devargs_parse(devargs);
> > +	else if (layers[0].kvlist != NULL)
> > +		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
> [...]
> > +/**
> > + * Parse device arguments, setting the device name in the devargs as a result.
> 
> It should be
> "
> Parse bus part of the device arguments.
> 
> The field name of the struct rte_devargs will be set.
> "
> 
> > + *
> > + * On error rte_errno is set.
> 
> This sentence can be below  (in @return section).
> 
> > + *
> > + * @param da
> > + *	Pointer to the devargs to parse.
> > + *	The 'bus_str' field must be set.
> 
> Why "must"?
> It should be optional, so this sentence should be removed.
> 
> > + *
> > + * @return
> > + *	0 on successful parsing.
> > + *	-EINVAL: on parsing error.
> > + *	-ENODEV: if no key matching a device argument is specified.
> > + *	-E2BIG: device name is too long.
> > + */
> > +typedef int (*rte_bus_devargs_parse_t)(struct rte_devargs *da);
> 
> [...]
> > +	rte_bus_devargs_parse_t devargs_parse; /**< Parse device arguments */
> 
> Should be "Parse bus devargs"

Thanks, will fix all issues in next version.
> 
>
diff mbox series

Patch

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 9b8d769287..61d3f51452 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -760,6 +760,7 @@  struct rte_pci_bus rte_pci_bus = {
 		.dev_iterate = rte_pci_dev_iterate,
 		.hot_unplug_handler = pci_hot_unplug_handler,
 		.sigbus_handler = pci_sigbus_handler,
+		.devargs_parse = rte_pci_devargs_parse,
 	},
 	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
 	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),
diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 3192e9c967..c0c282e948 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -8,6 +8,8 @@ 
 #include <rte_errno.h>
 #include <rte_kvargs.h>
 #include <rte_pci.h>
+#include <rte_devargs.h>
+#include <rte_debug.h>
 
 #include "private.h"
 
@@ -76,3 +78,48 @@  rte_pci_dev_iterate(const void *start,
 	rte_kvargs_free(kvargs);
 	return dev;
 }
+
+int
+rte_pci_devargs_parse(struct rte_devargs *da)
+{
+	struct rte_kvargs *kvargs;
+	const char *addr_str;
+	struct rte_pci_addr addr;
+	int ret;
+
+	if (da == NULL)
+		return 0;
+	RTE_ASSERT(da->bus_str != NULL);
+
+	kvargs = rte_kvargs_parse(da->bus_str, NULL);
+	if (kvargs == NULL) {
+		RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
+			da->bus_str);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
+	if (addr_str == NULL) {
+		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	ret = rte_pci_addr_parse(addr_str, &addr);
+	if (ret != 0) {
+		RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	rte_pci_device_name(&addr, da->name, sizeof(da->name));
+
+out:
+	if (kvargs != NULL)
+		rte_kvargs_free(kvargs);
+	if (ret != 0)
+		rte_errno = -ret;
+	return ret;
+}
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index f566943f5e..8bc5140e97 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -267,4 +267,18 @@  rte_pci_dev_iterate(const void *start,
 		    const char *str,
 		    const struct rte_dev_iterator *it);
 
+/*
+ * Parse device arguments and update name.
+ *
+ * @param da
+ *   device arguments to parse.
+ *
+ * @return
+ *   0 on success.
+ *   -EINVAL: kvargs string is invalid and cannot be parsed.
+ *   -ENODEV: no key matching a device ID is found in the kv list.
+ */
+int
+rte_pci_devargs_parse(struct rte_devargs *da);
+
 #endif /* _PCI_PRIVATE_H_ */
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index e40b91ea66..3a62521e05 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -19,6 +19,7 @@ 
 #include <rte_kvargs.h>
 #include <rte_log.h>
 #include <rte_tailq.h>
+#include <rte_string_fns.h>
 #include "eal_private.h"
 
 /** user device double-linked queue type definition */
@@ -40,6 +41,28 @@  devargs_layer_count(const char *s)
 	return i;
 }
 
+/* Resolve devargs name from bus arguments. */
+static int
+devargs_bus_parse_default(struct rte_devargs *devargs,
+			  struct rte_kvargs *bus_args)
+{
+	const char *name;
+
+	/* Parse devargs name from bus key-value list. */
+	name = rte_kvargs_get(bus_args, "name");
+	if (name == NULL) {
+		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+			devargs->data);
+		return 0;
+	}
+	if (rte_strscpy(devargs->name, name, sizeof(devargs->name)) < 0) {
+		RTE_LOG(ERR, EAL, "devargs name too long: %s\n",
+			devargs->data);
+		return -E2BIG;
+	}
+	return 0;
+}
+
 int
 rte_devargs_layers_parse(struct rte_devargs *devargs,
 			 const char *devstr)
@@ -118,6 +141,8 @@  rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (layers[i].kvlist == NULL)
 			continue;
 		kv = &layers[i].kvlist->pairs[0];
+		if (kv->key == NULL)
+			continue;
 		if (strcmp(kv->key, "bus") == 0) {
 			bus = rte_bus_find_by_name(kv->value);
 			if (bus == NULL) {
@@ -160,6 +185,12 @@  rte_devargs_layers_parse(struct rte_devargs *devargs,
 		}
 	}
 
+	/* Resolve devarg's name. */
+	if (bus && bus->devargs_parse)
+		ret = bus->devargs_parse(devargs);
+	else if (layers[0].kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
 		if (layers[i].kvlist)
diff --git a/lib/librte_eal/include/rte_bus.h b/lib/librte_eal/include/rte_bus.h
index 80b154fb98..0a99f3b5a3 100644
--- a/lib/librte_eal/include/rte_bus.h
+++ b/lib/librte_eal/include/rte_bus.h
@@ -210,6 +210,23 @@  typedef int (*rte_bus_hot_unplug_handler_t)(struct rte_device *dev);
  */
 typedef int (*rte_bus_sigbus_handler_t)(const void *failure_addr);
 
+/**
+ * Parse device arguments, setting the device name in the devargs as a result.
+ *
+ * On error rte_errno is set.
+ *
+ * @param da
+ *	Pointer to the devargs to parse.
+ *	The 'bus_str' field must be set.
+ *
+ * @return
+ *	0 on successful parsing.
+ *	-EINVAL: on parsing error.
+ *	-ENODEV: if no key matching a device argument is specified.
+ *	-E2BIG: device name is too long.
+ */
+typedef int (*rte_bus_devargs_parse_t)(struct rte_devargs *da);
+
 /**
  * Bus scan policies
  */
@@ -267,6 +284,7 @@  struct rte_bus {
 				/**< handle hot-unplug failure on the bus */
 	rte_bus_sigbus_handler_t sigbus_handler;
 					/**< handle sigbus error on the bus */
+	rte_bus_devargs_parse_t devargs_parse; /**< Parse device arguments */
 
 };