| Message ID | 20170417210258.6920-1-dmitry.ereminsolenikov@linaro.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Dmitry Eremin-Solenikov > Sent: Tuesday, April 18, 2017 12:03 AM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [API-NEXT v4] api: ipsec: factor out definitions for > feature support levels > > Instead of having magic 0-1-2 numbers, let's have the special enum for > feature support levels (unsupported/supported/preferred). > > Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> > --- > include/odp/api/spec/feature.h | 53 > ++++++++++++++++++++++++ > include/odp/api/spec/ipsec.h | 39 ++++++----------- > include/odp_api.h | 1 + > platform/Makefile.inc | 1 + > platform/linux-generic/Makefile.am | 1 + > platform/linux-generic/include/odp/api/feature.h | 34 +++++++++++++++ > 6 files changed, 102 insertions(+), 27 deletions(-) > create mode 100644 include/odp/api/spec/feature.h > create mode 100644 platform/linux-generic/include/odp/api/feature.h > > diff --git a/include/odp/api/spec/feature.h > b/include/odp/api/spec/feature.h > new file mode 100644 > index 00000000..7ee2ae04 > --- /dev/null > +++ b/include/odp/api/spec/feature.h > @@ -0,0 +1,53 @@ > +/* Copyright (c) 2017, Linaro Limited > + * All rights reserved. > + * > + * SPDX-License-Identifier: BSD-3-Clause > + */ > + > +/** > + * @file > + * > + * ODP feature API > + */ > + > +#ifndef ODP_API_FEATURE_H_ > +#define ODP_API_FEATURE_H_ > +#include <odp/visibility_begin.h> > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +/** @defgroup odp_feature ODP feature > + * Common API > + * @{ > + */ > + > +/** > + * ODP feature support > + */ > +typedef enum odp_feature_t { > + /** > + * Feature is not supported > + */ > + ODP_FEATURE_UNSUPPORTED, > + /** > + * Feature is supported > + */ > + ODP_FEATURE_SUPPORTED, > + /** > + * Feature is supported and preferred > + */ > + ODP_FEATURE_PREFERRED > +} odp_feature_t; I'm going to introduce odp_feature_t for list of ODP features - e.g. a bit field listing all major ODP features (timer, tm, plain queue, scheduled queued, etc). It can be used e.g. in global init phase to list which APIs / API features the application is going to use. This one is support level. No support should be zero (good default for memset 0). The relative order must be documented, so that application can simply compare between levels. /** * ODP feature support level * * Support levels are specified in the relative order, where * ODP_SUPPORT_NO is the lowest level. E.g. if the examined support * level is greater than ODP_SUPPORT_NO, the feature is supported * in some form. */ typedef enum odp_support_t { /** * Feature is not supported */ ODP_SUPPORT_NO = 0, /** * Feature is supported */ ODP_SUPPORT_YES, /** * Feature is supported and preferred */ ODP_SUPPORT_PREFERRED } odp_support_t; > > @@ -230,38 +231,22 @@ typedef struct odp_ipsec_capability_t { > /** Maximum number of IPSEC SAs */ > uint32_t max_num_sa; > > - /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) > support > - * > - * 0: Synchronous mode is not supported > - * 1: Synchronous mode is supported > - * 2: Synchronous mode is supported and preferred > - */ > - uint8_t op_mode_sync; > + /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) > support */ > + odp_feature_t op_mode_sync; Enum takes 4x the space of uint8_t. Maybe that's not a problem on these structs. -Petri
Maxim, Why this API change is now in api-next *without* by review? I did OBJECT to it yesterday. See under. Please revert it. -Petri > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Savolainen, Petri (Nokia - FI/Espoo) > Sent: Tuesday, April 18, 2017 2:46 PM > To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>; lng- > odp@lists.linaro.org > Subject: Re: [lng-odp] [API-NEXT v4] api: ipsec: factor out definitions > for feature support levels > > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Dmitry Eremin-Solenikov > > Sent: Tuesday, April 18, 2017 12:03 AM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [API-NEXT v4] api: ipsec: factor out definitions for > > feature support levels > > > > Instead of having magic 0-1-2 numbers, let's have the special enum for > > feature support levels (unsupported/supported/preferred). > > > > Signed-off-by: Dmitry Eremin-Solenikov > <dmitry.ereminsolenikov@linaro.org> > > --- > > include/odp/api/spec/feature.h | 53 > > ++++++++++++++++++++++++ > > include/odp/api/spec/ipsec.h | 39 ++++++----------- > > include/odp_api.h | 1 + > > platform/Makefile.inc | 1 + > > platform/linux-generic/Makefile.am | 1 + > > platform/linux-generic/include/odp/api/feature.h | 34 +++++++++++++++ > > 6 files changed, 102 insertions(+), 27 deletions(-) > > create mode 100644 include/odp/api/spec/feature.h > > create mode 100644 platform/linux-generic/include/odp/api/feature.h > > > > diff --git a/include/odp/api/spec/feature.h > > b/include/odp/api/spec/feature.h > > new file mode 100644 > > index 00000000..7ee2ae04 > > --- /dev/null > > +++ b/include/odp/api/spec/feature.h > > @@ -0,0 +1,53 @@ > > +/* Copyright (c) 2017, Linaro Limited > > + * All rights reserved. > > + * > > + * SPDX-License-Identifier: BSD-3-Clause > > + */ > > + > > +/** > > + * @file > > + * > > + * ODP feature API > > + */ > > + > > +#ifndef ODP_API_FEATURE_H_ > > +#define ODP_API_FEATURE_H_ > > +#include <odp/visibility_begin.h> > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +/** @defgroup odp_feature ODP feature > > + * Common API > > + * @{ > > + */ > > + > > +/** > > + * ODP feature support > > + */ > > +typedef enum odp_feature_t { > > + /** > > + * Feature is not supported > > + */ > > + ODP_FEATURE_UNSUPPORTED, > > + /** > > + * Feature is supported > > + */ > > + ODP_FEATURE_SUPPORTED, > > + /** > > + * Feature is supported and preferred > > + */ > > + ODP_FEATURE_PREFERRED > > +} odp_feature_t; > > > I'm going to introduce odp_feature_t for list of ODP features - e.g. a bit > field listing all major ODP features (timer, tm, plain queue, scheduled > queued, etc). It can be used e.g. in global init phase to list which APIs > / API features the application is going to use. > > > This one is support level. No support should be zero (good default for > memset 0). The relative order must be documented, so that application can > simply compare between levels. > > /** > * ODP feature support level > * > * Support levels are specified in the relative order, where > * ODP_SUPPORT_NO is the lowest level. E.g. if the examined support > * level is greater than ODP_SUPPORT_NO, the feature is supported > * in some form. > */ > typedef enum odp_support_t { > /** > * Feature is not supported > */ > ODP_SUPPORT_NO = 0, > > /** > * Feature is supported > */ > ODP_SUPPORT_YES, > > /** > * Feature is supported and preferred > */ > ODP_SUPPORT_PREFERRED > > } odp_support_t; > > > > > > > > @@ -230,38 +231,22 @@ typedef struct odp_ipsec_capability_t { > > /** Maximum number of IPSEC SAs */ > > uint32_t max_num_sa; > > > > - /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) > > support > > - * > > - * 0: Synchronous mode is not supported > > - * 1: Synchronous mode is supported > > - * 2: Synchronous mode is supported and preferred > > - */ > > - uint8_t op_mode_sync; > > + /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) > > support */ > > + odp_feature_t op_mode_sync; > > > Enum takes 4x the space of uint8_t. Maybe that's not a problem on these > structs. > > > -Petri >
On 19 April 2017 at 10:42, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia-bell-labs.com> wrote: > Maxim, > > Why this API change is now in api-next *without* by review? > > We discussed that on meeting and everybody said that it is good improvement and we need to apply that. > I did OBJECT to it yesterday. See under. Please revert it. > > > -Petri > > Yes, will be reverted shortly. Sorry Petri, I did not understand that it was objection. I read it as that you will have plans to do future work. If possible be more exact saying "NACK. ....". Thank you, Maxim. > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Savolainen, Petri (Nokia - FI/Espoo) > > Sent: Tuesday, April 18, 2017 2:46 PM > > To: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>; lng- > > odp@lists.linaro.org > > Subject: Re: [lng-odp] [API-NEXT v4] api: ipsec: factor out definitions > > for feature support levels > > > > > > > > > -----Original Message----- > > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > > Dmitry Eremin-Solenikov > > > Sent: Tuesday, April 18, 2017 12:03 AM > > > To: lng-odp@lists.linaro.org > > > Subject: [lng-odp] [API-NEXT v4] api: ipsec: factor out definitions for > > > feature support levels > > > > > > Instead of having magic 0-1-2 numbers, let's have the special enum for > > > feature support levels (unsupported/supported/preferred). > > > > > > Signed-off-by: Dmitry Eremin-Solenikov > > <dmitry.ereminsolenikov@linaro.org> > > > --- > > > include/odp/api/spec/feature.h | 53 > > > ++++++++++++++++++++++++ > > > include/odp/api/spec/ipsec.h | 39 > ++++++----------- > > > include/odp_api.h | 1 + > > > platform/Makefile.inc | 1 + > > > platform/linux-generic/Makefile.am | 1 + > > > platform/linux-generic/include/odp/api/feature.h | 34 +++++++++++++++ > > > 6 files changed, 102 insertions(+), 27 deletions(-) > > > create mode 100644 include/odp/api/spec/feature.h > > > create mode 100644 platform/linux-generic/include/odp/api/feature.h > > > > > > diff --git a/include/odp/api/spec/feature.h > > > b/include/odp/api/spec/feature.h > > > new file mode 100644 > > > index 00000000..7ee2ae04 > > > --- /dev/null > > > +++ b/include/odp/api/spec/feature.h > > > @@ -0,0 +1,53 @@ > > > +/* Copyright (c) 2017, Linaro Limited > > > + * All rights reserved. > > > + * > > > + * SPDX-License-Identifier: BSD-3-Clause > > > + */ > > > + > > > +/** > > > + * @file > > > + * > > > + * ODP feature API > > > + */ > > > + > > > +#ifndef ODP_API_FEATURE_H_ > > > +#define ODP_API_FEATURE_H_ > > > +#include <odp/visibility_begin.h> > > > + > > > +#ifdef __cplusplus > > > +extern "C" { > > > +#endif > > > + > > > +/** @defgroup odp_feature ODP feature > > > + * Common API > > > + * @{ > > > + */ > > > + > > > +/** > > > + * ODP feature support > > > + */ > > > +typedef enum odp_feature_t { > > > + /** > > > + * Feature is not supported > > > + */ > > > + ODP_FEATURE_UNSUPPORTED, > > > + /** > > > + * Feature is supported > > > + */ > > > + ODP_FEATURE_SUPPORTED, > > > + /** > > > + * Feature is supported and preferred > > > + */ > > > + ODP_FEATURE_PREFERRED > > > +} odp_feature_t; > > > > > > I'm going to introduce odp_feature_t for list of ODP features - e.g. a > bit > > field listing all major ODP features (timer, tm, plain queue, scheduled > > queued, etc). It can be used e.g. in global init phase to list which APIs > > / API features the application is going to use. > > > > > > This one is support level. No support should be zero (good default for > > memset 0). The relative order must be documented, so that application can > > simply compare between levels. > > > > /** > > * ODP feature support level > > * > > * Support levels are specified in the relative order, where > > * ODP_SUPPORT_NO is the lowest level. E.g. if the examined support > > * level is greater than ODP_SUPPORT_NO, the feature is supported > > * in some form. > > */ > > typedef enum odp_support_t { > > /** > > * Feature is not supported > > */ > > ODP_SUPPORT_NO = 0, > > > > /** > > * Feature is supported > > */ > > ODP_SUPPORT_YES, > > > > /** > > * Feature is supported and preferred > > */ > > ODP_SUPPORT_PREFERRED > > > > } odp_support_t; > > > > > > > > > > > > > > @@ -230,38 +231,22 @@ typedef struct odp_ipsec_capability_t { > > > /** Maximum number of IPSEC SAs */ > > > uint32_t max_num_sa; > > > > > > - /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) > > > support > > > - * > > > - * 0: Synchronous mode is not supported > > > - * 1: Synchronous mode is supported > > > - * 2: Synchronous mode is supported and preferred > > > - */ > > > - uint8_t op_mode_sync; > > > + /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) > > > support */ > > > + odp_feature_t op_mode_sync; > > > > > > Enum takes 4x the space of uint8_t. Maybe that's not a problem on these > > structs. > > > > > > -Petri > > > >
diff --git a/include/odp/api/spec/feature.h b/include/odp/api/spec/feature.h new file mode 100644 index 00000000..7ee2ae04 --- /dev/null +++ b/include/odp/api/spec/feature.h @@ -0,0 +1,53 @@ +/* Copyright (c) 2017, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP feature API + */ + +#ifndef ODP_API_FEATURE_H_ +#define ODP_API_FEATURE_H_ +#include <odp/visibility_begin.h> + +#ifdef __cplusplus +extern "C" { +#endif + +/** @defgroup odp_feature ODP feature + * Common API + * @{ + */ + +/** + * ODP feature support + */ +typedef enum odp_feature_t { + /** + * Feature is not supported + */ + ODP_FEATURE_UNSUPPORTED, + /** + * Feature is supported + */ + ODP_FEATURE_SUPPORTED, + /** + * Feature is supported and preferred + */ + ODP_FEATURE_PREFERRED +} odp_feature_t; + +/** + * @} + */ + +#ifdef __cplusplus +} +#endif + +#include <odp/visibility_end.h> +#endif diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h index a0ceb11a..e15eb590 100644 --- a/include/odp/api/spec/ipsec.h +++ b/include/odp/api/spec/ipsec.h @@ -19,6 +19,7 @@ extern "C" { #endif #include <odp/api/crypto.h> +#include <odp/api/feature.h> #include <odp/api/packet_io.h> #include <odp/api/classification.h> @@ -230,38 +231,22 @@ typedef struct odp_ipsec_capability_t { /** Maximum number of IPSEC SAs */ uint32_t max_num_sa; - /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) support - * - * 0: Synchronous mode is not supported - * 1: Synchronous mode is supported - * 2: Synchronous mode is supported and preferred - */ - uint8_t op_mode_sync; + /** Synchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_SYNC) support */ + odp_feature_t op_mode_sync; - /** Asynchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_ASYNC) support - * - * 0: Asynchronous mode is not supported - * 1: Asynchronous mode is supported - * 2: Asynchronous mode is supported and preferred + /** + * Asynchronous IPSEC operation mode (ODP_IPSEC_OP_MODE_ASYNC) support */ - uint8_t op_mode_async; + odp_feature_t op_mode_async; - /** Inline IPSEC operation mode (ODP_IPSEC_OP_MODE_INLINE) support - * - * 0: Inline IPSEC operation is not supported - * 1: Inline IPSEC operation is supported - * 2: Inline IPSEC operation is supported and preferred - */ - uint8_t op_mode_inline; + /** Inline IPSEC operation mode (ODP_IPSEC_OP_MODE_INLINE) support */ + odp_feature_t op_mode_inline; - /** Support of pipelined classification (ODP_IPSEC_PIPELINE_CLS) of - * resulting inbound packets. - * - * 0: Classification of resulting packets is not supported - * 1: Classification of resulting packets is supported - * 2: Classification of resulting packets is supported and preferred + /** + * Support of pipelined classification (ODP_IPSEC_PIPELINE_CLS) of + * resulting inbound packets */ - uint8_t pipeline_cls; + odp_feature_t pipeline_cls; /** Soft expiry limit in seconds support * diff --git a/include/odp_api.h b/include/odp_api.h index 73e5309a..b736fb88 100644 --- a/include/odp_api.h +++ b/include/odp_api.h @@ -57,6 +57,7 @@ extern "C" { #include <odp/api/spinlock_recursive.h> #include <odp/api/rwlock_recursive.h> #include <odp/api/std_clib.h> +#include <odp/api/feature.h> #include <odp/api/ipsec.h> #ifdef __cplusplus diff --git a/platform/Makefile.inc b/platform/Makefile.inc index 874cf887..5a63b24b 100644 --- a/platform/Makefile.inc +++ b/platform/Makefile.inc @@ -31,6 +31,7 @@ odpapispecinclude_HEADERS = \ $(top_srcdir)/include/odp/api/spec/debug.h \ $(top_srcdir)/include/odp/api/spec/errno.h \ $(top_srcdir)/include/odp/api/spec/event.h \ + $(top_srcdir)/include/odp/api/spec/feature.h \ $(top_srcdir)/include/odp/api/spec/hash.h \ $(top_srcdir)/include/odp/api/spec/hints.h \ $(top_srcdir)/include/odp/api/spec/init.h \ diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am index 0d5299cb..95f6369c 100644 --- a/platform/linux-generic/Makefile.am +++ b/platform/linux-generic/Makefile.am @@ -37,6 +37,7 @@ odpapiinclude_HEADERS = \ $(srcdir)/include/odp/api/debug.h \ $(srcdir)/include/odp/api/errno.h \ $(srcdir)/include/odp/api/event.h \ + $(srcdir)/include/odp/api/feature.h \ $(srcdir)/include/odp/api/hash.h \ $(srcdir)/include/odp/api/hints.h \ $(srcdir)/include/odp/api/init.h \ diff --git a/platform/linux-generic/include/odp/api/feature.h b/platform/linux-generic/include/odp/api/feature.h new file mode 100644 index 00000000..d0aa8179 --- /dev/null +++ b/platform/linux-generic/include/odp/api/feature.h @@ -0,0 +1,34 @@ +/* Copyright (c) 2017, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP feature API - platform specific header + */ + +#ifndef ODP_PLAT_FEATURE_H_ +#define ODP_PLAT_FEATURE_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +/** @ingroup odp_feature + * @{ + */ + +/** + * @} + */ + +#include <odp/api/spec/feature.h> + +#ifdef __cplusplus +} +#endif + +#endif
Instead of having magic 0-1-2 numbers, let's have the special enum for feature support levels (unsupported/supported/preferred). Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org> --- include/odp/api/spec/feature.h | 53 ++++++++++++++++++++++++ include/odp/api/spec/ipsec.h | 39 ++++++----------- include/odp_api.h | 1 + platform/Makefile.inc | 1 + platform/linux-generic/Makefile.am | 1 + platform/linux-generic/include/odp/api/feature.h | 34 +++++++++++++++ 6 files changed, 102 insertions(+), 27 deletions(-) create mode 100644 include/odp/api/spec/feature.h create mode 100644 platform/linux-generic/include/odp/api/feature.h -- 2.11.0