[RFC,11/11] bus: hide bus object

Message ID 20220628144643.1213026-12-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Bus cleanup for 22.11 |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

David Marchand June 28, 2022, 2:46 p.m. UTC
  Make rte_bus opaque for non internal users.
This will make extending this object possible without breaking the ABI.

Introduce a new driver header and move rte_bus definition and helpers.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_devargs.c                 |   2 +-
 app/test/test_vdev.c                    |   2 +-
 drivers/bus/auxiliary/private.h         |   2 +-
 drivers/bus/dpaa/dpaa_bus.c             |   2 +-
 drivers/bus/fslmc/private.h             |   2 +-
 drivers/bus/ifpga/ifpga_bus.c           |   2 +-
 drivers/bus/pci/private.h               |   2 +-
 drivers/bus/vdev/vdev.c                 |   2 +-
 drivers/bus/vmbus/private.h             |   2 +-
 drivers/dma/idxd/idxd_bus.c             |   2 +-
 drivers/net/bonding/rte_eth_bond_args.c |   2 +-
 drivers/net/vdev_netvsc/vdev_netvsc.c   |   2 +-
 lib/eal/common/eal_common_bus.c         |   2 +-
 lib/eal/common/eal_common_dev.c         |   2 +-
 lib/eal/common/eal_common_devargs.c     |   2 +-
 lib/eal/common/hotplug_mp.c             |   2 +-
 lib/eal/include/bus_driver.h            | 295 ++++++++++++++++++++++++
 lib/eal/include/meson.build             |   4 +
 lib/eal/include/rte_bus.h               | 275 +---------------------
 lib/eal/linux/eal_dev.c                 |   2 +-
 lib/eal/version.map                     |   4 +-
 lib/ethdev/rte_ethdev.c                 |   2 +-
 22 files changed, 322 insertions(+), 292 deletions(-)
 create mode 100644 lib/eal/include/bus_driver.h
  

Comments

Tyler Retzlaff June 28, 2022, 4:22 p.m. UTC | #1
On Tue, Jun 28, 2022 at 04:46:43PM +0200, David Marchand wrote:
> Make rte_bus opaque for non internal users.
> This will make extending this object possible without breaking the ABI.
> 
> Introduce a new driver header and move rte_bus definition and helpers.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

... snip ...

> -struct rte_bus {
> -	RTE_TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */
> -	const char *name;            /**< Name of the bus */
> -	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
> -	rte_bus_probe_t probe;       /**< Probe devices on bus */
> -	rte_bus_find_device_t find_device; /**< Find a device on the bus */
> -	rte_bus_plug_t plug;         /**< Probe single device for drivers */
> -	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
> -	rte_bus_parse_t parse;       /**< Parse a device name */
> -	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
> -	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
> -	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
> -	struct rte_bus_conf conf;    /**< Bus configuration */
> -	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
> -	rte_dev_iterate_t dev_iterate; /**< Device iterator. */
> -	rte_bus_hot_unplug_handler_t hot_unplug_handler;
> -				/**< handle hot-unplug failure on the bus */
> -	rte_bus_sigbus_handler_t sigbus_handler;
> -					/**< handle sigbus error on the bus */
> -
> -};

since we're overhauling anyway we could follow suit with a number of the
lessons from posix apis e.g. pthread and eliminate typed pointers for a
little extra value.

> +struct rte_bus;
> +struct rte_device;

to avoid people tripping over mishandling pointers in/out of the api
surface taking the opaque object you could declare opaque handle for the
api to operate on instead. it would force the use of a cast in the
implementation but would avoid accidental void * of the wrong thing that
got cached being passed in. if the cast was really undesirable just
whack it under an inline / internal function.

e.g. make the opaque object an explicit type.

struct {
    uintptr_t opaque;
} rte_bus_handle_t;

// implementation
rte_bus_handle_t
rte_bus_find(rte_bus_handle_t start,
	rte_bus_cmp_t cmp, const void *data)
{
	struct rte_bus *bus = (struct rte_bus *)start.opaque;

	// do bus things on bus

	return {.opaque = whatever};
}

then you will get hard compile time failure if someone messes up
argument passing.

e.g.

void *somerandomp = ...;

...
rte_bus_find(somerandomp, ...); // compile fail

i would actually suggest this wrapping in a struct / handle approach for
any opaque object that is passed over the api surface. the change is
bigger of course...

it has various other advantages when/if we decide to make the bus/driver
surface stable in abi which i understand is not a promise dpdk makes
right now but still we might one day.

anyway, just a suggestion. i like the series either way.
  
Tyler Retzlaff June 28, 2022, 4:24 p.m. UTC | #2
On Tue, Jun 28, 2022 at 09:22:13AM -0700, Tyler Retzlaff wrote:
> 
> e.g. make the opaque object an explicit type.
> 
oops missed the typedef there but you probably know what i meant.

typedef
> struct {
>     uintptr_t opaque;
> } rte_bus_handle_t;
>
  
Stephen Hemminger June 28, 2022, 4:29 p.m. UTC | #3
On Tue, 28 Jun 2022 09:22:13 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Tue, Jun 28, 2022 at 04:46:43PM +0200, David Marchand wrote:
> > Make rte_bus opaque for non internal users.
> > This will make extending this object possible without breaking the ABI.
> > 
> > Introduce a new driver header and move rte_bus definition and helpers.
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---  
> 
> ... snip ...
> 
> > -struct rte_bus {
> > -	RTE_TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */
> > -	const char *name;            /**< Name of the bus */
> > -	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
> > -	rte_bus_probe_t probe;       /**< Probe devices on bus */
> > -	rte_bus_find_device_t find_device; /**< Find a device on the bus */
> > -	rte_bus_plug_t plug;         /**< Probe single device for drivers */
> > -	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
> > -	rte_bus_parse_t parse;       /**< Parse a device name */
> > -	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
> > -	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
> > -	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
> > -	struct rte_bus_conf conf;    /**< Bus configuration */
> > -	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
> > -	rte_dev_iterate_t dev_iterate; /**< Device iterator. */
> > -	rte_bus_hot_unplug_handler_t hot_unplug_handler;
> > -				/**< handle hot-unplug failure on the bus */
> > -	rte_bus_sigbus_handler_t sigbus_handler;
> > -					/**< handle sigbus error on the bus */
> > -
> > -};  
> 
> since we're overhauling anyway we could follow suit with a number of the
> lessons from posix apis e.g. pthread and eliminate typed pointers for a
> little extra value.
> 
> > +struct rte_bus;
> > +struct rte_device;  
> 
> to avoid people tripping over mishandling pointers in/out of the api
> surface taking the opaque object you could declare opaque handle for the
> api to operate on instead. it would force the use of a cast in the
> implementation but would avoid accidental void * of the wrong thing that
> got cached being passed in. if the cast was really undesirable just
> whack it under an inline / internal function.

