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

[Thomas Monjalon <thomas@monjalon.net>]

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.