On Sun, Dec 09, 2018 at 07:01:15PM +0200, Leon Romanovsky wrote:
> On Sun, Dec 09, 2018 at 09:45:46AM -0700, Jason Gunthorpe wrote:
> > On Sun, Dec 09, 2018 at 10:56:46AM +0200, Leon Romanovsky wrote:
> > > On Thu, Dec 06, 2018 at 08:46:42PM -0700, Jason Gunthorpe wrote:
> > > > 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.
> > >
> > > I'm preparing v1 now and not sure how to handle this comment.
> > > First, it wasn't part of this patch, second (more important),
> > > ibdev is declared as pointer of mlx5_ib_dev, so unclear how
> > > do you see container_of be used here with uninitialized ibdev.
> >
> > ibdev = container_of(ib_alloc_device(sizeof(*ibdev)), struct
> > mlx6_ib_dev, ib_dev);
> >
> > And this whole pattern really should be a standard macro:
>
> Interesting, I worked on something similar right now, except the fact
> that ib_alloc_device() can return NULL.
>
I actually think that the cleanest solution will be to move kzalloc to
the drivers and change ib_alloc_device (need to rename too). It will
allow us to get rid of IB wrapper and make device allocation scheme
be similar to other part of kernel.
Are you ok with this approach?
Something like this:
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 5a8f5d86f4de..e8f0431d4b5d 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -309,17 +309,8 @@ void rdma_init_coredev(struct ib_core_device *coredev, struct ib_device *dev)
* ib_dealloc_device() must be used to free structures allocated with
* ib_alloc_device().
*/
-struct ib_device *ib_alloc_device(size_t size)
+void ib_alloc_device(struct ib_device *device)
{
- struct ib_device *device;
-
- if (WARN_ON(size < sizeof(struct ib_device)))
- return NULL;
-
- device = kzalloc(size, GFP_KERNEL);
- if (!device)
- return NULL;
-
device->groups[0] = &ib_dev_attr_group;
rdma_init_coredev(&device->coredev, device);
rdma_dev_net_set(&device->coredev, &init_net);
@@ -331,8 +322,6 @@ struct ib_device *ib_alloc_device(size_t size)
refcount_set(&device->refcount, 1);
init_completion(&device->unreg_completion);
rdma_restrack_init(&device->res);
-
- return device;
}
EXPORT_SYMBOL(ib_alloc_device);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 3477fa1b2289..84dd1a526eb2 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -6453,10 +6453,12 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
if (mlx5_core_is_mp_slave(mdev) && ll == IB_LINK_LAYER_ETHERNET)
return mlx5_ib_add_slave_port(mdev);
- dev = (struct mlx5_ib_dev *)ib_alloc_device(sizeof(*dev));
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
return NULL;
+ ib_alloc_device(&dev->ib_dev);
+
dev->mdev = mdev;
dev->num_ports = max(MLX5_CAP_GEN(mdev, num_ports),
MLX5_CAP_GEN(mdev, num_vhca_ports));
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index ff970f89cbbe..2d5e463cd068 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2684,7 +2684,7 @@ struct ib_client {
struct list_head list;
};
-struct ib_device *ib_alloc_device(size_t size);
+void ib_alloc_device(struct ib_device *device);
void ib_dealloc_device(struct ib_device *device);
void ib_get_device_fw_str(struct ib_device *device, char *str);> >
> > #define ib_alloc_device_t(_struct, _member) \
> > container_of(ib_alloc_device(sizeof(_struct) + \
> > BUILD_BUG_ON(offsetof(_struct, _member) != 0)), _struct, _member)
> >
> > Jason
Attachment:
signature.asc
Description: PGP signature
