diff mbox series

[rdma-next,1/5] RDMA/core: Provide getter and setter to access IB device name

Message ID 20180920112202.9181-2-leon@kernel.org (mailing list archive)
State Superseded
Headers show
Series IB device rename support | expand

Commit Message

Leon Romanovsky Sept. 20, 2018, 11:21 a.m. UTC 
From: Leon Romanovsky <leonro@mellanox.com>

Prepare IB device name field to rename operation by ensuring that all
accesses to it are protected with lock and users don't see part of name.

The protection is done with global device_lock because it is used in
allocation and deallocation phases. At this stage, this lock is not
busy and easily can be moved to be per-device, once it will be needed.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
 include/rdma/ib_verbs.h          |  8 +++++++-
 2 files changed, 30 insertions(+), 2 deletions(-)

--
2.14.4

Comments

Steve Wise Sept. 20, 2018, 2:32 p.m. UTC | #1 
On 9/20/2018 6:21 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Prepare IB device name field to rename operation by ensuring that all
> accesses to it are protected with lock and users don't see part of name.
> 
> The protection is done with global device_lock because it is used in
> allocation and deallocation phases. At this stage, this lock is not
> busy and easily can be moved to be per-device, once it will be needed.
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
>  include/rdma/ib_verbs.h          |  8 +++++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 5a680a88aa87..3270cde6d806 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -170,6 +170,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
>  	return NULL;
>  }
> 
> +void ib_device_get_name(struct ib_device *ibdev, char *name)
> +{
> +	down_read(&lists_rwsem);
> +	strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
> +	up_read(&lists_rwsem);
> +}
> +EXPORT_SYMBOL(ib_device_get_name);
> +
>  static int alloc_name(char *name)
>  {
>  	unsigned long *inuse;
> @@ -202,6 +210,21 @@ static int alloc_name(char *name)
>  	return 0;
>  }
> 
> +int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&device_mutex);
> +	strlcpy(ibdev->name, pattern, IB_DEVICE_NAME_MAX);
> +	if (strchr(ibdev->name, '%'))
> +		ret = alloc_name(ibdev->name);
> +
> +	mutex_unlock(&device_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ib_device_alloc_name);
> +
>  static void ib_device_release(struct device *device)
>  {
>  	struct ib_device *dev = container_of(device, struct ib_device, dev);
> @@ -499,7 +522,6 @@ int ib_register_device(struct ib_device *device,
>  		ret = alloc_name(device->name);
>  		if (ret)
>  			goto out;
> -	}

I don't think this is correct...


> 
>  	if (ib_device_check_mandatory(device)) {
>  		ret = -EINVAL;
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index e764ed1f6025..66660e7b9854 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2260,6 +2260,11 @@ struct ib_device {
>  	/* Do not access @dma_device directly from ULP nor from HW drivers. */
>  	struct device                *dma_device;
> 
> +	/*
> +	 * Do not access @name directly,
> +	 * use ib_device_get_name()/ib_device_alloc_name()
> +	 * and don't assume that it can't change after access.
> +	 */
>  	char                          name[IB_DEVICE_NAME_MAX];
> 
>  	struct list_head              event_handler_list;
> @@ -2638,7 +2643,8 @@ struct ib_device *ib_alloc_device(size_t size);
>  void ib_dealloc_device(struct ib_device *device);
> 
>  void ib_get_device_fw_str(struct ib_device *device, char *str);
> -
> +int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern);
> +void ib_device_get_name(struct ib_device *ibdev, char *name);
>  int ib_register_device(struct ib_device *device,
>  		       int (*port_callback)(struct ib_device *,
>  					    u8, struct kobject *));
> --
> 2.14.4
>
Jason Gunthorpe Sept. 20, 2018, 3:15 p.m. UTC | #2 
On Thu, Sep 20, 2018 at 02:21:58PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> Prepare IB device name field to rename operation by ensuring that all
> accesses to it are protected with lock and users don't see part of name.

Oh dear, no, that isn't going to work, there is too much stuff using
dev_name.. Did you read the comment on device_rename??

https://elixir.bootlin.com/linux/v4.19-rc4/source/drivers/base/core.c#L2715

> The protection is done with global device_lock because it is used in
> allocation and deallocation phases. At this stage, this lock is not
> busy and easily can be moved to be per-device, once it will be needed.
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
>  include/rdma/ib_verbs.h          |  8 +++++++-
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 5a680a88aa87..3270cde6d806 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -170,6 +170,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
>  	return NULL;
>  }
> 
> +void ib_device_get_name(struct ib_device *ibdev, char *name)
> +{
> +	down_read(&lists_rwsem);
> +	strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
> +	up_read(&lists_rwsem);
> +}
> +EXPORT_SYMBOL(ib_device_get_name);

I think we have to follow netdev and just rely on device_rename()
being 'good enough'.  

Switch everything to use dev_name()/etc rather than try and do
something like this so the responsibility is on the device core to
keep this working, not us.

Turns out I have a series for that for unrelated reasons..

>  static int alloc_name(char *name)
>  {
>  	unsigned long *inuse;
> @@ -202,6 +210,21 @@ static int alloc_name(char *name)
>  	return 0;
>  }
> 
> +int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&device_mutex);
> +	strlcpy(ibdev->name, pattern, IB_DEVICE_NAME_MAX);
> +	if (strchr(ibdev->name, '%'))
> +		ret = alloc_name(ibdev->name);
> +
> +	mutex_unlock(&device_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ib_device_alloc_name);

Can't call alloc_name() without also adding to the list, this will
allow duplicates.

