From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 34107A0350; Mon, 29 Jun 2020 17:50:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E56991BFA4; Mon, 29 Jun 2020 17:50:08 +0200 (CEST) Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by dpdk.org (Postfix) with ESMTP id 04E5F1BF82 for ; Mon, 29 Jun 2020 17:50:06 +0200 (CEST) X-Originating-IP: 86.246.31.132 Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 671C6240002; Mon, 29 Jun 2020 15:50:05 +0000 (UTC) Date: Mon, 29 Jun 2020 17:49:57 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Parav Pandit Cc: ferruh.yigit@intel.com, thomas@monjalon.net, dev@dpdk.org, orika@mellanox.com, matan@mellanox.com Message-ID: <20200629154957.laaew2777qur5tux@u256.net> References: <20200610171728.89-2-parav@mellanox.com> <20200621191200.28120-1-parav@mellanox.com> <20200621191200.28120-5-parav@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200621191200.28120-5-parav@mellanox.com> Subject: Re: [dpdk-dev] [PATCH v2 4/6] bus/mlx5_pci: register a PCI driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 21/06/20 19:11 +0000, Parav Pandit wrote: > Create a mlx5 bus driver framework for invoking drivers of > multiple classes who have registered with the mlx5_pci bus > driver. > > Validate user class arguments for supported class combinations. > > Signed-off-by: Parav Pandit > --- > Changelog: > v1->v2: > - Address comments from Thomas and Gaetan > - Enhanced driver to honor RTE_PCI_DRV_PROBE_AGAIN drv_flag > - Use anonymous structure for class search and code changes around it > - Define static for class comination array > - Use RTE_DIM to find array size > - Added OOM check for strdup() > - Renamed copy variable to nstr_orig > - Returning negagive error code > - Returning directly if match entry found > - Use compat condition check > - Avoided cutting error message string > - USe uint32_t datatype instead of enum mlx5_class > - Changed logic to parse device arguments only once during probe() > - Added check to fail driver probe if multiple classes register with > DMA ops > - Renamed function to parse_class_options > --- > drivers/bus/mlx5_pci/Makefile | 2 + > drivers/bus/mlx5_pci/meson.build | 2 +- > drivers/bus/mlx5_pci/mlx5_pci_bus.c | 290 ++++++++++++++++++++++++ > drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h | 1 + > 4 files changed, 294 insertions(+), 1 deletion(-) > > diff --git a/drivers/bus/mlx5_pci/Makefile b/drivers/bus/mlx5_pci/Makefile > index 7db977ba8..e53ed8856 100644 > --- a/drivers/bus/mlx5_pci/Makefile > +++ b/drivers/bus/mlx5_pci/Makefile > @@ -13,7 +13,9 @@ CFLAGS += $(WERROR_FLAGS) > CFLAGS += -I$(RTE_SDK)/drivers/common/mlx5 > CFLAGS += -I$(BUILDDIR)/drivers/common/mlx5 > CFLAGS += -I$(RTE_SDK)/drivers/bus/pci > +CFLAGS += -D_DEFAULT_SOURCE > LDLIBS += -lrte_eal > +LDLIBS += -lrte_kvargs > LDLIBS += -lrte_common_mlx5 > LDLIBS += -lrte_pci -lrte_bus_pci > > diff --git a/drivers/bus/mlx5_pci/meson.build b/drivers/bus/mlx5_pci/meson.build > index cc4a84e23..5111baa4e 100644 > --- a/drivers/bus/mlx5_pci/meson.build > +++ b/drivers/bus/mlx5_pci/meson.build > @@ -1,6 +1,6 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2020 Mellanox Technologies Ltd > > -deps += ['pci', 'bus_pci', 'common_mlx5'] > +deps += ['pci', 'bus_pci', 'common_mlx5', 'kvargs'] > install_headers('rte_bus_mlx5_pci.h') > sources = files('mlx5_pci_bus.c') > diff --git a/drivers/bus/mlx5_pci/mlx5_pci_bus.c b/drivers/bus/mlx5_pci/mlx5_pci_bus.c > index 66db3c7b0..e8f1649a3 100644 > --- a/drivers/bus/mlx5_pci/mlx5_pci_bus.c > +++ b/drivers/bus/mlx5_pci/mlx5_pci_bus.c > @@ -3,12 +3,302 @@ > */ > > #include "rte_bus_mlx5_pci.h" > +#include > > static TAILQ_HEAD(mlx5_pci_bus_drv_head, rte_mlx5_pci_driver) drv_list = > TAILQ_HEAD_INITIALIZER(drv_list); > > +static const struct { > + const char *name; > + unsigned int dev_class; Let me quote you when you refused to follow my comment: >> Yes, I acked to changed to define, but I forgot that I use the enum here. >> So I am going to keep the enum as code reads more clear with enum. You refused to use a fixed-width integer type as per my past comments, for readability reasons, but changed the type to "unsigned int" instead. I insisted in the previous commit on uint32_t for exposed ABI (even if between internal libs). Here I accept some leeway given the compilation-unit scope of the definition. But at this point, your choice is certainly *NOT* to use a vague type instead. > +} mlx5_classes[] = { mlx5_class_names. > + { .name = "vdpa", .dev_class = MLX5_CLASS_VDPA }, > + { .name = "net", .dev_class = MLX5_CLASS_NET }, > +}; > + > +static const unsigned int mlx5_valid_class_combo[] = { > + MLX5_CLASS_NET, > + MLX5_CLASS_VDPA, > + /* New class combination should be added here */ This comment seems redundant, new class combo will be added wherever appropriate, leave it to future dev. > +}; > + > +static int class_name_to_val(const char *class_name) I think mlx5_class_from_name() is better. (with mlx5_ namespace.) > +{ > + unsigned int i; In general, size_t is the type of array iterators in C. > + > + for (i = 0; i < RTE_DIM(mlx5_classes); i++) { > + if (strcmp(class_name, mlx5_classes[i].name) == 0) > + return mlx5_classes[i].dev_class; > + > + } > + return -EINVAL; You're mixing signed int and enum mlx5_class as return type. Please find another way of signaling error that will make you keep the enum. You have a sentinel value describing explicitly an invalid class, it seems perfectly suited instead of -EINVAL. Use it instead. > +} > + > +static int > +mlx5_bus_opt_handler(__rte_unused const char *key, const char *class_names, > + void *opaque) > +{ > + int *ret = opaque; > + char *nstr_org; > + int class_val; > + char *found; > + char *nstr; > + > + *ret = 0; > + nstr = strdup(class_names); > + if (!nstr) { Please be explicit and use (nstr == NULL). > + *ret = -ENOMEM; > + return *ret; > + } > + > + nstr_org = nstr; nstr_orig is more readable. > + while (nstr) { while (nstr != NULL) { > + /* Extract each individual class name */ > + found = strsep(&nstr, ":"); > + if (!found) ditto > + continue; > + > + /* Check if its a valid class */ > + class_val = class_name_to_val(found); > + if (class_val < 0) { if (class_val == MLX5_CLASS_INVALID), with the proper API change. > + *ret = -EINVAL; > + goto err; > + } > + > + *ret |= class_val; Once again, mixing ints and enum mlx5_class. You don't *have* to set *ret on error. * Change your opaque out_arg to uint32_t, stop using variable width types for bitmaps. * Do not set it on error, use a tmp u32 for parsing and only set it once everything is ok. * rte_kvargs_process() will mask your error values anyway, so instead set rte_errno and return -1. On negative return, it will itself return -1. Check for < 0 in bus_options_valid() > + } > +err: > + free(nstr_org); > + if (*ret < 0) > + DRV_LOG(ERR, "Invalid mlx5 class options %s. Maybe typo in device class argument setting?", Find a way to give the exact source of error. If it is an invalid name, show which token failed to be parsed (meaning move your error code before nstr_orig is freed). Remove the "Maybe" formulation. By the way, Thomas' comment was correct instead of mine, you should just cut your format string after the "%s.". > + class_names); > + return *ret; > +} > + > +static int > +parse_class_options(const struct rte_devargs *devargs) > +{ > + const char *key = MLX5_CLASS_ARG_NAME; > + struct rte_kvargs *kvlist; > + int ret = 0; > + > + if (devargs == NULL) > + return 0; > + kvlist = rte_kvargs_parse(devargs->args, NULL); > + if (kvlist == NULL) > + return 0; > + if (rte_kvargs_count(kvlist, key)) > + rte_kvargs_process(kvlist, key, mlx5_bus_opt_handler, &ret); Set ret to rte_kvargs_process() return value instead, define a specific u32 for bitmap. Find a way to output the bitmap *separately* from the error code, or set MLX5_CLASS_INVALID in the bitmap before returning it as sole return value for this function. (meaning having a proper bit value for MLX5_CLASS_INVALID, if you go this way.) I already said it in previous review, I will reformulate: stop overloading your types, relying on implicit casts between correct and incorrect values, and merging your returned values and the error channel. Please be proactive into cleaning up your APIs. > + rte_kvargs_free(kvlist); > + return ret; > +} > + > void > rte_mlx5_pci_driver_register(struct rte_mlx5_pci_driver *driver) > { > TAILQ_INSERT_TAIL(&drv_list, driver, next); > } > + > +static bool > +mlx5_bus_match(const struct rte_mlx5_pci_driver *drv, > + const struct rte_pci_device *pci_dev) > +{ > + const struct rte_pci_id *id_table; > + > + for (id_table = drv->pci_driver.id_table; id_table->vendor_id != 0; > + id_table++) { > + /* check if device's ids match the class driver's ones */ > + if (id_table->vendor_id != pci_dev->id.vendor_id && > + id_table->vendor_id != PCI_ANY_ID) > + continue; > + if (id_table->device_id != pci_dev->id.device_id && > + id_table->device_id != PCI_ANY_ID) > + continue; > + if (id_table->subsystem_vendor_id != > + pci_dev->id.subsystem_vendor_id && > + id_table->subsystem_vendor_id != PCI_ANY_ID) > + continue; > + if (id_table->subsystem_device_id != > + pci_dev->id.subsystem_device_id && > + id_table->subsystem_device_id != PCI_ANY_ID) > + continue; > + if (id_table->class_id != pci_dev->id.class_id && > + id_table->class_id != RTE_CLASS_ANY_ID) > + continue; > + > + return true; > + } > + return false; > +} > + > +static int is_valid_class_combo(uint32_t user_classes) > +{ > + unsigned int i; size_t > + > + /* Verify if user specified valid supported combination */ a valid combination. > + for (i = 0; i < RTE_DIM(mlx5_valid_class_combo); i++) { > + if (mlx5_valid_class_combo[i] == user_classes) You simplified the scope of this function, which is good. However, given the more limited scope, now it becomes a boolean yes/no. You are returning (0 | false) for yes, which is not ok. reading if (is_valid_class_combo(combo)) { handle_error(combo); } is pretty awkward. While you're at it, you might want to use a proper bool instead. > + return 0; > + } > + /* Not found any valid class combination */ > + return -EINVAL; > +} > + > +static int validate_single_class_dma_ops(void) > +{ > + struct rte_mlx5_pci_driver *class; > + int dma_map_classes = 0; > + > + TAILQ_FOREACH(class, &drv_list, next) { > + if (class->pci_driver.dma_map) > + dma_map_classes++; > + } > + if (dma_map_classes > 1) { > + DRV_LOG(ERR, "Multiple classes with DMA ops is unsupported"); > + return -EINVAL; > + } > + return 0; > +} > + > +/** > + * DPDK callback to register to probe multiple PCI class devices. > + * > + * @param[in] pci_drv > + * PCI driver structure. > + * @param[in] dev > + * PCI device information. > + * > + * @return > + * 0 on success, 1 to skip this driver, a negative errno value otherwise > + * and rte_errno is set. > + */ > +static int > +mlx5_bus_pci_probe(struct rte_pci_driver *drv __rte_unused, drv is not unused, you are passing it to sub-drivers below. > + struct rte_pci_device *dev) > +{ > + struct rte_mlx5_pci_driver *class; This compilation unit targets a C compiler. I think only headers should ensure compat with C++, but this name is not great still. driver seems more appropriate anyway to designate a driver. > + uint32_t user_classes = 0; > + int ret; > + Mixing ret and user_classes as you do afterward is the result of the above API issues already outlined. I won't go over them again, please fix everything to have proper type discipline. > + ret = validate_single_class_dma_ops(); > + if (ret) > + return ret; > + > + ret = parse_class_options(dev->device.devargs); > + if (ret < 0) > + return ret; > + > + user_classes = ret; > + if (user_classes) { > + /* Validate combination here */ > + ret = is_valid_class_combo(user_classes); > + if (ret) { > + DRV_LOG(ERR, "Unsupported mlx5 classes supplied"); > + return ret; > + } > + } > + > + /* Default to net class */ > + if (user_classes == 0) > + user_classes = MLX5_CLASS_NET; > + > + TAILQ_FOREACH(class, &drv_list, next) { > + if (!mlx5_bus_match(class, dev)) > + continue; > + > + if ((class->dev_class & user_classes) == 0) > + continue; > + > + ret = -EINVAL; > + if (class->loaded) { > + /* If already loaded and class driver can handle > + * reprobe, probe such class driver again. > + */ > + if (class->pci_driver.drv_flags & RTE_PCI_DRV_PROBE_AGAIN) > + ret = class->pci_driver.probe(drv, dev); Using "drv" here instead of "class" means you are overriding the DRV_FLAG set by the sub-driver. Why not use "class" instead? dev->driver is setup by the upper layer, so will be correctly set to drv instead of class. > + } else { > + ret = class->pci_driver.probe(drv, dev); > + } You are ignoring probe() < 0 here, seems wrong. > + if (!ret) > + class->loaded = true; loaded flag is not properly set. You will set it on first successful probe, even on further errors. Instead, use a u32 to mark each properly probed classes, then set loaded outside of this loop, only if this "probed" bitmap matches exactly the "user_classes" bitmap. This means also not silently ignoring dev and class mismatch. If this is the behavior you explicitly want, then you will need to unset the mismatched class in the user_classes, so that the exact match on probed is correct. Otherwise, logging an error is more appropriate. > + } > + return 0; > +} > + > +/** > + * DPDK callback to remove one or more class devices for a PCI device. > + * > + * This function removes all class devices belong to a given PCI device. > + * > + * @param[in] pci_dev > + * Pointer to the PCI device. > + * > + * @return > + * 0 on success, the function cannot fail. > + */ > +static int > +mlx5_bus_pci_remove(struct rte_pci_device *dev) > +{ > + struct rte_mlx5_pci_driver *class; > + > + /* Remove each class driver in reverse order */ > + TAILQ_FOREACH_REVERSE(class, &drv_list, mlx5_pci_bus_drv_head, next) { > + if (class->loaded) > + class->pci_driver.remove(dev); > + } > + return 0; > +} > + > +static int > +mlx5_bus_pci_dma_map(struct rte_pci_device *dev, void *addr, > + uint64_t iova, size_t len) > +{ > + struct rte_mlx5_pci_driver *class; > + int ret = -EINVAL; > + > + TAILQ_FOREACH(class, &drv_list, next) { > + if (!class->pci_driver.dma_map) > + continue; > + > + return class->pci_driver.dma_map(dev, addr, iova, len); > + } > + return ret; > +} > + > +static int > +mlx5_bus_pci_dma_unmap(struct rte_pci_device *dev, void *addr, > + uint64_t iova, size_t len) > +{ > + struct rte_mlx5_pci_driver *class; > + int ret = -EINVAL; > + > + TAILQ_FOREACH_REVERSE(class, &drv_list, mlx5_pci_bus_drv_head, next) { > + if (!class->pci_driver.dma_unmap) > + continue; > + I see no additional logging about edge-cases that were discussed previously. You can add them to the register function. > + return class->pci_driver.dma_unmap(dev, addr, iova, len); > + } > + return ret; > +} > + > +static const struct rte_pci_id mlx5_bus_pci_id_map[] = { > + { > + .vendor_id = 0 > + } > +}; > + > +static struct rte_pci_driver mlx5_bus_driver = { > + .driver = { > + .name = "mlx5_bus_pci", > + }, > + .id_table = mlx5_bus_pci_id_map, > + .probe = mlx5_bus_pci_probe, > + .remove = mlx5_bus_pci_remove, > + .dma_map = mlx5_bus_pci_dma_map, > + .dma_unmap = mlx5_bus_pci_dma_unmap, > + .drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV | > + RTE_PCI_DRV_PROBE_AGAIN, > +}; > + > +RTE_PMD_REGISTER_PCI(mlx5_bus, mlx5_bus_driver); > +RTE_PMD_REGISTER_PCI_TABLE(mlx5_bus, mlx5_bus_pci_id_map); > diff --git a/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > index 571f7dfd6..c8cd7187b 100644 > --- a/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > +++ b/drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h > @@ -55,6 +55,7 @@ struct rte_mlx5_pci_driver { > enum mlx5_class dev_class; /**< Class of this driver */ > struct rte_pci_driver pci_driver; /**< Inherit core pci driver. */ > TAILQ_ENTRY(rte_mlx5_pci_driver) next; > + bool loaded; > }; > > /** > -- > 2.25.4 > -- Gaëtan