mbox series

[mlx5-next,v1,00/11] Convert mlx5 to use auxiliary bus

Message ID 20201101201542.2027568-1-leon@kernel.org
Headers show
Series Convert mlx5 to use auxiliary bus | expand

Message

Leon Romanovsky Nov. 1, 2020, 8:15 p.m. UTC
From: Leon Romanovsky <leonro@nvidia.com>

Changelog:
v1:
 * Renamed _mlx5_rescan_driver to be mlx5_rescan_driver_locked like in
   other parts of the mlx5 driver.
 * Renamed MLX5_INTERFACE_PROTOCOL_VDPA to tbe MLX5_INTERFACE_PROTOCOL_VNET as
   a preparation to coming series from Eli C.
 * Some small naming renames in mlx5_vdpa.
 * Refactored adev index code to make Parav's SF series to apply more easily.
 * Fixed devlink reload bug that caused to lost TCP connection.
v0:
https://lore.kernel.org/lkml/20201026111849.1035786-1-leon@kernel.org/

--------------------------------------------------------------

Hi,

This patch set converts mlx5 driver to use auxiliary bus [1].

In this series, we are connecting three subsystems (VDPA, netdev and
RDMA) through mlx5_core PCI driver. That driver is responsible to create
proper devices based on supported firmware.

First four patches are preparitions and fixes that were spotted during
code development, rest is the conversion itself.

Thanks

[1]
https://lore.kernel.org/lkml/20201023003338.1285642-1-david.m.ertman@intel.com

Leon Romanovsky (11):
  net/mlx5: Don't skip vport check
  net/mlx5: Properly convey driver version to firmware
  net/mlx5_core: Clean driver version and name
  vdpa/mlx5: Make hardware definitions visible to all mlx5 devices
  net/mlx5: Register mlx5 devices to auxiliary virtual bus
  vdpa/mlx5: Connect mlx5_vdpa to auxiliary bus
  net/mlx5e: Connect ethernet part to auxiliary bus
  RDMA/mlx5: Convert mlx5_ib to use auxiliary bus
  net/mlx5: Delete custom device management logic
  net/mlx5: Simplify eswitch mode check
  RDMA/mlx5: Remove IB representors dead code

 drivers/infiniband/hw/mlx5/counters.c         |   7 -
 drivers/infiniband/hw/mlx5/ib_rep.c           | 113 ++--
 drivers/infiniband/hw/mlx5/ib_rep.h           |  45 +-
 drivers/infiniband/hw/mlx5/main.c             | 148 +++--
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |   4 +-
 .../net/ethernet/mellanox/mlx5/core/Kconfig   |   1 +
 drivers/net/ethernet/mellanox/mlx5/core/dev.c | 567 ++++++++++++------
 .../net/ethernet/mellanox/mlx5/core/devlink.c |   4 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |   4 +-
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 135 ++---
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  42 +-
 .../net/ethernet/mellanox/mlx5/core/en_rep.h  |   6 +-
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |   8 +-
 .../mellanox/mlx5/core/esw/devlink_port.c     |   2 +-
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |  28 +-
 .../mellanox/mlx5/core/eswitch_offloads.c     |   6 +
 .../mellanox/mlx5/core/ipoib/ethtool.c        |   2 +-
 drivers/net/ethernet/mellanox/mlx5/core/lag.c |  58 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    |  50 +-
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |  33 +-
 drivers/vdpa/mlx5/Makefile                    |   2 +-
 drivers/vdpa/mlx5/net/main.c                  |  76 ---
 drivers/vdpa/mlx5/net/mlx5_vnet.c             |  53 +-
 drivers/vdpa/mlx5/net/mlx5_vnet.h             |  24 -
 include/linux/mlx5/driver.h                   |  34 +-
 include/linux/mlx5/eswitch.h                  |   8 +-
 .../linux/mlx5/mlx5_ifc_vdpa.h                |   6 +-
 27 files changed, 818 insertions(+), 648 deletions(-)
 delete mode 100644 drivers/vdpa/mlx5/net/main.c
 delete mode 100644 drivers/vdpa/mlx5/net/mlx5_vnet.h
 rename drivers/vdpa/mlx5/core/mlx5_vdpa_ifc.h => include/linux/mlx5/mlx5_ifc_vdpa.h (97%)

--
2.28.0

Comments

Roi Dayan Nov. 3, 2020, 7:10 a.m. UTC | #1
On 2020-11-01 10:15 PM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>

