diff mbox series

[1/7] common/dpaax: move internal symbols into INTERNAL section

Message ID 20200505140832.646-1-hemant.agrawal@nxp.com (mailing list archive)
State Superseded, archived
Headers
Series [1/7] common/dpaax: move internal symbols into INTERNAL section |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/iol-testing fail Testing issues

Commit Message

Hemant Agrawal May 5, 2020, 2:08 p.m. UTC 
  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>
---
 drivers/common/dpaax/dpaa_of.h                    | 15 +++++++++++++++
 drivers/common/dpaax/dpaax_iova_table.h           |  4 ++++
 drivers/common/dpaax/rte_common_dpaax_version.map |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

Comments

David Marchand May 5, 2020, 5:07 p.m. UTC | #1 
Cc: Ray.

On Tue, May 5, 2020 at 4:10 PM Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>
> This patch moves the internal symbols to INTERNAL sections
> so that any change in them is not reported as ABI breakage.

Talking about the series (not just this patch as I did not look at the
details), travis reported a build issue:
https://travis-ci.com/github/ovsrobot/dpdk/builds/163985766

It looks like there is a versioned symbol for a static function of one
of those drivers.
$ git grep dpaa2_get_qbman_swp origin/master
origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:static struct
dpaa2_dpio_dev *dpaa2_get_qbman_swp(int lcoreid)
origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:
dpaa2_io_portal[lcore_id].dpio_dev = dpaa2_get_qbman_swp(lcore_id);
origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:
dpaa2_get_qbman_swp(lcore_id);
origin/master:drivers/bus/fslmc/rte_bus_fslmc_version.map:
dpaa2_get_qbman_swp;


Once fixed, I don't understand how you chose to move some symbols to
INTERNAL and not others.
Can you explain the differences?


Finally, I would still expect a failure in the ABI check even if using
__rte_internal marking.
Marking them internal will make you free of changing those symbols in
the future.
The problem is the transient state we are in.

In 20.02 (and I suppose 19.11 too), those symbols were exported and
versioned as stable DPDK_20.
So with the current ABI check script, this move will still be seen as a removal.
Hemant Agrawal May 7, 2020, 6:20 a.m. UTC | #2 
Hi David

> On Tue, May 5, 2020 at 4:10 PM Hemant Agrawal
> <hemant.agrawal@nxp.com> wrote:
> >
> > This patch moves the internal symbols to INTERNAL sections so that any
> > change in them is not reported as ABI breakage.
> 
> Talking about the series (not just this patch as I did not look at the details),
> travis reported a build issue:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-
> ci.com%2Fgithub%2Fovsrobot%2Fdpdk%2Fbuilds%2F163985766&amp;data=02
> %7C01%7Chemant.agrawal%40nxp.com%7C555963f208a3446deba708d7f116e
> 1cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6372429528900581
> 54&amp;sdata=j2fqNXovCPfaLlPtZwS9TaMBKMBP5l7inwX%2BsndavS4%3D&am
> p;reserved=0
> 
> It looks like there is a versioned symbol for a static function of one of those
> drivers.
> $ git grep dpaa2_get_qbman_swp origin/master
> origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:static struct
> dpaa2_dpio_dev *dpaa2_get_qbman_swp(int lcoreid)
> origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:
> dpaa2_io_portal[lcore_id].dpio_dev = dpaa2_get_qbman_swp(lcore_id);
> origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:
> dpaa2_get_qbman_swp(lcore_id);
> origin/master:drivers/bus/fslmc/rte_bus_fslmc_version.map:
> dpaa2_get_qbman_swp;

[Hemant] yes, I saw it. It will be fixed in v2

> 
> 
> Once fixed, I don't understand how you chose to move some symbols to
> INTERNAL and not others.
> Can you explain the differences?

[Hemant] In most of the libraries the symbols are internal only. i.e. they are suppose to be used by other dpdk internal drivers.
Only very few cases rte_dpaa2_mempool.h, rte_pmd_dpaa2.h rte_pmd_dpaa.h have few functions, which are suppose to be used by the external entities or applications.

