| Message ID | 20201101201542.2027568-7-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 |
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
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.
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
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.
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
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
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c index 8ddf469b2d05..76eb09f4adba 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c @@ -31,6 +31,7 @@ */ #include <linux/mlx5/driver.h> +#include <linux/mlx5/mlx5_ifc_vdpa.h> #include "mlx5_core.h" static LIST_HEAD(intf_list); @@ -51,10 +52,35 @@ enum { MLX5_INTERFACE_ATTACHED, }; +static bool is_vnet_supported(struct mlx5_core_dev *dev) +{ + if (!IS_ENABLED(CONFIG_MLX5_VDPA_NET)) + return false; + + if (mlx5_core_is_pf(dev)) + return false; + + if (!(MLX5_CAP_GEN_64(dev, general_obj_types) & + MLX5_GENERAL_OBJ_TYPES_CAP_VIRTIO_NET_Q)) + return false; + + if (!(MLX5_CAP_DEV_VDPA_EMULATION(dev, event_mode) & + MLX5_VIRTIO_Q_EVENT_MODE_QP_MODE)) + return false; + + if (!MLX5_CAP_DEV_VDPA_EMULATION(dev, eth_frame_offload_type)) + return false; + + return true; +} + static const struct mlx5_adev_device { const char *suffix; bool (*is_supported)(struct mlx5_core_dev *dev); -} mlx5_adev_devices[1] = {}; +} mlx5_adev_devices[] = { + [MLX5_INTERFACE_PROTOCOL_VDPA] = { .suffix = "vnet", + .is_supported = &is_vnet_supported }, +}; int mlx5_adev_idx_alloc(void) { diff --git a/drivers/vdpa/mlx5/Makefile b/drivers/vdpa/mlx5/Makefile index 89a5bededc9f..f717978c83bf 100644 --- a/drivers/vdpa/mlx5/Makefile +++ b/drivers/vdpa/mlx5/Makefile @@ -1,4 +1,4 @@ subdir-ccflags-y += -I$(srctree)/drivers/vdpa/mlx5/core obj-$(CONFIG_MLX5_VDPA_NET) += mlx5_vdpa.o -mlx5_vdpa-$(CONFIG_MLX5_VDPA_NET) += net/main.o net/mlx5_vnet.o core/resources.o core/mr.o +mlx5_vdpa-$(CONFIG_MLX5_VDPA_NET) += net/mlx5_vnet.o core/resources.o core/mr.o diff --git a/drivers/vdpa/mlx5/net/main.c b/drivers/vdpa/mlx5/net/main.c deleted file mode 100644 index 4dd3f00f2306..000000000000 --- a/drivers/vdpa/mlx5/net/main.c +++ /dev/null @@ -1,76 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB -/* Copyright (c) 2020 Mellanox Technologies Ltd. */ - -#include <linux/module.h> -#include <linux/mlx5/driver.h> -#include <linux/mlx5/device.h> -#include <linux/mlx5/mlx5_ifc_vdpa.h> -#include "mlx5_vnet.h" - -MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>"); -MODULE_DESCRIPTION("Mellanox VDPA driver"); -MODULE_LICENSE("Dual BSD/GPL"); - -static bool required_caps_supported(struct mlx5_core_dev *mdev) -{ - u8 event_mode; - u64 got; - - got = MLX5_CAP_GEN_64(mdev, general_obj_types); - - if (!(got & MLX5_GENERAL_OBJ_TYPES_CAP_VIRTIO_NET_Q)) - return false; - - event_mode = MLX5_CAP_DEV_VDPA_EMULATION(mdev, event_mode); - if (!(event_mode & MLX5_VIRTIO_Q_EVENT_MODE_QP_MODE)) - return false; - - if (!MLX5_CAP_DEV_VDPA_EMULATION(mdev, eth_frame_offload_type)) - return false; - - return true; -} - -static void *mlx5_vdpa_add(struct mlx5_core_dev *mdev) -{ - struct mlx5_vdpa_dev *vdev; - - if (mlx5_core_is_pf(mdev)) - return NULL; - - if (!required_caps_supported(mdev)) { - dev_info(mdev->device, "virtio net emulation not supported\n"); - return NULL; - } - vdev = mlx5_vdpa_add_dev(mdev); - if (IS_ERR(vdev)) - return NULL; - - return vdev; -} - -static void mlx5_vdpa_remove(struct mlx5_core_dev *mdev, void *context) -{ - struct mlx5_vdpa_dev *vdev = context; - - mlx5_vdpa_remove_dev(vdev); -} - -static struct mlx5_interface mlx5_vdpa_interface = { - .add = mlx5_vdpa_add, - .remove = mlx5_vdpa_remove, - .protocol = MLX5_INTERFACE_PROTOCOL_VDPA, -}; - -static int __init mlx5_vdpa_init(void) -{ - return mlx5_register_interface(&mlx5_vdpa_interface); -} - -static void __exit mlx5_vdpa_exit(void) -{ - mlx5_unregister_interface(&mlx5_vdpa_interface); -} - -module_init(mlx5_vdpa_init); -module_exit(mlx5_vdpa_exit); diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index 6c218b47b9f1..5316e51e72d4 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ 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, +}; + +module_auxiliary_driver(mlx5v_driver); diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.h b/drivers/vdpa/mlx5/net/mlx5_vnet.h deleted file mode 100644 index f2d6d68b020e..000000000000 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.h +++ /dev/null @@ -1,24 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */ -/* Copyright (c) 2020 Mellanox Technologies Ltd. */ - -#ifndef __MLX5_VNET_H_ -#define __MLX5_VNET_H_ - -#include <linux/vdpa.h> -#include <linux/virtio_net.h> -#include <linux/vringh.h> -#include <linux/mlx5/driver.h> -#include <linux/mlx5/cq.h> -#include <linux/mlx5/qp.h> -#include "mlx5_vdpa.h" - -static inline u32 mlx5_vdpa_max_qps(int max_vqs) -{ - return max_vqs / 2; -} - -#define to_mlx5_vdpa_ndev(__mvdev) container_of(__mvdev, struct mlx5_vdpa_net, mvdev) -void *mlx5_vdpa_add_dev(struct mlx5_core_dev *mdev); -void mlx5_vdpa_remove_dev(struct mlx5_vdpa_dev *mvdev); - -#endif /* __MLX5_VNET_H_ */