Re: [dpdk-dev] [RFC PATCH 5/6] bus/mlx5_pci: register a PCI driver

[Parav Pandit <parav@mellanox.com>]

> From: Gaëtan Rivet <grive@u256.net>
> Sent: Thursday, June 18, 2020 8:05 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Ori Kam <orika@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Subject: Re: [dpdk-dev] [RFC PATCH 5/6] bus/mlx5_pci: register a PCI driver
> 
> On 18/06/20 10:03 +0000, Parav Pandit wrote:
> >
> > > From: Gaëtan Rivet <grive@u256.net>
> > > Sent: Tuesday, June 16, 2020 3:17 AM
> > >
> > > On 10/06/20 17:17 +0000, Parav Pandit wrote:
> > > > Create a mlx5 bus driver framework for invoking drivers of
> > > > multiple classes who have registered with the mlx5_pci bus driver.
> > > >
> > > > Validate user class arguments for supported class combinations.
> > > >
> > > > Signed-off-by: Parav Pandit <parav@mellanox.com>
> > > > ---
> > > >  drivers/bus/mlx5_pci/Makefile           |   1 +
> > > >  drivers/bus/mlx5_pci/meson.build        |   2 +-
> > > >  drivers/bus/mlx5_pci/mlx5_pci_bus.c     | 253
> > > ++++++++++++++++++++++++
> > > >  drivers/bus/mlx5_pci/rte_bus_mlx5_pci.h |   1 +
> > > >  4 files changed, 256 insertions(+), 1 deletion(-)
> > > >
> 
> [...]
> 
> > > > +
> > > > +	while (nstr) {
> > > > +		/* Extract each individual class name */
> > > > +		found = strsep(&nstr, ":");
> > >
> > > I have not seen the feature test macros (_DEFAULT_SOURCE) in the
> > > Makefile, it seems required for strsep()?
> > >
> > If its mandatory meson build should have complained?
> >
> 
> Invoking the compiler without specific standard conformance will work.
> If someone adds for example -std=c11 however then _DEFAULT_SOURCE
> becomes necessary.
> 
> It all depends on the range of compiler versions targeted by this code.
> I don't know the full coverage, but I see -std=c11 + -D_DEFAULT_SOURCE in
> most mlx5 code, which is why I'm asking for a double check here.
> 
> 
> [...]
> 
> > > > +			continue;
> > > > +
> > > > +		if (class->loaded)
> > > > +			class->remove(dev);
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +mlx5_bus_pci_dma_map(struct rte_pci_device *dev, void *addr,
> > > > +		     uint64_t iova, size_t len) {
> > > > +	struct rte_mlx5_pci_driver *class;
> > > > +	int ret = -EINVAL;
> > > > +
> > > > +	TAILQ_FOREACH(class, &drv_list, next) {
> > > > +		if (!class->dma_map)
> > > > +			continue;
> > > > +
> > > > +		return class->dma_map(dev, addr, iova, len);
> > >
> > > Is there a specific class that could have priority for the DMA?
> > >
> > No.
> >
> 
> The code being written this way seems to point to multiple classes being able
> to have DMA ops. If that's not the case, you can add a sanity check to enforce
> than only the right classes have DMA ops defined.
> 
Yes. I will add it. good point.

> > > > +	}
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int
> > > > +mlx5_bus_pci_dma_unmap(struct rte_pci_device *dev, void *addr,
> > > > +		       uint64_t iova, size_t len) {
> > > > +	struct rte_mlx5_pci_driver *class;
> > > > +	int ret = -EINVAL;
> > > > +
> > > > +	TAILQ_FOREACH_REVERSE(class, &drv_list, mlx5_pci_bus_drv_head,
> > > next) {
> > > > +		if (!class->dma_unmap)
> > > > +			continue;
> > > > +
> > > > +		return class->dma_unmap(dev, addr, iova, len);
> > >
> > > If you have two classes A -> B having dma_map() + dma_unmap(), you
> > > will
> > > dma_map() with A then dma_unmap() with B, due to the _REVERSE()
> > > iteration?
> > >
> > There isn't plan for two drivers to do so.
> > If two classes do that its source of an error.
> > Will enhance the bus when that need arise.
> >
> 
> You have well-defined edge-cases, but they are not apparent reading the
> code. Such error could be warned about and / or documented.
> 
> > > Why use reversed iteration at all by the way for dinit? If your ops
> > > is sound any order should be ok.
> > >
> > Because deinit must be always reverse of init() code regardless.
> >
> 
> This is a strong statement :)
> 
:-)
Deinit not following reverse is almost an invitation to bugs.
Given there are two different drivers, there shouldn't be much dependency that way.
But it is always a good practice to follow de-init as reverse sequence.
It eliminates plenty of null and other validation checks all over.

> If this is a requirement for driver inter-dependencies to be properly
> implemented, this should be documented as such. Probably also explained in
> the high-level design documentation in the header exposing this driver API.
> 
At the moment we don't have such dependency and will continue to steer the design that way.
Only dependency to have is between bus driver (rte_bus_pci_mlx5) and its upper layer class drivers.

> Best,
> --
> Gaëtan