Re: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, April 14, 2021 4:18 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Asaf Penso <asafp@nvidia.com>; Parav Pandit <parav@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>
> Subject: Re: [dpdk-dev] [PATCH v1] bus/auxiliary: introduce auxiliary bus
> 
> 14/04/2021 04:59, Wang, Haiyue:
> > From: Xueming Li
> [...]
> > > +void
> > > +auxiliary_on_scan(struct rte_auxiliary_device *dev) {
> > > +	struct rte_devargs *devargs;
> > > +
> > > +	devargs = auxiliary_devargs_lookup(dev->name);
> > > +	dev->device.devargs = devargs;
> >
> > Can be simple as:
> >
> > dev->device.devargs = auxiliary_devargs_lookup(dev->name);
> >
> > > +}
> > > +
> > > +/*
> > > + * Match the auxiliary Driver and Device using driver function.
> > > + */
> > > +bool
> > > +auxiliary_match(const struct rte_auxiliary_driver *auxiliary_drv,
> > > +		    const struct rte_auxiliary_device *auxiliary_dev)
> >
> > How about these auxiliary variable name style ?
> >
> > const struct rte_auxiliary_driver *aux_drv, const struct
> > rte_auxiliary_device *aux_dev
> 
> +1
> 
> [...]
> > > +static int
> > > +rte_auxiliary_probe_one_driver(struct rte_auxiliary_driver *dr,
> > > +			       struct rte_auxiliary_device *dev) {
> > > +	int ret;
> > > +	enum rte_iova_mode iova_mode;
> > > +
> >
> > RCT style ?
> > 	enum rte_iova_mode iova_mode;
> > 	int ret;
> 
> I don't see the benefit of reverse christmas tree.
> 
> > > +	if ((dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA) > 0 &&
> >
> > '(dr->drv_flags & RTE_AUXILIARY_DRV_NEED_IOVA_AS_VA)' should work, no need '> 0'
> 
> Yes it's acceptable to consider bit testing as a boolean.
> 
> [...]
> > > +static int
> > > +auxiliary_dma_map(struct rte_device *dev, void *addr, uint64_t
> > > +iova, size_t len) {
> > > +	struct rte_auxiliary_device *adev = RTE_DEV_TO_AUXILIARY(dev);
> >
> > How about to use 'aux_dev', instead of 'adev' ?
> >
> > > +
> > > +	if (!adev || !adev->driver) {
> >
> > ' RTE_DEV_TO_AUXILIARY' is container of 'dev', so it should check 'dev
> > != NULL', not '!adev'. ; -)
> 
> Yes and should be explicit NULL comparison.
> 
> [...]
> > > --- /dev/null
> > > +++ b/drivers/bus/auxiliary/linux/auxiliary.c
> >                               ^
> >                               |
> > Seems no need to add one more directory 'linux' layer, as the meson said "linux only".
> 
> I disagree.
> Linux sub-directory is more explicit.
> And who knows? There could be an implementation on other OSes in future.

This reminds me to change meson, allows bus always available to avoid compilation error,
also need to add stubs with __rte_weak for all functions in common file.

> 
>