[PATCH 0/4] imsm: lowest namespace support

[PATCH 0/4] imsm: lowest namespace support

[Mariusz Tkaczyk]

Nvme specification doesn't guarantee the first namespace existence.  
Imsm shall use lowest one instead of first. In this patchset imsm parts
responsible for reading sysfs attrs were refactored and simplified to
generic routines. Imsm compatibility checks were moved to
validate_geometry_imsm_container. This prevents from writing metadata
to rejected drive.

Mariusz Tkaczyk (4):
  imsm: add generic method to resolve "device" links
  imsm: add devpath_to_char method
  imsm: Limit support to the lowest namespace
  Manage: Call validate_geometry when adding drive to external container

 Manage.c         |   7 ++
 platform-intel.c | 179 +++++++++++++++++++++++++++---------
 platform-intel.h |   8 +-
 super-ddf.c      |   9 +-
 super-intel.c    | 233 +++++++++++++++++++++--------------------------
 5 files changed, 256 insertions(+), 180 deletions(-)

-- 
2.26.2

[PATCH 1/4] imsm: add generic method to resolve "device" links

[Mariusz Tkaczyk]

Each virtual device is linked with parent by "device". This patch adds
possibility to get previous device in sysfs tree.

Depending on device type, there is a different amount of virutal
layers. The best we can do is allow to directly specify how many
"device" links need to be resolved. This approach also allows to get
previous virtual device, which may contain some attributes.

Simplify fd2devname, this function doesn't require new functionality and
shall use generic fd2kname.

For nvme drives represented via nvme-subystem when path to block
device if requested, then return it without translation.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 platform-intel.c | 75 ++++++++++++++++++++++++++++++++----------------
 platform-intel.h |  4 +--
 super-intel.c    | 63 ++++++++++++++++------------------------
 3 files changed, 77 insertions(+), 65 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 2da152f..2ed63ed 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -712,28 +712,61 @@ char *get_nvme_multipath_dev_hw_path(const char *dev_path)
 	return rp;
 }
 