> 
> 
> Finally, I would still expect a failure in the ABI check even if using
> __rte_internal marking.
> Marking them internal will make you free of changing those symbols in the
> future.
> The problem is the transient state we are in.

[Hemant] I also expect it like this. But still facing few issues w.r.t variables.
__rte_internal can not be used as a predecessor for variable names. 
In map files dpaa have some of the variables also defined as part of symbols for the faster access across libraries. 
I am still looking a right way to handle them. 

> 
> In 20.02 (and I suppose 19.11 too), those symbols were exported and
> versioned as stable DPDK_20.
> So with the current ABI check script, this move will still be seen as a removal.
> 
> 
> --
> David Marchand
Ray Kinsella May 7, 2020, 6:54 a.m. UTC | #3 
On 07/05/2020 07:20, Hemant Agrawal wrote:
> Hi David
> 
>> On Tue, May 5, 2020 at 4:10 PM Hemant Agrawal
>> <hemant.agrawal@nxp.com> wrote:
>>>
>>> This patch moves the internal symbols to INTERNAL sections so that any
>>> change in them is not reported as ABI breakage.
>>
>> Talking about the series (not just this patch as I did not look at the details),
>> travis reported a build issue:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-
>> ci.com%2Fgithub%2Fovsrobot%2Fdpdk%2Fbuilds%2F163985766&amp;data=02
>> %7C01%7Chemant.agrawal%40nxp.com%7C555963f208a3446deba708d7f116e
>> 1cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6372429528900581
>> 54&amp;sdata=j2fqNXovCPfaLlPtZwS9TaMBKMBP5l7inwX%2BsndavS4%3D&am
>> p;reserved=0
>>
>> It looks like there is a versioned symbol for a static function of one of those
>> drivers.
>> $ git grep dpaa2_get_qbman_swp origin/master
>> origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:static struct
>> dpaa2_dpio_dev *dpaa2_get_qbman_swp(int lcoreid)
>> origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:
>> dpaa2_io_portal[lcore_id].dpio_dev = dpaa2_get_qbman_swp(lcore_id);
>> origin/master:drivers/bus/fslmc/portal/dpaa2_hw_dpio.c:
>> dpaa2_get_qbman_swp(lcore_id);
>> origin/master:drivers/bus/fslmc/rte_bus_fslmc_version.map:
>> dpaa2_get_qbman_swp;
> 
> [Hemant] yes, I saw it. It will be fixed in v2
> 
>>
>>
>> Once fixed, I don't understand how you chose to move some symbols to
>> INTERNAL and not others.
>> Can you explain the differences?
> 
> [Hemant] In most of the libraries the symbols are internal only. i.e. they are suppose to be used by other dpdk internal drivers.
> Only very few cases rte_dpaa2_mempool.h, rte_pmd_dpaa2.h rte_pmd_dpaa.h have few functions, which are suppose to be used by the external entities or applications.
> 
>>
>>
>> Finally, I would still expect a failure in the ABI check even if using
>> __rte_internal marking.
>> Marking them internal will make you free of changing those symbols in the
>> future.
>> The problem is the transient state we are in.
> 
> [Hemant] I also expect it like this. But still facing few issues w.r.t variables.
> __rte_internal can not be used as a predecessor for variable names. 
> In map files dpaa have some of the variables also defined as part of symbols for the faster access across libraries. 
> I am still looking a right way to handle them. 

I think your least error prone way is going to be to use an internal function to return a pointer to the variable(s).
rte_dpaa2+get_my_important variables.

> 
>>
>> In 20.02 (and I suppose 19.11 too), those symbols were exported and
>> versioned as stable DPDK_20.
>> So with the current ABI check script, this move will still be seen as a removal.
>>
>>
>> --
>> David Marchand
>
diff mbox series

Patch

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;