From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v7 6/6] devargs: parse bus info Date: Thu, 6 Jul 2017 14:37:16 +0530 Message-ID: <39810ff7-d5fd-921b-4fad-81a8013bf1c4@nxp.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: To: Gaetan Rivet Return-path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0084.outbound.protection.outlook.com [104.47.40.84]) by dpdk.org (Postfix) with ESMTP id AE8191094 for ; Thu, 6 Jul 2017 10:58:12 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Gaetan, On Wednesday 05 July 2017 05:25 AM, Gaetan Rivet wrote: > Signed-off-by: Gaetan Rivet > --- > lib/librte_eal/common/eal_common_devargs.c | 42 ++++++++++++++++++++++++----- > lib/librte_eal/common/eal_common_vdev.c | 6 +++-- > lib/librte_eal/common/include/rte_devargs.h | 3 +++ > 3 files changed, 42 insertions(+), 9 deletions(-) > > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c > index ffa8ad9..102bd8d 100644 > --- a/lib/librte_eal/common/eal_common_devargs.c > +++ b/lib/librte_eal/common/eal_common_devargs.c > @@ -78,12 +78,23 @@ rte_eal_parse_devargs_str(const char *devargs_str, > return 0; > } > > +static int > +bus_name_cmp(const struct rte_bus *bus, const void *_name) > +{ > + const char *name = _name; > + > + return strncmp(bus->name, name, > + strlen(bus->name)); Trivial: Any specific reason why this is split across multiple lines even though it is less than 80 chars in total? > +} > + > /* store a whitelist parameter for later parsing */ > int > rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > { > struct rte_devargs *devargs = NULL; > - char *buf = NULL; > + struct rte_bus *bus = NULL; > + char *dev = NULL; > + const char *devname; > int ret; > > /* use malloc instead of rte_malloc as it's called early at init */ > @@ -94,34 +105,51 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > memset(devargs, 0, sizeof(*devargs)); > devargs->type = devtype; > > - if (rte_eal_parse_devargs_str(devargs_str, &buf, &devargs->args)) > + if (rte_eal_parse_devargs_str(devargs_str, &dev, &devargs->args)) A very basic (and possibly incorrect) question: here, for example "'net_pcap0,rx_pcap=input.pcap,tx_pcap=output.pcap'" was passed to application, which would mean; dev = "net_pcap0" (name of the device) devargs = "rx_pcap=input.pcap,tx_pcap=output.pcap" > goto fail; > + devname = dev; > + do { > + bus = rte_bus_find(bus, bus_name_cmp, dev); > + if (bus == NULL) > + break; > + devname = dev + strlen(bus->name) + 1; Assuming "vdev" bus: "net_pcap0" | devname points here --------' (4+1) chars from start of "net_pcap0". Is that the expectation? Probably I am missing something here (maybe the input already has a bus name.) Or, if this is not the case, then we will have to change the test application (test_devargs.c) which passes such strings to "rte_eal_devargs_add". > + if (rte_bus_find_by_device_name(devname) == bus) Obviously, if my assumption is correct, this fails. Or, maybe I am completely off the mark. > + break; > + devname = dev; > + } while (1); > + if (bus == NULL) { > + bus = rte_bus_find_by_device_name(devname); > + if (bus == NULL) { > + fprintf(stderr, "ERROR: failed to parse bus info from device declaration\n"); > + goto fail; > + } > + } > + devargs->bus = bus; > switch (devargs->type) { > case RTE_DEVTYPE_WHITELISTED_PCI: > case RTE_DEVTYPE_BLACKLISTED_PCI: > /* try to parse pci identifier */ > - if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 && > - eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) > + if (bus->parse(devname, &devargs->pci.addr) != 0) > goto fail; > > break; > case RTE_DEVTYPE_VIRTUAL: > /* save driver name */ > ret = snprintf(devargs->virt.drv_name, > - sizeof(devargs->virt.drv_name), "%s", buf); > + sizeof(devargs->virt.drv_name), "%s", devname); > if (ret < 0 || ret >= (int)sizeof(devargs->virt.drv_name)) > goto fail; > > break; > } I think all this will change in the devargs series. > > - free(buf); > + free(dev); > TAILQ_INSERT_TAIL(&devargs_list, devargs, next); > return 0; > > fail: > - free(buf); > + free(dev); > if (devargs) { > free(devargs->args); > free(devargs); > diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c > index 6ecd1b5..8fd1ef7 100644 > --- a/lib/librte_eal/common/eal_common_vdev.c > +++ b/lib/librte_eal/common/eal_common_vdev.c > @@ -177,6 +177,7 @@ alloc_devargs(const char *name, const char *args) > return NULL; > > devargs->type = RTE_DEVTYPE_VIRTUAL; > + devargs->bus = rte_bus_find_by_name("vdev"); > if (args) > devargs->args = strdup(args); > > @@ -289,12 +290,13 @@ vdev_scan(void) > { > struct rte_vdev_device *dev; > struct rte_devargs *devargs; > + struct rte_bus *vbus; > > /* for virtual devices we scan the devargs_list populated via cmdline */ > - > + vbus = rte_bus_find_by_name("vdev"); > TAILQ_FOREACH(devargs, &devargs_list, next) { > > - if (devargs->type != RTE_DEVTYPE_VIRTUAL) > + if (devargs->bus != vbus) > continue; > > dev = find_vdev(devargs->virt.drv_name); > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h > index 88120a1..1f50a24 100644 > --- a/lib/librte_eal/common/include/rte_devargs.h > +++ b/lib/librte_eal/common/include/rte_devargs.h > @@ -51,6 +51,7 @@ extern "C" { > #include > #include > #include > +#include > > /** > * Type of generic device > @@ -89,6 +90,8 @@ struct rte_devargs { > char drv_name[32]; > } virt; > }; > + /** Bus handle for the device. */ > + struct rte_bus *bus; > /** Arguments string as given by user or "" for no argument. */ > char *args; > }; > - Shreyansh