I don't like that because it least to dangerous casts in the internal code.
Better to keep the the type of the object. As long as the API only passes
around an pointer to a struct, without exposing the contents of the struct;
it is safer and easier to debug.
  
Tyler Retzlaff June 28, 2022, 5:07 p.m. UTC | #4
On Tue, Jun 28, 2022 at 09:29:05AM -0700, Stephen Hemminger wrote:
> On Tue, 28 Jun 2022 09:22:13 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > On Tue, Jun 28, 2022 at 04:46:43PM +0200, David Marchand wrote:
> > > Make rte_bus opaque for non internal users.
> > > This will make extending this object possible without breaking the ABI.
> > > 
> > > Introduce a new driver header and move rte_bus definition and helpers.
> > > 
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---  
> > 
> > ... snip ...
> > 
> > > -struct rte_bus {
> > > -	RTE_TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */
> > > -	const char *name;            /**< Name of the bus */
> > > -	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
> > > -	rte_bus_probe_t probe;       /**< Probe devices on bus */
> > > -	rte_bus_find_device_t find_device; /**< Find a device on the bus */
> > > -	rte_bus_plug_t plug;         /**< Probe single device for drivers */
> > > -	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
> > > -	rte_bus_parse_t parse;       /**< Parse a device name */
> > > -	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
> > > -	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
> > > -	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
> > > -	struct rte_bus_conf conf;    /**< Bus configuration */
> > > -	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
> > > -	rte_dev_iterate_t dev_iterate; /**< Device iterator. */
> > > -	rte_bus_hot_unplug_handler_t hot_unplug_handler;
> > > -				/**< handle hot-unplug failure on the bus */
> > > -	rte_bus_sigbus_handler_t sigbus_handler;
> > > -					/**< handle sigbus error on the bus */
> > > -
> > > -};  
> > 
> > since we're overhauling anyway we could follow suit with a number of the
> > lessons from posix apis e.g. pthread and eliminate typed pointers for a
> > little extra value.
> > 
> > > +struct rte_bus;
> > > +struct rte_device;  
> > 
> > to avoid people tripping over mishandling pointers in/out of the api
> > surface taking the opaque object you could declare opaque handle for the
> > api to operate on instead. it would force the use of a cast in the
> > implementation but would avoid accidental void * of the wrong thing that
> > got cached being passed in. if the cast was really undesirable just
> > whack it under an inline / internal function.
> 
> I don't like that because it least to dangerous casts in the internal code.
> Better to keep the the type of the object. As long as the API only passes
> around an pointer to a struct, without exposing the contents of the struct;
> it is safer and easier to debug.

as i mentioned you can use an inline/internal function or even a macro
to hide the cast, you could provide some additional integrity checks
here if desired as a value add.

the fact that you expose that it is a struct is an internal
implementation detail, if truly opaque tomorrow you could convert it
to a simple integer that indexes or enumerates something and prevents
any meaningful interpretation in the application.

when you say it is safer to debug i think you mean for dpdk devs not the
application developer because unless the app developer does something
really gross/dangerous casting they really can't "mishandle" the opaque
object except to use one that isn't initialized at all which we
can detect and handle internally in a general way.

i will however concede there would be slightly more finger work when
debugging dpdk itself since gdb / debugger doesn't automatically infer
type so you end up having to tell gdb what is in the uintptr_t.

anyway just drawing from experience in the driver frameworks we maintain
in windows, i think one of our regrets is that we didn't do this from
day 1 and subsequentl that we initially only used one opaque type
instead of defining separate (not implicitly convertable) types to each
opaque type.
  
Stephen Hemminger June 28, 2022, 5:38 p.m. UTC | #5
On Tue, 28 Jun 2022 10:07:12 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> > > to avoid people tripping over mishandling pointers in/out of the api
> > > surface taking the opaque object you could declare opaque handle for the
> > > api to operate on instead. it would force the use of a cast in the
> > > implementation but would avoid accidental void * of the wrong thing that
> > > got cached being passed in. if the cast was really undesirable just
> > > whack it under an inline / internal function.  
> > 
> > I don't like that because it least to dangerous casts in the internal code.
> > Better to keep the the type of the object. As long as the API only passes
> > around an pointer to a struct, without exposing the contents of the struct;
> > it is safer and easier to debug.  
> 
> as i mentioned you can use an inline/internal function or even a macro
> to hide the cast, you could provide some additional integrity checks
> here if desired as a value add.
> 
> the fact that you expose that it is a struct is an internal
> implementation detail, if truly opaque tomorrow you could convert it
> to a simple integer that indexes or enumerates something and prevents
> any meaningful interpretation in the application.
> 
> when you say it is safer to debug i think you mean for dpdk devs not the
> application developer because unless the app developer does something
> really gross/dangerous casting they really can't "mishandle" the opaque
> object except to use one that isn't initialized at all which we
> can detect and handle internally in a general way.
> 
> i will however concede there would be slightly more finger work when
> debugging dpdk itself since gdb / debugger doesn't automatically infer
> type so you end up having to tell gdb what is in the uintptr_t.
> 
> anyway just drawing from experience in the driver frameworks we maintain
> in windows, i think one of our regrets is that we didn't do this from
> day 1 and subsequentl that we initially only used one opaque type
> instead of defining separate (not implicitly convertable) types to each
> opaque type.

It seems to be a difference in style/taste.
The Linux/Unix side prefers opaque structure pointers.
Windows (and LLVM) uses numeric handles.

At this point DPDK should follow the Linux bus.
  
