From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Sun,  3 Oct 2021 20:05:07 +0200 (CEST)
Received: by mail-lf1-f44.google.com with SMTP id y23so26906890lfj.7
 for <dev@dpdk.org>; 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 <dmitry.kozliuk@gmail.com>
To: Harman Kalra <hkalra@marvell.com>
Cc: <dev@dpdk.org>, Ray Kinsella <mdr@ashroe.eu>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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 <stdlib.h>
> +#include <string.h>
> +
> +#include <rte_errno.h>
> +#include <rte_log.h>
> +#include <rte_malloc.h>
> +
> +#include <rte_interrupts.h>
> +
> +
> +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).