| Message ID | 20200513132745.9006-2-hemant.agrawal@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NXP DPAAx: move internal symbols to INTERNAL | expand |
Hi Ray, I could not find validate_abi using libabigail.abignore. Am I missing something. Though check_abi.sh uses it. When I am checking with validate_abi.sh, I still see warnings. Regards, Hemant > -----Original Message----- > From: Hemant Agrawal <hemant.agrawal@nxp.com> > Sent: Wednesday, May 13, 2020 6:58 PM > To: dev@dpdk.org; david.marchand@redhat.com; mdr@ashroe.eu > Cc: Hemant Agrawal <hemant.agrawal@nxp.com> > Subject: [PATCH v3 01/12] common/dpaax: move internal symbols into > INTERNAL section > > This patch moves the internal symbols to INTERNAL sections so that any > change in them is not reported as ABI breakage. > > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> > --- > devtools/libabigail.abignore | 3 +++ > drivers/common/dpaax/dpaa_of.h | 15 +++++++++++++++ > drivers/common/dpaax/dpaax_iova_table.h | 4 ++++ > drivers/common/dpaax/rte_common_dpaax_version.map | 2 +- > 4 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore index > c9ee73cb3c..b1488d5549 100644 > --- a/devtools/libabigail.abignore > +++ b/devtools/libabigail.abignore > @@ -48,3 +48,6 @@ > changed_enumerators = RTE_CRYPTO_AEAD_LIST_END > [suppress_variable] > name = rte_crypto_aead_algorithm_strings > +; Ignore moving DPAAx stable functions to INTERNAL tag [suppress_file] > + file_name_regexp = ^librte_common_dpaax\. > diff --git a/drivers/common/dpaax/dpaa_of.h > b/drivers/common/dpaax/dpaa_of.h index 960b421766..38d91a1afe 100644 > --- a/drivers/common/dpaax/dpaa_of.h > +++ b/drivers/common/dpaax/dpaa_of.h > @@ -24,6 +24,7 @@ > #include <limits.h> > #include <rte_common.h> > #include <dpaa_list.h> > +#include <rte_compat.h> > > #ifndef OF_INIT_DEFAULT_PATH > #define OF_INIT_DEFAULT_PATH "/proc/device-tree" > @@ -102,6 +103,7 @@ struct dt_file { > uint64_t buf[OF_FILE_BUF_MAX >> 3]; > }; > > +__rte_internal > const struct device_node *of_find_compatible_node( > const struct device_node *from, > const char *type __rte_unused, > @@ -113,32 +115,44 @@ const struct device_node > *of_find_compatible_node( > dev_node != NULL; \ > dev_node = of_find_compatible_node(dev_node, type, > compatible)) > > +__rte_internal > const void *of_get_property(const struct device_node *from, const char > *name, > size_t *lenp) __attribute__((nonnull(2))); > +__rte_internal > bool of_device_is_available(const struct device_node *dev_node); > > + > +__rte_internal > const struct device_node *of_find_node_by_phandle(uint64_t ph); > > +__rte_internal > const struct device_node *of_get_parent(const struct device_node > *dev_node); > > +__rte_internal > const struct device_node *of_get_next_child(const struct device_node > *dev_node, > const struct device_node *prev); > > +__rte_internal > const void *of_get_mac_address(const struct device_node *np); > > #define for_each_child_node(parent, child) \ > for (child = of_get_next_child(parent, NULL); child != NULL; \ > child = of_get_next_child(parent, child)) > > + > +__rte_internal > uint32_t of_n_addr_cells(const struct device_node *dev_node); uint32_t > of_n_size_cells(const struct device_node *dev_node); > > +__rte_internal > const uint32_t *of_get_address(const struct device_node *dev_node, size_t > idx, > uint64_t *size, uint32_t *flags); > > +__rte_internal > uint64_t of_translate_address(const struct device_node *dev_node, > const uint32_t *addr) __attribute__((nonnull)); > > +__rte_internal > bool of_device_is_compatible(const struct device_node *dev_node, > const char *compatible); > > @@ -146,6 +160,7 @@ bool of_device_is_compatible(const struct > device_node *dev_node, > * subsystem that is device-tree-dependent. Eg. Qman/Bman, config layers, > etc. > * The path should usually be "/proc/device-tree". > */ > +__rte_internal > int of_init_path(const char *dt_path); > > /* of_finish() allows a controlled tear-down of the device-tree layer, eg. if a > diff --git a/drivers/common/dpaax/dpaax_iova_table.h > b/drivers/common/dpaax/dpaax_iova_table.h > index fc3b9e7a8f..230fba8ba0 100644 > --- a/drivers/common/dpaax/dpaax_iova_table.h > +++ b/drivers/common/dpaax/dpaax_iova_table.h > @@ -61,9 +61,13 @@ extern struct dpaax_iova_table *dpaax_iova_table_p; > #define DPAAX_MEM_SPLIT_MASK_OFF (DPAAX_MEM_SPLIT - 1) /**< Offset */ > > /* APIs exposed */ > +__rte_internal > int dpaax_iova_table_populate(void); > +__rte_internal > void dpaax_iova_table_depopulate(void); > +__rte_internal > int dpaax_iova_table_update(phys_addr_t paddr, void *vaddr, size_t length); > +__rte_internal > void dpaax_iova_table_dump(void); > > static inline void *dpaax_iova_table_get_va(phys_addr_t paddr) __rte_hot; > diff --git a/drivers/common/dpaax/rte_common_dpaax_version.map > b/drivers/common/dpaax/rte_common_dpaax_version.map > index f72eba761d..ad2b2b3fec 100644 > --- a/drivers/common/dpaax/rte_common_dpaax_version.map > +++ b/drivers/common/dpaax/rte_common_dpaax_version.map > @@ -1,4 +1,4 @@ > -DPDK_20.0 { > +INTERNAL { > global: > > dpaax_iova_table_depopulate; > -- > 2.17.1
Hi Hemant, So validate_abi.sh has been somewhat deprecated - it may be removed in future. To run the libabigail abi checks, you need to run the magic command. DPDK_ABI_REF_DIR=/build/dpdk/reference/ DPDK_ABI_REF_VERSION=v20.02 ./devtools/test-meson-builds.sh DPDK_ABI_REF_DIR - needs an absolute path, for reasons that are still unclear to me. DPDK_ABI_REF_VERSION - you need to use the last DPDK release. Thanks, Ray K On 13/05/2020 15:06, Hemant Agrawal (OSS) wrote: > Hi Ray, > I could not find validate_abi using libabigail.abignore. Am I missing something. > Though check_abi.sh uses it. > > When I am checking with validate_abi.sh, I still see warnings. > > Regards, > Hemant > >> -----Original Message----- >> From: Hemant Agrawal <hemant.agrawal@nxp.com> >> Sent: Wednesday, May 13, 2020 6:58 PM >> To: dev@dpdk.org; david.marchand@redhat.com; mdr@ashroe.eu >> Cc: Hemant Agrawal <hemant.agrawal@nxp.com> >> Subject: [PATCH v3 01/12] common/dpaax: move internal symbols into >> INTERNAL section >> >> This patch moves the internal symbols to INTERNAL sections so that any >> change in them is not reported as ABI breakage. >> >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> >> --- >> devtools/libabigail.abignore | 3 +++ >> drivers/common/dpaax/dpaa_of.h | 15 +++++++++++++++ >> drivers/common/dpaax/dpaax_iova_table.h | 4 ++++ >> drivers/common/dpaax/rte_common_dpaax_version.map | 2 +- >> 4 files changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore index >> c9ee73cb3c..b1488d5549 100644 >> --- a/devtools/libabigail.abignore >> +++ b/devtools/libabigail.abignore >> @@ -48,3 +48,6 @@ >> changed_enumerators = RTE_CRYPTO_AEAD_LIST_END >> [suppress_variable] >> name = rte_crypto_aead_algorithm_strings >> +; Ignore moving DPAAx stable functions to INTERNAL tag [suppress_file] >> + file_name_regexp = ^librte_common_dpaax\. >> diff --git a/drivers/common/dpaax/dpaa_of.h >> b/drivers/common/dpaax/dpaa_of.h index 960b421766..38d91a1afe 100644 >> --- a/drivers/common/dpaax/dpaa_of.h >> +++ b/drivers/common/dpaax/dpaa_of.h >> @@ -24,6 +24,7 @@ >> #include <limits.h> >> #include <rte_common.h> >> #include <dpaa_list.h> >> +#include <rte_compat.h> >> >> #ifndef OF_INIT_DEFAULT_PATH >> #define OF_INIT_DEFAULT_PATH "/proc/device-tree" >> @@ -102,6 +103,7 @@ struct dt_file { >> uint64_t buf[OF_FILE_BUF_MAX >> 3]; >> }; >> >> +__rte_internal >> const struct device_node *of_find_compatible_node( >> const struct device_node *from, >> const char *type __rte_unused, >> @@ -113,32 +115,44 @@ const struct device_node >> *of_find_compatible_node( >> dev_node != NULL; \ >> dev_node = of_find_compatible_node(dev_node, type, >> compatible)) >> >> +__rte_internal >> const void *of_get_property(const struct device_node *from, const char >> *name, >> size_t *lenp) __attribute__((nonnull(2))); >> +__rte_internal >> bool of_device_is_available(const struct device_node *dev_node); >> >> + >> +__rte_internal >> const struct device_node *of_find_node_by_phandle(uint64_t ph); >> >> +__rte_internal >> const struct device_node *of_get_parent(const struct device_node >> *dev_node); >> >> +__rte_internal >> const struct device_node *of_get_next_child(const struct device_node >> *dev_node, >> const struct device_node *prev); >> >> +__rte_internal >> const void *of_get_mac_address(const struct device_node *np); >> >> #define for_each_child_node(parent, child) \ >> for (child = of_get_next_child(parent, NULL); child != NULL; \ >> child = of_get_next_child(parent, child)) >> >> + >> +__rte_internal >> uint32_t of_n_addr_cells(const struct device_node *dev_node); uint32_t >> of_n_size_cells(const struct device_node *dev_node); >> >> +__rte_internal >> const uint32_t *of_get_address(const struct device_node *dev_node, size_t >> idx, >> uint64_t *size, uint32_t *flags); >> >> +__rte_internal >> uint64_t of_translate_address(const struct device_node *dev_node, >> const uint32_t *addr) __attribute__((nonnull)); >> >> +__rte_internal >> bool of_device_is_compatible(const struct device_node *dev_node, >> const char *compatible); >> >> @@ -146,6 +160,7 @@ bool of_device_is_compatible(const struct >> device_node *dev_node, >> * subsystem that is device-tree-dependent. Eg. Qman/Bman, config layers, >> etc. >> * The path should usually be "/proc/device-tree". >> */ >> +__rte_internal >> int of_init_path(const char *dt_path); >> >> /* of_finish() allows a controlled tear-down of the device-tree layer, eg. if a >> diff --git a/drivers/common/dpaax/dpaax_iova_table.h >> b/drivers/common/dpaax/dpaax_iova_table.h >> index fc3b9e7a8f..230fba8ba0 100644 >> --- a/drivers/common/dpaax/dpaax_iova_table.h >> +++ b/drivers/common/dpaax/dpaax_iova_table.h >> @@ -61,9 +61,13 @@ extern struct dpaax_iova_table *dpaax_iova_table_p; >> #define DPAAX_MEM_SPLIT_MASK_OFF (DPAAX_MEM_SPLIT - 1) /**< Offset */ >> >> /* APIs exposed */ >> +__rte_internal >> int dpaax_iova_table_populate(void); >> +__rte_internal >> void dpaax_iova_table_depopulate(void); >> +__rte_internal >> int dpaax_iova_table_update(phys_addr_t paddr, void *vaddr, size_t length); >> +__rte_internal >> void dpaax_iova_table_dump(void); >> >> static inline void *dpaax_iova_table_get_va(phys_addr_t paddr) __rte_hot; >> diff --git a/drivers/common/dpaax/rte_common_dpaax_version.map >> b/drivers/common/dpaax/rte_common_dpaax_version.map >> index f72eba761d..ad2b2b3fec 100644 >> --- a/drivers/common/dpaax/rte_common_dpaax_version.map >> +++ b/drivers/common/dpaax/rte_common_dpaax_version.map >> @@ -1,4 +1,4 @@ >> -DPDK_20.0 { >> +INTERNAL { >> global: >> >> dpaax_iova_table_depopulate; >> -- >> 2.17.1 >
> Hi Hemant, > > So validate_abi.sh has been somewhat deprecated - it may be removed in > future. > To run the libabigail abi checks, you need to run the magic command. > > DPDK_ABI_REF_DIR=/build/dpdk/reference/ DPDK_ABI_REF_VERSION=v20.02 > ./devtools/test-meson-builds.sh > > DPDK_ABI_REF_DIR - needs an absolute path, for reasons that are still unclear > to me. > DPDK_ABI_REF_VERSION - you need to use the last DPDK release. [Hemant] Thanks it worked for me. It is not showing any error for the changes in this patchset. Regards, Hemant > > Thanks, > > Ray K > > On 13/05/2020 15:06, Hemant Agrawal (OSS) wrote: > > Hi Ray, > > I could not find validate_abi using libabigail.abignore. Am I missing > something. > > Though check_abi.sh uses it. > > > > When I am checking with validate_abi.sh, I still see warnings. > > > > Regards, > > Hemant > > > >> -----Original Message----- > >> From: Hemant Agrawal <hemant.agrawal@nxp.com> > >> Sent: Wednesday, May 13, 2020 6:58 PM > >> To: dev@dpdk.org; david.marchand@redhat.com; mdr@ashroe.eu > >> Cc: Hemant Agrawal <hemant.agrawal@nxp.com> > >> Subject: [PATCH v3 01/12] common/dpaax: move internal symbols into > >> INTERNAL section > >> > >> This patch moves the internal symbols to INTERNAL sections so that > >> any change in them is not reported as ABI breakage. > >> > >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> > >> --- > >> devtools/libabigail.abignore | 3 +++ > >> drivers/common/dpaax/dpaa_of.h | 15 +++++++++++++++ > >> drivers/common/dpaax/dpaax_iova_table.h | 4 ++++ > >> drivers/common/dpaax/rte_common_dpaax_version.map | 2 +- > >> 4 files changed, 23 insertions(+), 1 deletion(-) > >> > >> diff --git a/devtools/libabigail.abignore > >> b/devtools/libabigail.abignore index > >> c9ee73cb3c..b1488d5549 100644 > >> --- a/devtools/libabigail.abignore > >> +++ b/devtools/libabigail.abignore > >> @@ -48,3 +48,6 @@ > >> changed_enumerators = RTE_CRYPTO_AEAD_LIST_END > >> [suppress_variable] > >> name = rte_crypto_aead_algorithm_strings > >> +; Ignore moving DPAAx stable functions to INTERNAL tag [suppress_file] > >> + file_name_regexp = ^librte_common_dpaax\. > >> diff --git a/drivers/common/dpaax/dpaa_of.h > >> b/drivers/common/dpaax/dpaa_of.h index 960b421766..38d91a1afe > 100644 > >> --- a/drivers/common/dpaax/dpaa_of.h > >> +++ b/drivers/common/dpaax/dpaa_of.h > >> @@ -24,6 +24,7 @@ > >> #include <limits.h> > >> #include <rte_common.h> > >> #include <dpaa_list.h> > >> +#include <rte_compat.h> > >> > >> #ifndef OF_INIT_DEFAULT_PATH > >> #define OF_INIT_DEFAULT_PATH "/proc/device-tree" > >> @@ -102,6 +103,7 @@ struct dt_file { > >> uint64_t buf[OF_FILE_BUF_MAX >> 3]; }; > >> > >> +__rte_internal > >> const struct device_node *of_find_compatible_node( > >> const struct device_node *from, > >> const char *type __rte_unused, > >> @@ -113,32 +115,44 @@ const struct device_node > >> *of_find_compatible_node( > >> dev_node != NULL; \ > >> dev_node = of_find_compatible_node(dev_node, type, > >> compatible)) > >> > >> +__rte_internal > >> const void *of_get_property(const struct device_node *from, const > >> char *name, > >> size_t *lenp) __attribute__((nonnull(2))); > >> +__rte_internal > >> bool of_device_is_available(const struct device_node *dev_node); > >> > >> + > >> +__rte_internal > >> const struct device_node *of_find_node_by_phandle(uint64_t ph); > >> > >> +__rte_internal > >> const struct device_node *of_get_parent(const struct device_node > >> *dev_node); > >> > >> +__rte_internal > >> const struct device_node *of_get_next_child(const struct device_node > >> *dev_node, > >> const struct device_node *prev); > >> > >> +__rte_internal > >> const void *of_get_mac_address(const struct device_node *np); > >> > >> #define for_each_child_node(parent, child) \ > >> for (child = of_get_next_child(parent, NULL); child != NULL; \ > >> child = of_get_next_child(parent, child)) > >> > >> + > >> +__rte_internal > >> uint32_t of_n_addr_cells(const struct device_node *dev_node); > >> uint32_t of_n_size_cells(const struct device_node *dev_node); > >> > >> +__rte_internal > >> const uint32_t *of_get_address(const struct device_node *dev_node, > >> size_t idx, > >> uint64_t *size, uint32_t *flags); > >> > >> +__rte_internal > >> uint64_t of_translate_address(const struct device_node *dev_node, > >> const uint32_t *addr) __attribute__((nonnull)); > >> > >> +__rte_internal > >> bool of_device_is_compatible(const struct device_node *dev_node, > >> const char *compatible); > >> > >> @@ -146,6 +160,7 @@ bool of_device_is_compatible(const struct > >> device_node *dev_node, > >> * subsystem that is device-tree-dependent. Eg. Qman/Bman, config > >> layers, etc. > >> * The path should usually be "/proc/device-tree". > >> */ > >> +__rte_internal > >> int of_init_path(const char *dt_path); > >> > >> /* of_finish() allows a controlled tear-down of the device-tree > >> layer, eg. if a diff --git a/drivers/common/dpaax/dpaax_iova_table.h > >> b/drivers/common/dpaax/dpaax_iova_table.h > >> index fc3b9e7a8f..230fba8ba0 100644 > >> --- a/drivers/common/dpaax/dpaax_iova_table.h > >> +++ b/drivers/common/dpaax/dpaax_iova_table.h > >> @@ -61,9 +61,13 @@ extern struct dpaax_iova_table > >> *dpaax_iova_table_p; #define DPAAX_MEM_SPLIT_MASK_OFF > >> (DPAAX_MEM_SPLIT - 1) /**< Offset */ > >> > >> /* APIs exposed */ > >> +__rte_internal > >> int dpaax_iova_table_populate(void); > >> +__rte_internal > >> void dpaax_iova_table_depopulate(void); > >> +__rte_internal > >> int dpaax_iova_table_update(phys_addr_t paddr, void *vaddr, size_t > >> length); > >> +__rte_internal > >> void dpaax_iova_table_dump(void); > >> > >> static inline void *dpaax_iova_table_get_va(phys_addr_t paddr) > >> __rte_hot; diff --git > >> a/drivers/common/dpaax/rte_common_dpaax_version.map > >> b/drivers/common/dpaax/rte_common_dpaax_version.map > >> index f72eba761d..ad2b2b3fec 100644 > >> --- a/drivers/common/dpaax/rte_common_dpaax_version.map > >> +++ b/drivers/common/dpaax/rte_common_dpaax_version.map > >> @@ -1,4 +1,4 @@ > >> -DPDK_20.0 { > >> +INTERNAL { > >> global: > >> > >> dpaax_iova_table_depopulate; > >> -- > >> 2.17.1 > >
On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote:
> [Hemant] Thanks it worked for me. It is not showing any error for the changes in this patchset.
I tested it also - and it didn't show an error either, which confused me. As you are dropping a whole bunch of symbols from DPDK v20, it _should_ baulk at this.
Need to dig a bit deeper on this.
Ray K
> On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote: > > [Hemant] Thanks it worked for me. It is not showing any error for the > changes in this patchset. > > I tested it also - and it didn't show an error either, which confused me. As you > are dropping a whole bunch of symbols from DPDK v20, it _should_ baulk at > this. > > Need to dig a bit deeper on this. [Hemant] I have also made change in libabigail.abignore. That should be the reason for no complains? Hemant > > Ray K > >
On 14/05/2020 12:06, Hemant Agrawal (OSS) wrote: > >> On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote: >>> [Hemant] Thanks it worked for me. It is not showing any error for the >> changes in this patchset. >> >> I tested it also - and it didn't show an error either, which confused me. As you >> are dropping a whole bunch of symbols from DPDK v20, it _should_ baulk at >> this. >> >> Need to dig a bit deeper on this. > > [Hemant] I have also made change in libabigail.abignore. That should be the reason for no complains? yes ... I discovered that since. are we 100% certain that no end user application would ever have used these symbol? [suppress_variable] name = rte_crypto_aead_algorithm_strings +; Ignore moving DPAAx stable functions to INTERNAL tag +[suppress_file] + file_name_regexp = ^librte_common_dpaax\. +[suppress_file] + file_name_regexp = ^librte_mempool_dpaa\. +[suppress_file] + file_name_regexp = ^librte_bus_fslmc\. +[suppress_file] + file_name_regexp = ^librte_bus_dpaa\. +[suppress_file] + file_name_regexp = ^librte_pmd_dpaa\. > Hemant >> >> Ray K >> >> >
On Thu, May 14, 2020 at 1:10 PM Ray Kinsella <mdr@ashroe.eu> wrote: > > > > On 14/05/2020 12:06, Hemant Agrawal (OSS) wrote: > > > >> On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote: > >>> [Hemant] Thanks it worked for me. It is not showing any error for the > >> changes in this patchset. > >> > >> I tested it also - and it didn't show an error either, which confused me. As you > >> are dropping a whole bunch of symbols from DPDK v20, it _should_ baulk at > >> this. > >> > >> Need to dig a bit deeper on this. > > > > [Hemant] I have also made change in libabigail.abignore. That should be the reason for no complains? > > yes ... I discovered that since. > are we 100% certain that no end user application would ever have used these symbol? > > [suppress_variable] > name = rte_crypto_aead_algorithm_strings > +; Ignore moving DPAAx stable functions to INTERNAL tag > +[suppress_file] > + file_name_regexp = ^librte_common_dpaax\. > +[suppress_file] > + file_name_regexp = ^librte_mempool_dpaa\. > +[suppress_file] > + file_name_regexp = ^librte_bus_fslmc\. > +[suppress_file] > + file_name_regexp = ^librte_bus_dpaa\. > +[suppress_file] > + file_name_regexp = ^librte_pmd_dpaa\. Ignoring the whole dpaa net driver is wrong as the symbol rte_pmd_dpaa_set_tx_loopback is marked stable. This is used by applications, like testpmd: app/test-pmd/cmdline.c: ret = rte_pmd_dpaa_set_tx_loopback(res->port_id, is_on); You need a more refined filter for this. Public APIs are prefixed with rte_ so maybe whitelisting ^dpaa_ variables/functions? -- David Marchand
> On 14/05/2020 12:06, Hemant Agrawal (OSS) wrote: > > > >> On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote: > >>> [Hemant] Thanks it worked for me. It is not showing any error for > >>> the > >> changes in this patchset. > >> > >> I tested it also - and it didn't show an error either, which confused > >> me. As you are dropping a whole bunch of symbols from DPDK v20, it > >> _should_ baulk at this. > >> > >> Need to dig a bit deeper on this. > > > > [Hemant] I have also made change in libabigail.abignore. That should be > the reason for no complains? > > yes ... I discovered that since. > are we 100% certain that no end user application would ever have used these > symbol? [Hemant] No, apps are not using & can not easily use them. The header files containing these symbols are also not installed. > > [suppress_variable] > name = rte_crypto_aead_algorithm_strings > +; Ignore moving DPAAx stable functions to INTERNAL tag [suppress_file] > + file_name_regexp = ^librte_common_dpaax\. > +[suppress_file] > + file_name_regexp = ^librte_mempool_dpaa\. > +[suppress_file] > + file_name_regexp = ^librte_bus_fslmc\. > +[suppress_file] > + file_name_regexp = ^librte_bus_dpaa\. > +[suppress_file] > + file_name_regexp = ^librte_pmd_dpaa\. > > > Hemant > >> > >> Ray K > >> > >> > >
> -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of David Marchand > Sent: Thursday, May 14, 2020 4:49 PM > To: Ray Kinsella <mdr@ashroe.eu> > Cc: Hemant Agrawal (OSS) <hemant.agrawal@oss.nxp.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3 01/12] common/dpaax: move internal > symbols into INTERNAL section > > On Thu, May 14, 2020 at 1:10 PM Ray Kinsella <mdr@ashroe.eu> wrote: > > > > > > > > On 14/05/2020 12:06, Hemant Agrawal (OSS) wrote: > > > > > >> On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote: > > >>> [Hemant] Thanks it worked for me. It is not showing any error for > > >>> the > > >> changes in this patchset. > > >> > > >> I tested it also - and it didn't show an error either, which > > >> confused me. As you are dropping a whole bunch of symbols from DPDK > > >> v20, it _should_ baulk at this. > > >> > > >> Need to dig a bit deeper on this. > > > > > > [Hemant] I have also made change in libabigail.abignore. That should be > the reason for no complains? > > > > yes ... I discovered that since. > > are we 100% certain that no end user application would ever have used > these symbol? > > > > [suppress_variable] > > name = rte_crypto_aead_algorithm_strings > > +; Ignore moving DPAAx stable functions to INTERNAL tag > > +[suppress_file] > > + file_name_regexp = ^librte_common_dpaax\. > > +[suppress_file] > > + file_name_regexp = ^librte_mempool_dpaa\. > > +[suppress_file] > > + file_name_regexp = ^librte_bus_fslmc\. > > +[suppress_file] > > + file_name_regexp = ^librte_bus_dpaa\. > > +[suppress_file] > > + file_name_regexp = ^librte_pmd_dpaa\. > > Ignoring the whole dpaa net driver is wrong as the symbol > rte_pmd_dpaa_set_tx_loopback is marked stable. > This is used by applications, like testpmd: > app/test-pmd/cmdline.c: ret = > rte_pmd_dpaa_set_tx_loopback(res->port_id, is_on); > > You need a more refined filter for this. > Public APIs are prefixed with rte_ so maybe whitelisting ^dpaa_ > variables/functions? > > [Hemant] Ok. I will try it. > -- > David Marchand
Hi David/Ray > > On Thu, May 14, 2020 at 1:10 PM Ray Kinsella <mdr@ashroe.eu> wrote: > > > > > > > > > > > > On 14/05/2020 12:06, Hemant Agrawal (OSS) wrote: > > > > > > > >> On 14/05/2020 10:53, Hemant Agrawal (OSS) wrote: > > > >>> [Hemant] Thanks it worked for me. It is not showing any error > > > >>> for the > > > >> changes in this patchset. > > > >> > > > >> I tested it also - and it didn't show an error either, which > > > >> confused me. As you are dropping a whole bunch of symbols from > > > >> DPDK v20, it _should_ baulk at this. > > > >> > > > >> Need to dig a bit deeper on this. > > > > > > > > [Hemant] I have also made change in libabigail.abignore. That > > > > should be > > the reason for no complains? > > > > > > yes ... I discovered that since. > > > are we 100% certain that no end user application would ever have > > > used > > these symbol? > > > > > > [suppress_variable] > > > name = rte_crypto_aead_algorithm_strings > > > +; Ignore moving DPAAx stable functions to INTERNAL tag > > > +[suppress_file] > > > + file_name_regexp = ^librte_common_dpaax\. > > > +[suppress_file] > > > + file_name_regexp = ^librte_mempool_dpaa\. > > > +[suppress_file] > > > + file_name_regexp = ^librte_bus_fslmc\. > > > +[suppress_file] > > > + file_name_regexp = ^librte_bus_dpaa\. > > > +[suppress_file] > > > + file_name_regexp = ^librte_pmd_dpaa\. > > > > Ignoring the whole dpaa net driver is wrong as the symbol > > rte_pmd_dpaa_set_tx_loopback is marked stable. > > This is used by applications, like testpmd: > > app/test-pmd/cmdline.c: ret = > > rte_pmd_dpaa_set_tx_loopback(res->port_id, is_on); > > > > You need a more refined filter for this. > > Public APIs are prefixed with rte_ so maybe whitelisting ^dpaa_ > > variables/functions? [Hemant] this is working fine for pmd_dpaa but not for pmd_dpaa2 I removed the filename_exp and introduced function based name= Now the issue is the following warning SONAME changed from 'librte_pmd_dpaa2.so.20.0' to 'librte_pmd_dpaa2.so.0.200.2' The primary reason is that now pmd_dpaa2 has no symbol left for 20.0 section. Following is not helping. [suppress_file] soname_regexp = ^librte_pmd_dpaa2 so, it seems for now, the filename_exp is the only option > > > > > [Hemant] Ok. I will try it. > > > > -- > > David Marchand
On Thu, May 14, 2020 at 2:39 PM Hemant Agrawal (OSS) <hemant.agrawal@oss.nxp.com> wrote: > > [Hemant] this is working fine for pmd_dpaa but not for pmd_dpaa2 > > I removed the filename_exp and introduced function based name= > Now the issue is the following warning > SONAME changed from 'librte_pmd_dpaa2.so.20.0' to 'librte_pmd_dpaa2.so.0.200.2' > > The primary reason is that now pmd_dpaa2 has no symbol left for 20.0 section. > Following is not helping. > [suppress_file] > soname_regexp = ^librte_pmd_dpaa2 > so, it seems for now, the filename_exp is the only option That's interesting. Because I wondered about this point when reviewing __rte_internal. For components providing only internal symbols like components providing only experimental symbols, the build framework will select a soname with .0.200.x. Here, your dpaa2 driver was seen as a stable library so far. Moving everything to internal changes this and the build framework changes the soname to non stable. You could keep an empty DPDK_20.0 block to avoid this and the soname will be kept as is. -- David Marchand
> > [Hemant] this is working fine for pmd_dpaa but not for pmd_dpaa2 > > > > I removed the filename_exp and introduced function based name= Now the > > issue is the following warning SONAME changed from > > 'librte_pmd_dpaa2.so.20.0' to 'librte_pmd_dpaa2.so.0.200.2' > > > > The primary reason is that now pmd_dpaa2 has no symbol left for 20.0 > section. > > Following is not helping. > > [suppress_file] > > soname_regexp = ^librte_pmd_dpaa2 so, it seems for now, the > > filename_exp is the only option > > That's interesting. > Because I wondered about this point when reviewing __rte_internal. > For components providing only internal symbols like components providing > only experimental symbols, the build framework will select a soname with > .0.200.x. > > Here, your dpaa2 driver was seen as a stable library so far. > Moving everything to internal changes this and the build framework changes > the soname to non stable. > > You could keep an empty DPDK_20.0 block to avoid this and the soname will > be kept as is. [Hemant] Thanks! It worked. I am sending v5 to fix it. Regards, Hemant > > > -- > David Marchand
On Thu, May 14, 2020 at 3:31 PM David Marchand <david.marchand@redhat.com> wrote: > > On Thu, May 14, 2020 at 2:39 PM Hemant Agrawal (OSS) > <hemant.agrawal@oss.nxp.com> wrote: > > > > [Hemant] this is working fine for pmd_dpaa but not for pmd_dpaa2 > > > > I removed the filename_exp and introduced function based name= > > Now the issue is the following warning > > SONAME changed from 'librte_pmd_dpaa2.so.20.0' to 'librte_pmd_dpaa2.so.0.200.2' > > > > The primary reason is that now pmd_dpaa2 has no symbol left for 20.0 section. > > Following is not helping. > > [suppress_file] > > soname_regexp = ^librte_pmd_dpaa2 > > so, it seems for now, the filename_exp is the only option > > That's interesting. > Because I wondered about this point when reviewing __rte_internal. > For components providing only internal symbols like components > providing only experimental symbols, the build framework will select a > soname with .0.200.x. > > Here, your dpaa2 driver was seen as a stable library so far. > Moving everything to internal changes this and the build framework > changes the soname to non stable. Looking at a v19.11 testpmd binary: $ readelf -d $HOME/abi/v19.11/build-gcc-shared/usr/local/bin/dpdk-testpmd |grep dpaa 0x0000000000000001 (NEEDED) Shared library: [librte_bus_dpaa.so.20.0] 0x0000000000000001 (NEEDED) Shared library: [librte_common_dpaax.so.20.0] 0x0000000000000001 (NEEDED) Shared library: [librte_mempool_dpaa.so.20.0] 0x0000000000000001 (NEEDED) Shared library: [librte_pmd_dpaa.so.20.0] Changing the soname would break this. > You could keep an empty DPDK_20.0 block to avoid this and the soname > will be kept as is. We will have to maintain such soname for all dpaa libraries until 20.11. -- David Marchand
> > On Thu, May 14, 2020 at 3:31 PM David Marchand > <david.marchand@redhat.com> wrote: > > > > On Thu, May 14, 2020 at 2:39 PM Hemant Agrawal (OSS) > > <hemant.agrawal@oss.nxp.com> wrote: > > > > > > [Hemant] this is working fine for pmd_dpaa but not for pmd_dpaa2 > > > > > > I removed the filename_exp and introduced function based name= Now > > > the issue is the following warning SONAME changed from > > > 'librte_pmd_dpaa2.so.20.0' to 'librte_pmd_dpaa2.so.0.200.2' > > > > > > The primary reason is that now pmd_dpaa2 has no symbol left for 20.0 > section. > > > Following is not helping. > > > [suppress_file] > > > soname_regexp = ^librte_pmd_dpaa2 so, it seems for now, the > > > filename_exp is the only option > > > > That's interesting. > > Because I wondered about this point when reviewing __rte_internal. > > For components providing only internal symbols like components > > providing only experimental symbols, the build framework will select a > > soname with .0.200.x. > > > > Here, your dpaa2 driver was seen as a stable library so far. > > Moving everything to internal changes this and the build framework > > changes the soname to non stable. > > Looking at a v19.11 testpmd binary: > $ readelf -d $HOME/abi/v19.11/build-gcc-shared/usr/local/bin/dpdk-testpmd > |grep dpaa > 0x0000000000000001 (NEEDED) Shared library: > [librte_bus_dpaa.so.20.0] > 0x0000000000000001 (NEEDED) Shared library: > [librte_common_dpaax.so.20.0] > 0x0000000000000001 (NEEDED) Shared library: > [librte_mempool_dpaa.so.20.0] > 0x0000000000000001 (NEEDED) Shared library: > [librte_pmd_dpaa.so.20.0] > > Changing the soname would break this. > > > You could keep an empty DPDK_20.0 block to avoid this and the soname > > will be kept as is. [Hemant] Yes, I was thinking about it but missed to make this change while sending patch. Will do it asap. > > We will have to maintain such soname for all dpaa libraries until 20.11. > > > -- > David Marchand
14/05/2020 19:15, Hemant Agrawal (OSS): > > On Thu, May 14, 2020 at 3:31 PM David Marchand > > <david.marchand@redhat.com> wrote: > > > On Thu, May 14, 2020 at 2:39 PM Hemant Agrawal (OSS) > > > <hemant.agrawal@oss.nxp.com> wrote: > > > > > > > > [Hemant] this is working fine for pmd_dpaa but not for pmd_dpaa2 > > > > > > > > I removed the filename_exp and introduced function based name > > > > Now the issue is the following warning SONAME changed from > > > > 'librte_pmd_dpaa2.so.20.0' to 'librte_pmd_dpaa2.so.0.200.2' > > > > > > > > The primary reason is that now pmd_dpaa2 has no symbol left for > > > > 20.0 section. > > > > Following is not helping. > > > > [suppress_file] > > > > soname_regexp = ^librte_pmd_dpaa2 so, it seems for now, the > > > > filename_exp is the only option > > > > > > That's interesting. > > > Because I wondered about this point when reviewing __rte_internal. > > > For components providing only internal symbols like components > > > providing only experimental symbols, the build framework will select a > > > soname with .0.200.x. I will remind once again that I was against this rule. Distinguishing "stable or partially stable" and "completely non-stable" libraries is an useless complication. > > > Here, your dpaa2 driver was seen as a stable library so far. > > > Moving everything to internal changes this and the build framework > > > changes the soname to non stable. > > > > Looking at a v19.11 testpmd binary: > > $ readelf -d $HOME/abi/v19.11/build-gcc-shared/usr/local/bin/dpdk-testpmd > > |grep dpaa > > 0x0000000000000001 (NEEDED) Shared library: > > [librte_bus_dpaa.so.20.0] > > 0x0000000000000001 (NEEDED) Shared library: > > [librte_common_dpaax.so.20.0] > > 0x0000000000000001 (NEEDED) Shared library: > > [librte_mempool_dpaa.so.20.0] > > 0x0000000000000001 (NEEDED) Shared library: > > [librte_pmd_dpaa.so.20.0] > > > > Changing the soname would break this. > > > > > You could keep an empty DPDK_20.0 block to avoid this and the soname > > > will be kept as is. > > [Hemant] Yes, I was thinking about it but missed to make this change while sending patch. Will do it asap. > > > > We will have to maintain such soname for all dpaa libraries until 20.11. Thank you for maintaining the soname compatibility in v7. Now the question is: what to do in v20.11? This question will have to be voted in the Technical Board which voted the "pure experimental" versioning rule. We have 3 options: a) "Pure internal" libs are versioned as "stable" libs, while "pure experimental" libs have version 0.x. It looks inconsistent and non-sense. b) "Pure internal" libs are versioned as "pure experimental" libs: version 0.x. It makes "pure internal" libs version decreasing in 20.11. c) Forget about the different versioning scheme, i.e. increase 0.x versions to x as "stable" libs. Of course, I vote for option c.
Adding a bit more definitions to better understand. A "stable" library exports at least one symbol in the current stable ABI. Its soname is suffixed with the current ABI version. If the library exports no symbol in the current ABI, but has a symbol in the next ABI, the soname is suffixed also with the current ABI version. A "pure experimental" library exports only experimental symbols. Its soname is suffixed with 0. and the stable ABI version. A "pure internal" library has only internal symbols, or no exported symbols at all, like in most PMDs. Its soname is suffixed with the current ABI version. An "experimental & internal" library exports experimental and internal symbols, but none in current or next stable ABI. We don't have such case yet. I think the original intent was to use the suffix 0.x for libs which export no stable ABI. But it is inconsistent currently. Please read the options below, and give your opinion. Thanks 15/05/2020 11:26, Thomas Monjalon: > Now the question is: what to do in v20.11? > This question will have to be voted in the Technical Board which voted > the "pure experimental" versioning rule. > We have 3 options: > > a) "Pure internal" libs are versioned as "stable" libs, > while "pure experimental" libs have version 0.x. > It looks inconsistent and non-sense. > > b) "Pure internal" libs are versioned as > "pure experimental" libs: version 0.x. > It makes "pure internal" libs version decreasing in 20.11. > > c) Forget about the different versioning scheme, > i.e. increase 0.x versions to x as "stable" libs. > > Of course, I vote for option c.
diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore index c9ee73cb3c..b1488d5549 100644 --- a/devtools/libabigail.abignore +++ b/devtools/libabigail.abignore @@ -48,3 +48,6 @@ changed_enumerators = RTE_CRYPTO_AEAD_LIST_END [suppress_variable] name = rte_crypto_aead_algorithm_strings +; Ignore moving DPAAx stable functions to INTERNAL tag +[suppress_file] + file_name_regexp = ^librte_common_dpaax\. diff --git a/drivers/common/dpaax/dpaa_of.h b/drivers/common/dpaax/dpaa_of.h index 960b421766..38d91a1afe 100644 --- a/drivers/common/dpaax/dpaa_of.h +++ b/drivers/common/dpaax/dpaa_of.h @@ -24,6 +24,7 @@ #include <limits.h> #include <rte_common.h> #include <dpaa_list.h> +#include <rte_compat.h> #ifndef OF_INIT_DEFAULT_PATH #define OF_INIT_DEFAULT_PATH "/proc/device-tree" @@ -102,6 +103,7 @@ struct dt_file { uint64_t buf[OF_FILE_BUF_MAX >> 3]; }; +__rte_internal const struct device_node *of_find_compatible_node( const struct device_node *from, const char *type __rte_unused, @@ -113,32 +115,44 @@ const struct device_node *of_find_compatible_node( dev_node != NULL; \ dev_node = of_find_compatible_node(dev_node, type, compatible)) +__rte_internal const void *of_get_property(const struct device_node *from, const char *name, size_t *lenp) __attribute__((nonnull(2))); +__rte_internal bool of_device_is_available(const struct device_node *dev_node); + +__rte_internal const struct device_node *of_find_node_by_phandle(uint64_t ph); +__rte_internal const struct device_node *of_get_parent(const struct device_node *dev_node); +__rte_internal const struct device_node *of_get_next_child(const struct device_node *dev_node, const struct device_node *prev); +__rte_internal const void *of_get_mac_address(const struct device_node *np); #define for_each_child_node(parent, child) \ for (child = of_get_next_child(parent, NULL); child != NULL; \ child = of_get_next_child(parent, child)) + +__rte_internal uint32_t of_n_addr_cells(const struct device_node *dev_node); uint32_t of_n_size_cells(const struct device_node *dev_node); +__rte_internal const uint32_t *of_get_address(const struct device_node *dev_node, size_t idx, uint64_t *size, uint32_t *flags); +__rte_internal uint64_t of_translate_address(const struct device_node *dev_node, const uint32_t *addr) __attribute__((nonnull)); +__rte_internal bool of_device_is_compatible(const struct device_node *dev_node, const char *compatible); @@ -146,6 +160,7 @@ bool of_device_is_compatible(const struct device_node *dev_node, * subsystem that is device-tree-dependent. Eg. Qman/Bman, config layers, etc. * The path should usually be "/proc/device-tree". */ +__rte_internal int of_init_path(const char *dt_path); /* of_finish() allows a controlled tear-down of the device-tree layer, eg. if a diff --git a/drivers/common/dpaax/dpaax_iova_table.h b/drivers/common/dpaax/dpaax_iova_table.h index fc3b9e7a8f..230fba8ba0 100644 --- a/drivers/common/dpaax/dpaax_iova_table.h +++ b/drivers/common/dpaax/dpaax_iova_table.h @@ -61,9 +61,13 @@ extern struct dpaax_iova_table *dpaax_iova_table_p; #define DPAAX_MEM_SPLIT_MASK_OFF (DPAAX_MEM_SPLIT - 1) /**< Offset */ /* APIs exposed */ +__rte_internal int dpaax_iova_table_populate(void); +__rte_internal void dpaax_iova_table_depopulate(void); +__rte_internal int dpaax_iova_table_update(phys_addr_t paddr, void *vaddr, size_t length); +__rte_internal void dpaax_iova_table_dump(void); static inline void *dpaax_iova_table_get_va(phys_addr_t paddr) __rte_hot; diff --git a/drivers/common/dpaax/rte_common_dpaax_version.map b/drivers/common/dpaax/rte_common_dpaax_version.map index f72eba761d..ad2b2b3fec 100644 --- a/drivers/common/dpaax/rte_common_dpaax_version.map +++ b/drivers/common/dpaax/rte_common_dpaax_version.map @@ -1,4 +1,4 @@ -DPDK_20.0 { +INTERNAL { global: dpaax_iova_table_depopulate;
This patch moves the internal symbols to INTERNAL sections so that any change in them is not reported as ABI breakage. Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> --- devtools/libabigail.abignore | 3 +++ drivers/common/dpaax/dpaa_of.h | 15 +++++++++++++++ drivers/common/dpaax/dpaax_iova_table.h | 4 ++++ drivers/common/dpaax/rte_common_dpaax_version.map | 2 +- 4 files changed, 23 insertions(+), 1 deletion(-) -- 2.17.1