> 

> Delete dead code.

> 

> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

> ---

>   drivers/infiniband/hw/mlx5/ib_rep.c | 31 +++++++----------------------

>   drivers/infiniband/hw/mlx5/ib_rep.h | 31 -----------------------------

>   2 files changed, 7 insertions(+), 55 deletions(-)

> 

> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c

> index 9810bdd7f3bc..a1a9450ed92c 100644

> --- a/drivers/infiniband/hw/mlx5/ib_rep.c

> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c

> @@ -13,7 +13,7 @@ mlx5_ib_set_vport_rep(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)

>   	struct mlx5_ib_dev *ibdev;

>   	int vport_index;

> 

> -	ibdev = mlx5_ib_get_uplink_ibdev(dev->priv.eswitch);

> +	ibdev = mlx5_eswitch_uplink_get_proto_dev(dev->priv.eswitch, REP_IB);

>   	vport_index = rep->vport_index;

> 

>   	ibdev->port[vport_index].rep = rep;

> @@ -74,6 +74,11 @@ mlx5_ib_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)

>   	return ret;

>   }

> 

> +static void *mlx5_ib_rep_to_dev(struct mlx5_eswitch_rep *rep)

> +{

> +	return rep->rep_data[REP_IB].priv;

> +}

> +

>   static void

>   mlx5_ib_vport_rep_unload(struct mlx5_eswitch_rep *rep)

>   {

> @@ -91,40 +96,18 @@ mlx5_ib_vport_rep_unload(struct mlx5_eswitch_rep *rep)

>   		__mlx5_ib_remove(dev, dev->profile, MLX5_IB_STAGE_MAX);

>   }

> 

> -static void *mlx5_ib_vport_get_proto_dev(struct mlx5_eswitch_rep *rep)

> -{

> -	return mlx5_ib_rep_to_dev(rep);

> -}

> -

>   static const struct mlx5_eswitch_rep_ops rep_ops = {

>   	.load = mlx5_ib_vport_rep_load,

>   	.unload = mlx5_ib_vport_rep_unload,

> -	.get_proto_dev = mlx5_ib_vport_get_proto_dev,

> +	.get_proto_dev = mlx5_ib_rep_to_dev,

>   };

> 

> -struct mlx5_ib_dev *mlx5_ib_get_rep_ibdev(struct mlx5_eswitch *esw,

> -					  u16 vport_num)

> -{

> -	return mlx5_eswitch_get_proto_dev(esw, vport_num, REP_IB);

> -}

> -

>   struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw,

>   					  u16 vport_num)

>   {

>   	return mlx5_eswitch_get_proto_dev(esw, vport_num, REP_ETH);

>   }

> 

> -struct mlx5_ib_dev *mlx5_ib_get_uplink_ibdev(struct mlx5_eswitch *esw)

> -{

> -	return mlx5_eswitch_uplink_get_proto_dev(esw, REP_IB);

> -}

> -

> -struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw,

> -					   u16 vport_num)

> -{

> -	return mlx5_eswitch_vport_rep(esw, vport_num);

> -}

> -

>   struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev,

>   						   struct mlx5_ib_sq *sq,

>   						   u16 port)

> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.h b/drivers/infiniband/hw/mlx5/ib_rep.h

> index 93f562735e89..ce1dcb105dbd 100644

> --- a/drivers/infiniband/hw/mlx5/ib_rep.h

> +++ b/drivers/infiniband/hw/mlx5/ib_rep.h

> @@ -12,11 +12,6 @@

>   extern const struct mlx5_ib_profile raw_eth_profile;

> 

>   #ifdef CONFIG_MLX5_ESWITCH

> -struct mlx5_ib_dev *mlx5_ib_get_rep_ibdev(struct mlx5_eswitch *esw,

> -					  u16 vport_num);

> -struct mlx5_ib_dev *mlx5_ib_get_uplink_ibdev(struct mlx5_eswitch *esw);

> -struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw,

> -					   u16 vport_num);

>   int mlx5r_rep_init(void);

>   void mlx5r_rep_cleanup(void);

>   struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev,

> @@ -25,26 +20,6 @@ struct mlx5_flow_handle *create_flow_rule_vport_sq(struct mlx5_ib_dev *dev,

>   struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw,

>   					  u16 vport_num);

>   #else /* CONFIG_MLX5_ESWITCH */

> -static inline

