| Message ID | 1504212659-9674-5-git-send-email-jgunthorpe@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Aug 31, 2017 at 02:50:58PM -0600, Jason Gunthorpe wrote: > Now that we have ccan lists the logic here can be simplified greatly. > Eliminate the confusing used and have_driver values, instead just > delete items from the sysfs list as we run down the process. This > directly guarantees discovered sysfs items are handled only once, > and makes the lifetime of the sysfs pointers much clearer. > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > --- > libibverbs/init.c | 93 +++++++++++++++++++++++++++++-------------------------- > 1 file changed, 49 insertions(+), 44 deletions(-) > > diff --git a/libibverbs/init.c b/libibverbs/init.c > index 91ce8b4e378458..04583bb5137c96 100644 > --- a/libibverbs/init.c > +++ b/libibverbs/init.c > @@ -60,9 +60,7 @@ struct ibv_sysfs_dev { > char sysfs_path[IBV_SYSFS_PATH_MAX]; > char ibdev_path[IBV_SYSFS_PATH_MAX]; > int abi_ver; > - int have_driver; > struct timespec time_created; > - int used; > }; > > struct ibv_driver_name { > @@ -148,8 +146,6 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list) > > sysfs_dev->time_created = buf.st_mtim; > > - sysfs_dev->have_driver = 0; > - sysfs_dev->used = 0; > if (ibv_read_sysfs_file(sysfs_dev->sysfs_path, "abi_version", > value, sizeof value) > 0) > sysfs_dev->abi_ver = strtol(value, NULL, 10); > @@ -412,7 +408,6 @@ static struct verbs_device *try_driver(struct ibv_driver *driver, > strcpy(dev->name, sysfs_dev->ibdev_name); > strcpy(dev->ibdev_path, sysfs_dev->ibdev_path); > vdev->sysfs = sysfs_dev; > - sysfs_dev->used = 1; > > return vdev; > } > @@ -481,25 +476,51 @@ static int same_sysfs_dev(struct ibv_sysfs_dev *sysfs1, > return 0; > } > > -int ibverbs_get_device_list(struct list_head *list) > +/* Match every ibv_sysfs_dev in the sysfs_list to a driver and add a new entry > + * to device_list. Once matched to a driver the entry in sysfs_list is > + * removed. > + */ > +static void try_all_drivers(struct list_head *sysfs_list, > + struct list_head *device_list, > + unsigned int *num_devices) > +{ > + struct ibv_sysfs_dev *sysfs_dev; > + struct ibv_sysfs_dev *tmp; > + struct verbs_device *vdev; > + > + list_for_each_safe(sysfs_list, sysfs_dev, tmp, entry) { > + vdev = try_drivers(sysfs_dev); > + if (vdev) { > + list_del(&sysfs_dev->entry); > + // Ownership of sysfs_dev moves into vdev->sysfs Please don't use "//" C++ style in C files. Thanks > + list_add(device_list, &vdev->entry); > + (*num_devices)++; > + } > + } > +} > + > +int ibverbs_get_device_list(struct list_head *device_list) > { > - LIST_HEAD(tmp_sysfs_dev_list); > + LIST_HEAD(sysfs_list); > struct ibv_sysfs_dev *sysfs_dev, *next_dev; > struct verbs_device *vdev, *tmp; > static int drivers_loaded; > - int num_devices = 0; > + unsigned int num_devices = 0; > int statically_linked = 0; > - int no_driver = 0; > int ret; > > - ret = find_sysfs_devs(&tmp_sysfs_dev_list); > + ret = find_sysfs_devs(&sysfs_list); > if (ret) > return -ret; > > - list_for_each_safe(list, vdev, tmp, entry) { > + /* Remove entries from the sysfs_list that are already preset in the > + * device_list, and remove entries from the device_list that are not > + * present in the sysfs_list. > + */ > + list_for_each_safe(device_list, vdev, tmp, entry) { > struct ibv_sysfs_dev *old_sysfs = NULL; > > - list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) { > + list_for_each(&sysfs_list, sysfs_dev, entry) { > if (same_sysfs_dev(vdev->sysfs, sysfs_dev)) { > old_sysfs = sysfs_dev; > break; > @@ -507,7 +528,8 @@ int ibverbs_get_device_list(struct list_head *list) > } > > if (old_sysfs) { > - sysfs_dev->have_driver = 1; > + list_del(&old_sysfs->entry); > + free(old_sysfs); > num_devices++; > } else { > list_del(&vdev->entry); > @@ -515,19 +537,9 @@ int ibverbs_get_device_list(struct list_head *list) > } > } > > - list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) { > - if (sysfs_dev->have_driver) > - continue; > - vdev = try_drivers(sysfs_dev); > - if (vdev) { > - sysfs_dev->have_driver = 1; > - list_add(list, &vdev->entry); > - num_devices++; > - } else > - no_driver = 1; > - } > + try_all_drivers(&sysfs_list, device_list, &num_devices); > > - if (!no_driver || drivers_loaded) > + if (list_empty(&sysfs_list) || drivers_loaded) > goto out; > > /* > @@ -553,29 +565,22 @@ int ibverbs_get_device_list(struct list_head *list) > load_drivers(); > drivers_loaded = 1; > > - list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) { > - if (sysfs_dev->have_driver) > - continue; > - > - vdev = try_drivers(sysfs_dev); > - if (vdev) { > - sysfs_dev->have_driver = 1; > - list_add(list, &vdev->entry); > - num_devices++; > - } > - } > + try_all_drivers(&sysfs_list, device_list, &num_devices); > > out: > - list_for_each_safe(&tmp_sysfs_dev_list, sysfs_dev, next_dev, entry) { > - if (!sysfs_dev->have_driver && getenv("IBV_SHOW_WARNINGS")) { > - fprintf(stderr, PFX "Warning: no userspace device-specific " > - "driver found for %s\n", sysfs_dev->sysfs_path); > + /* Anything left in sysfs_list was not assoicated with a > + * driver. > + */ > + list_for_each_safe(&sysfs_list, sysfs_dev, next_dev, entry) { > + if (getenv("IBV_SHOW_WARNINGS")) { > + fprintf(stderr, PFX > + "Warning: no userspace device-specific driver found for %s\n", > + sysfs_dev->sysfs_path); > if (statically_linked) > - fprintf(stderr, " When linking libibverbs statically, " > - "driver must be statically linked too.\n"); > + fprintf(stderr, > + " When linking libibverbs statically, driver must be statically linked too.\n"); > } > - if (!sysfs_dev->used) > - free(sysfs_dev); > + free(sysfs_dev); > } > > return num_devices; > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 03, 2017 at 05:03:51PM +0300, Leon Romanovsky wrote: > > + list_del(&sysfs_dev->entry); > > + // Ownership of sysfs_dev moves into vdev->sysfs > > Please don't use "//" C++ style in C files. I thought we were following the C11 rules? We use all the other features now and don't support pre C11 compilers. (unlike the kernel) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 03, 2017 at 08:48:39AM -0600, Jason Gunthorpe wrote: > On Sun, Sep 03, 2017 at 05:03:51PM +0300, Leon Romanovsky wrote: > > > + list_del(&sysfs_dev->entry); > > > + // Ownership of sysfs_dev moves into vdev->sysfs > > > > Please don't use "//" C++ style in C files. > > I thought we were following the C11 rules? Not in the coding style, to simplify development, it is better to stick with one known to everyone here style - kernel coding style. > We use all the other features now and don't support pre C11 compilers. (unlike the kernel) > > Jason
diff --git a/libibverbs/init.c b/libibverbs/init.c index 91ce8b4e378458..04583bb5137c96 100644 --- a/libibverbs/init.c +++ b/libibverbs/init.c @@ -60,9 +60,7 @@ struct ibv_sysfs_dev { char sysfs_path[IBV_SYSFS_PATH_MAX]; char ibdev_path[IBV_SYSFS_PATH_MAX]; int abi_ver; - int have_driver; struct timespec time_created; - int used; }; struct ibv_driver_name { @@ -148,8 +146,6 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list) sysfs_dev->time_created = buf.st_mtim; - sysfs_dev->have_driver = 0; - sysfs_dev->used = 0; if (ibv_read_sysfs_file(sysfs_dev->sysfs_path, "abi_version", value, sizeof value) > 0) sysfs_dev->abi_ver = strtol(value, NULL, 10); @@ -412,7 +408,6 @@ static struct verbs_device *try_driver(struct ibv_driver *driver, strcpy(dev->name, sysfs_dev->ibdev_name); strcpy(dev->ibdev_path, sysfs_dev->ibdev_path); vdev->sysfs = sysfs_dev; - sysfs_dev->used = 1; return vdev; } @@ -481,25 +476,51 @@ static int same_sysfs_dev(struct ibv_sysfs_dev *sysfs1, return 0; } -int ibverbs_get_device_list(struct list_head *list) +/* Match every ibv_sysfs_dev in the sysfs_list to a driver and add a new entry + * to device_list. Once matched to a driver the entry in sysfs_list is + * removed. + */ +static void try_all_drivers(struct list_head *sysfs_list, + struct list_head *device_list, + unsigned int *num_devices) +{ + struct ibv_sysfs_dev *sysfs_dev; + struct ibv_sysfs_dev *tmp; + struct verbs_device *vdev; + + list_for_each_safe(sysfs_list, sysfs_dev, tmp, entry) { + vdev = try_drivers(sysfs_dev); + if (vdev) { + list_del(&sysfs_dev->entry); + // Ownership of sysfs_dev moves into vdev->sysfs + list_add(device_list, &vdev->entry); + (*num_devices)++; + } + } +} + +int ibverbs_get_device_list(struct list_head *device_list) { - LIST_HEAD(tmp_sysfs_dev_list); + LIST_HEAD(sysfs_list); struct ibv_sysfs_dev *sysfs_dev, *next_dev; struct verbs_device *vdev, *tmp; static int drivers_loaded; - int num_devices = 0; + unsigned int num_devices = 0; int statically_linked = 0; - int no_driver = 0; int ret; - ret = find_sysfs_devs(&tmp_sysfs_dev_list); + ret = find_sysfs_devs(&sysfs_list); if (ret) return -ret; - list_for_each_safe(list, vdev, tmp, entry) { + /* Remove entries from the sysfs_list that are already preset in the + * device_list, and remove entries from the device_list that are not + * present in the sysfs_list. + */ + list_for_each_safe(device_list, vdev, tmp, entry) { struct ibv_sysfs_dev *old_sysfs = NULL; - list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) { + list_for_each(&sysfs_list, sysfs_dev, entry) { if (same_sysfs_dev(vdev->sysfs, sysfs_dev)) { old_sysfs = sysfs_dev; break; @@ -507,7 +528,8 @@ int ibverbs_get_device_list(struct list_head *list) } if (old_sysfs) { - sysfs_dev->have_driver = 1; + list_del(&old_sysfs->entry); + free(old_sysfs); num_devices++; } else { list_del(&vdev->entry); @@ -515,19 +537,9 @@ int ibverbs_get_device_list(struct list_head *list) } } - list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) { - if (sysfs_dev->have_driver) - continue; - vdev = try_drivers(sysfs_dev); - if (vdev) { - sysfs_dev->have_driver = 1; - list_add(list, &vdev->entry); - num_devices++; - } else - no_driver = 1; - } + try_all_drivers(&sysfs_list, device_list, &num_devices); - if (!no_driver || drivers_loaded) + if (list_empty(&sysfs_list) || drivers_loaded) goto out; /* @@ -553,29 +565,22 @@ int ibverbs_get_device_list(struct list_head *list) load_drivers(); drivers_loaded = 1; - list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) { - if (sysfs_dev->have_driver) - continue; - - vdev = try_drivers(sysfs_dev); - if (vdev) { - sysfs_dev->have_driver = 1; - list_add(list, &vdev->entry); - num_devices++; - } - } + try_all_drivers(&sysfs_list, device_list, &num_devices); out: - list_for_each_safe(&tmp_sysfs_dev_list, sysfs_dev, next_dev, entry) { - if (!sysfs_dev->have_driver && getenv("IBV_SHOW_WARNINGS")) { - fprintf(stderr, PFX "Warning: no userspace device-specific " - "driver found for %s\n", sysfs_dev->sysfs_path); + /* Anything left in sysfs_list was not assoicated with a + * driver. + */ + list_for_each_safe(&sysfs_list, sysfs_dev, next_dev, entry) { + if (getenv("IBV_SHOW_WARNINGS")) { + fprintf(stderr, PFX + "Warning: no userspace device-specific driver found for %s\n", + sysfs_dev->sysfs_path); if (statically_linked) - fprintf(stderr, " When linking libibverbs statically, " - "driver must be statically linked too.\n"); + fprintf(stderr, + " When linking libibverbs statically, driver must be statically linked too.\n"); } - if (!sysfs_dev->used) - free(sysfs_dev); + free(sysfs_dev); } return num_devices;
Now that we have ccan lists the logic here can be simplified greatly. Eliminate the confusing used and have_driver values, instead just delete items from the sysfs list as we run down the process. This directly guarantees discovered sysfs items are handled only once, and makes the lifetime of the sysfs pointers much clearer. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- libibverbs/init.c | 93 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 49 insertions(+), 44 deletions(-)