On Sun, Dec 09, 2018 at 08:07:15PM +0200, Leon Romanovsky wrote:
> 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.
The build_bug_on takes care of the NULL issue.
> 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.
No, this is a kref'd structure with the core code calling kfree, so it
is saner for the core code to control the allocation.
In both cases the ibdev must be at offset 0, and really, it still
needs the macro to enforce that..
Jason
