From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8AE53A034F; Sun, 3 Oct 2021 20:05:09 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B9C324124D; Sun, 3 Oct 2021 20:05:08 +0200 (CEST) Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) by mails.dpdk.org (Postfix) with ESMTP id 99C8C40DF6 for ; Sun, 3 Oct 2021 20:05:07 +0200 (CEST) Received: by mail-lf1-f44.google.com with SMTP id y23so26906890lfj.7 for ; Sun, 03 Oct 2021 11:05:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=YCqRsjP3KaqIrIZWEW/Z6JcMGK+3/k98kICKaMSGWKo=; b=XyZq+hbXncXs0Bq3iucHdHFOxMJowqUw5LSCHnL8JVFvUMEXA/cNpj1nKN1BETrlV5 DoQcTdyPhVwCjKZ6qwArBl2Zn0QixWULROITqWzncRBvZ5ocGJXbgZIxHg6Ee4S1xngS SFoXuGJ89FzGdJvkiFw0o1AaKlRWy4YyPvXNGb9o8Kr9pYAilaRjQvLJ4YnOPWzaAxih /DQYopxzEaA/Pfr9qPi1VZhh2ZahJ4362pLcIxP6GeiidsLMxRKSIB7i+rWXNFSRRH5N nhQcgXSCxj6OeuS5jp8ZFJX/GTxM/Z7Vzeu5yJL/JOAzkZ7amVtgqwMlm6yzMIRW1SRc BOCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=YCqRsjP3KaqIrIZWEW/Z6JcMGK+3/k98kICKaMSGWKo=; b=MJnF5OsK7uFC34czUF8uyPn5ogUE7fypGc3IFrmc28oOO/uZ9mc0zG/1Uq8jwnKBbQ ZhJDdKS5R7HUHhWdc7R/H9RB6tznYRVWYQpr1u7UZ4/PVASWWoGcMxqWpJeiUoxtiY4s oxTpIQIa0GY+vNkcZ8lG9/AWLVckVckEY+Cehd0F+rFcV/EhXquDk5/VzAi57J1/7fi6 RsfAptKGBKSwhfofiKqOs551+NSkPylzr/rZPXZ8WjrY7KjNzG3032lXqDBKk37ftE99 N+qHMkSWkZ0fu2fOzPc8VBcPxtKNhV9mX5WuvodqdklVsJ0GcqxD98FnJln4SSXUkfD8 UVAw== X-Gm-Message-State: AOAM531dzXq0oxgSc88TqIvEFz3cFvc+33eHu3cHg+QC1g/j4RQYGHAw sW32F9rIkU3vC7jXVe7oWuU= X-Google-Smtp-Source: ABdhPJwX4Sj5My1+LzIw96Z5mmtAFrvsDb6/o1nUR145dhXZvV6uJzaFLpZls6v7zBSexfk9hDS9zw== X-Received: by 2002:ac2:495b:: with SMTP id o27mr10230119lfi.501.1633284306990; Sun, 03 Oct 2021 11:05:06 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id i3sm1330478lja.47.2021.10.03.11.05.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Oct 2021 11:05:06 -0700 (PDT) Date: Sun, 3 Oct 2021 21:05:05 +0300 From: Dmitry Kozlyuk To: Harman Kalra Cc: , Ray Kinsella Message-ID: <20211003210505.1c852bd4@sovereign> In-Reply-To: <20210903124102.47425-3-hkalra@marvell.com> References: <20210826145726.102081-1-hkalra@marvell.com> <20210903124102.47425-1-hkalra@marvell.com> <20210903124102.47425-3-hkalra@marvell.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get set APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" 2021-09-03 18:10 (UTC+0530), Harman Kalra: > [...] > diff --git a/lib/eal/common/eal_common_interrupts.c b/lib/eal/common/eal_common_interrupts.c > new file mode 100644 > index 0000000000..2e4fed96f0 > --- /dev/null > +++ b/lib/eal/common/eal_common_interrupts.c > @@ -0,0 +1,506 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(C) 2021 Marvell. > + */ > + > +#include > +#include > + > +#include > +#include > +#include > + > +#include > + > + > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size, > + bool from_hugepage) Since the purpose of the series is to reduce future ABI breakages, how about making the second parameter "flags" to have some spare bits? (If not removing it completely per David's suggestion.) > +{ > + struct rte_intr_handle *intr_handle; > + int i; > + > + if (from_hugepage) > + intr_handle = rte_zmalloc(NULL, > + size * sizeof(struct rte_intr_handle), > + 0); > + else > + intr_handle = calloc(1, size * sizeof(struct rte_intr_handle)); > + if (!intr_handle) { > + RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n"); > + rte_errno = ENOMEM; > + return NULL; > + } > + > + for (i = 0; i < size; i++) { > + intr_handle[i].nb_intr = RTE_MAX_RXTX_INTR_VEC_ID; > + intr_handle[i].alloc_from_hugepage = from_hugepage; > + } > + > + return intr_handle; > +} > + > +struct rte_intr_handle *rte_intr_handle_instance_index_get( > + struct rte_intr_handle *intr_handle, int index) If rte_intr_handle_instance_alloc() returns a pointer to an array, this function is useless since the user can simply manipulate a pointer. If we want to make a distinction between a single struct rte_intr_handle and a commonly allocated bunch of such (but why?), then they should be represented by distinct types. > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOMEM; Why it's sometimes ENOMEM and sometimes ENOTSUP when the handle is not allocated? > + return NULL; > + } > + > + return &intr_handle[index]; > +} > + > +int rte_intr_handle_instance_index_set(struct rte_intr_handle *intr_handle, > + const struct rte_intr_handle *src, > + int index) See above regarding the "index" parameter. If it can be removed, a better name for this function would be rte_intr_handle_copy(). > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + if (src == NULL) { > + RTE_LOG(ERR, EAL, "Source interrupt instance unallocated\n"); > + rte_errno = EINVAL; > + goto fail; > + } > + > + if (index < 0) { > + RTE_LOG(ERR, EAL, "Index cany be negative"); > + rte_errno = EINVAL; > + goto fail; > + } How about making this parameter "size_t"? > + > + intr_handle[index].fd = src->fd; > + intr_handle[index].vfio_dev_fd = src->vfio_dev_fd; > + intr_handle[index].type = src->type; > + intr_handle[index].max_intr = src->max_intr; > + intr_handle[index].nb_efd = src->nb_efd; > + intr_handle[index].efd_counter_size = src->efd_counter_size; > + > + memcpy(intr_handle[index].efds, src->efds, src->nb_intr); > + memcpy(intr_handle[index].elist, src->elist, src->nb_intr); > + > + return 0; > +fail: > + return rte_errno; Should be (-rte_errno) per documentation. Please check all functions in this file that return an "int" status. > [...] > +int rte_intr_handle_dev_fd_get(const struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + return intr_handle->vfio_dev_fd; > +fail: > + return rte_errno; > +} Returning a errno value instead of an FD is very error-prone. Probably returning (-1) is both safe and convenient? > + > +int rte_intr_handle_max_intr_set(struct rte_intr_handle *intr_handle, > + int max_intr) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + if (max_intr > intr_handle->nb_intr) { > + RTE_LOG(ERR, EAL, "Max_intr=%d greater than PLT_MAX_RXTX_INTR_VEC_ID=%d", Seems like this common/cnxk name leaked here by mistake? > + max_intr, intr_handle->nb_intr); > + rte_errno = ERANGE; > + goto fail; > + } > + > + intr_handle->max_intr = max_intr; > + > + return 0; > +fail: > + return rte_errno; > +} > + > +int rte_intr_handle_max_intr_get(const struct rte_intr_handle *intr_handle) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + return intr_handle->max_intr; > +fail: > + return rte_errno; > +} Should be negative per documentation and to avoid returning a positive value that cannot be distinguished from a successful return. Please also check other functions in this file returning an "int" result (not status).