linux-kernel - Re: [PATCH mlx5-next v1 06/11] vdpa/mlx5: Connect mlx5

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201103154525.GO36674@ziepe.ca>
Date:   Tue, 3 Nov 2020 11:45:25 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Leon Romanovsky <leon@...nel.org>,
        gregkh <gregkh@...uxfoundation.org>
Cc:     Doug Ledford <dledford@...hat.com>,
        Leon Romanovsky <leonro@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jason Wang <jasowang@...hat.com>, linux-rdma@...r.kernel.org,
        "Michael S. Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org,
        Parav Pandit <parav@...dia.com>, Roi Dayan <roid@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        virtualization@...ts.linux-foundation.org,
        alsa-devel@...a-project.org, tiwai@...e.de, broonie@...nel.org,
        "David S . Miller" <davem@...emloft.net>,
        ranjani.sridharan@...ux.intel.com,
        pierre-louis.bossart@...ux.intel.com, fred.oh@...ux.intel.com,
        shiraz.saleem@...el.com, dan.j.williams@...el.com,
        kiran.patil@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH mlx5-next v1 06/11] vdpa/mlx5: Connect mlx5_vdpa to
 auxiliary bus

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@...lanox.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