On Wed, Dec 05, 2018 at 04:06:12PM +0200, Leon Romanovsky wrote:
> From: Saeed Mahameed <saeedm@xxxxxxxxxxxx>
>
> __mlx5_ib_add didn't allocate ib_device, this means __mlx5_ib_remove
> shouldn't free it, hence make the alloc/dealloc symmetric.
>
> Put dealloc_device outside __mlx5_ib_remove and convert __mlx5_ib_add
> to return int value rather than dev pointer on success and null on failure.
>
> Signed-off-by: Saeed Mahameed <saeedm@xxxxxxxxxxxx>
> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> drivers/infiniband/hw/mlx5/ib_rep.c | 9 +++++++--
> drivers/infiniband/hw/mlx5/main.c | 23 +++++++++++++++--------
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 ++--
> 3 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> index 8a682d86d634..5ee383b0fc32 100644
> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> @@ -61,6 +61,7 @@ static int
> mlx5_ib_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
> {
> struct mlx5_ib_dev *ibdev;
> + int err;
>
> ibdev = (struct mlx5_ib_dev *)ib_alloc_device(sizeof(*ibdev));
This should be container_of not a cast.
> if (!ibdev)
> @@ -70,8 +71,11 @@ mlx5_ib_vport_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
> ibdev->mdev = dev;
> ibdev->num_ports = max(MLX5_CAP_GEN(dev, num_ports),
> MLX5_CAP_GEN(dev, num_vhca_ports));
> - if (!__mlx5_ib_add(ibdev, &rep_profile))
> - return -EINVAL;
> + err = __mlx5_ib_add(ibdev, &rep_profile);
> + if (err) {
> + ib_dealloc_device((struct ib_device *)dev);
What kind of crazy cast is this? 'dev' is a mlx5_core_dev so how on
earth is this right? Shouldn't it be just &ibdev->ib_dev?
Every occurance of (struct ib_device *) in this patch looks like
something I don't want to see. Use the proper cast-free things please.
Jason