Jason
Leon Romanovsky Sept. 20, 2018, 4:40 p.m. UTC | #3 
On Thu, Sep 20, 2018 at 09:15:41AM -0600, Jason Gunthorpe wrote:
> On Thu, Sep 20, 2018 at 02:21:58PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Prepare IB device name field to rename operation by ensuring that all
> > accesses to it are protected with lock and users don't see part of name.
>
> Oh dear, no, that isn't going to work, there is too much stuff using
> dev_name.. Did you read the comment on device_rename??
>
> https://elixir.bootlin.com/linux/v4.19-rc4/source/drivers/base/core.c#L2715

Yes, I read, it was mentioned in the cover letter.

>
> > The protection is done with global device_lock because it is used in
> > allocation and deallocation phases. At this stage, this lock is not
> > busy and easily can be moved to be per-device, once it will be needed.
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
> >  include/rdma/ib_verbs.h          |  8 +++++++-
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index 5a680a88aa87..3270cde6d806 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -170,6 +170,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
> >  	return NULL;
> >  }
> >
> > +void ib_device_get_name(struct ib_device *ibdev, char *name)
> > +{
> > +	down_read(&lists_rwsem);
> > +	strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
> > +	up_read(&lists_rwsem);
> > +}
> > +EXPORT_SYMBOL(ib_device_get_name);
>
> I think we have to follow netdev and just rely on device_rename()
> being 'good enough'.
>
> Switch everything to use dev_name()/etc rather than try and do
> something like this so the responsibility is on the device core to
> keep this working, not us.
>
> Turns out I have a series for that for unrelated reasons..

And what should I do now with this knowledge?

>
> >  static int alloc_name(char *name)
> >  {
> >  	unsigned long *inuse;
> > @@ -202,6 +210,21 @@ static int alloc_name(char *name)
> >  	return 0;
> >  }
> >
> > +int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&device_mutex);
> > +	strlcpy(ibdev->name, pattern, IB_DEVICE_NAME_MAX);
> > +	if (strchr(ibdev->name, '%'))
> > +		ret = alloc_name(ibdev->name);
> > +
> > +	mutex_unlock(&device_mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(ib_device_alloc_name);
>
> Can't call alloc_name() without also adding to the list, this will
> allow duplicates.

I planned to change it in the future by moving to different name scheme
with unique naming.

>
> Jason
Jason Gunthorpe Sept. 20, 2018, 4:46 p.m. UTC | #4 
On Thu, Sep 20, 2018 at 07:40:39PM +0300, Leon Romanovsky wrote:

> > > The protection is done with global device_lock because it is used in
> > > allocation and deallocation phases. At this stage, this lock is not
> > > busy and easily can be moved to be per-device, once it will be needed.
> > >
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > >  drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
> > >  include/rdma/ib_verbs.h          |  8 +++++++-
> > >  2 files changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index 5a680a88aa87..3270cde6d806 100644
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -170,6 +170,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
> > >  	return NULL;
> > >  }
> > >
> > > +void ib_device_get_name(struct ib_device *ibdev, char *name)
> > > +{
> > > +	down_read(&lists_rwsem);
> > > +	strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
> > > +	up_read(&lists_rwsem);
> > > +}
> > > +EXPORT_SYMBOL(ib_device_get_name);
> >
> > I think we have to follow netdev and just rely on device_rename()
> > being 'good enough'.
> >
> > Switch everything to use dev_name()/etc rather than try and do
> > something like this so the responsibility is on the device core to
> > keep this working, not us.
> >
> > Turns out I have a series for that for unrelated reasons..
> 
> And what should I do now with this knowledge?

Maybe I can send it today..

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 5a680a88aa87..3270cde6d806 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -170,6 +170,14 @@  static struct ib_device *__ib_device_get_by_name(const char *name)
 	return NULL;
 }

+void ib_device_get_name(struct ib_device *ibdev, char *name)
+{
+	down_read(&lists_rwsem);
+	strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
+	up_read(&lists_rwsem);
+}
+EXPORT_SYMBOL(ib_device_get_name);
+
 static int alloc_name(char *name)
 {
 	unsigned long *inuse;
@@ -202,6 +210,21 @@  static int alloc_name(char *name)
 	return 0;
 }

+int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern)
+{
+	int ret = 0;
+
+	mutex_lock(&device_mutex);
+	strlcpy(ibdev->name, pattern, IB_DEVICE_NAME_MAX);
+	if (strchr(ibdev->name, '%'))
+		ret = alloc_name(ibdev->name);
+
+	mutex_unlock(&device_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(ib_device_alloc_name);
+
 static void ib_device_release(struct device *device)
 {
 	struct ib_device *dev = container_of(device, struct ib_device, dev);
@@ -499,7 +522,6 @@  int ib_register_device(struct ib_device *device,
 		ret = alloc_name(device->name);
 		if (ret)
 			goto out;
-	}

 	if (ib_device_check_mandatory(device)) {
 		ret = -EINVAL;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index e764ed1f6025..66660e7b9854 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2260,6 +2260,11 @@  struct ib_device {
 	/* Do not access @dma_device directly from ULP nor from HW drivers. */
 	struct device                *dma_device;

+	/*
+	 * Do not access @name directly,
+	 * use ib_device_get_name()/ib_device_alloc_name()
+	 * and don't assume that it can't change after access.
+	 */
 	char                          name[IB_DEVICE_NAME_MAX];

 	struct list_head              event_handler_list;
@@ -2638,7 +2643,8 @@  struct ib_device *ib_alloc_device(size_t size);
 void ib_dealloc_device(struct ib_device *device);

 void ib_get_device_fw_str(struct ib_device *device, char *str);
-
+int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern);
+void ib_device_get_name(struct ib_device *ibdev, char *name);
 int ib_register_device(struct ib_device *device,
 		       int (*port_callback)(struct ib_device *,
 					    u8, struct kobject *));