> -struct mlx5_ib_dev *mlx5_ib_get_rep_ibdev(struct mlx5_eswitch *esw,

> -					  u16 vport_num)

> -{

> -	return NULL;

> -}

> -

> -static inline

> -struct mlx5_ib_dev *mlx5_ib_get_uplink_ibdev(struct mlx5_eswitch *esw)

> -{

> -	return NULL;

> -}

> -

> -static inline

> -struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw,

> -					   u16 vport_num)

> -{

> -	return NULL;

> -}

> -

>   static inline int mlx5r_rep_init(void) { return 0; }

>   static inline void mlx5r_rep_cleanup(void) {}

>   static inline

> @@ -62,10 +37,4 @@ struct net_device *mlx5_ib_get_rep_netdev(struct mlx5_eswitch *esw,

>   	return NULL;

>   }

>   #endif

> -

> -static inline

> -struct mlx5_ib_dev *mlx5_ib_rep_to_dev(struct mlx5_eswitch_rep *rep)

> -{

> -	return rep->rep_data[REP_IB].priv;

> -}

>   #endif /* __MLX5_IB_REP_H__ */

> --

> 2.28.0

> 


Reviewed-by: Roi Dayan <roid@nvidia.com>
Jason Gunthorpe Nov. 3, 2020, 3:45 p.m. UTC | #2
On Sun, Nov 01, 2020 at 10:15:37PM +0200, Leon Romanovsky wrote:
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c

> index 6c218b47b9f1..5316e51e72d4 100644

> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

> @@ -1,18 +1,27 @@

>  // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB

>  /* Copyright (c) 2020 Mellanox Technologies Ltd. */

> 

> +#include <linux/module.h>

>  #include <linux/vdpa.h>

> +#include <linux/vringh.h>

> +#include <uapi/linux/virtio_net.h>

>  #include <uapi/linux/virtio_ids.h>

>  #include <linux/virtio_config.h>

> +#include <linux/auxiliary_bus.h>

> +#include <linux/mlx5/cq.h>

>  #include <linux/mlx5/qp.h>

>  #include <linux/mlx5/device.h>

> +#include <linux/mlx5/driver.h>

>  #include <linux/mlx5/vport.h>

>  #include <linux/mlx5/fs.h>

> -#include <linux/mlx5/device.h>

>  #include <linux/mlx5/mlx5_ifc_vdpa.h>

> -#include "mlx5_vnet.h"

>  #include "mlx5_vdpa.h"

> 

> +MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");

> +MODULE_DESCRIPTION("Mellanox VDPA driver");

> +MODULE_LICENSE("Dual BSD/GPL");

> +

> +#define to_mlx5_vdpa_ndev(__mvdev) container_of(__mvdev, struct mlx5_vdpa_net, mvdev)

>  #define to_mvdev(__vdev) container_of((__vdev), struct mlx5_vdpa_dev, vdev)

> 

>  #define VALID_FEATURES_MASK                                                                        \

> @@ -159,6 +168,11 @@ static bool mlx5_vdpa_debug;

>  			mlx5_vdpa_info(mvdev, "%s\n", #_status);                                   \

>  	} while (0)

> 

> +static inline u32 mlx5_vdpa_max_qps(int max_vqs)

> +{

> +	return max_vqs / 2;

> +}

> +

>  static void print_status(struct mlx5_vdpa_dev *mvdev, u8 status, bool set)

>  {

>  	if (status & ~VALID_STATUS_MASK)

> @@ -1928,8 +1942,11 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)

>  	}

>  }

> 

> -void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)

> +static int mlx5v_probe(struct auxiliary_device *adev,

> +		       const struct auxiliary_device_id *id)

>  {

> +	struct mlx5_adev *madev = container_of(adev, struct mlx5_adev, adev);

> +	struct mlx5_core_dev *mdev = madev->mdev;

>  	struct virtio_net_config *config;

>  	struct mlx5_vdpa_dev *mvdev;

>  	struct mlx5_vdpa_net *ndev;

> @@ -1943,7 +1960,7 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)

>  	ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops,

>  				 2 * mlx5_vdpa_max_qps(max_vqs));

>  	if (IS_ERR(ndev))

> -		return ndev;

> +		return PTR_ERR(ndev);

> 

>  	ndev->mvdev.max_vqs = max_vqs;

>  	mvdev = &ndev->mvdev;

> @@ -1972,7 +1989,8 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)

>  	if (err)

>  		goto err_reg;

> 

> -	return ndev;

