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.
> 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
>
