On Tue, Sep 18, 2018 at 06:41:30AM +0000, Parav Pandit wrote:
>
>
> > -----Original Message-----
> > From: Jason Gunthorpe
> > Sent: Monday, September 17, 2018 4:45 PM
> > To: linux-rdma@xxxxxxxxxxxxxxx; Parav Pandit <parav@xxxxxxxxxxxx>
> > Subject: [PATCH] RDMA/uverbs: Fix error unwind in ib_uverbs_add_one
> >
> > The error path has several mistakes
> >
> > - cdev_del should not be called if cdev_device_add fails
> > - We must call put_device on all the goto exit paths
> > as that is what frees the uapi, SRCU and the struct itself.
> >
> > Fixes: c5c4d92e70f3 ("RDMA/uverbs: Use cdev_device_add() instead of
> > cdev_add()")
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > ---
> > drivers/infiniband/core/uverbs_main.c | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > Parav, yes?
> >
> Looks good.
>
> > diff --git a/drivers/infiniband/core/uverbs_main.c
> > b/drivers/infiniband/core/uverbs_main.c
> > index 16e5f714ca536e..ed8db441adb90e 100644
> > --- a/drivers/infiniband/core/uverbs_main.c
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -1031,6 +1031,11 @@ static void ib_uverbs_add_one(struct ib_device
> > *device)
> > return;
> > }
> >
> > + device_initialize(&uverbs_dev->dev);
> > + uverbs_dev->dev.class = uverbs_class;
> > + uverbs_dev->dev.parent = device->dev.parent;
> > + uverbs_dev->dev.release = ib_uverbs_release_dev;
> > +
> Can you please move below two lines also here so that most static fields gets initialized together and its easy to read.
> uverbs_dev->groups[0] = &dev_attr_group;
> uverbs_dev->dev.groups = uverbs_dev->groups;
>
> > atomic_set(&uverbs_dev->refcount, 1);
> > init_completion(&uverbs_dev->comp);
> > uverbs_dev->xrcd_tree = RB_ROOT;
> > @@ -1055,11 +1060,7 @@ static void ib_uverbs_add_one(struct ib_device
> > *device)
> > if (ib_uverbs_create_uapi(device, uverbs_dev))
> > goto err_uapi;
> >
> > - device_initialize(&uverbs_dev->dev);
> > - uverbs_dev->dev.class = uverbs_class;
> > - uverbs_dev->dev.parent = device->dev.parent;
> > uverbs_dev->dev.devt = base;
> > - uverbs_dev->dev.release = ib_uverbs_release_dev;
> > uverbs_dev->groups[0] = &dev_attr_group;
> > uverbs_dev->dev.groups = uverbs_dev->groups;
> > dev_set_name(&uverbs_dev->dev, "uverbs%d", uverbs_dev-
> > >devnum); @@ -1070,20 +1071,18 @@ static void ib_uverbs_add_one(struct
> > ib_device *device)
> >
> > ret = cdev_device_add(&uverbs_dev->cdev, &uverbs_dev->dev);
> > if (ret)
> > - goto err_cdev;
> > + goto err_uapi;
> >
> > ib_set_client_data(device, &uverbs_client, uverbs_dev);
> > return;
> >
> > -err_cdev:
> > - cdev_del(&uverbs_dev->cdev);
> > - put_device(&uverbs_dev->dev);
> > err_uapi:
> > clear_bit(devnum, dev_map);
> > err:
> > if (atomic_dec_and_test(&uverbs_dev->refcount))
> > ib_uverbs_comp_dev(uverbs_dev);
> > wait_for_completion(&uverbs_dev->comp);
> > + put_device(&uverbs_dev->dev);
> > return;
> > }
> >
> > --
> > 2.19.0
>
> Reviewed-by: parav@xxxxxxxxxxxx
>
Parav,
You need to write full tag, so it will be recorded in patchworks.
Thanks,
Attachment:
signature.asc
Description: PGP signature
