diff mbox series

[mlx5-next,v1,04/11] vdpa/mlx5: Make hardware definitions visible to all mlx5 devices

Message ID 20201101201542.2027568-5-leon@kernel.org (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [mlx5-next,v1,01/11] net/mlx5: Don't skip vport check | expand

Commit Message

Leon Romanovsky Nov. 1, 2020, 8:15 p.m. UTC
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.

As part of this move, update the header file name to mlx5 general
naming scheme.

Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/vdpa/mlx5/net/main.c                                | 2 +-
 drivers/vdpa/mlx5/net/mlx5_vnet.c                           | 2 +-
 .../mlx5_vdpa_ifc.h => include/linux/mlx5/mlx5_ifc_vdpa.h   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename drivers/vdpa/mlx5/core/mlx5_vdpa_ifc.h => include/linux/mlx5/mlx5_ifc_vdpa.h (97%)

--
2.28.0

Comments

Saeed Mahameed Nov. 5, 2020, 8:31 p.m. UTC | #1
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 | #2
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 | #3
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
diff mbox series

Patch

diff --git a/drivers/vdpa/mlx5/net/main.c b/drivers/vdpa/mlx5/net/main.c
index 838cd98386ff..4dd3f00f2306 100644
--- a/drivers/vdpa/mlx5/net/main.c
+++ b/drivers/vdpa/mlx5/net/main.c
@@ -4,7 +4,7 @@ 
 #include <linux/module.h>
 #include <linux/mlx5/driver.h>
 #include <linux/mlx5/device.h>
-#include "mlx5_vdpa_ifc.h"
+#include <linux/mlx5/mlx5_ifc_vdpa.h>
 #include "mlx5_vnet.h"

 MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 1fa6fcac8299..6c218b47b9f1 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -9,8 +9,8 @@ 
 #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_ifc.h"
 #include "mlx5_vdpa.h"

 #define to_mvdev(__vdev) container_of((__vdev), struct mlx5_vdpa_dev, vdev)
diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa_ifc.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
similarity index 97%
rename from drivers/vdpa/mlx5/core/mlx5_vdpa_ifc.h
rename to include/linux/mlx5/mlx5_ifc_vdpa.h
index f6f57a29b38e..97784098a992 100644
--- a/drivers/vdpa/mlx5/core/mlx5_vdpa_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -1,8 +1,8 @@ 
 /* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
 /* Copyright (c) 2020 Mellanox Technologies Ltd. */

-#ifndef __MLX5_VDPA_IFC_H_
-#define __MLX5_VDPA_IFC_H_
+#ifndef __MLX5_IFC_VDPA_H_
+#define __MLX5_IFC_VDPA_H_

 #include <linux/mlx5/mlx5_ifc.h>

@@ -165,4 +165,4 @@  struct mlx5_ifc_modify_virtio_net_q_out_bits {
 	struct mlx5_ifc_general_obj_out_cmd_hdr_bits general_obj_out_cmd_hdr;
 };

-#endif /* __MLX5_VDPA_IFC_H_ */
+#endif /* __MLX5_IFC_VDPA_H_ */