diff mbox series

[2/2] bus/pci: cleanup private symbols

Message ID 20200506124314.14009-2-david.marchand@redhat.com (mailing list archive)
State Rejected, archived
Delegated to: David Marchand
Headers
Series [1/2] remove references to private PCI probe function |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

David Marchand May 6, 2020, 12:43 p.m. UTC 
  Internal symbols do not need the rte_ prefix.
Some symbols do not need to be exposed in the private header and have
been made static.

Fixes: c752998b5e2e ("pci: introduce library and driver")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/bus/pci/bsd/pci.c    |  8 ++++----
 drivers/bus/pci/linux/pci.c  |  8 ++++----
 drivers/bus/pci/pci_common.c | 26 ++++++++++++------------
 drivers/bus/pci/pci_params.c |  5 ++---
 drivers/bus/pci/private.h    | 38 ++++++------------------------------
 5 files changed, 29 insertions(+), 56 deletions(-)

Comments

Gaëtan Rivet May 6, 2020, 5:21 p.m. UTC | #1 
On 06/05/20 14:43 +0200, David Marchand wrote:
> Internal symbols do not need the rte_ prefix.
> Some symbols do not need to be exposed in the private header and have
> been made static.
> 
> Fixes: c752998b5e2e ("pci: introduce library and driver")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

For this patch, I would like to understand why we are having this
policy. Symbols that are emitted for later linking will be present in
archives generated by the framework. Am I wrong to think they can
conflict with user app symbols?

If that is correct, we should use pci_* prefix for static symbols,
rte_* for everything else, even "internal" symbols -- in the sense
that they are meant to be opaque to the user, but will still be linked
in static build.

If I'm wrong in thinking this, then ok with this policy and let's go
forward to align naming in PCI bus.
Stephen Hemminger May 6, 2020, 8:25 p.m. UTC | #2 
On Wed, 6 May 2020 19:21:23 +0200
Gaëtan Rivet <grive@u256.net> wrote:

> On 06/05/20 14:43 +0200, David Marchand wrote:
> > Internal symbols do not need the rte_ prefix.
> > Some symbols do not need to be exposed in the private header and have
> > been made static.
> > 
> > Fixes: c752998b5e2e ("pci: introduce library and driver")
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>  
> 
> For this patch, I would like to understand why we are having this
> policy. Symbols that are emitted for later linking will be present in
> archives generated by the framework. Am I wrong to think they can
> conflict with user app symbols?
> 
> If that is correct, we should use pci_* prefix for static symbols,
> rte_* for everything else, even "internal" symbols -- in the sense
> that they are meant to be opaque to the user, but will still be linked
> in static build.
> 
> If I'm wrong in thinking this, then ok with this policy and let's go
> forward to align naming in PCI bus.
> 

Agree that all symbols need a prefix.
Any symbol that is not static is visible to the application if static linking.
There is some pre-linking magic can be done but DPDK isn't doing it.
David Marchand May 7, 2020, 12:41 p.m. UTC | #3 
On Wed, May 6, 2020 at 7:21 PM Gaëtan Rivet <grive@u256.net> wrote:
>
> On 06/05/20 14:43 +0200, David Marchand wrote:
> > Internal symbols do not need the rte_ prefix.
> > Some symbols do not need to be exposed in the private header and have
> > been made static.
> >
> > Fixes: c752998b5e2e ("pci: introduce library and driver")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> For this patch, I would like to understand why we are having this
> policy. Symbols that are emitted for later linking will be present in
> archives generated by the framework. Am I wrong to think they can
> conflict with user app symbols?
>
> If that is correct, we should use pci_* prefix for static symbols,
> rte_* for everything else, even "internal" symbols -- in the sense
> that they are meant to be opaque to the user, but will still be linked
> in static build.
>
> If I'm wrong in thinking this, then ok with this policy and let's go
> forward to align naming in PCI bus.

I see your point.
This is a pain to read code which mixes internal and public functions
with the rte_ prefix.
I have no answer atm, I am ok with dropping this patch.
David Marchand May 7, 2020, 12:43 p.m. UTC | #4 
On Wed, May 6, 2020 at 10:25 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> > If that is correct, we should use pci_* prefix for static symbols,
> > rte_* for everything else, even "internal" symbols -- in the sense
> > that they are meant to be opaque to the user, but will still be linked
> > in static build.
> >
> > If I'm wrong in thinking this, then ok with this policy and let's go
> > forward to align naming in PCI bus.
> >
>
> Agree that all symbols need a prefix.
> Any symbol that is not static is visible to the application if static linking.
> There is some pre-linking magic can be done but DPDK isn't doing it.

Like automatically prefixing symbols?
diff mbox series

