>-----Original Message----- >From: Jason Gunthorpe [mailto:jgg@xxxxxxxxxxxx] >Sent: Tuesday, September 25, 2018 6:58 PM >To: linux-rdma@xxxxxxxxxxxxxxx; Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; >Marciniszyn, Mike <mike.marciniszyn@xxxxxxxxx>; Dalessandro, Dennis ><dennis.dalessandro@xxxxxxxxx> >Subject: [PATCH v1] RDMA: Fully setup the device name in ib_register_device > >The current code has two copies of the device name, ibdev->dev and >dev_name(&ibdev->dev), and they are setup at different times, which is >very confusing. > >Set them both up at the same time and make dev_name() the lead name, which >is the proper use of the driver core APIs. To make it very clear that the >name is not valid until registration pass it in to the >ib_register_device() call rather than messing with ibdev->name directly. > >Also the reorganization now checks that dev_name is unique even if it does >not contain a %. > >Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx> >Acked-by: Adit Ranadive <aditr@xxxxxxxxxx> >Reviewed-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx> >Acked-by: Devesh Sharma <devesh.sharma@xxxxxxxxxxxx> >Reviewed-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx> >Reviewed-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> >--- > drivers/infiniband/core/device.c | 35 +++++++++++-------- > drivers/infiniband/core/sysfs.c | 4 --- > drivers/infiniband/sw/rdmavt/vt.c | 3 +- > drivers/infiniband/sw/rxe/rxe_verbs.c | 3 +- > include/rdma/ib_verbs.h | 6 ++-- > include/rdma/rdma_vt.h | 9 ++++- > 19 files changed, 53 insertions(+), 59 deletions(-) > >v1: >- Revise rvt so that the name is still set, but do so properly by > setting dev_name as well. > >Mike, this should handle your concern.. > >diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c >index 5a680a88aa87b7..faacf95699d794 100644 >--- a/drivers/infiniband/core/device.c >+++ b/drivers/infiniband/core/device.c >@@ -170,10 +170,9 @@ static struct ib_device >*__ib_device_get_by_name(const char *name) > return NULL; > } > >-static int alloc_name(char *name) >+static int alloc_name(struct ib_device *ibdev, const char *name) > { > unsigned long *inuse; >- char buf[IB_DEVICE_NAME_MAX]; > struct ib_device *device; > int i; > >@@ -182,24 +181,21 @@ static int alloc_name(char *name) > return -ENOMEM; > > list_for_each_entry(device, &device_list, core_list) { >- if (!sscanf(device->name, name, &i)) >+ char buf[IB_DEVICE_NAME_MAX]; >+ >+ if (sscanf(device->name, name, &i) != 1) > continue; > if (i < 0 || i >= PAGE_SIZE * 8) > continue; > snprintf(buf, sizeof buf, name, i); >- if (!strncmp(buf, device->name, IB_DEVICE_NAME_MAX)) >+ if (!strcmp(buf, dev_name(&device->dev))) > set_bit(i, inuse); > } > > i = find_first_zero_bit(inuse, PAGE_SIZE * 8); > free_page((unsigned long) inuse); >- snprintf(buf, sizeof buf, name, i); >- >- if (__ib_device_get_by_name(buf)) >- return -ENFILE; > >- strlcpy(name, buf, IB_DEVICE_NAME_MAX); >- return 0; >+ return dev_set_name(&ibdev->dev, name, i); > } > > static void ib_device_release(struct device *device) >@@ -454,9 +450,9 @@ static u32 __dev_new_index(void) > * callback for each device that is added. @device must be allocated > * with ib_alloc_device(). > */ >-int ib_register_device(struct ib_device *device, >- int (*port_callback)(struct ib_device *, >- u8, struct kobject *)) >+int ib_register_device(struct ib_device *device, const char *name, >+ int (*port_callback)(struct ib_device *, u8, >+ struct kobject *)) > { > int ret; > struct ib_client *client; >@@ -495,11 +491,20 @@ int ib_register_device(struct ib_device *device, > > mutex_lock(&device_mutex); > >- if (strchr(device->name, '%')) { >- ret = alloc_name(device->name); >+ if (strchr(name, '%')) { >+ ret = alloc_name(device, name); >+ if (ret) >+ goto out; >+ } else { >+ ret = dev_set_name(&device->dev, name); > if (ret) > goto out; > } >+ if (__ib_device_get_by_name(dev_name(&device->dev))) { >+ ret = -ENFILE; >+ goto out; >+ } >+ strlcpy(device->name, dev_name(&device->dev), >IB_DEVICE_NAME_MAX); > > if (ib_device_check_mandatory(device)) { > ret = -EINVAL; >diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c >index 0b04dbff884f48..bc947a863b34c7 100644 >--- a/drivers/infiniband/core/sysfs.c >+++ b/drivers/infiniband/core/sysfs.c >@@ -1311,10 +1311,6 @@ int ib_device_register_sysfs(struct ib_device *device, > int ret; > int i; > >- ret = dev_set_name(class_dev, "%s", device->name); >- if (ret) >- return ret; >- > device->groups[0] = &dev_attr_group; > class_dev->groups = device->groups; > >diff --git a/drivers/infiniband/sw/rdmavt/vt.c b/drivers/infiniband/sw/rdmavt/vt.c >index 17e4abc067afa3..e3249d46bcef54 100644 >--- a/drivers/infiniband/sw/rdmavt/vt.c >+++ b/drivers/infiniband/sw/rdmavt/vt.c >@@ -828,7 +828,8 @@ int rvt_register_device(struct rvt_dev_info *rdi, u32 >driver_id) > > rdi->ibdev.driver_id = driver_id; > /* We are now good to announce we exist */ >- ret = ib_register_device(&rdi->ibdev, rdi->driver_f.port_callback); >+ ret = ib_register_device(&rdi->ibdev, dev_name(&rdi->ibdev.dev), >+ rdi->driver_f.port_callback); > if (ret) { > rvt_pr_err(rdi, "Failed to register driver with ib core.\n"); > goto bail_mr; >diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >index 0d822a9db300a7..9897d2329f2c96 100644 >--- a/include/rdma/ib_verbs.h >+++ b/include/rdma/ib_verbs.h >@@ -2625,9 +2625,9 @@ void ib_dealloc_device(struct ib_device *device); > > void ib_get_device_fw_str(struct ib_device *device, char *str); > >-int ib_register_device(struct ib_device *device, >- int (*port_callback)(struct ib_device *, >- u8, struct kobject *)); >+int ib_register_device(struct ib_device *device, const char *name, >+ int (*port_callback)(struct ib_device *, u8, >+ struct kobject *)); > void ib_unregister_device(struct ib_device *device); > > int ib_register_client (struct ib_client *client); >diff --git a/include/rdma/rdma_vt.h b/include/rdma/rdma_vt.h >index e32facdd9fd3d0..065c9fbe658995 100644 >--- a/include/rdma/rdma_vt.h >+++ b/include/rdma/rdma_vt.h >@@ -429,7 +429,14 @@ static inline void rvt_set_ibdev_name(struct >rvt_dev_info *rdi, > const char *fmt, const char *name, > const int unit) > { >- snprintf(rdi->ibdev.name, sizeof(rdi->ibdev.name), fmt, name, unit); >+ /* >+ * FIXME: rvt and its users want to touch the ibdev before >+ * registration and have things like the name work. We don't have the >+ * infrastructure in the core to support this directly today, hack it >+ * to work by setting the name manually here. >+ */ >+ dev_set_name(&rdi->ibdev.dev, fmt, name, unit); >+ strlcpy(rdi->ibdev.name, dev_name(&rdi->ibdev.dev), >IB_DEVICE_NAME_MAX); > } This addresses my concern. Thank you. I will look at our current usage and see what I can do to fix it. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx> Mike > /** >-- >2.19.0