Tyler Retzlaff June 28, 2022, 6:23 p.m. UTC | #6
On Tue, Jun 28, 2022 at 10:38:27AM -0700, Stephen Hemminger wrote:
> On Tue, 28 Jun 2022 10:07:12 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > > > to avoid people tripping over mishandling pointers in/out of the api
> > > > surface taking the opaque object you could declare opaque handle for the
> > > > api to operate on instead. it would force the use of a cast in the
> > > > implementation but would avoid accidental void * of the wrong thing that
> > > > got cached being passed in. if the cast was really undesirable just
> > > > whack it under an inline / internal function.  
> > > 
> > > I don't like that because it least to dangerous casts in the internal code.
> > > Better to keep the the type of the object. As long as the API only passes
> > > around an pointer to a struct, without exposing the contents of the struct;
> > > it is safer and easier to debug.  
> > 
> > as i mentioned you can use an inline/internal function or even a macro
> > to hide the cast, you could provide some additional integrity checks
> > here if desired as a value add.
> > 
> > the fact that you expose that it is a struct is an internal
> > implementation detail, if truly opaque tomorrow you could convert it
> > to a simple integer that indexes or enumerates something and prevents
> > any meaningful interpretation in the application.
> > 
> > when you say it is safer to debug i think you mean for dpdk devs not the
> > application developer because unless the app developer does something
> > really gross/dangerous casting they really can't "mishandle" the opaque
> > object except to use one that isn't initialized at all which we
> > can detect and handle internally in a general way.
> > 
> > i will however concede there would be slightly more finger work when
> > debugging dpdk itself since gdb / debugger doesn't automatically infer
> > type so you end up having to tell gdb what is in the uintptr_t.
> > 
> > anyway just drawing from experience in the driver frameworks we maintain
> > in windows, i think one of our regrets is that we didn't do this from
> > day 1 and subsequentl that we initially only used one opaque type
> > instead of defining separate (not implicitly convertable) types to each
> > opaque type.
> 
> It seems to be a difference in style/taste.

it's not i've sited at least one example of a mistake that becomes a
compile time failure where application code is incorrectly authored
where use of a pointer offers no such protection.

> The Linux/Unix side prefers opaque structure pointers.
> Windows (and LLVM) uses numeric handles.
> 
> At this point DPDK should follow the Linux bus.

dpdk is multi-platform and unix does not necessarily standardize on
pointer to struct for opaque objects. freebsd has many apis notably
bus_space that does uses handles and as previously mentioned posix
threads uses handles.

i understand that linux is an important platform but it isn't the only
platform dpdk targets and just because it is important doesn't mean it
should always enjoy being the defacto standard.

anyway, i'll leave it for the patch author to decide. i still like the
patch series either way. i just think this would make applications more
robust.
  
David Marchand July 9, 2022, 8:16 a.m. UTC | #7
On Tue, Jun 28, 2022 at 8:23 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Tue, Jun 28, 2022 at 10:38:27AM -0700, Stephen Hemminger wrote:
> > On Tue, 28 Jun 2022 10:07:12 -0700
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >
> > > > > to avoid people tripping over mishandling pointers in/out of the api
> > > > > surface taking the opaque object you could declare opaque handle for the
> > > > > api to operate on instead. it would force the use of a cast in the
> > > > > implementation but would avoid accidental void * of the wrong thing that
> > > > > got cached being passed in. if the cast was really undesirable just
> > > > > whack it under an inline / internal function.
> > > >
> > > > I don't like that because it least to dangerous casts in the internal code.
> > > > Better to keep the the type of the object. As long as the API only passes
> > > > around an pointer to a struct, without exposing the contents of the struct;
> > > > it is safer and easier to debug.
> > >
> > > as i mentioned you can use an inline/internal function or even a macro
> > > to hide the cast, you could provide some additional integrity checks
> > > here if desired as a value add.
> > >
> > > the fact that you expose that it is a struct is an internal
> > > implementation detail, if truly opaque tomorrow you could convert it
> > > to a simple integer that indexes or enumerates something and prevents
> > > any meaningful interpretation in the application.
> > >
> > > when you say it is safer to debug i think you mean for dpdk devs not the
> > > application developer because unless the app developer does something
> > > really gross/dangerous casting they really can't "mishandle" the opaque
> > > object except to use one that isn't initialized at all which we
> > > can detect and handle internally in a general way.
> > >
> > > i will however concede there would be slightly more finger work when
> > > debugging dpdk itself since gdb / debugger doesn't automatically infer
> > > type so you end up having to tell gdb what is in the uintptr_t.
> > >
> > > anyway just drawing from experience in the driver frameworks we maintain
> > > in windows, i think one of our regrets is that we didn't do this from
> > > day 1 and subsequentl that we initially only used one opaque type
> > > instead of defining separate (not implicitly convertable) types to each
> > > opaque type.
> >
> > It seems to be a difference in style/taste.
>
> it's not i've sited at least one example of a mistake that becomes a
> compile time failure where application code is incorrectly authored
> where use of a pointer offers no such protection.
>
> > The Linux/Unix side prefers opaque structure pointers.
> > Windows (and LLVM) uses numeric handles.
> >
> > At this point DPDK should follow the Linux bus.
>
> dpdk is multi-platform and unix does not necessarily standardize on
> pointer to struct for opaque objects. freebsd has many apis notably
> bus_space that does uses handles and as previously mentioned posix
> threads uses handles.
>
> i understand that linux is an important platform but it isn't the only
> platform dpdk targets and just because it is important doesn't mean it
> should always enjoy being the defacto standard.
>
> anyway, i'll leave it for the patch author to decide. i still like the
> patch series either way. i just think this would make applications more
> robust.

Thanks for this feedback Tyler.
I would lean towards Stephen opinion atm, but I am not decided yet.

For now, I'll post a v2, extending the series to other internal objects.
We can conclude on this topic during 22.11.
  
