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 CECCDA04BB; Tue, 6 Oct 2020 14:32:42 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 328B24C93; Tue, 6 Oct 2020 14:32:41 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by dpdk.org (Postfix) with ESMTP id 15ADF2BC7 for ; Tue, 6 Oct 2020 14:32:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601987556; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=KUoxxdBd+F2p4ieeYmmb4ophwS3QL86Jllw6TnuvjqA=; b=AmM8cIQJQj67QctKTHp0hOcq3kr6FpuKMXuS/SaCeiy1w0UEtgzHOH8wAWGMkMPJE46J2+ FS3EzsERR35IGUcpM9MTGSLc5/+bYe7GicMyD+x0RWEl5m3jB+V0B3S6QNa2wLbmRkky5Y Q+eCKZ4IVXniz7dJ0+j93FInw/5o7ec= Received: from mail-vk1-f197.google.com (mail-vk1-f197.google.com [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-488-PTuR0O87MR-gBxO3anZqzA-1; Tue, 06 Oct 2020 08:32:35 -0400 X-MC-Unique: PTuR0O87MR-gBxO3anZqzA-1 Received: by mail-vk1-f197.google.com with SMTP id h196so1062085vke.8 for ; Tue, 06 Oct 2020 05:32:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KUoxxdBd+F2p4ieeYmmb4ophwS3QL86Jllw6TnuvjqA=; b=e6/K4N0FHiRYub4vXygpDmaRYNeBoha/rpnX36sUSIY8oa151rI1G7RcBVw+8yxoz1 j0vN+deU3BpX5Q6h6NSWB65PDtlF770Nhvq00hEJpFwh54mRnR2T8Ub+qYWuNs7xX/u0 MGgCUZiMPJQ0by9nZHm+LyPFAMEKjfN3y44jQ4guOPUZmptNraFMetcUS9MEtJ7Ndhb+ elMwwZGogg0WAwKqNoF1i9keBfrSLjtsZAYjA0pShFtFGC3hLLSWOB8ghZXZnlIYmTyA 5G5z6a/lX0sRK9aZgDOYh43SZnOR5d/tTPMK2OBoN32jodaZS5LFhcsyPIXfzNbWIFg7 vuvA== X-Gm-Message-State: AOAM532iW6hWWMbLW1B/ol1RoraqlWjVaNY5sojGuZsoZU9/FgkLPJnZ v2PT/rAGsU6F6kPtkfr2CaRDeo2n6OiBvgkKqM1xIoOTudnbg/m431agbZCoukOMc07g0cTqcjU dv0VY8qPT54kj175AGUM= X-Received: by 2002:a05:6102:5d:: with SMTP id k29mr2701295vsp.17.1601987554405; Tue, 06 Oct 2020 05:32:34 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzMJZ0A99FYygmhBI1IpGsAeEdSkWIvk3ifyP0Y3vmjQuKuN51H9MknwC27AI+4X+RdB6ZX+pvomYzagTyCJYQ= X-Received: by 2002:a05:6102:5d:: with SMTP id k29mr2701274vsp.17.1601987553961; Tue, 06 Oct 2020 05:32:33 -0700 (PDT) MIME-Version: 1.0 References: <20200814095942.1726-1-adamx.dybkowski@intel.com> In-Reply-To: <20200814095942.1726-1-adamx.dybkowski@intel.com> From: David Marchand Date: Tue, 6 Oct 2020 14:32:23 +0200 Message-ID: To: Adam Dybkowski Cc: dev , "Trahe, Fiona" , Akhil Goyal , Arek Kusztal , Thomas Monjalon , Ray Kinsella Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH] cryptodev: revert ABI compatibility for ChaCha20-Poly1305 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" For the title, I would suggest: "cryptodev: remove v20 ABI compatibility" You did this change using a revert, but still, we can avoid restoring coding style issues, see nits below. On Fri, Aug 14, 2020 at 12:00 PM Adam Dybkowski wrote: > > This reverts commit a0f0de06d457753c94688d551a6e8659b4d4e041 as the > rte_cryptodev_info_get function versioning was a temporary solution > to maintain ABI compatibility for ChaCha20-Poly1305 and is not > needed in 20.11. > > Fixes: a0f0de06d457 ("cryptodev: fix ABI compatibility for ChaCha20-Poly1305") > > Signed-off-by: Adam Dybkowski > Reviewed-by: Arek Kusztal > --- > lib/librte_cryptodev/meson.build | 1 - > lib/librte_cryptodev/rte_cryptodev.c | 147 +----------------- > lib/librte_cryptodev/rte_cryptodev.h | 34 +--- > .../rte_cryptodev_version.map | 6 - > 4 files changed, 6 insertions(+), 182 deletions(-) > > diff --git a/lib/librte_cryptodev/meson.build b/lib/librte_cryptodev/meson.build > index df1144058..c4c6b3b6a 100644 > --- a/lib/librte_cryptodev/meson.build > +++ b/lib/librte_cryptodev/meson.build > @@ -1,7 +1,6 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2017-2019 Intel Corporation > > -use_function_versioning = true > sources = files('rte_cryptodev.c', 'rte_cryptodev_pmd.c', 'cryptodev_trace_points.c') > headers = files('rte_cryptodev.h', > 'rte_cryptodev_pmd.h', > diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c > index 1dd795bcb..6c9a19f25 100644 > --- a/lib/librte_cryptodev/rte_cryptodev.c > +++ b/lib/librte_cryptodev/rte_cryptodev.c > @@ -36,8 +36,6 @@ > #include > #include > #include > -#include > -#include > > #include "rte_crypto.h" > #include "rte_cryptodev.h" > @@ -59,14 +57,6 @@ static struct rte_cryptodev_global cryptodev_globals = { > /* spinlock for crypto device callbacks */ > static rte_spinlock_t rte_cryptodev_cb_lock = RTE_SPINLOCK_INITIALIZER; > > -static const struct rte_cryptodev_capabilities > - cryptodev_undefined_capabilities[] = { > - RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST() > -}; > - > -static struct rte_cryptodev_capabilities > - *capability_copy[RTE_CRYPTO_MAX_DEVS]; > -static uint8_t is_capability_checked[RTE_CRYPTO_MAX_DEVS]; > Nit: remove empty line. > /** > * The user application callback description. > @@ -291,43 +281,8 @@ rte_crypto_auth_operation_strings[] = { > [RTE_CRYPTO_AUTH_OP_GENERATE] = "generate" > }; > > -const struct rte_cryptodev_symmetric_capability __vsym * > -rte_cryptodev_sym_capability_get_v20(uint8_t dev_id, > - const struct rte_cryptodev_sym_capability_idx *idx) > -{ > - const struct rte_cryptodev_capabilities *capability; > - struct rte_cryptodev_info dev_info; > - int i = 0; > - > - rte_cryptodev_info_get_v20(dev_id, &dev_info); > - > - while ((capability = &dev_info.capabilities[i++])->op != > - RTE_CRYPTO_OP_TYPE_UNDEFINED) { > - if (capability->op != RTE_CRYPTO_OP_TYPE_SYMMETRIC) > - continue; > - > - if (capability->sym.xform_type != idx->type) > - continue; > - > - if (idx->type == RTE_CRYPTO_SYM_XFORM_AUTH && > - capability->sym.auth.algo == idx->algo.auth) > - return &capability->sym; > - > - if (idx->type == RTE_CRYPTO_SYM_XFORM_CIPHER && > - capability->sym.cipher.algo == idx->algo.cipher) > - return &capability->sym; > - > - if (idx->type == RTE_CRYPTO_SYM_XFORM_AEAD && > - capability->sym.aead.algo == idx->algo.aead) > - return &capability->sym; > - } > - > - return NULL; > -} > -VERSION_SYMBOL(rte_cryptodev_sym_capability_get, _v20, 20.0); > - > -const struct rte_cryptodev_symmetric_capability __vsym * > -rte_cryptodev_sym_capability_get_v21(uint8_t dev_id, > +const struct rte_cryptodev_symmetric_capability * > +rte_cryptodev_sym_capability_get(uint8_t dev_id, > const struct rte_cryptodev_sym_capability_idx *idx) > { > const struct rte_cryptodev_capabilities *capability; > @@ -358,12 +313,8 @@ rte_cryptodev_sym_capability_get_v21(uint8_t dev_id, > } > > return NULL; > + Nit: remove unneeded extra line. > } > -MAP_STATIC_SYMBOL(const struct rte_cryptodev_symmetric_capability * > - rte_cryptodev_sym_capability_get(uint8_t dev_id, > - const struct rte_cryptodev_sym_capability_idx *idx), > - rte_cryptodev_sym_capability_get_v21); > -BIND_DEFAULT_SYMBOL(rte_cryptodev_sym_capability_get, _v21, 21); > > static int > param_range_check(uint16_t size, const struct rte_crypto_param_range *range) > @@ -1085,12 +1036,6 @@ rte_cryptodev_close(uint8_t dev_id) > retval = (*dev->dev_ops->dev_close)(dev); > rte_cryptodev_trace_close(dev_id, retval); > > - if (capability_copy[dev_id]) { > - free(capability_copy[dev_id]); > - capability_copy[dev_id] = NULL; > - } > - is_capability_checked[dev_id] = 0; > - > if (retval < 0) > return retval; > > @@ -1233,61 +1178,9 @@ rte_cryptodev_stats_reset(uint8_t dev_id) > (*dev->dev_ops->stats_reset)(dev); > } > > -static void > -get_v20_capabilities(uint8_t dev_id, struct rte_cryptodev_info *dev_info) > -{ > - const struct rte_cryptodev_capabilities *capability; > - uint8_t found_invalid_capa = 0; > - uint8_t counter = 0; > - > - for (capability = dev_info->capabilities; > - capability->op != RTE_CRYPTO_OP_TYPE_UNDEFINED; > - ++capability, ++counter) { > - if (capability->op == RTE_CRYPTO_OP_TYPE_SYMMETRIC && > - capability->sym.xform_type == > - RTE_CRYPTO_SYM_XFORM_AEAD > - && capability->sym.aead.algo >= > - RTE_CRYPTO_AEAD_CHACHA20_POLY1305) { > - found_invalid_capa = 1; > - counter--; > - } > - } > - is_capability_checked[dev_id] = 1; > - if (!found_invalid_capa) > - return; > - capability_copy[dev_id] = malloc(counter * > - sizeof(struct rte_cryptodev_capabilities)); > - if (capability_copy[dev_id] == NULL) { > - /* > - * error case - no memory to store the trimmed > - * list, so have to return an empty list > - */ > - dev_info->capabilities = > - cryptodev_undefined_capabilities; > - is_capability_checked[dev_id] = 0; > - } else { > - counter = 0; > - for (capability = dev_info->capabilities; > - capability->op != > - RTE_CRYPTO_OP_TYPE_UNDEFINED; > - capability++) { > - if (!(capability->op == > - RTE_CRYPTO_OP_TYPE_SYMMETRIC > - && capability->sym.xform_type == > - RTE_CRYPTO_SYM_XFORM_AEAD > - && capability->sym.aead.algo >= > - RTE_CRYPTO_AEAD_CHACHA20_POLY1305)) { > - capability_copy[dev_id][counter++] = > - *capability; > - } > - } > - dev_info->capabilities = > - capability_copy[dev_id]; > - } > -} Nit: remove empty line. > > -void __vsym > -rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info *dev_info) > +void > +rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info *dev_info) > { > struct rte_cryptodev *dev; > > @@ -1303,40 +1196,10 @@ rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info *dev_info) > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > (*dev->dev_ops->dev_infos_get)(dev, dev_info); > > - if (capability_copy[dev_id] == NULL) { > - if (!is_capability_checked[dev_id]) > - get_v20_capabilities(dev_id, dev_info); > - } else > - dev_info->capabilities = capability_copy[dev_id]; > - > dev_info->driver_name = dev->device->driver->name; > dev_info->device = dev->device; > } > -VERSION_SYMBOL(rte_cryptodev_info_get, _v20, 20.0); > Nit: remove empty line. > -void __vsym > -rte_cryptodev_info_get_v21(uint8_t dev_id, struct rte_cryptodev_info *dev_info) > -{ > - struct rte_cryptodev *dev; > - > - if (!rte_cryptodev_pmd_is_valid_dev(dev_id)) { > - CDEV_LOG_ERR("Invalid dev_id=%d", dev_id); > - return; > - } > - > - dev = &rte_crypto_devices[dev_id]; > - > - memset(dev_info, 0, sizeof(struct rte_cryptodev_info)); > - > - RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > - (*dev->dev_ops->dev_infos_get)(dev, dev_info); > - > - dev_info->driver_name = dev->device->driver->name; > - dev_info->device = dev->device; > -} > -MAP_STATIC_SYMBOL(void rte_cryptodev_info_get(uint8_t dev_id, > - struct rte_cryptodev_info *dev_info), rte_cryptodev_info_get_v21); > -BIND_DEFAULT_SYMBOL(rte_cryptodev_info_get, _v21, 21); > > int > rte_cryptodev_callback_register(uint8_t dev_id, > diff --git a/lib/librte_cryptodev/rte_cryptodev.h b/lib/librte_cryptodev/rte_cryptodev.h > index 7b3ebc20f..26abd0c52 100644 > --- a/lib/librte_cryptodev/rte_cryptodev.h > +++ b/lib/librte_cryptodev/rte_cryptodev.h > @@ -219,14 +219,6 @@ struct rte_cryptodev_asym_capability_idx { > * - Return NULL if the capability not exist. > */ > const struct rte_cryptodev_symmetric_capability * > -rte_cryptodev_sym_capability_get_v20(uint8_t dev_id, > - const struct rte_cryptodev_sym_capability_idx *idx); > - > -const struct rte_cryptodev_symmetric_capability * > -rte_cryptodev_sym_capability_get_v21(uint8_t dev_id, > - const struct rte_cryptodev_sym_capability_idx *idx); > - > -const struct rte_cryptodev_symmetric_capability * > rte_cryptodev_sym_capability_get(uint8_t dev_id, > const struct rte_cryptodev_sym_capability_idx *idx); > > @@ -789,33 +781,9 @@ rte_cryptodev_stats_reset(uint8_t dev_id); > * the last valid element has it's op field set to > * RTE_CRYPTO_OP_TYPE_UNDEFINED. > */ > - > -void > +extern void Nit: no need for extern. > rte_cryptodev_info_get(uint8_t dev_id, struct rte_cryptodev_info *dev_info); > > -/* An extra element RTE_CRYPTO_AEAD_CHACHA20_POLY1305 is added > - * to enum rte_crypto_aead_algorithm, also changing the value of > - * RTE_CRYPTO_AEAD_LIST_END. To maintain ABI compatibility with applications > - * which linked against earlier versions, preventing them, for example, from > - * picking up the new value and using it to index into an array sized too small > - * for it, it is necessary to have two versions of rte_cryptodev_info_get() > - * The latest version just returns directly the capabilities retrieved from > - * the device. The compatible version inspects the capabilities retrieved > - * from the device, but only returns them directly if the new value > - * is not included. If the new value is included, it allocates space > - * for a copy of the device capabilities, trims the new value from this > - * and returns this copy. It only needs to do this once per device. > - * For the corner case of a corner case when the alloc may fail, > - * an empty capability list is returned, as there is no mechanism to return > - * an error and adding such a mechanism would itself be an ABI breakage. > - * The compatible version can be removed after the next major ABI release. > - */ > - > -void > -rte_cryptodev_info_get_v20(uint8_t dev_id, struct rte_cryptodev_info *dev_info); > - > -void > -rte_cryptodev_info_get_v21(uint8_t dev_id, struct rte_cryptodev_info *dev_info); > Nit: remove empty line. > /** > * Register a callback function for specific device id. > diff --git a/lib/librte_cryptodev/rte_cryptodev_version.map b/lib/librte_cryptodev/rte_cryptodev_version.map > index 02f6dcf72..7727286ac 100644 > --- a/lib/librte_cryptodev/rte_cryptodev_version.map > +++ b/lib/librte_cryptodev/rte_cryptodev_version.map > @@ -58,12 +58,6 @@ DPDK_21 { > local: *; > }; > > -DPDK_20.0 { > - global: > - rte_cryptodev_info_get; > - rte_cryptodev_sym_capability_get; > -}; > - > EXPERIMENTAL { > global: > > -- > 2.25.1 > Thanks for working on this. Note to others watching ABI, with this, it should be the last patch about DPDK_20 ABI. -- David Marchand