[openib-general] [PATCH] RFC: start weaning userspace drivers from sysfs

[Roland Dreier]

As part of the libibverbs 1.1 release, I would like to remove the
dependency on libsysfs, since libsysfs is not very well maintained,
not consistent across distros, and the simple sysfs stuff we need is
easy to do directly.

In this direction, I've already made some changes to libibverbs to
reduce its internal use of sysfs.  However, sysfs is embedded in the
ABI between libibverbs and low-level drivers: libibverbs looks for a
function in each driver with the name "openib_driver_init" and calls
it with a struct sysfs_class_device *.

To fix this in libibverbs 1.1 (which will break ABI from libibverbs
1.0), I propose to replace the driver entry point with a new entry
point that looks like

	struct ibv_device *ibv_driver_init(const char *uverbs_sys_path,
					   int abi_version);

where uverbs_sys_path will be a string like "/sys/class/infiniband_verbs/uverbs0" 
and abi_version will be the contents of the file "abi_version" under
that path, or 0 if the file is not present.  (This last parameter is
just to save every low-level driver from implementing the same code to
read the standard abi_version sysfs attribute).

However, we can move low-level drivers in this direction in a
piecemeal, forwards and backwards compatible way: just add a new
ibv_driver_init entry point, but leave the old openib_driver_init
entry point there and make it a simple wrapper around the new
function.  As an example, here's a patch to libmthca that does that.

Thoughts?

Thanks,
  Roland

--- src/userspace/libmthca/configure.in	(revision 6431)
+++ src/userspace/libmthca/configure.in	(working copy)
@@ -12,16 +12,21 @@ dnl Checks for programs
 AC_PROG_CC
 
 dnl Checks for libraries
+AC_CHECK_LIB(ibverbs, ibv_get_device_list, [],
+    AC_MSG_ERROR([ibv_get_device_list() not found.  libmthca requires libibverbs.]))
 
 dnl Checks for header files.
 AC_CHECK_HEADER(infiniband/driver.h, [],
-    AC_MSG_ERROR([<infiniband/driver.h> not found.  Is libibverbs installed?]))
+    AC_MSG_ERROR([<infiniband/driver.h> not found.  libmthca requires libibverbs.]))
 AC_HEADER_STDC
 
 dnl Checks for typedefs, structures, and compiler characteristics.
 AC_C_CONST
 AC_CHECK_SIZEOF(long)
 
+dnl Checks for library functions
+AC_CHECK_FUNCS(ibv_read_sysfs_file)
+
 AC_CACHE_CHECK(whether ld accepts --version-script, ac_cv_version_script,
     if test -n "`$LD --help < /dev/null 2>/dev/null | grep version-script`"; then
         ac_cv_version_script=yes
--- src/userspace/libmthca/src/mthca.map	(revision 6431)
+++ src/userspace/libmthca/src/mthca.map	(working copy)
@@ -1,4 +1,6 @@
 {
-	global: openib_driver_init;
+	global:
+		ibv_driver_init;
+		openib_driver_init;
 	local: *;
 };
--- src/userspace/libmthca/src/mthca.c	(revision 6431)
+++ src/userspace/libmthca/src/mthca.c	(working copy)
@@ -217,29 +217,53 @@ static struct ibv_device_ops mthca_dev_o
 	.free_context  = mthca_free_context
 };
 
-struct ibv_device *openib_driver_init(struct sysfs_class_device *sysdev)
+/*
+ * Keep a private implementation of HAVE_IBV_READ_SYSFS_FILE to handle
+ * old versions of libibverbs that didn't implement it.  This can be
+ * removed when libibverbs 1.0.3 or newer is available "everywhere."
+ */
+#ifndef HAVE_IBV_READ_SYSFS_FILE
+static int ibv_read_sysfs_file(const char *dir, const char *file,
+			       char *buf, size_t size)
+{
+	char path[256];
+	int fd;
+	int len;
+
+	snprintf(path, sizeof path, "%s/%s", dir, file);
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0)
+		return -1;
+
+	len = read(fd, buf, size);
+
+	close(fd);
+
+	if (len > 0 && buf[len - 1] == '\n')
+		buf[--len] = '\0';
+
+	return len;
+}
+#endif /* HAVE_IBV_READ_SYSFS_FILE */
+
+struct ibv_device *ibv_driver_init(const char *uverbs_sys_path,
+				   int abi_version)
 {
-	struct sysfs_device    *pcidev;
-	struct sysfs_attribute *attr;
+	char			value[8];
 	struct mthca_device    *dev;
 	unsigned                vendor, device;
 	int                     i;
 
-	pcidev = sysfs_get_classdev_device(sysdev);
-	if (!pcidev)
+	if (ibv_read_sysfs_file(uverbs_sys_path, "device/vendor",
+				value, sizeof value) < 0)
 		return NULL;
+	sscanf(value, "%i", &vendor);
 
-	attr = sysfs_get_device_attr(pcidev, "vendor");
-	if (!attr)
+	if (ibv_read_sysfs_file(uverbs_sys_path, "device/device",
+				value, sizeof value) < 0)
 		return NULL;
-	sscanf(attr->value, "%i", &vendor);
-	sysfs_close_attribute(attr);
-
-	attr = sysfs_get_device_attr(pcidev, "device");
-	if (!attr)
-		return NULL;
-	sscanf(attr->value, "%i", &device);
-	sysfs_close_attribute(attr);
+	sscanf(value, "%i", &device);
 
 	for (i = 0; i < sizeof hca_table / sizeof hca_table[0]; ++i)
 		if (vendor == hca_table[i].vendor &&
@@ -252,8 +276,8 @@ found:
 	dev = malloc(sizeof *dev);
 	if (!dev) {
 		fprintf(stderr, PFX "Fatal: couldn't allocate device for %s\n",
-			sysdev->name);
-		abort();
+			uverbs_sys_path);
+		return NULL;
 	}
 
 	dev->ibv_dev.ops = mthca_dev_ops;
@@ -262,3 +286,15 @@ found:
 
 	return &dev->ibv_dev;
 }
+
+struct ibv_device *openib_driver_init(struct sysfs_class_device *sysdev)
+{
+	int abi_ver = 0;
+	char value[8];
+
+	if (ibv_read_sysfs_file(sysdev->path, "abi_version",
+				value, sizeof value) > 0)
+		abi_ver = strtol(value, NULL, 10);
+
+	return ibv_driver_init(sysdev->path, abi_ver);
+}
--- src/userspace/libmthca/ChangeLog	(revision 6431)
+++ src/userspace/libmthca/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2006-04-11  Roland Dreier  <rdreier at cisco.com>
+
+	* src/mthca.c (ibv_driver_init, openib_driver_init): Add new
+	forward-compatible driver entry point.  Make old entry point a
+	simple wrapper for the new one.
+
 2006-03-14  Roland Dreier  <rdreier at cisco.com>
 
 	* Release version 1.0.1.