On Sun, Dec 09, 2018 at 09:06:45PM +0200, Leon Romanovsky wrote:
> On Sun, Dec 09, 2018 at 11:11:20AM -0700, Jason Gunthorpe wrote:
> > 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 doubt about that. NULL will be returned at the runtime if kmalloc fails,
> but build_bug_on is checked in compilation phase.
>
> >
> > > 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..
>
> I saw it, but disliked the aesthetic side of your proposal.
Actually, the kfree is performed in ib_device_release() and not in
ib_dealloc_device() as sane and symmetrical core code would do.
So, I still think that the right approach is to allow allocation by the
driver, but management and release by IB/core.
Thanks
>
> >
> > Jason
Attachment:
signature.asc
Description: PGP signature