Stephen Hemminger July 9, 2022, 4:28 p.m. UTC | #8
On Sat, 9 Jul 2022 10:16:43 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Tue, Jun 28, 2022 at 8:23 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > On Tue, Jun 28, 2022 at 10:38:27AM -0700, Stephen Hemminger wrote:  
> > > On Tue, 28 Jun 2022 10:07:12 -0700
> > > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> > >  
> > > > > > to avoid people tripping over mishandling pointers in/out of the api
> > > > > > surface taking the opaque object you could declare opaque handle for the
> > > > > > api to operate on instead. it would force the use of a cast in the
> > > > > > implementation but would avoid accidental void * of the wrong thing that
> > > > > > got cached being passed in. if the cast was really undesirable just
> > > > > > whack it under an inline / internal function.  
> > > > >
> > > > > I don't like that because it least to dangerous casts in the internal code.
> > > > > Better to keep the the type of the object. As long as the API only passes
> > > > > around an pointer to a struct, without exposing the contents of the struct;
> > > > > it is safer and easier to debug.  
> > > >
> > > > as i mentioned you can use an inline/internal function or even a macro
> > > > to hide the cast, you could provide some additional integrity checks
> > > > here if desired as a value add.
> > > >
> > > > the fact that you expose that it is a struct is an internal
> > > > implementation detail, if truly opaque tomorrow you could convert it
> > > > to a simple integer that indexes or enumerates something and prevents
> > > > any meaningful interpretation in the application.
> > > >
> > > > when you say it is safer to debug i think you mean for dpdk devs not the
> > > > application developer because unless the app developer does something
> > > > really gross/dangerous casting they really can't "mishandle" the opaque
> > > > object except to use one that isn't initialized at all which we
> > > > can detect and handle internally in a general way.
> > > >
> > > > i will however concede there would be slightly more finger work when
> > > > debugging dpdk itself since gdb / debugger doesn't automatically infer
> > > > type so you end up having to tell gdb what is in the uintptr_t.
> > > >
> > > > anyway just drawing from experience in the driver frameworks we maintain
> > > > in windows, i think one of our regrets is that we didn't do this from
> > > > day 1 and subsequentl that we initially only used one opaque type
> > > > instead of defining separate (not implicitly convertable) types to each
> > > > opaque type.  
> > >
> > > It seems to be a difference in style/taste.  
> >
> > it's not i've sited at least one example of a mistake that becomes a
> > compile time failure where application code is incorrectly authored
> > where use of a pointer offers no such protection.
> >  
> > > The Linux/Unix side prefers opaque structure pointers.
> > > Windows (and LLVM) uses numeric handles.
> > >
> > > At this point DPDK should follow the Linux bus.  
> >
> > dpdk is multi-platform and unix does not necessarily standardize on
> > pointer to struct for opaque objects. freebsd has many apis notably
> > bus_space that does uses handles and as previously mentioned posix
> > threads uses handles.
> >
> > i understand that linux is an important platform but it isn't the only
> > platform dpdk targets and just because it is important doesn't mean it
> > should always enjoy being the defacto standard.
> >
> > anyway, i'll leave it for the patch author to decide. i still like the
> > patch series either way. i just think this would make applications more
> > robust.  
> 
> Thanks for this feedback Tyler.
> I would lean towards Stephen opinion atm, but I am not decided yet.
> 
> For now, I'll post a v2, extending the series to other internal objects.
> We can conclude on this topic during 22.11.

If you get chance to deconstruct API, switching to a numeric index
is safest similar to what Tyler suggested.  Think of ethdev port number
and Posix file descriptor model. The advantage of an index is that
it can be validated more easily by the code that is called.
  
David Marchand Sept. 23, 2022, 8:49 a.m. UTC | #9
On Sat, Jul 9, 2022 at 6:28 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> > > > > > > to avoid people tripping over mishandling pointers in/out of the api
> > > > > > > surface taking the opaque object you could declare opaque handle for the
> > > > > > > api to operate on instead. it would force the use of a cast in the
> > > > > > > implementation but would avoid accidental void * of the wrong thing that
> > > > > > > got cached being passed in. if the cast was really undesirable just
> > > > > > > whack it under an inline / internal function.
> > > > > >
> > > > > > I don't like that because it least to dangerous casts in the internal code.
> > > > > > Better to keep the the type of the object. As long as the API only passes
> > > > > > around an pointer to a struct, without exposing the contents of the struct;
> > > > > > it is safer and easier to debug.
> > > > >
> > > > > as i mentioned you can use an inline/internal function or even a macro
> > > > > to hide the cast, you could provide some additional integrity checks
> > > > > here if desired as a value add.
> > > > >
> > > > > the fact that you expose that it is a struct is an internal
> > > > > implementation detail, if truly opaque tomorrow you could convert it
> > > > > to a simple integer that indexes or enumerates something and prevents
> > > > > any meaningful interpretation in the application.
> > > > >
> > > > > when you say it is safer to debug i think you mean for dpdk devs not the
> > > > > application developer because unless the app developer does something
> > > > > really gross/dangerous casting they really can't "mishandle" the opaque
> > > > > object except to use one that isn't initialized at all which we
> > > > > can detect and handle internally in a general way.
> > > > >
> > > > > i will however concede there would be slightly more finger work when
> > > > > debugging dpdk itself since gdb / debugger doesn't automatically infer
> > > > > type so you end up having to tell gdb what is in the uintptr_t.
> > > > >
> > > > > anyway just drawing from experience in the driver frameworks we maintain
> > > > > in windows, i think one of our regrets is that we didn't do this from
> > > > > day 1 and subsequentl that we initially only used one opaque type
> > > > > instead of defining separate (not implicitly convertable) types to each
> > > > > opaque type.
> > > >
> > > > It seems to be a difference in style/taste.
> > >
> > > it's not i've sited at least one example of a mistake that becomes a
> > > compile time failure where application code is incorrectly authored
> > > where use of a pointer offers no such protection.
> > >
> > > > The Linux/Unix side prefers opaque structure pointers.
> > > > Windows (and LLVM) uses numeric handles.
> > > >
> > > > At this point DPDK should follow the Linux bus.
> > >
> > > dpdk is multi-platform and unix does not necessarily standardize on
> > > pointer to struct for opaque objects. freebsd has many apis notably
> > > bus_space that does uses handles and as previously mentioned posix
> > > threads uses handles.
> > >
> > > i understand that linux is an important platform but it isn't the only
> > > platform dpdk targets and just because it is important doesn't mean it
> > > should always enjoy being the defacto standard.
> > >
> > > anyway, i'll leave it for the patch author to decide. i still like the
> > > patch series either way. i just think this would make applications more
> > > robust.
> >
> > Thanks for this feedback Tyler.
> > I would lean towards Stephen opinion atm, but I am not decided yet.
> >
> > For now, I'll post a v2, extending the series to other internal objects.
> > We can conclude on this topic during 22.11.
>
> If you get chance to deconstruct API, switching to a numeric index
> is safest similar to what Tyler suggested.  Think of ethdev port number
> and Posix file descriptor model. The advantage of an index is that
> it can be validated more easily by the code that is called.

This is still a heavy change in the APIs.
The deprecation notice did not mention such change, and I don't think
many people are aware of this suggestion.
This would need more discussion but I don't want to block the series
on concluding on this question.

So I am for going ahead with the series in its current form.
  