> +	dev_set_drvdata(&adev->dev, ndev);

> +	return 0;

> 

>  err_reg:

>  	free_resources(ndev);

> @@ -1981,10 +1999,29 @@ void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev)

>  err_mtu:

>  	mutex_destroy(&ndev->reslock);

>  	put_device(&mvdev->vdev.dev);

> -	return ERR_PTR(err);

> +	return err;

>  }

> 

> -void mlx5_vdpa_remove_dev(struct mlx5_vdpa_dev *mvdev)

> +static int mlx5v_remove(struct auxiliary_device *adev)

>  {

> +	struct mlx5_vdpa_dev *mvdev = dev_get_drvdata(&adev->dev);

> +

>  	vdpa_unregister_device(&mvdev->vdev);

> +	return 0;

>  }

> +

> +static const struct auxiliary_device_id mlx5v_id_table[] = {

> +	{ .name = MLX5_ADEV_NAME ".vnet", },

> +	{},

> +};

> +

> +MODULE_DEVICE_TABLE(auxiliary, mlx5v_id_table);

> +

> +static struct auxiliary_driver mlx5v_driver = {

> +	.name = "vnet",

> +	.probe = mlx5v_probe,

> +	.remove = mlx5v_remove,

> +	.id_table = mlx5v_id_table,

> +};


It is hard to see from the diff, but when this patch is applied the
vdpa module looks like I imagined things would look with the auxiliary
bus. It is very similar in structure to a PCI driver with the probe()
function cleanly registering with its subsystem. This is what I'd like
to see from the new Intel RDMA driver.

Greg, I think this patch is the best clean usage example.

I've looked over this series and it has the right idea and
parts. There is definitely more that can be done to improve mlx5 in
this area, but this series is well scoped and cleans a good part of
it.

Jason
Dan Williams Nov. 4, 2020, 11:21 p.m. UTC | #3
On Tue, Nov 3, 2020 at 7:45 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
[..]
> > +MODULE_DEVICE_TABLE(auxiliary, mlx5v_id_table);

> > +

> > +static struct auxiliary_driver mlx5v_driver = {

> > +     .name = "vnet",

> > +     .probe = mlx5v_probe,

> > +     .remove = mlx5v_remove,

> > +     .id_table = mlx5v_id_table,

> > +};

>

> It is hard to see from the diff, but when this patch is applied the

> vdpa module looks like I imagined things would look with the auxiliary

> bus. It is very similar in structure to a PCI driver with the probe()

> function cleanly registering with its subsystem. This is what I'd like

> to see from the new Intel RDMA driver.

>

> Greg, I think this patch is the best clean usage example.

>

> I've looked over this series and it has the right idea and

> parts. There is definitely more that can be done to improve mlx5 in

> this area, but this series is well scoped and cleans a good part of

> it.


Greg?

I know you alluded to going your own way if the auxiliary bus patches
did not shape up soon, but it seems they have and the stakeholders
have reached this consensus point.

Were there any additional changes you wanted to see happen? I'll go
give the final set another once over, but David has been diligently
fixing up all the declared major issues so I expect to find at most
minor incremental fixups.
Greg Kroah-Hartman Nov. 5, 2020, 7:33 a.m. UTC | #4
On Wed, Nov 04, 2020 at 03:21:23PM -0800, Dan Williams wrote:
> On Tue, Nov 3, 2020 at 7:45 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> [..]
> > > +MODULE_DEVICE_TABLE(auxiliary, mlx5v_id_table);
> > > +
> > > +static struct auxiliary_driver mlx5v_driver = {
> > > +     .name = "vnet",
> > > +     .probe = mlx5v_probe,
> > > +     .remove = mlx5v_remove,
> > > +     .id_table = mlx5v_id_table,
> > > +};
> >
> > It is hard to see from the diff, but when this patch is applied the
> > vdpa module looks like I imagined things would look with the auxiliary
> > bus. It is very similar in structure to a PCI driver with the probe()
> > function cleanly registering with its subsystem. This is what I'd like
> > to see from the new Intel RDMA driver.
> >
> > Greg, I think this patch is the best clean usage example.
> >
> > I've looked over this series and it has the right idea and
> > parts. There is definitely more that can be done to improve mlx5 in
> > this area, but this series is well scoped and cleans a good part of
> > it.
> 
> Greg?
> 
> I know you alluded to going your own way if the auxiliary bus patches
> did not shape up soon, but it seems they have and the stakeholders
> have reached this consensus point.
> 
> Were there any additional changes you wanted to see happen? I'll go
> give the final set another once over, but David has been diligently
> fixing up all the declared major issues so I expect to find at most
> minor incremental fixups.

