On Tue, Jan 22, 2019 at 12:26:32PM -0500, Ira Weiny wrote:
> On Tue, Jan 22, 2019 at 08:33:00AM +0200, Leon Romanovsky wrote:
> > From: Parav Pandit <parav@xxxxxxxxxxxx>
> >
> > ib_umad_init_port_dev() holds the reference of ib_umad_device instance.
> > ib_umad_device contains standard core device and cdev.
> > cdev holds the reference of its parent core device.
>
> I don't see where this happens.
1175 static void ib_umad_init_port_dev(struct device *dev,
1176 struct ib_umad_port *port,
1177 const struct ib_device *device)
1178 {
1179 device_initialize(dev);
1180 ib_umad_dev_get(port->umad_dev);
1181 dev->class = &umad_class;
1182 dev->parent = device->dev.parent;
^^^^ ib_device
1183 dev_set_drvdata(dev, port);
1184 dev->release = ib_umad_release_port;
1185 }
1186
>
> > file ops holds the reference to cdev using core kernel.
> >
> > Therefore, there is no need to hold additional reference while opening
> > umad related char devices.
> > While at it, add comments to bring clarity on releasing references to
> > ib_umd_device.
> >
> > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>
> It seems like this could be even more simplified... It seems like you are
> getting rid of the need for the ib_umad_device in favor of using the ib_device
> object for reference tracking? Is that correct? If so I'm not seeing how this
> works in that way.
>
> Also, if removing ib_umad_device is the goal (which I'm not opposed to), do you
> plan to remove ib_umad_device in a future patch?
>
> Ira
>
> > ---
> > drivers/infiniband/core/user_mad.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> > index de8d31ab8945..097f153f0f22 100644
> > --- a/drivers/infiniband/core/user_mad.c
> > +++ b/drivers/infiniband/core/user_mad.c
> > @@ -989,7 +989,6 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
> > goto out;
> > }
> >
> > - ib_umad_dev_get(port->umad_dev);
> > out:
> > mutex_unlock(&port->file_mutex);
> > return ret;
> > @@ -998,7 +997,6 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
> > static int ib_umad_close(struct inode *inode, struct file *filp)
> > {
> > struct ib_umad_file *file = filp->private_data;
> > - struct ib_umad_device *dev = file->port->umad_dev;
> > struct ib_umad_packet *packet, *tmp;
> > int already_dead;
> > int i;
> > @@ -1027,7 +1025,6 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
> > mutex_unlock(&file->port->file_mutex);
> >
> > kfree(file);
> > - ib_umad_dev_put(dev);
> > return 0;
> > }
> >
> > @@ -1077,7 +1074,6 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
> > if (ret)
> > goto err_clr_sm_cap;
> >
> > - ib_umad_dev_get(port->umad_dev);
> > return 0;
> >
> > err_clr_sm_cap:
> > @@ -1106,7 +1102,6 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
> >
> > up(&port->sm_sem);
> >
> > - ib_umad_dev_put(port->umad_dev);
> > return ret;
> > }
> >
> > @@ -1283,8 +1278,10 @@ static void ib_umad_kill_port(struct ib_umad_port *port)
> > mutex_unlock(&port->file_mutex);
> >
> > cdev_device_del(&port->sm_cdev, &port->sm_dev);
> > + /* balances device_initialize() */
> > put_device(&port->sm_dev);
> > cdev_device_del(&port->cdev, &port->dev);
> > + /* balances device_initialize() */
> > put_device(&port->dev);
> > ida_free(&umad_ida, port->dev_num);
> > }
> > @@ -1329,6 +1326,7 @@ static void ib_umad_add_one(struct ib_device *device)
> > ib_umad_kill_port(&umad_dev->ports[i - s]);
> > }
> > free:
> > + /* balances kref_init */
> > ib_umad_dev_put(umad_dev);
> > }
> >
> > @@ -1344,6 +1342,7 @@ static void ib_umad_remove_one(struct ib_device *device, void *client_data)
> > if (rdma_cap_ib_mad(device, i + rdma_start_port(device)))
> > ib_umad_kill_port(&umad_dev->ports[i]);
> > }
> > + /* balances kref_init() */
> > ib_umad_dev_put(umad_dev);
> > }
> >
> > --
> > 2.19.1
> >
Attachment:
signature.asc
Description: PGP signature
