diff mbox series

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

Message ID 20200505140832.646-1-hemant.agrawal@nxp.com
State Superseded
Headers show
Series [1/7] common/dpaax: move internal symbols into INTERNAL section | expand

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(-)

-- 
2.17.1

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.


-- 
David Marchand
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;