RE: [RFC net-next 8/8] net/mlx5: Add subdev driver to bind to subdev devices

[Parav Pandit]

Hi Saeed,

> -----Original Message-----
> From: Saeed Mahameed
> Sent: Friday, March 1, 2019 4:12 PM
> To: Jiri Pirko <jiri@xxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Parav Pandit <parav@xxxxxxxxxxxx>;
> davem@xxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> michal.lkml@xxxxxxxxxxx
> Subject: Re: [RFC net-next 8/8] net/mlx5: Add subdev driver to bind to
> subdev devices
> 
> On Thu, 2019-02-28 at 23:37 -0600, Parav Pandit wrote:
> > Add a subdev driver to probe the subdev devices and create fake
> > netdevice for it.
> >
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/mellanox/mlx5/core/Makefile   |  2 +-
> >  drivers/net/ethernet/mellanox/mlx5/core/main.c     |  8 +-
> >  .../net/ethernet/mellanox/mlx5/core/mlx5_core.h    |  3 +
> >  .../ethernet/mellanox/mlx5/core/subdev_driver.c    | 93
> > ++++++++++++++++++++++
> >  4 files changed, 104 insertions(+), 2 deletions(-)  create mode
> > 100644 drivers/net/ethernet/mellanox/mlx5/core/subdev_driver.c
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> > b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> > index f218789..c8aeaf1 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> > @@ -16,7 +16,7 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o
> > eq.o uar.o pagealloc.o \
> >  		transobj.o vport.o sriov.o fs_cmd.o fs_core.o \
> >  		fs_counters.o rl.o lag.o dev.o events.o wq.o lib/gid.o \
> >  		lib/devcom.o diag/fs_tracepoint.o diag/fw_tracer.o
> > -mlx5_core-$(CONFIG_SUBDEV) += subdev.o
> > +mlx5_core-$(CONFIG_SUBDEV) += subdev.o subdev_driver.o
> >
> >  #
> >  # Netdev basic
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 5f8cf0d..7dfa8c4 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -1548,7 +1548,11 @@ static int __init init(void)
> >  	mlx5e_init();
> >  #endif
> >
> > -	return 0;
> > +	err = subdev_register_driver(&mlx5_subdev_driver);
> > +	if (err)
> > +		pci_unregister_driver(&mlx5_core_driver);
> > +
> > +	return err;
> >
> >  err_debug:
> >  	mlx5_unregister_debugfs();
> > @@ -1557,6 +1561,8 @@ static int __init init(void)
> >
> >  static void __exit cleanup(void)
> >  {
> > +	subdev_unregister_driver(&mlx5_subdev_driver);
> > +
> >  #ifdef CONFIG_MLX5_CORE_EN
> >  	mlx5e_cleanup();
> >  #endif
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> > b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> > index 2a54148..1b733c7 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
> > @@ -41,12 +41,15 @@
> >  #include <linux/ptp_clock_kernel.h>
> >  #include <linux/mlx5/cq.h>
> >  #include <linux/mlx5/fs.h>
> > +#include <linux/subdev_bus.h>
> >
> >  #define DRIVER_NAME "mlx5_core"
> >  #define DRIVER_VERSION "5.0-0"
> >
> >  extern uint mlx5_core_debug_mask;
> >
> > +extern struct subdev_driver mlx5_subdev_driver;
> > +
> >  #define mlx5_core_dbg(__dev, format, ...)
> > \
> >  	dev_dbg(&(__dev)->pdev->dev, "%s:%d:(pid %d): " format,
> 
> > \
> >  		 __func__, __LINE__, current->pid,
> > \
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/subdev_driver.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/subdev_driver.c
> > new file mode 100644
> > index 0000000..880aa4f
> > --- /dev/null
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/subdev_driver.c
> > @@ -0,0 +1,93 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2018-19 Mellanox Technologies
> > +
> > +#include <linux/module.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/subdev_bus.h>
> > +#include <linux/subdev_ids.h>
> > +#include <linux/etherdevice.h>
> > +
> > +struct mlx5_subdev_ndev {
> > +	struct net_device ndev;
> > +};
> > +
> > +static void mlx5_dma_test(struct device *dev) {
> > +	dma_addr_t pa;
> > +	void *va;
> > +
> > +	va = dma_alloc_coherent(dev, 4096, &pa, GFP_KERNEL);
> > +	if (va)
> > +		dma_free_coherent(dev, 4096, va, pa); }
> > +
> > +static struct net_device *ndev;
> > +
> > +static int mlx5e_subdev_open(struct net_device *netdev) {
> > +	return 0;
> > +}
> > +
> > +static int mlx5e_subdev_close(struct net_device *netdev) {
> > +	return 0;
> > +}
> > +
> > +static netdev_tx_t
> > +mlx5e_subdev_xmit(struct sk_buff *skb, struct net_device *netdev) {
> > +	return NETDEV_TX_BUSY;
> > +}
> > +
> > +const struct net_device_ops mlx5e_subdev_netdev_ops = {
> > +	.ndo_open                = mlx5e_subdev_open,
> > +	.ndo_stop                = mlx5e_subdev_close,
> > +	.ndo_start_xmit          = mlx5e_subdev_xmit,
> > +};
> > +
> > +static int mlx5_subdev_probe(struct device *dev) {
> > +	int err;
> > +
> > +	mlx5_dma_test(dev);
> 
> Hi Parav, can you please shed some light on how do you plan to
> communicate with the parent device ? (pci_dev and its running driver
> instance), We will need to share some resources, such as IRQs/BARs/etc ..,
> and maybe some HW objects which are going to be managed by the parent
> pci device driver.
> 
Since mlx5 driver works on its pci device, in mlx5_subdev_probe(struct device *device)
device->parent is a PCI device for driver to use.

> Just allocating a dma buffer doesn't mean anything, the dma buffer is just
> bound to the generic device.
>

dma buffer allocation is just to make sure that stack and core and rdma ULPs dma allocations in same way as PCI device.

 > > +	/* Only one device supported in rfc */
> > +	if (ndev)
> > +		return 0;
> > +
> > +	ndev = alloc_etherdev_mqs(sizeof(struct mlx5_subdev_ndev), 1,
> > 1);
> > +	if (!ndev)
> > +		return -ENOMEM;
> > +
> > +	SET_NETDEV_DEV(ndev, dev);
> > +	ndev->netdev_ops = &mlx5e_subdev_netdev_ops;
> > +	err = register_netdev(ndev);
> > +	if (err) {
> > +		free_netdev(ndev);
> > +		ndev = NULL;
> > +	}
> > +	return err;
> > +}
> > +
> > +static int mlx5_subdev_remove(struct device *dev) {
> > +	if (ndev) {
> > +		unregister_netdev(ndev);
> > +		free_netdev(ndev);
> > +		ndev = NULL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static const struct subdev_id mlx5_subdev_id_table[] = {
> > +	{ .vendor_id = SUBDEV_VENDOR_ID_MELLANOX,
> > +	  .device_id = SUBDEV_DEVICE_ID_MELLANOX_SF },
> > +	{ 0, }
> > +};
> > +MODULE_DEVICE_TABLE(subdev, mlx5_subdev_id_table);
> > +
> > +struct subdev_driver mlx5_subdev_driver = {
> > +	.id_table = mlx5_subdev_id_table,
> > +	.driver.name = "mlx5_subdev_driver",
> > +	.driver.probe = mlx5_subdev_probe,
> > +	.driver.remove = mlx5_subdev_remove, };