-char *devt_to_devpath(dev_t dev)
+/* Description: Return part or whole realpath for the dev
+ * Parameters:
+ *	dev - the device to be quered
+ *	dev_level - level of "/device" entries. It allows to caller to access
+ *		    virtual or physical devices which are on "path" to quered
+ *		    one.
+ *	buf - optional, must be PATH_MAX size. If set, then will be used.
+ */
+char *devt_to_devpath(dev_t dev, int dev_level, char *buf)
 {
-	char device[46];
-	char *rp;
-	char *buf;
+	char device[PATH_MAX];
+	char *hw_path;
+	int i;
+	unsigned long device_free_len = sizeof(device) - 1;
+	char dev_str[] = "/device";
+	unsigned long dev_str_len = strlen(dev_str);
+
+	snprintf(device, sizeof(device), "/sys/dev/block/%d:%d", major(dev),
+		 minor(dev));
+
+	/* If caller wants block device, return path to it even if it is exposed
+	 * via virtual layer.
+	 */
+	if (dev_level == 0)
+		return realpath(device, buf);
 
-	sprintf(device, "/sys/dev/block/%d:%d/device", major(dev), minor(dev));
+	device_free_len -= strlen(device);
+	for (i = 0; i < dev_level; i++) {
+		if (device_free_len < dev_str_len)
+			return NULL;
 
-	rp = realpath(device, NULL);
-	if (!rp)
-		return NULL;
+		strncat(device, dev_str, device_free_len);
 
-	buf = get_nvme_multipath_dev_hw_path(rp);
-	if (buf) {
-		free(rp);
-		return buf;
+		/* Resolve nvme-subsystem abstraction if needed
+		 */
+		device_free_len -= dev_str_len;
+		if (i == 0) {
+			char rp[PATH_MAX];
+
+			if (!realpath(device, rp))
+				return NULL;
+			hw_path = get_nvme_multipath_dev_hw_path(rp);
+			if (hw_path) {
+				strcpy(device, hw_path);
+				device_free_len = sizeof(device) -
+						  strlen(device) - 1;
+				free(hw_path);
+			}
+		}
 	}
 
-	return rp;
+	return realpath(device, buf);
 }
 
-char *diskfd_to_devpath(int fd)
+char *diskfd_to_devpath(int fd, int dev_level, char *buf)
 {
 	/* return the device path for a disk, return NULL on error or fd
 	 * refers to a partition
@@ -745,7 +778,7 @@ char *diskfd_to_devpath(int fd)
 	if (!S_ISBLK(st.st_mode))
 		return NULL;
 
-	return devt_to_devpath(st.st_rdev);
+	return devt_to_devpath(st.st_rdev, dev_level, buf);
 }
 
 int path_attached_to_hba(const char *disk_path, const char *hba_path)
@@ -770,7 +803,7 @@ int path_attached_to_hba(const char *disk_path, const char *hba_path)
 
 int devt_attached_to_hba(dev_t dev, const char *hba_path)
 {
-	char *disk_path = devt_to_devpath(dev);
+	char *disk_path = devt_to_devpath(dev, 1, NULL);
 	int rc = path_attached_to_hba(disk_path, hba_path);
 
 	if (disk_path)
@@ -781,7 +814,7 @@ int devt_attached_to_hba(dev_t dev, const char *hba_path)
 
 int disk_attached_to_hba(int fd, const char *hba_path)
 {
-	char *disk_path = diskfd_to_devpath(fd);
+	char *disk_path = diskfd_to_devpath(fd, 1, NULL);
 	int rc = path_attached_to_hba(disk_path, hba_path);
 
 	if (disk_path)
@@ -862,15 +895,9 @@ int imsm_is_nvme_supported(int disk_fd, int verbose)
  */
 int is_multipath_nvme(int disk_fd)
 {
-	char path_buf[PATH_MAX];
 	char ns_path[PATH_MAX];
-	char *kname = fd2kname(disk_fd);
-
-	if (!kname)
-		return 0;
-	sprintf(path_buf, "/sys/block/%s", kname);
 
-	if (!realpath(path_buf, ns_path))
+	if (!diskfd_to_devpath(disk_fd, 0, ns_path))
 		return 0;
 
 	if (strncmp(ns_path, NVME_SUBSYS_PATH, strlen(NVME_SUBSYS_PATH)) == 0)
diff --git a/platform-intel.h b/platform-intel.h
index 8396a0f..f93add5 100644
--- a/platform-intel.h
+++ b/platform-intel.h
@@ -237,7 +237,7 @@ static inline char *guid_str(char *buf, struct efi_guid guid)
 }
 
 char *get_nvme_multipath_dev_hw_path(const char *dev_path);
-char *diskfd_to_devpath(int fd);
+char *diskfd_to_devpath(int fd, int dev_level, char *buf);
 __u16 devpath_to_vendor(const char *dev_path);
 struct sys_dev *find_driver_devices(const char *bus, const char *driver);
 struct sys_dev *find_intel_devices(void);
@@ -245,7 +245,7 @@ const struct imsm_orom *find_imsm_capability(struct sys_dev *hba);
 const struct imsm_orom *find_imsm_orom(void);
 int disk_attached_to_hba(int fd, const char *hba_path);
 int devt_attached_to_hba(dev_t dev, const char *hba_path);
-char *devt_to_devpath(dev_t dev);
+char *devt_to_devpath(dev_t dev, int dev_level, char *buf);
 int path_attached_to_hba(const char *disk_path, const char *hba_path);
 const char *get_sys_dev_type(enum sys_dev_type);
 const struct orom_entry *get_orom_entry_by_device_id(__u16 dev_id);
diff --git a/super-intel.c b/super-intel.c
index 5469912..cff8550 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -694,7 +694,7 @@ static struct sys_dev* find_disk_attached_hba(int fd, const char *devname)
 	if (fd < 0)
 		disk_path  = (char *) devname;
 	else
-		disk_path = diskfd_to_devpath(fd);
+		disk_path = diskfd_to_devpath(fd, 1, NULL);
 
 	if (!disk_path)
 		return 0;
@@ -2253,7 +2253,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 
 		if (sscanf(ent->d_name, "%d:%d", &major, &minor) != 2)
 			continue;
-		path = devt_to_devpath(makedev(major, minor));
+		path = devt_to_devpath(makedev(major, minor), 1, NULL);
 		if (!path)
 			continue;
 		if (!path_attached_to_hba(path, hba_path)) {
@@ -2407,7 +2407,7 @@ static int print_nvme_info(struct sys_dev *hba)
 				continue;
 			}
 
-			device_path = diskfd_to_devpath(fd);
+			device_path = diskfd_to_devpath(fd, 1, NULL);
 			if (!device_path) {
 				close(fd);
 				continue;
@@ -4015,28 +4015,13 @@ static int compare_super_imsm(struct supertype *st, struct supertype *tst,
 
 static void fd2devname(int fd, char *name)
 {
-	struct stat st;
-	char path[256];
-	char dname[PATH_MAX];
 	char *nm;
-	int rv;
-
-	name[0] = '\0';
-	if (fstat(fd, &st) != 0)
-		return;
-	sprintf(path, "/sys/dev/block/%d:%d",
-		major(st.st_rdev), minor(st.st_rdev));
 
-	rv = readlink(path, dname, sizeof(dname)-1);
-	if (rv <= 0)
+	nm = fd2kname(fd);
+	if (!nm)
 		return;
 
-	dname[rv] = '\0';
-	nm = strrchr(dname, '/');
-	if (nm) {
-		nm++;
-		snprintf(name, MAX_RAID_SERIAL_LEN, "/dev/%s", nm);
-	}
+	snprintf(name, MAX_RAID_SERIAL_LEN, "/dev/%s", nm);
 }
 
 static int nvme_get_serial(int fd, void *buf, size_t buf_len)
@@ -5941,29 +5926,23 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 		free(dd);
 		abort();
 	}
+
 	if (super->hba && ((super->hba->type == SYS_DEV_NVME) ||
 	   (super->hba->type == SYS_DEV_VMD))) {
 		int i;
-		char *devpath = diskfd_to_devpath(fd);
-		char controller_path[PATH_MAX];
-		char *controller_name;
+		char cntrl_path[PATH_MAX];
+		char *cntrl_name;
+		char pci_dev_path[PATH_MAX];
 
-		if (!devpath) {
-			pr_err("failed to get devpath, aborting\n");
+		if (!diskfd_to_devpath(fd, 2, pci_dev_path) ||
+		    !diskfd_to_devpath(fd, 1, cntrl_path)) {
+			pr_err("failed to get dev_path, aborting\n");
 			if (dd->devname)
 				free(dd->devname);
 			free(dd);
 			return 1;
 		}
 
-		snprintf(controller_path, PATH_MAX-1, "%s/device", devpath);
-
-		controller_name = basename(devpath);
-		if (is_multipath_nvme(fd))
-			pr_err("%s controller supports Multi-Path I/O, Intel (R) VROC does not support multipathing\n", controller_name);
-
-		free(devpath);
-
 		if (!imsm_is_nvme_supported(dd->fd, 1)) {
 			if (dd->devname)
 				free(dd->devname);
@@ -5971,7 +5950,12 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 			return 1;
 		}
 
-		if (devpath_to_vendor(controller_path) == 0x8086) {
+		cntrl_name = basename(cntrl_path);
+		if (is_multipath_nvme(fd))
+			pr_err("%s controller supports Multi-Path I/O, Intel (R) VROC does not support multipathing\n",
+			       cntrl_name);
+
+		if (devpath_to_vendor(pci_dev_path) == 0x8086) {
 			/*
 			 * If Intel's NVMe drive has serial ended with
 			 * "-A","-B","-1" or "-2" it means that this is "x8"
@@ -6985,7 +6969,7 @@ get_devices(const char *hba_path)
 		char *path = NULL;
 		if (sscanf(ent->d_name, "%d:%d", &major, &minor) != 2)
 			continue;
-		path = devt_to_devpath(makedev(major, minor));
+		path = devt_to_devpath(makedev(major, minor), 1, NULL);
 		if (!path)
 			continue;
 		if (!path_attached_to_hba(path, hba_path)) {
@@ -10648,7 +10632,7 @@ int validate_container_imsm(struct mdinfo *info)
 	struct sys_dev *hba = NULL;
 	struct sys_dev *intel_devices = find_intel_devices();
 	char *dev_path = devt_to_devpath(makedev(info->disk.major,
-									info->disk.minor));
+						 info->disk.minor), 1, NULL);
 
 	for (idev = intel_devices; idev; idev = idev->next) {
 		if (dev_path && strstr(dev_path, idev->path)) {
@@ -10669,7 +10653,8 @@ int validate_container_imsm(struct mdinfo *info)
 	struct mdinfo *dev;
 
 	for (dev = info->next; dev; dev = dev->next) {
-		dev_path = devt_to_devpath(makedev(dev->disk.major, dev->disk.minor));
+		dev_path = devt_to_devpath(makedev(dev->disk.major,
+						   dev->disk.minor), 1, NULL);
 
 		struct sys_dev *hba2 = NULL;
 		for (idev = intel_devices; idev; idev = idev->next) {
@@ -11181,7 +11166,7 @@ static const char *imsm_get_disk_controller_domain(const char *path)
 		struct sys_dev* hba;
 		char *path;
 
-		path = devt_to_devpath(st.st_rdev);
+		path = devt_to_devpath(st.st_rdev, 1, NULL);
 		if (path == NULL)
 			return "unknown";
 		hba = find_disk_attached_hba(-1, path);
-- 
2.26.2

[PATCH 2/4] imsm: add devpath_to_char method

[Mariusz Tkaczyk]

Add method for reading sysfs attributes and propagate it across IMSM code.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 platform-intel.c | 23 +++++++++++++++++++++++
 platform-intel.h |  2 ++
 super-intel.c    | 33 +++++++++++++++------------------
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 2ed63ed..9401784 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -239,6 +239,29 @@ __u16 devpath_to_vendor(const char *dev_path)
 	return id;
 }
 
+/* Description: Read text value of dev_path/entry field
+ * Parameters:
+ *	dev_path - sysfs path to the device
+ *	entry - entry to be read
+ *	buf - buffer for read value
+ *	len - size of buf
+ *	verbose - error logging level
+ */
+int devpath_to_char(const char *dev_path, const char *entry, char *buf, int len,
+		    int verbose)
+{
+	char path[PATH_MAX];
+
+	snprintf(path, sizeof(path), "%s/%s", dev_path, entry);
+	if (load_sys(path, buf, len)) {
+		if (verbose)
+			pr_err("Cannot read %s, aborting\n", path);
+		return 1;
+	}
+
+	return 0;
+}
+
 struct sys_dev *find_intel_devices(void)
 {
 	struct sys_dev *ahci, *isci, *nvme;
diff --git a/platform-intel.h b/platform-intel.h
index f93add5..45d98cd 100644
--- a/platform-intel.h
+++ b/platform-intel.h
@@ -238,6 +238,8 @@ static inline char *guid_str(char *buf, struct efi_guid guid)
 
 char *get_nvme_multipath_dev_hw_path(const char *dev_path);
 char *diskfd_to_devpath(int fd, int dev_level, char *buf);
+int devpath_to_char(const char *dev_path, const char *entry, char *buf,
+		    int len, int verbose);
 __u16 devpath_to_vendor(const char *dev_path);
 struct sys_dev *find_driver_devices(const char *bus, const char *driver);
 struct sys_dev *find_intel_devices(void);
diff --git a/super-intel.c b/super-intel.c
index cff8550..c352f50 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2246,7 +2246,7 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 		char vendor[64];
 		char buf[1024];
 		int major, minor;
-		char *device;
+		char device[PATH_MAX];
 		char *c;
 		int port;
 		int type;
@@ -2262,20 +2262,15 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 			continue;
 		}
 
-		/* retrieve the scsi device type */
-		if (asprintf(&device, "/sys/dev/block/%d:%d/device/xxxxxxx", major, minor) < 0) {
+		/* retrieve the scsi device */
+		if (!devt_to_devpath(makedev(major, minor), 1, device)) {
 			if (verbose > 0)
-				pr_err("failed to allocate 'device'\n");
+				pr_err("failed to get device\n");
 			err = 2;
 			break;
 		}
-		sprintf(device, "/sys/dev/block/%d:%d/device/type", major, minor);
-		if (load_sys(device, buf, sizeof(buf)) != 0) {
-			if (verbose > 0)
-				pr_err("failed to read device type for %s\n",
-					path);
+		if (devpath_to_char(device, "type", buf, sizeof(buf), 0)) {
 			err = 2;
-			free(device);
 			break;
 		}
 		type = strtoul(buf, NULL, 10);
@@ -2284,8 +2279,9 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 		if (!(type == 0 || type == 7 || type == 14)) {
 			vendor[0] = '\0';
 			model[0] = '\0';
-			sprintf(device, "/sys/dev/block/%d:%d/device/vendor", major, minor);
-			if (load_sys(device, buf, sizeof(buf)) == 0) {
+
+			if (devpath_to_char(device, "vendor", buf,
+					    sizeof(buf), 0) == 0) {
 				strncpy(vendor, buf, sizeof(vendor));
 				vendor[sizeof(vendor) - 1] = '\0';
 				c = (char *) &vendor[sizeof(vendor) - 1];
@@ -2293,8 +2289,9 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 					*c-- = '\0';
 
 			}
-			sprintf(device, "/sys/dev/block/%d:%d/device/model", major, minor);
-			if (load_sys(device, buf, sizeof(buf)) == 0) {
+
+			if (devpath_to_char(device, "model", buf,
+					    sizeof(buf), 0) == 0) {
 				strncpy(model, buf, sizeof(model));
 				model[sizeof(model) - 1] = '\0';
 				c = (char *) &model[sizeof(model) - 1];
@@ -2319,7 +2316,6 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 				}
 		} else
 			buf[0] = '\0';
-		free(device);
 
 		/* chop device path to 'host%d' and calculate the port number */
 		c = strchr(&path[hba_len], '/');
@@ -4026,7 +4022,7 @@ static void fd2devname(int fd, char *name)
 
 static int nvme_get_serial(int fd, void *buf, size_t buf_len)
 {
-	char path[60];
+	char path[PATH_MAX];
 	char *name = fd2kname(fd);
 
 	if (!name)
@@ -4035,9 +4031,10 @@ static int nvme_get_serial(int fd, void *buf, size_t buf_len)
 	if (strncmp(name, "nvme", 4) != 0)
 		return 1;
 
-	snprintf(path, sizeof(path) - 1, "/sys/block/%s/device/serial", name);
+	if (!diskfd_to_devpath(fd, 1, path))
+		return 1;
 
-	return load_sys(path, buf, buf_len);
+	return devpath_to_char(path, "serial", buf, buf_len, 0);
 }
 
 extern int scsi_get_serial(int fd, void *buf, size_t buf_len);
-- 
2.26.2

[PATCH 3/4] imsm: Limit support to the lowest namespace

[Mariusz Tkaczyk]

First namespace existence is not quaranted by NVMe specification.
Instead first the smallest one shall be chosen.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 platform-intel.c |  81 +++++++++++++++++++++++--------
 platform-intel.h |   2 +-
 super-intel.c    | 124 +++++++++++++++++++++++------------------------
 3 files changed, 123 insertions(+), 84 deletions(-)

diff --git a/platform-intel.c b/platform-intel.c
index 9401784..5a8729e 100644
--- a/platform-intel.c
+++ b/platform-intel.c
@@ -879,36 +879,75 @@ char *vmd_domain_to_controller(struct sys_dev *hba, char *buf)
 	closedir(dir);
 	return NULL;
 }
-/* Verify that NVMe drive is supported by IMSM
+
+/* Scan over all controller's namespaces and compare nsid value to verify if
+ * current one is supported. The routine doesn't check IMSM capabilities for
+ * namespace. Only one nvme namespace is supported by IMSM.
+ * Paramteres:
+ *	fd - open descriptor to the nvme namespace
+ *	verbose - error logging level
  * Returns:
- *	0 - not supported
- *	1 - supported
+ *	1 - if namespace is supported
+ *	0 - otherwise
  */
-int imsm_is_nvme_supported(int disk_fd, int verbose)
+int imsm_is_nvme_namespace_supported(int fd, int verbose)
 {
-	char nsid_path[PATH_MAX];
-	char buf[PATH_MAX];
-	struct stat stb;
+	DIR *dir = NULL;
+	struct dirent *ent;
+	char cntrl_path[PATH_MAX];
+	char ns_path[PATH_MAX];
+	unsigned long long lowest_nsid = ULLONG_MAX;
+	unsigned long long this_nsid;
+	int rv = 0;
 
-	if (disk_fd < 0)
-		return 0;
 
-	if (fstat(disk_fd, &stb))
-		return 0;
+	if (!diskfd_to_devpath(fd, 1, cntrl_path) ||
+	    !diskfd_to_devpath(fd, 0, ns_path)) {
+		if (verbose)
+			pr_err("Cannot get device paths\n");
+		goto abort;
+	}
 
-	snprintf(nsid_path, PATH_MAX-1, "/sys/dev/block/%d:%d/nsid",
-		 major(stb.st_rdev), minor(stb.st_rdev));
 
-	if (load_sys(nsid_path, buf, sizeof(buf))) {
-		pr_err("Cannot read %s, rejecting drive\n", nsid_path);
-		return 0;
-	}
-	if (strtoll(buf, NULL, 10) != 1) {
+	if (devpath_to_ll(ns_path, "nsid", &this_nsid)) {
 		if (verbose)
-			pr_err("Only first namespace is supported by IMSM, aborting\n");
-		return 0;
+			pr_err("Cannot read nsid value for %s",
+			       basename(ns_path));
+		goto abort;
 	}
-	return 1;
+
+	dir = opendir(cntrl_path);
+	if (!dir)
+		goto abort;
+
+	/* The lowest nvme namespace is supported */
+	for (ent = readdir(dir); ent; ent = readdir(dir)) {
+		unsigned long long curr_nsid;
+		char curr_ns_path[PATH_MAX + 256];
+
+		if (!strstr(ent->d_name, "nvme"))
+			continue;
+
+		snprintf(curr_ns_path, sizeof(curr_ns_path), "%s/%s",
+			 cntrl_path, ent->d_name);
+
+		if (devpath_to_ll(curr_ns_path, "nsid", &curr_nsid))
+			goto abort;
+
+		if (lowest_nsid > curr_nsid)
+			lowest_nsid = curr_nsid;
+	}
+
+	if (this_nsid == lowest_nsid)
+		rv = 1;
+	else if (verbose)
+		pr_err("IMSM is supported on the lowest NVMe namespace\n");
+
+abort:
+	if (dir)
+		closedir(dir);
+
+	return rv;
 }
 
 /* Verify if multipath is supported by NVMe controller
diff --git a/platform-intel.h b/platform-intel.h
index 45d98cd..6238d23 100644
--- a/platform-intel.h
+++ b/platform-intel.h
@@ -254,6 +254,6 @@ const struct orom_entry *get_orom_entry_by_device_id(__u16 dev_id);
 const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
 struct sys_dev *device_by_id(__u16 device_id);
 struct sys_dev *device_by_id_and_path(__u16 device_id, const char *path);
-int imsm_is_nvme_supported(int disk_fd, int verbose);
 int is_multipath_nvme(int disk_fd);
+int imsm_is_nvme_namespace_supported(int disk_fd, int verbose);
 char *vmd_domain_to_controller(struct sys_dev *hba, char *buf);
diff --git a/super-intel.c b/super-intel.c
index c352f50..fdcefb6 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -2381,49 +2381,51 @@ static int ahci_enumerate_ports(const char *hba_path, int port_count, int host_b
 
 static int print_nvme_info(struct sys_dev *hba)
 {
-	char buf[1024];
-	char *device_path;
 	struct dirent *ent;
 	DIR *dir;
-	int fd;
 
 	dir = opendir("/sys/block/");
 	if (!dir)
 		return 1;
 
 	for (ent = readdir(dir); ent; ent = readdir(dir)) {
-		if (strstr(ent->d_name, "nvme")) {
-			fd = open_dev(ent->d_name);
-			if (fd < 0)
-				continue;
+		char ns_path[PATH_MAX];
+		char cntrl_path[PATH_MAX];
+		char buf[PATH_MAX];
+		int fd = -1;
 
-			if (!imsm_is_nvme_supported(fd, 0)) {
-				if (fd >= 0)
-					close(fd);
-				continue;
-			}
+		if (!strstr(ent->d_name, "nvme"))
+			goto skip;
 
-			device_path = diskfd_to_devpath(fd, 1, NULL);
-			if (!device_path) {
-				close(fd);
-				continue;
-			}
+		fd = open_dev(ent->d_name);
+		if (fd < 0)
+			goto skip;
 
-			if (path_attached_to_hba(device_path, hba->path)) {
-				fd2devname(fd, buf);
-				if (hba->type == SYS_DEV_VMD)
-					printf(" NVMe under VMD : %s", buf);
-				else if (hba->type == SYS_DEV_NVME)
-					printf("    NVMe Device : %s", buf);
-				if (!imsm_read_serial(fd, NULL, (__u8 *)buf,
-						      sizeof(buf)))
-					printf(" (%s)\n", buf);
-				else
-					printf("()\n");
-			}
-			free(device_path);
+		if (!diskfd_to_devpath(fd, 0, ns_path) ||
+		    !diskfd_to_devpath(fd, 1, cntrl_path))
+			goto skip;
+
+		if (!path_attached_to_hba(cntrl_path, hba->path))
+			goto skip;
+
+		if (!imsm_is_nvme_namespace_supported(fd, 0))
+			goto skip;
+
+		fd2devname(fd, buf);
+		if (hba->type == SYS_DEV_VMD)
+			printf(" NVMe under VMD : %s", buf);
+		else if (hba->type == SYS_DEV_NVME)
+			printf("    NVMe Device : %s", buf);
+
+		if (!imsm_read_serial(fd, NULL, (__u8 *)buf,
+				      sizeof(buf)))
+			printf(" (%s)\n", buf);
+		else
+			printf("()\n");
+
+skip:
+		if (fd > -1)
 			close(fd);
-		}
 	}
 
 	closedir(dir);
@@ -5933,14 +5935,8 @@ static int add_to_super_imsm(struct supertype *st, mdu_disk_info_t *dk,
 
 		if (!diskfd_to_devpath(fd, 2, pci_dev_path) ||
 		    !diskfd_to_devpath(fd, 1, cntrl_path)) {
-			pr_err("failed to get dev_path, aborting\n");
-			if (dd->devname)
-				free(dd->devname);
-			free(dd);
-			return 1;
-		}
+			pr_err("failed to get dev paths, aborting\n");
 
-		if (!imsm_is_nvme_supported(dd->fd, 1)) {
 			if (dd->devname)
 				free(dd->devname);
 			free(dd);
@@ -6665,7 +6661,7 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
 {
 	int fd;
 	unsigned long long ldsize;
-	struct intel_super *super;
+	struct intel_super *super = NULL;
 	int rv = 0;
 
 	if (level != LEVEL_CONTAINER)
@@ -6680,24 +6676,18 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
 				dev, strerror(errno));
 		return 0;
 	}
-	if (!get_dev_size(fd, dev, &ldsize)) {
-		close(fd);
-		return 0;
-	}
+	if (!get_dev_size(fd, dev, &ldsize))
+		goto exit;
 
 	/* capabilities retrieve could be possible
 	 * note that there is no fd for the disks in array.
 	 */
 	super = alloc_super();
-	if (!super) {
-		close(fd);
-		return 0;
-	}
-	if (!get_dev_sector_size(fd, NULL, &super->sector_size)) {
-		close(fd);
-		free_imsm(super);
-		return 0;
-	}
+	if (!super)
+		goto exit;
+
+	if (!get_dev_sector_size(fd, NULL, &super->sector_size))
+		goto exit;
 
 	rv = find_intel_hba_capability(fd, super, verbose > 0 ? dev : NULL);
 	if (rv != 0) {
@@ -6708,32 +6698,42 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
 			fd, str, super->orom, rv, raiddisks);
 #endif
 		/* no orom/efi or non-intel hba of the disk */
-		close(fd);
-		free_imsm(super);
-		return 0;
+		rv = 0;
+		goto exit;
 	}
-	close(fd);
 	if (super->orom) {
 		if (raiddisks > super->orom->tds) {
 			if (verbose)
 				pr_err("%d exceeds maximum number of platform supported disks: %d\n",
 					raiddisks, super->orom->tds);
-			free_imsm(super);
-			return 0;
+			goto exit;
 		}
 		if ((super->orom->attr & IMSM_OROM_ATTR_2TB_DISK) == 0 &&
 		    (ldsize >> 9) >> 32 > 0) {
 			if (verbose)
 				pr_err("%s exceeds maximum platform supported size\n", dev);
-			free_imsm(super);
-			return 0;
+			goto exit;
+		}
+
+		if (super->hba->type == SYS_DEV_VMD ||
+		    super->hba->type == SYS_DEV_NVME) {
+			if (!imsm_is_nvme_namespace_supported(fd, 1)) {
+				if (verbose)
+					pr_err("NVMe namespace %s is not supported by IMSM\n",
+						basename(dev));
+				goto exit;
+			}
 		}
 	}
 
 	*freesize = avail_size_imsm(st, ldsize >> 9, data_offset);
-	free_imsm(super);
+	rv = 1;
+exit:
+	if (super)
+		free_imsm(super);
+	close(fd);
 
-	return 1;
+	return rv;
 }
 
 static unsigned long long find_size(struct extent *e, int *idx, int num_extents)
-- 
2.26.2

[PATCH 4/4] Manage: Call validate_geometry when adding drive to external container

[Mariusz Tkaczyk]

When adding drive to container call validate_geometry to verify whether
drive is supported and can be addded to container.

Remove unused parameters from validate_geometry_imsm_container().
There is no need to pass them.
Don't calculate freesize if it is not mandatory. Make it configurable.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Manage.c      |  7 +++++++
 super-ddf.c   |  9 +++++----
 super-intel.c | 19 +++++++------------
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/Manage.c b/Manage.c
index 0a5f09b..f789e0c 100644
--- a/Manage.c
+++ b/Manage.c
@@ -992,6 +992,13 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 			return -1;
 		}
 
+		/* Check if metadata handler is able to accept the drive */
+		if (!tst->ss->validate_geometry(tst, LEVEL_CONTAINER, 0, 1, NULL,
+		    0, 0, dv->devname, NULL, 0, 1)) {
+			close(container_fd);
+			return -1;
+		}
+
 		Kill(dv->devname, NULL, 0, -1, 0);
 		dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
 		if (tst->ss->add_to_super(tst, &disc, dfd,
diff --git a/super-ddf.c b/super-ddf.c
index 2314762..80a40f8 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -3475,10 +3475,11 @@ validate_geometry_ddf_container(struct supertype *st,
 		return 0;
 	}
 	close(fd);
-
-	*freesize = avail_size_ddf(st, ldsize >> 9, INVALID_SECTORS);
-	if (*freesize == 0)
-		return 0;
+	if (freesize) {
+		*freesize = avail_size_ddf(st, ldsize >> 9, INVALID_SECTORS);
+		if (*freesize == 0)
+			return 0;
+	}
 
 	return 1;
 }
diff --git a/super-intel.c b/super-intel.c
index fdcefb6..fe45d93 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -6652,8 +6652,7 @@ static int store_super_imsm(struct supertype *st, int fd)
 }
 
 static int validate_geometry_imsm_container(struct supertype *st, int level,
-					    int layout, int raiddisks, int chunk,
-					    unsigned long long size,
+					    int raiddisks,
 					    unsigned long long data_offset,
 					    char *dev,
 					    unsigned long long *freesize,
@@ -6725,8 +6724,8 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
 			}
 		}
 	}
-
-	*freesize = avail_size_imsm(st, ldsize >> 9, data_offset);
+	if (freesize)
+		*freesize = avail_size_imsm(st, ldsize >> 9, data_offset);
 	rv = 1;
 exit:
 	if (super)
@@ -7586,15 +7585,11 @@ static int validate_geometry_imsm(struct supertype *st, int level, int layout,
 	 * if given unused devices create a container
 	 * if given given devices in a container create a member volume
 	 */
-	if (level == LEVEL_CONTAINER) {
+	if (level == LEVEL_CONTAINER)
 		/* Must be a fresh device to add to a container */
-		return validate_geometry_imsm_container(st, level, layout,
-							raiddisks,
-							*chunk,
-							size, data_offset,
-							dev, freesize,
-							verbose);
-	}
+		return validate_geometry_imsm_container(st, level, raiddisks,
+							data_offset, dev,
+							freesize, verbose);
 
 	/*
 	 * Size is given in sectors.
-- 
2.26.2

Re: [PATCH 0/4] imsm: lowest namespace support

[Jes Sorensen]

On 5/17/21 10:38 AM, Mariusz Tkaczyk wrote:
> Nvme specification doesn't guarantee the first namespace existence.  
> Imsm shall use lowest one instead of first. In this patchset imsm parts
> responsible for reading sysfs attrs were refactored and simplified to
> generic routines. Imsm compatibility checks were moved to
> validate_geometry_imsm_container. This prevents from writing metadata
> to rejected drive.
> 
> Mariusz Tkaczyk (4):
>   imsm: add generic method to resolve "device" links
>   imsm: add devpath_to_char method
>   imsm: Limit support to the lowest namespace
>   Manage: Call validate_geometry when adding drive to external container
> 
>  Manage.c         |   7 ++
>  platform-intel.c | 179 +++++++++++++++++++++++++++---------
>  platform-intel.h |   8 +-
>  super-ddf.c      |   9 +-
>  super-intel.c    | 233 +++++++++++++++++++++--------------------------
>  5 files changed, 256 insertions(+), 180 deletions(-)
> 

Applied!

Thanks,
Jes