This is in my to-review pile, along with a load of other stuff at the
moment:
	$ ~/bin/mdfrm -c ~/mail/todo/
	1709 messages in /home/gregkh/mail/todo/

So give me a chance.  There is no rush on my side for this given the
huge delays that have happened here on the authorship side many times in
the past :)

If you can review it, or anyone else, that is always most appreciated.

thanks,

greg k-h
Dan Williams Nov. 5, 2020, 7:49 a.m. UTC | #5
On Wed, Nov 4, 2020 at 11:32 PM gregkh <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Nov 04, 2020 at 03:21:23PM -0800, Dan Williams wrote:
> > On Tue, Nov 3, 2020 at 7:45 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > [..]
> > > > +MODULE_DEVICE_TABLE(auxiliary, mlx5v_id_table);
> > > > +
> > > > +static struct auxiliary_driver mlx5v_driver = {
> > > > +     .name = "vnet",
> > > > +     .probe = mlx5v_probe,
> > > > +     .remove = mlx5v_remove,
> > > > +     .id_table = mlx5v_id_table,
> > > > +};
> > >
> > > It is hard to see from the diff, but when this patch is applied the
> > > vdpa module looks like I imagined things would look with the auxiliary
> > > bus. It is very similar in structure to a PCI driver with the probe()
> > > function cleanly registering with its subsystem. This is what I'd like
> > > to see from the new Intel RDMA driver.
> > >
> > > Greg, I think this patch is the best clean usage example.
> > >
> > > I've looked over this series and it has the right idea and
> > > parts. There is definitely more that can be done to improve mlx5 in
> > > this area, but this series is well scoped and cleans a good part of
> > > it.
> >
> > Greg?
> >
> > I know you alluded to going your own way if the auxiliary bus patches
> > did not shape up soon, but it seems they have and the stakeholders
> > have reached this consensus point.
> >
> > Were there any additional changes you wanted to see happen? I'll go
> > give the final set another once over, but David has been diligently
> > fixing up all the declared major issues so I expect to find at most
> > minor incremental fixups.
>
> This is in my to-review pile, along with a load of other stuff at the
> moment:
>         $ ~/bin/mdfrm -c ~/mail/todo/
>         1709 messages in /home/gregkh/mail/todo/
>
> So give me a chance.  There is no rush on my side for this given the
> huge delays that have happened here on the authorship side many times in
> the past :)

Sure, I was more looking to confirm that it's worth continuing to
polish this set given your mention of possibly going a different
direction.

> If you can review it, or anyone else, that is always most appreciated.

Thanks, will do.
Jason Gunthorpe Nov. 5, 2020, 4:47 p.m. UTC | #6
On Thu, Nov 05, 2020 at 08:33:02AM +0100, gregkh wrote:
> > Were there any additional changes you wanted to see happen? I'll go
> > give the final set another once over, but David has been diligently
> > fixing up all the declared major issues so I expect to find at most
> > minor incremental fixups.
> 
> This is in my to-review pile, along with a load of other stuff at the
> moment:
> 	$ ~/bin/mdfrm -c ~/mail/todo/
> 	1709 messages in /home/gregkh/mail/todo/
> 
> So give me a chance.  There is no rush on my side for this given the
> huge delays that have happened here on the authorship side many times in
> the past :)

On the other hand Leon and his team did invest alot of time and
effort, very quickly, to build and QA this large mlx5 series here to
give a better/second example as you requested only a few weeks ago.

> If you can review it, or anyone else, that is always most appreciated.

Dan, Leon, Myself and others have looked at the auxiliary bus patch a
more than a few times now. Leon in particular went over it very
carefully and a number of bugs were fixed while he developed this
series.

There seems to be nothing fundamentally wrong with it, so long as
people are fine with the colour of the shed...