Thomas Monjalon Sept. 23, 2022, 8:57 a.m. UTC | #10
23/09/2022 10:49, David Marchand:
> On Sat, Jul 9, 2022 at 6:28 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> > > For now, I'll post a v2, extending the series to other internal objects.
> > > We can conclude on this topic during 22.11.
> >
> > If you get chance to deconstruct API, switching to a numeric index
> > is safest similar to what Tyler suggested.  Think of ethdev port number
> > and Posix file descriptor model. The advantage of an index is that
> > it can be validated more easily by the code that is called.
> 
> This is still a heavy change in the APIs.
> The deprecation notice did not mention such change, and I don't think
> many people are aware of this suggestion.
> This would need more discussion but I don't want to block the series
> on concluding on this question.
> 
> So I am for going ahead with the series in its current form.

I agree that changing the object type would too much heavy.
The goal of this series is to hide the object,
not changing the way we refer to it.

I understand the compilation check is stronger on a plain object
than on a pointer, but I don't think this benefit is worth
changing some basic API of DPDK.
It seems our users prefer having less API change.
  

Patch

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index ac5bc34c18..0a4c34a1ad 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -9,7 +9,7 @@ 
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_kvargs.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_class.h>
 
 #include "test.h"
diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
index 5eeff3106d..1904e76e44 100644
--- a/app/test/test_vdev.c
+++ b/app/test/test_vdev.c
@@ -8,7 +8,7 @@ 
 
 #include <rte_common.h>
 #include <rte_kvargs.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_bus_vdev.h>
 
 #include "test.h"
diff --git a/drivers/bus/auxiliary/private.h b/drivers/bus/auxiliary/private.h
index 06a920114c..7e8ae8c2f9 100644
--- a/drivers/bus/auxiliary/private.h
+++ b/drivers/bus/auxiliary/private.h
@@ -9,7 +9,7 @@ 
 #include <stdio.h>
 #include <sys/queue.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 
 #include "rte_bus_auxiliary.h"
 
diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
index ad4ea156a6..4f12944470 100644
--- a/drivers/bus/dpaa/dpaa_bus.c
+++ b/drivers/bus/dpaa/dpaa_bus.c
@@ -29,7 +29,7 @@ 
 #include <ethdev_driver.h>
 #include <rte_malloc.h>
 #include <rte_ring.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_mbuf_pool_ops.h>
 #include <rte_mbuf_dyn.h>
 
diff --git a/drivers/bus/fslmc/private.h b/drivers/bus/fslmc/private.h
index 80d4673ca8..f08dc7716b 100644
--- a/drivers/bus/fslmc/private.h
+++ b/drivers/bus/fslmc/private.h
@@ -5,7 +5,7 @@ 
 #ifndef __PRIVATE_H__
 #define __PRIVATE_H__
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 
 #include <rte_fslmc.h>
 
diff --git a/drivers/bus/ifpga/ifpga_bus.c b/drivers/bus/ifpga/ifpga_bus.c
index e005f2cb70..a2c2f13cf7 100644
--- a/drivers/bus/ifpga/ifpga_bus.c
+++ b/drivers/bus/ifpga/ifpga_bus.c
@@ -14,7 +14,7 @@ 
 #include <fcntl.h>
 
 #include <rte_errno.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_per_lcore.h>
 #include <rte_memory.h>
 #include <rte_memzone.h>
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index 6ccec15655..0cefed5edf 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -8,7 +8,7 @@ 
 #include <stdbool.h>
 #include <stdio.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_os_shim.h>
 #include <rte_pci.h>
 
diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index a8d8b2327e..dd4e931687 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -12,7 +12,7 @@ 
 
 #include <rte_eal.h>
 #include <rte_dev.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_common.h>
 #include <rte_devargs.h>
 #include <rte_memory.h>
diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 732cb6d583..5205e17a3f 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -9,7 +9,7 @@ 
 #include <stdbool.h>
 #include <sys/uio.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_log.h>
 #include <rte_eal_paging.h>
 #include <rte_vmbus_reg.h>
diff --git a/drivers/dma/idxd/idxd_bus.c b/drivers/dma/idxd/idxd_bus.c
index 13cb967f6d..e30dcfc281 100644
--- a/drivers/dma/idxd/idxd_bus.c
+++ b/drivers/dma/idxd/idxd_bus.c
@@ -8,7 +8,7 @@ 
 #include <sys/mman.h>
 #include <libgen.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_devargs.h>
 #include <rte_eal.h>
 #include <rte_log.h>
diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index b90757756a..f461bf9207 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -4,7 +4,7 @@ 
 
 #include <rte_devargs.h>
 #include <rte_pci.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_bus_pci.h>
 #include <rte_kvargs.h>
 
diff --git a/drivers/net/vdev_netvsc/vdev_netvsc.c b/drivers/net/vdev_netvsc/vdev_netvsc.c
index 2587195168..24edf9f3c4 100644
--- a/drivers/net/vdev_netvsc/vdev_netvsc.c
+++ b/drivers/net/vdev_netvsc/vdev_netvsc.c
@@ -24,7 +24,7 @@ 
 #include <unistd.h>
 
 #include <rte_alarm.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_bus_vdev.h>
 #include <rte_common.h>
 #include <rte_dev.h>
diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
index cbf382f967..be64d31b0f 100644
--- a/lib/eal/common/eal_common_bus.c
+++ b/lib/eal/common/eal_common_bus.c
@@ -6,7 +6,7 @@ 
 #include <string.h>
 #include <sys/queue.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_debug.h>
 #include <rte_string_fns.h>
 #include <rte_errno.h>
diff --git a/lib/eal/common/eal_common_dev.c b/lib/eal/common/eal_common_dev.c
index bbaf518570..a366bb3689 100644
--- a/lib/eal/common/eal_common_dev.c
+++ b/lib/eal/common/eal_common_dev.c
@@ -7,7 +7,7 @@ 
 #include <string.h>
 #include <sys/queue.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_class.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index fa95c52708..adccfa713f 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -10,7 +10,7 @@ 
 #include <string.h>
 #include <stdarg.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_class.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
diff --git a/lib/eal/common/hotplug_mp.c b/lib/eal/common/hotplug_mp.c
index 252f147b6f..9e6eddec92 100644
--- a/lib/eal/common/hotplug_mp.c
+++ b/lib/eal/common/hotplug_mp.c
@@ -3,7 +3,7 @@ 
  */
 #include <string.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_eal.h>
 #include <rte_errno.h>
 #include <rte_alarm.h>