Patch

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index ebbfeb13a8..ef6b953cd1 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -288,7 +288,7 @@  pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&rte_pci_bus.device_list)) {
-		rte_pci_add_device(dev);
+		pci_add_device(dev);
 	}
 	else {
 		struct rte_pci_device *dev2 = NULL;
@@ -299,7 +299,7 @@  pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 			if (ret > 0)
 				continue;
 			else if (ret < 0) {
-				rte_pci_insert_device(dev2, dev);
+				pci_insert_device(dev2, dev);
 			} else { /* already registered */
 				dev2->kdrv = dev->kdrv;
 				dev2->max_vfs = dev->max_vfs;
@@ -311,7 +311,7 @@  pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
 			}
 			return 0;
 		}
-		rte_pci_add_device(dev);
+		pci_add_device(dev);
 	}
 
 	return 0;
@@ -326,7 +326,7 @@  pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
  * list. Call pci_scan_one() for each pci entry found.
  */
 int
-rte_pci_scan(void)
+pci_scan(void)
 {
 	int fd;
 	unsigned dev_count = 0;
diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
index ca783b1575..4dc37c8cad 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -335,7 +335,7 @@  pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 
 	/* device is valid, add in list (sorted) */
 	if (TAILQ_EMPTY(&rte_pci_bus.device_list)) {
-		rte_pci_add_device(dev);
+		pci_add_device(dev);
 	} else {
 		struct rte_pci_device *dev2;
 		int ret;
@@ -346,7 +346,7 @@  pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 				continue;
 
 			if (ret < 0) {
-				rte_pci_insert_device(dev2, dev);
+				pci_insert_device(dev2, dev);
 			} else { /* already registered */
 				if (!rte_dev_is_probed(&dev2->device)) {
 					dev2->kdrv = dev->kdrv;
@@ -388,7 +388,7 @@  pci_scan_one(const char *dirname, const struct rte_pci_addr *addr)
 			return 0;
 		}
 
-		rte_pci_add_device(dev);
+		pci_add_device(dev);
 	}
 
 	return 0;
@@ -457,7 +457,7 @@  parse_pci_addr_format(const char *buf, int bufsize, struct rte_pci_addr *addr)
  * list
  */
 int
-rte_pci_scan(void)
+pci_scan(void)
 {
 	struct dirent *e;
 	DIR *dir;
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index ab73c009ac..e3729bdb9a 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -81,8 +81,8 @@  pci_name_set(struct rte_pci_device *dev)
 /*
  * Match the PCI Driver and Device using the ID Table
  */
-int
-rte_pci_match(const struct rte_pci_driver *pci_drv,
+static int
+pci_match(const struct rte_pci_driver *pci_drv,
 	      const struct rte_pci_device *pci_dev)
 {
 	const struct rte_pci_id *id_table;
@@ -132,7 +132,7 @@  rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	loc = &dev->addr;
 
 	/* The device is not blacklisted; Check if driver supports it */
-	if (!rte_pci_match(dr, dev))
+	if (!pci_match(dr, dev))
 		/* Match of device and driver failed */
 		return 1;
 
@@ -388,14 +388,14 @@  rte_pci_unregister(struct rte_pci_driver *driver)
 
 /* Add a device to PCI bus */
 void
-rte_pci_add_device(struct rte_pci_device *pci_dev)
+pci_add_device(struct rte_pci_device *pci_dev)
 {
 	TAILQ_INSERT_TAIL(&rte_pci_bus.device_list, pci_dev, next);
 }
 
 /* Insert a device into a predefined position in PCI bus */
 void
-rte_pci_insert_device(struct rte_pci_device *exist_pci_dev,
+pci_insert_device(struct rte_pci_device *exist_pci_dev,
 		      struct rte_pci_device *new_pci_dev)
 {
 	TAILQ_INSERT_BEFORE(exist_pci_dev, new_pci_dev, next);
@@ -403,7 +403,7 @@  rte_pci_insert_device(struct rte_pci_device *exist_pci_dev,
 
 /* Remove a device from PCI bus */
 static void
-rte_pci_remove_device(struct rte_pci_device *pci_dev)
+pci_remove_device(struct rte_pci_device *pci_dev)
 {
 	TAILQ_REMOVE(&rte_pci_bus.device_list, pci_dev, next);
 }
@@ -536,7 +536,7 @@  pci_unplug(struct rte_device *dev)
 	pdev = RTE_DEV_TO_PCI(dev);
 	ret = rte_pci_detach_dev(pdev);
 	if (ret == 0) {
-		rte_pci_remove_device(pdev);
+		pci_remove_device(pdev);
 		rte_devargs_remove(dev->devargs);
 		free(pdev);
 	}
@@ -609,8 +609,8 @@  pci_ignore_device(const struct rte_pci_device *dev)
 	return true;
 }
 
-enum rte_iova_mode
-rte_pci_get_iommu_class(void)
+static enum rte_iova_mode
+pci_get_iommu_class(void)
 {
 	enum rte_iova_mode iova_mode = RTE_IOVA_DC;
 	const struct rte_pci_device *dev;
@@ -635,7 +635,7 @@  rte_pci_get_iommu_class(void)
 		FOREACH_DRIVER_ON_PCIBUS(drv) {
 			enum rte_iova_mode dev_iova_mode;
 
-			if (!rte_pci_match(drv, dev))
+			if (!pci_match(drv, dev))
 				continue;
 
 			dev_iova_mode = pci_device_iova_mode(drv, dev);
@@ -674,7 +674,7 @@  rte_pci_get_iommu_class(void)
 
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
-		.scan = rte_pci_scan,
+		.scan = pci_scan,
 		.probe = pci_probe,
 		.find_device = pci_find_device,
 		.plug = pci_plug,
@@ -682,8 +682,8 @@  struct rte_pci_bus rte_pci_bus = {
 		.parse = pci_parse,
 		.dma_map = pci_dma_map,
 		.dma_unmap = pci_dma_unmap,
-		.get_iommu_class = rte_pci_get_iommu_class,
-		.dev_iterate = rte_pci_dev_iterate,
+		.get_iommu_class = pci_get_iommu_class,
+		.dev_iterate = pci_dev_iterate,
 		.hot_unplug_handler = pci_hot_unplug_handler,
 		.sigbus_handler = pci_sigbus_handler,
 	},
diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 3192e9c967..16c64c9e9f 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -55,9 +55,8 @@  pci_dev_match(const struct rte_device *dev,
 }
 
 void *
-rte_pci_dev_iterate(const void *start,
-		    const char *str,
-		    const struct rte_dev_iterator *it __rte_unused)
+pci_dev_iterate(const void *start, const char *str,
+		const struct rte_dev_iterator *it __rte_unused)
 {
 	rte_bus_find_device_t find_device;
 	struct rte_kvargs *kvargs = NULL;
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index af1c7ae5fe..6992443bf9 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -24,7 +24,7 @@  extern struct rte_pci_bus rte_pci_bus;
  * @return
  *  0 on success, negative on error
  */
-int rte_pci_scan(void);
+int pci_scan(void);
 
 /**
  * Find the name of a PCI device.
@@ -41,7 +41,7 @@  pci_name_set(struct rte_pci_device *dev);
  *	PCI device to add
  * @return void
  */
-void rte_pci_add_device(struct rte_pci_device *pci_dev);
+void pci_add_device(struct rte_pci_device *pci_dev);
 
 /**
  * Insert a PCI device in the PCI Bus at a particular location in the device
@@ -54,7 +54,7 @@  void rte_pci_add_device(struct rte_pci_device *pci_dev);
  *	PCI device to be added before exist_pci_dev
  * @return void
  */
-void rte_pci_insert_device(struct rte_pci_device *exist_pci_dev,
+void pci_insert_device(struct rte_pci_device *exist_pci_dev,
 		struct rte_pci_device *new_pci_dev);
 
 /**
@@ -147,23 +147,8 @@  pci_uio_remap_resource(struct rte_pci_device *dev);
 int pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 		struct mapped_pci_resource *uio_res, int map_idx);
 
-/*
- * Match the PCI Driver and Device using the ID Table
- *
- * @param pci_drv
- *      PCI driver from which ID table would be extracted
- * @param pci_dev
- *      PCI device to match against the driver
- * @return
- *      1 for successful match
- *      0 for unsuccessful match
- */
-int
-rte_pci_match(const struct rte_pci_driver *pci_drv,
-	      const struct rte_pci_device *pci_dev);
-
 /**
- * OS specific callbacks for rte_pci_get_iommu_class
+ * OS specific callbacks for pci_get_iommu_class
  *
  */
 bool
@@ -173,16 +158,6 @@  enum rte_iova_mode
 pci_device_iova_mode(const struct rte_pci_driver *pci_drv,
 		     const struct rte_pci_device *pci_dev);
 
-/**
- * Get iommu class of PCI devices on the bus.
- * And return their preferred iova mapping mode.
- *
- * @return
- *   - enum rte_iova_mode.
- */
-enum rte_iova_mode
-rte_pci_get_iommu_class(void);
-
 /*
  * Iterate over internal devices,
  * matching any device against the provided
@@ -202,8 +177,7 @@  rte_pci_get_iommu_class(void);
  *   NULL otherwise.
  */
 void *
-rte_pci_dev_iterate(const void *start,
-		    const char *str,
-		    const struct rte_dev_iterator *it);
+pci_dev_iterate(const void *start, const char *str,
+		const struct rte_dev_iterator *it);
 
 #endif /* _PCI_PRIVATE_H_ */