Jason
Greg Kroah-Hartman Nov. 5, 2020, 4:57 p.m. UTC | #7
On Thu, Nov 05, 2020 at 12:47:38PM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 05, 2020 at 08:33:02AM +0100, gregkh wrote:
> > > Were there any additional changes you wanted to see happen? I'll go
> > > give the final set another once over, but David has been diligently
> > > fixing up all the declared major issues so I expect to find at most
> > > minor incremental fixups.
> > 
> > This is in my to-review pile, along with a load of other stuff at the
> > moment:
> > 	$ ~/bin/mdfrm -c ~/mail/todo/
> > 	1709 messages in /home/gregkh/mail/todo/
> > 
> > So give me a chance.  There is no rush on my side for this given the
> > huge delays that have happened here on the authorship side many times in
> > the past :)
> 
> On the other hand Leon and his team did invest alot of time and
> effort, very quickly, to build and QA this large mlx5 series here to
> give a better/second example as you requested only a few weeks ago.

Leon and his team have done a great job, and I never said otherwise.

greg k-h
Saeed Mahameed Nov. 5, 2020, 8:31 p.m. UTC | #8
On Sun, 2020-11-01 at 22:15 +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>

> 

> Move mlx5_vdpa IFC header file to the general include folder, so

> mlx5_core will be able to reuse it to check if VDPA is supported

> prior to creating an auxiliary device.

> 


I don't really like this, the whole idea of aux devices is that they
get to do own logic and hide details, now we are exposing aux specific
stuff to the bus .. 
let's figure a way to avoid such exposure as we discussed yesterday.

is_supported check shouldn't belong to mlx5_core and each aux device
(en/ib/vdpa) should implement own is_supported op and keep the details
hidden in the aux driver like it was before this patch.
Jason Gunthorpe Nov. 5, 2020, 8:36 p.m. UTC | #9
On Thu, Nov 05, 2020 at 12:31:52PM -0800, Saeed Mahameed wrote:
> On Sun, 2020-11-01 at 22:15 +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Move mlx5_vdpa IFC header file to the general include folder, so
> > mlx5_core will be able to reuse it to check if VDPA is supported
> > prior to creating an auxiliary device.
> > 
> 
> I don't really like this, the whole idea of aux devices is that they
> get to do own logic and hide details, now we are exposing aux
> specific stuff to the bus ..  let's figure a way to avoid such
> exposure as we discussed yesterday.

Not quite, the idea is we get to have a cleaner split between the two
sides.

The device side is responsible for things centric to the device, like
"does this device actually exists" which is what is_supported is
doing.

The driver side holds the driver specific logic.

> is_supported check shouldn't belong to mlx5_core and each aux device
> (en/ib/vdpa) should implement own is_supported op and keep the details
> hidden in the aux driver like it was before this patch.

No, it really should be in the device side.

Part of the point here is to properly fix module loading. That means
the core driver must only create devices that can actually have a
driver bound to them because creating a device triggers module
loading.

For instance we do not want to auto load vdpa modules on every mlx5
system for no reason, that is not clean at all.

Jason
Leon Romanovsky Nov. 6, 2020, 7:05 a.m. UTC | #10
On Thu, Nov 05, 2020 at 04:36:57PM -0400, Jason Gunthorpe wrote:
> On Thu, Nov 05, 2020 at 12:31:52PM -0800, Saeed Mahameed wrote:

> > On Sun, 2020-11-01 at 22:15 +0200, Leon Romanovsky wrote:

> > > From: Leon Romanovsky <leonro@nvidia.com>

> > >

> > > Move mlx5_vdpa IFC header file to the general include folder, so

> > > mlx5_core will be able to reuse it to check if VDPA is supported

> > > prior to creating an auxiliary device.

> > >

> >

> > I don't really like this, the whole idea of aux devices is that they

> > get to do own logic and hide details, now we are exposing aux

> > specific stuff to the bus ..  let's figure a way to avoid such

> > exposure as we discussed yesterday.

>

> Not quite, the idea is we get to have a cleaner split between the two

> sides.

>

> The device side is responsible for things centric to the device, like

> "does this device actually exists" which is what is_supported is

> doing.

>

> The driver side holds the driver specific logic.

>

> > is_supported check shouldn't belong to mlx5_core and each aux device

> > (en/ib/vdpa) should implement own is_supported op and keep the details

> > hidden in the aux driver like it was before this patch.

>

> No, it really should be in the device side.

>

> Part of the point here is to properly fix module loading. That means

> the core driver must only create devices that can actually have a

> driver bound to them because creating a device triggers module

> loading.

>

> For instance we do not want to auto load vdpa modules on every mlx5

> system for no reason, that is not clean at all.


Saeed,

Jason gave very good example and it is not far from the real life requirement.
We have an internal task to make sure that mlx5_vdpa is loaded without any
other mlx5_* modules (ib and eth). This series solves it naturally.

Thanks

>

> Jason