diff --git a/lib/eal/include/bus_driver.h b/lib/eal/include/bus_driver.h
new file mode 100644
index 0000000000..ac404c3d5e
--- /dev/null
+++ b/lib/eal/include/bus_driver.h
@@ -0,0 +1,295 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2022 Red Hat, Inc.
+ */
+
+#ifndef BUS_DRIVER_H
+#define BUS_DRIVER_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <rte_bus.h>
+#include <rte_compat.h>
+#include <rte_dev.h>
+#include <rte_eal.h>
+#include <rte_tailq.h>
+
+struct rte_device;
+
+/** Double linked list of buses */
+RTE_TAILQ_HEAD(rte_bus_list, rte_bus);
+
+/**
+ * Bus specific scan for devices attached on the bus.
+ * For each bus object, the scan would be responsible for finding devices and
+ * adding them to its private device list.
+ *
+ * A bus should mandatorily implement this method.
+ *
+ * @return
+ *	0 for successful scan
+ *	<0 for unsuccessful scan with error value
+ */
+typedef int (*rte_bus_scan_t)(void);
+
+/**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ *
+ * This is called while iterating over each registered bus.
+ *
+ * @return
+ *	0 for successful probe
+ *	!0 for any error while probing
+ */
+typedef int (*rte_bus_probe_t)(void);
+
+/**
+ * Device iterator to find a device on a bus.
+ *
+ * This function returns an rte_device if one of those held by the bus
+ * matches the data passed as parameter.
+ *
+ * If the comparison function returns zero this function should stop iterating
+ * over any more devices. To continue a search the device of a previous search
+ * can be passed via the start parameter.
+ *
+ * @param cmp
+ *	Comparison function.
+ *
+ * @param data
+ *	Data to compare each device against.
+ *
+ * @param start
+ *	starting point for the iteration
+ *
+ * @return
+ *	The first device matching the data, NULL if none exists.
+ */
+typedef struct rte_device *
+(*rte_bus_find_device_t)(const struct rte_device *start, rte_dev_cmp_t cmp,
+			 const void *data);
+
+/**
+ * Implementation specific probe function which is responsible for linking
+ * devices on that bus with applicable drivers.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error.
+ */
+typedef int (*rte_bus_plug_t)(struct rte_device *dev);
+
+/**
+ * Implementation specific remove function which is responsible for unlinking
+ * devices on that bus from assigned driver.
+ *
+ * @param dev
+ *	Device pointer that was returned by a previous call to find_device.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error.
+ */
+typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
+
+/**
+ * Bus specific parsing function.
+ * Validates the syntax used in the textual representation of a device,
+ * If the syntax is valid and ``addr`` is not NULL, writes the bus-specific
+ * device representation to ``addr``.
+ *
+ * @param[in] name
+ *	device textual description
+ *
+ * @param[out] addr
+ *	device information location address, into which parsed info
+ *	should be written. If NULL, nothing should be written, which
+ *	is not an error.
+ *
+ * @return
+ *	0 if parsing was successful.
+ *	!0 for any error.
+ */
+typedef int (*rte_bus_parse_t)(const char *name, void *addr);
+
+/**
+ * Parse bus part of the device arguments.
+ *
+ * The field name of the struct rte_devargs will be set.
+ *
+ * @param da
+ *	Pointer to the devargs to parse.
+ *
+ * @return
+ *	0 on successful parsing, otherwise rte_errno is set.
+ *	-EINVAL: on parsing error.
+ *	-ENODEV: if no key matching a device argument is specified.
+ *	-E2BIG: device name is too long.
+ */
+typedef int (*rte_bus_devargs_parse_t)(struct rte_devargs *da);
+
+/**
+ * Device level DMA map function.
+ * After a successful call, the memory segment will be mapped to the
+ * given device.
+ *
+ * @param dev
+ *	Device pointer.
+ * @param addr
+ *	Virtual address to map.
+ * @param iova
+ *	IOVA address to map.
+ * @param len
+ *	Length of the memory segment being mapped.
+ *
+ * @return
+ *	0 if mapping was successful.
+ *	Negative value and rte_errno is set otherwise.
+ */
+typedef int (*rte_dev_dma_map_t)(struct rte_device *dev, void *addr,
+				  uint64_t iova, size_t len);
+
+/**
+ * Device level DMA unmap function.
+ * After a successful call, the memory segment will no longer be
+ * accessible by the given device.
+ *
+ * @param dev
+ *	Device pointer.
+ * @param addr
+ *	Virtual address to unmap.
+ * @param iova
+ *	IOVA address to unmap.
+ * @param len
+ *	Length of the memory segment being mapped.
+ *
+ * @return
+ *	0 if un-mapping was successful.
+ *	Negative value and rte_errno is set otherwise.
+ */
+typedef int (*rte_dev_dma_unmap_t)(struct rte_device *dev, void *addr,
+				   uint64_t iova, size_t len);
+
+/**
+ * Implement a specific hot-unplug handler, which is responsible for
+ * handle the failure when device be hot-unplugged. When the event of
+ * hot-unplug be detected, it could call this function to handle
+ * the hot-unplug failure and avoid app crash.
+ * @param dev
+ *	Pointer of the device structure.
+ *
+ * @return
+ *	0 on success.
+ *	!0 on error.
+ */
+typedef int (*rte_bus_hot_unplug_handler_t)(struct rte_device *dev);
+
+/**
+ * Implement a specific sigbus handler, which is responsible for handling
+ * the sigbus error which is either original memory error, or specific memory
+ * error that caused of device be hot-unplugged. When sigbus error be captured,
+ * it could call this function to handle sigbus error.
+ * @param failure_addr
+ *	Pointer of the fault address of the sigbus error.
+ *
+ * @return
+ *	0 for success handle the sigbus for hot-unplug.
+ *	1 for not process it, because it is a generic sigbus error.
+ *	-1 for failed to handle the sigbus for hot-unplug.
+ */
+typedef int (*rte_bus_sigbus_handler_t)(const void *failure_addr);
+
+/**
+ * Bus scan policies
+ */
+enum rte_bus_scan_mode {
+	RTE_BUS_SCAN_UNDEFINED,
+	RTE_BUS_SCAN_ALLOWLIST,
+	RTE_BUS_SCAN_BLOCKLIST,
+};
+
+/**
+ * A structure used to configure bus operations.
+ */
+struct rte_bus_conf {
+	enum rte_bus_scan_mode scan_mode; /**< Scan policy. */
+};
+
+
+/**
+ * Get common iommu class of the all the devices on the bus. The bus may
+ * check that those devices are attached to iommu driver.
+ * If no devices are attached to the bus. The bus may return with don't care
+ * (_DC) value.
+ * Otherwise, The bus will return appropriate _pa or _va iova mode.
+ *
+ * @return
+ *      enum rte_iova_mode value.
+ */
+typedef enum rte_iova_mode (*rte_bus_get_iommu_class_t)(void);
+
+/**
+ * A structure describing a generic bus.
+ */
+struct rte_bus {
+	RTE_TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */
+	const char *name;            /**< Name of the bus */
+	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
+	rte_bus_probe_t probe;       /**< Probe devices on bus */
+	rte_bus_find_device_t find_device; /**< Find a device on the bus */
+	rte_bus_plug_t plug;         /**< Probe single device for drivers */
+	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
+	rte_bus_parse_t parse;       /**< Parse a device name */
+	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
+	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
+	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
+	struct rte_bus_conf conf;    /**< Bus configuration */
+	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
+	rte_dev_iterate_t dev_iterate; /**< Device iterator. */
+	rte_bus_hot_unplug_handler_t hot_unplug_handler;
+				/**< handle hot-unplug failure on the bus */
+	rte_bus_sigbus_handler_t sigbus_handler;
+					/**< handle sigbus error on the bus */
+};
+
+/**
+ * Register a Bus handler.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be registered.
+ */
+__rte_internal
+void rte_bus_register(struct rte_bus *bus);
+
+/**
+ * Helper for Bus registration.
+ * The constructor has higher priority than PMD constructors.
+ */
+#define RTE_REGISTER_BUS(nm, bus) \
+RTE_INIT_PRIO(businitfn_ ##nm, BUS) \
+{\
+	(bus).name = RTE_STR(nm);\
+	rte_bus_register(&bus); \
+}
+
+/**
+ * Unregister a Bus handler.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be unregistered.
+ */
+__rte_internal
+void rte_bus_unregister(struct rte_bus *bus);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* BUS_DRIVER_H */
diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
index fd6e844224..77d8621a51 100644
--- a/lib/eal/include/meson.build
+++ b/lib/eal/include/meson.build
@@ -55,6 +55,10 @@  headers += files(
         'rte_vfio.h',
 )
 
+driver_sdk_headers = files(
+        'bus_driver.h',
+)
+
 # special case install the generic headers, since they go in a subdir
 generic_headers = files(
         'generic/rte_atomic.h',
diff --git a/lib/eal/include/rte_bus.h b/lib/eal/include/rte_bus.h
index 9b70f2f7b2..1b08407e53 100644
--- a/lib/eal/include/rte_bus.h
+++ b/lib/eal/include/rte_bus.h
@@ -20,251 +20,11 @@  extern "C" {
 
 #include <stdio.h>
 
-#include <rte_dev.h>
+#include <rte_compat.h>
 #include <rte_eal.h>
-#include <rte_log.h>
 
-/** Double linked list of buses */
-RTE_TAILQ_HEAD(rte_bus_list, rte_bus);
-
-/**
- * Bus specific scan for devices attached on the bus.
- * For each bus object, the scan would be responsible for finding devices and
- * adding them to its private device list.
- *
- * A bus should mandatorily implement this method.
- *
- * @return
- *	0 for successful scan
- *	<0 for unsuccessful scan with error value
- */
-typedef int (*rte_bus_scan_t)(void);
-
-/**
- * Implementation specific probe function which is responsible for linking
- * devices on that bus with applicable drivers.
- *
- * This is called while iterating over each registered bus.
- *
- * @return
- *	0 for successful probe
- *	!0 for any error while probing
- */
-typedef int (*rte_bus_probe_t)(void);
-
-/**
- * Device iterator to find a device on a bus.
- *
- * This function returns an rte_device if one of those held by the bus
- * matches the data passed as parameter.
- *
- * If the comparison function returns zero this function should stop iterating
- * over any more devices. To continue a search the device of a previous search
- * can be passed via the start parameter.
- *
- * @param cmp
- *	Comparison function.
- *
- * @param data
- *	Data to compare each device against.
- *
- * @param start
- *	starting point for the iteration
- *
- * @return
- *	The first device matching the data, NULL if none exists.
- */
-typedef struct rte_device *
-(*rte_bus_find_device_t)(const struct rte_device *start, rte_dev_cmp_t cmp,
-			 const void *data);
-
-/**
- * Implementation specific probe function which is responsible for linking
- * devices on that bus with applicable drivers.
- *
- * @param dev
- *	Device pointer that was returned by a previous call to find_device.
- *
- * @return
- *	0 on success.
- *	!0 on error.
- */
-typedef int (*rte_bus_plug_t)(struct rte_device *dev);
-
-/**
- * Implementation specific remove function which is responsible for unlinking
- * devices on that bus from assigned driver.
- *
- * @param dev
- *	Device pointer that was returned by a previous call to find_device.
- *
- * @return
- *	0 on success.
- *	!0 on error.
- */
-typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
-
-/**
- * Bus specific parsing function.
- * Validates the syntax used in the textual representation of a device,
- * If the syntax is valid and ``addr`` is not NULL, writes the bus-specific
- * device representation to ``addr``.
- *
- * @param[in] name
- *	device textual description
- *
- * @param[out] addr
- *	device information location address, into which parsed info
- *	should be written. If NULL, nothing should be written, which
- *	is not an error.
- *
- * @return
- *	0 if parsing was successful.
- *	!0 for any error.
- */
-typedef int (*rte_bus_parse_t)(const char *name, void *addr);
-
-/**
- * Parse bus part of the device arguments.
- *
- * The field name of the struct rte_devargs will be set.
- *
- * @param da
- *	Pointer to the devargs to parse.
- *
- * @return
- *	0 on successful parsing, otherwise rte_errno is set.
- *	-EINVAL: on parsing error.
- *	-ENODEV: if no key matching a device argument is specified.
- *	-E2BIG: device name is too long.
- */
-typedef int (*rte_bus_devargs_parse_t)(struct rte_devargs *da);
-
-/**
- * Device level DMA map function.
- * After a successful call, the memory segment will be mapped to the
- * given device.
- *
- * @param dev
- *	Device pointer.
- * @param addr
- *	Virtual address to map.
- * @param iova
- *	IOVA address to map.
- * @param len
- *	Length of the memory segment being mapped.
- *
- * @return
- *	0 if mapping was successful.
- *	Negative value and rte_errno is set otherwise.
- */
-typedef int (*rte_dev_dma_map_t)(struct rte_device *dev, void *addr,
-				  uint64_t iova, size_t len);
-
-/**
- * Device level DMA unmap function.
- * After a successful call, the memory segment will no longer be
- * accessible by the given device.
- *
- * @param dev
- *	Device pointer.
- * @param addr
- *	Virtual address to unmap.
- * @param iova
- *	IOVA address to unmap.
- * @param len
- *	Length of the memory segment being mapped.
- *
- * @return
- *	0 if un-mapping was successful.
- *	Negative value and rte_errno is set otherwise.
- */
-typedef int (*rte_dev_dma_unmap_t)(struct rte_device *dev, void *addr,
-				   uint64_t iova, size_t len);
-
-/**
- * Implement a specific hot-unplug handler, which is responsible for
- * handle the failure when device be hot-unplugged. When the event of
- * hot-unplug be detected, it could call this function to handle
- * the hot-unplug failure and avoid app crash.
- * @param dev
- *	Pointer of the device structure.
- *
- * @return
- *	0 on success.
- *	!0 on error.
- */
-typedef int (*rte_bus_hot_unplug_handler_t)(struct rte_device *dev);
-
-/**
- * Implement a specific sigbus handler, which is responsible for handling
- * the sigbus error which is either original memory error, or specific memory
- * error that caused of device be hot-unplugged. When sigbus error be captured,
- * it could call this function to handle sigbus error.
- * @param failure_addr
- *	Pointer of the fault address of the sigbus error.
- *
- * @return
- *	0 for success handle the sigbus for hot-unplug.
- *	1 for not process it, because it is a generic sigbus error.
- *	-1 for failed to handle the sigbus for hot-unplug.
- */
-typedef int (*rte_bus_sigbus_handler_t)(const void *failure_addr);
-
-/**
- * Bus scan policies
- */
-enum rte_bus_scan_mode {
-	RTE_BUS_SCAN_UNDEFINED,
-	RTE_BUS_SCAN_ALLOWLIST,
-	RTE_BUS_SCAN_BLOCKLIST,
-};
-
-/**
- * A structure used to configure bus operations.
- */
-struct rte_bus_conf {
-	enum rte_bus_scan_mode scan_mode; /**< Scan policy. */
-};
-
-
-/**
- * Get common iommu class of the all the devices on the bus. The bus may
- * check that those devices are attached to iommu driver.
- * If no devices are attached to the bus. The bus may return with don't care
- * (_DC) value.
- * Otherwise, The bus will return appropriate _pa or _va iova mode.
- *
- * @return
- *      enum rte_iova_mode value.
- */
-typedef enum rte_iova_mode (*rte_bus_get_iommu_class_t)(void);
-
-
-/**
- * A structure describing a generic bus.
- */
-struct rte_bus {
-	RTE_TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */
-	const char *name;            /**< Name of the bus */
-	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
-	rte_bus_probe_t probe;       /**< Probe devices on bus */
-	rte_bus_find_device_t find_device; /**< Find a device on the bus */
-	rte_bus_plug_t plug;         /**< Probe single device for drivers */
-	rte_bus_unplug_t unplug;     /**< Remove single device from driver */
-	rte_bus_parse_t parse;       /**< Parse a device name */
-	rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
-	rte_dev_dma_map_t dma_map;   /**< DMA map for device in the bus */
-	rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
-	struct rte_bus_conf conf;    /**< Bus configuration */
-	rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
-	rte_dev_iterate_t dev_iterate; /**< Device iterator. */
-	rte_bus_hot_unplug_handler_t hot_unplug_handler;
-				/**< handle hot-unplug failure on the bus */
-	rte_bus_sigbus_handler_t sigbus_handler;
-					/**< handle sigbus error on the bus */
-
-};
+struct rte_bus;
+struct rte_device;
 
 /**
  * @warning
@@ -279,24 +39,6 @@  struct rte_bus {
 __rte_experimental
 const char *rte_bus_name(const struct rte_bus *bus);
 
-/**
- * Register a Bus handler.
- *
- * @param bus
- *   A pointer to a rte_bus structure describing the bus
- *   to be registered.
- */
-void rte_bus_register(struct rte_bus *bus);
-
-/**
- * Unregister a Bus handler.
- *
- * @param bus
- *   A pointer to a rte_bus structure describing the bus
- *   to be unregistered.
- */
-void rte_bus_unregister(struct rte_bus *bus);
-
 /**
  * Scan all the buses.
  *
@@ -386,17 +128,6 @@  struct rte_bus *rte_bus_find_by_name(const char *busname);
  */
 enum rte_iova_mode rte_bus_get_iommu_class(void);
 
-/**
- * Helper for Bus registration.
- * The constructor has higher priority than PMD constructors.
- */
-#define RTE_REGISTER_BUS(nm, bus) \
-RTE_INIT_PRIO(businitfn_ ##nm, BUS) \
-{\
-	(bus).name = RTE_STR(nm);\
-	rte_bus_register(&bus); \
-}
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/eal/linux/eal_dev.c b/lib/eal/linux/eal_dev.c
index 02ae1cde29..098a2c3076 100644
--- a/lib/eal/linux/eal_dev.c
+++ b/lib/eal/linux/eal_dev.c
@@ -13,7 +13,7 @@ 
 #include <rte_dev.h>
 #include <rte_interrupts.h>
 #include <rte_alarm.h>
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_spinlock.h>
 #include <rte_errno.h>
 
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 6f713c987d..48c8a2f511 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -13,9 +13,7 @@  DPDK_22 {
 	rte_bus_find_by_name;
 	rte_bus_get_iommu_class;
 	rte_bus_probe;
-	rte_bus_register;
 	rte_bus_scan;
-	rte_bus_unregister;
 	rte_calloc;
 	rte_calloc_socket;
 	rte_cpu_get_flag_enabled;
@@ -432,6 +430,8 @@  EXPERIMENTAL {
 INTERNAL {
 	global:
 
+	rte_bus_register;
+	rte_bus_unregister;
 	rte_eal_get_baseaddr;
 	rte_firmware_read;
 	rte_intr_allow_others;
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index c94d6573d5..6679bd8276 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -11,7 +11,7 @@ 
 #include <string.h>
 #include <sys/queue.h>
 
-#include <rte_bus.h>
+#include <bus_driver.h>
 #include <rte_log.h>
 #include <rte_interrupts.h>
 #include <rte_memcpy.h>