Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-18 Thread Lennart Poettering 
On Sat, 16.05.15 10:18, Per Bergqvist (p...@bst.lu) wrote:

 Lennart,
 
 Thank you for all the comments. 
 
 I have changed everything except the 'No space between function name and 
 opening “(“‘.
 Cannot find anything about that in CODING_STYLE or evidence in other
 sources.

Hmm? This is from CODING_STYLE:

- Do not write foo (), write foo().

 +
 +static int nvme_identify(struct udev *udev, int fd, void *buf, __u32 
 buf_len) {
 +struct nvme_admin_cmd command = {
 +.opcode   = nvme_admin_identify,
 +.addr = (unsigned long)buf,
 +.data_len = buf_len,
 +.cdw10= 1 };

Hmm, why __u32? First of all, try to use userspace types,
i.e. uint32_t. But secondly, shouldn't this be size_t?

 +int main(int argc, char *argv[]) {
 +_cleanup_udev_unref_ struct udev *udev = NULL;
 +
 +struct nvme_id_ctrl nvme_id_ctrl = { .vid = 0 };

initializer appears unnecessary, as the compiler initializes all
fields to 0 anyway if they are unspecified. i.e. just write = { };
here...

 +node = argv[optind];
 +if (node == NULL) {
 +log_error(no node specified);
 +return EXIT_FAILURE;
 +}
 +
 +fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
 +if (fd  0) {
 +log_error_errno(errno, Unable to open '%s': %m, node);
 +return -errno;

needs to be return EXIT_FAILURE. After all this is the main()
function, where errors are not errno-style, but EXIT_FAILURE,
EXIT_SUCCESS or something else betwee 0..255...

 +}
 +
 +if (nvme_identify(udev, fd, nvme_id_ctrl, sizeof(struct 
 nvme_id_ctrl)) == 0) {
 +memcpy (model, nvme_id_ctrl.mn,
 sizeof(nvme_id_ctrl.mn));

Please remove the space...

 +if (serial[0] != '\0') {
 +printf(ID_SERIAL=%s_%s\n, model, serial);
 +printf(ID_SERIAL_SHORT=%s\n, serial);
 +} else {
 +printf(ID_SERIAL=%s\n, model);
 +}

Please no {} for single-line if blocks (or else blocks...)

To me this looks fine otherwise, but I figure Kay has to decide if
this goes in or not. He might want this as built-in rather than as
external tool though...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-16 Thread Per Bergqvist 
Lennart,

Thank you for all the comments. 

I have changed everything except the 'No space between function name and 
opening “(“‘.
Cannot find anything about that in CODING_STYLE or evidence in other sources.
Kept the call to nvme_identify more or less as before, please let me know if it 
has to be changed.

Please find the new patch below.

BR
Per

From 6cf318688baead0b4a737cf2c2d7618b6a131954 Mon Sep 17 00:00:00 2001
From: Per Bergqvist p...@bst.lu
Date: Sat, 16 May 2015 11:01:17 +0200
Subject: [PATCH] nvme_id following CODING_STYLE

---
 Makefile.am   |  11 +++
 rules/60-persistent-storage.rules |   5 ++
 src/udev/nvme_id/nvme_id.c| 149 ++
 3 files changed, 165 insertions(+)
 create mode 100644 src/udev/nvme_id/nvme_id.c

diff --git a/Makefile.am b/Makefile.am
index bf04d31..0de0d18 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3904,6 +3904,17 @@ dist_udevrules_DATA += \
rules/60-persistent-v4l.rules
 
 # 
--
+nvme_id_SOURCES = \
+   src/udev/nvme_id/nvme_id.c
+
+nvme_id_LDADD = \
+   libudev-internal.la \
+   libsystemd-shared.la
+
+udevlibexec_PROGRAMS += \
+   nvme_id
+
+# 
--
 accelerometer_SOURCES = \
src/udev/accelerometer/accelerometer.c
 
diff --git a/rules/60-persistent-storage.rules 
b/rules/60-persistent-storage.rules
index 25b44a5..d3368a5 100644
--- a/rules/60-persistent-storage.rules
+++ b/rules/60-persistent-storage.rules
@@ -42,6 +42,11 @@ KERNEL==cciss*, ENV{DEVTYPE}==disk, 
ENV{ID_SERIAL}!=?*, IMPORT{program}=s
 KERNEL==sd*|sr*|cciss*, ENV{DEVTYPE}==disk, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/$env{ID_BUS}-$env{ID_SERIAL}
 KERNEL==sd*|cciss*, ENV{DEVTYPE}==partition, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/$env{ID_BUS}-$env{ID_SERIAL}-part%n
 
+# NVMe
+KERNEL==nvme*, ENV{ID_SERIAL}!=?*, IMPORT{program}=nvme_id --export 
$devnode
+KERNEL==nvme*, ENV{DEVTYPE}==disk, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/nvme-$env{ID_SERIAL}
+KERNEL==nvme*, ENV{DEVTYPE}==partition, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/nvme-$env{ID_SERIAL}-part%n
+
 # firewire
 KERNEL==sd*[!0-9]|sr*, ATTRS{ieee1394_id}==?*, 
SYMLINK+=disk/by-id/ieee1394-$attr{ieee1394_id}
 KERNEL==sd*[0-9], ATTRS{ieee1394_id}==?*, 
SYMLINK+=disk/by-id/ieee1394-$attr{ieee1394_id}-part%n
diff --git a/src/udev/nvme_id/nvme_id.c b/src/udev/nvme_id/nvme_id.c
new file mode 100644
index 000..8941977
--- /dev/null
+++ b/src/udev/nvme_id/nvme_id.c
@@ -0,0 +1,149 @@
+/* -*- mode:c; tab-width:8; intent-tabs-mode:nil;  -*-
+ *
+ * nvme_id - reads model/serial/firmware revision numbers from nvme drives
+ *
+ * Copyright (C) 2015 Per Bergqvist p...@bst.lu
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/.
+ */
+
+#include stdio.h
+#include stdlib.h
+#include stdint.h
+#include unistd.h
+#include fcntl.h
+#include ctype.h
+#include string.h
+#include errno.h
+#include getopt.h
+#include linux/nvme.h
+#include sys/ioctl.h
+#include sys/types.h
+#include sys/stat.h
+
+#include libudev.h
+#include libudev-private.h
+#include udev-util.h
+#include log.h
+
+static int nvme_identify(struct udev *udev, int fd, void *buf, __u32 buf_len) {
+struct nvme_admin_cmd command = {
+.opcode   = nvme_admin_identify,
+.addr = (unsigned long)buf,
+.data_len = buf_len,
+.cdw10= 1 };
+
+if (ioctl(fd, NVME_IOCTL_ADMIN_CMD, command)  0)
+return -errno;
+
+return 0;
+}
+
+int main(int argc, char *argv[]) {
+_cleanup_udev_unref_ struct udev *udev = NULL;
+
+struct nvme_id_ctrl nvme_id_ctrl = { .vid = 0 };
+
+char model[sizeof(nvme_id_ctrl.mn)+1];
+char model_enc[4*sizeof(nvme_id_ctrl.mn)+1];
+char serial[sizeof(nvme_id_ctrl.sn)+1];
+char revision[sizeof(nvme_id_ctrl.fr)+1];
+
+const char *node = NULL;
+int export = 0;
+
+_cleanup_close_ int fd = -1;
+
+static const struct option options[] = {
+{ export, no_argument, NULL, 'x' },
+{ help, no_argument, NULL, 'h' },
+{}
+};
+
+log_parse_environment();
+log_open();
+
+udev = udev_new();
+if (udev ==

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-16 Thread Mantas Mikulėnas 
On May 16, 2015 11:18, Per Bergqvist p...@bst.lu wrote:

 Lennart,

 Thank you for all the comments.

 I have changed everything except the 'No space between function name and
opening “(“‘.
 Cannot find anything about that in CODING_STYLE or evidence in other
sources.

Most every call in the entire source tree uses that style should be good
enough as evidence.

If you don't care about being consistent with existing code, at least be
consistent with *your own* code, where you also use 'func()' most of the
time.

 +log_parse_environment();
 +log_open();
 +
 +udev = udev_new();
 +if (udev == NULL)
 +return EXIT_SUCCESS;
 +
 +while (1) {
 +int option;
 +
 +option = getopt_long(argc, argv, xh, options, NULL);

Here,

 +fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
 +if (fd  0) {
 +log_error_errno(errno, Unable to open '%s': %m, node);
 +return -errno;
 +}
 +
 +if (nvme_identify(udev, fd, nvme_id_ctrl, sizeof(struct
nvme_id_ctrl)) == 0) {

here,

 +memcpy (model, nvme_id_ctrl.mn, sizeof(nvme_id_ctrl.mn));

but for some reason not here.

This could be easily fixed up at commit time, though.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Per Bergqvist 
Hej,Seems like nobody thinks using the scsi_id for NVMe drives is a good idea so the alternative must be to introduce a new nvme_id utility.Please find a patch (relative 2.19) to add this.It is using native nvme ioctl to get the model, serial and firmware revision info.To apply:patch -p0  nvme_id.patchaclocalautomake./configureThe resulting entries in /dev/disk/by-id will look like:lrwxrwxrwx 1 root root 13 May 15 15:34 /dev/disk/by-id/nvme-INTEL_SSDPEDME400G4_CVMD43750011400AGN - ../../nvme1n1lrwxrwxrwx 1 root root 15 May 15 15:34 /dev/disk/by-id/nvme-INTEL_SSDPEDME400G4_CVMD43750011400AGN-part1 - ../../nvme1n1p1lrwxrwxrwx 1 root root 15 May 15 15:34 /dev/disk/by-id/nvme-INTEL_SSDPEDME400G4_CVMD43750011400AGN-part2 - ../../nvme1n1p2lrwxrwxrwx 1 root root 13 May 15 15:34 /dev/disk/by-id/nvme-INTEL_SSDPEDME400G4_CVMD43750013400AGN - ../../nvme0n1lrwxrwxrwx 1 root root 15 May 15 15:34 /dev/disk/by-id/nvme-INTEL_SSDPEDME400G4_CVMD43750013400AGN-part1 - ../../nvme0n1p1lrwxrwxrwx 1 root root 15 May 15 15:34 /dev/disk/by-id/nvme-INTEL_SSDPEDME400G4_CVMD43750013400AGN-part2 - ../../nvme0n1p2BRPer

nvme_id.patch
Description: Binary data
On 14 May 2015, at 18:12 , Lennart Poettering lenn...@poettering.net wrote:On Thu, 14.05.15 09:10, Per Bergqvist (p...@bst.lu) wrote:Hej,There was a request for addition of NVMe disks Feb 10 2014 by Harald Hoyer which was sort of rejected by Kay Sievers by referring to “we should find out what to do for nvme beforeadding new users of scsi_id”.Well, nothing changed really: we'd like to remove scsi_id fromsystemd/udev upstream. Please ask sg_utils to pick it upinstead. We'll continue to support the status quo for a while longer,but it can't stay this way, and we shouldn't add new features toit while it's in limbo.Lennart-- Lennart Poettering, Red Hat


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Lennart Poettering 
On Fri, 15.05.15 15:56, Per Bergqvist (p...@bst.lu) wrote:

 Hej,
 
 Seems like nobody thinks using the scsi_id for NVMe drives is a good idea so 
 the alternative must be to introduce a new nvme_id utility.
 Please find a patch (relative 2.19) to add this.

Hmm, did you intend to attach something to your mail? You didn't...

Also, please make sure to send git-format-patch formatted patches. THanks.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Per Bergqvist 
Hej,

Please (finally) find a patched for v219 to add a nvme_id utility and add 
support for NVMe disks in 60-persistent-storage.rules. 

BR
Per

---
Makefile.am   |  11 +++
rules/60-persistent-storage.rules |   5 ++
src/udev/nvme_id/nvme_id.c| 155 ++
3 files changed, 171 insertions(+)
create mode 100644 src/udev/nvme_id/nvme_id.c

diff --git a/Makefile.am b/Makefile.am
index bf04d31..0de0d18 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3904,6 +3904,17 @@ dist_udevrules_DATA += \
rules/60-persistent-v4l.rules

# --
+nvme_id_SOURCES = \
+   src/udev/nvme_id/nvme_id.c
+
+nvme_id_LDADD = \
+   libudev-internal.la \
+   libsystemd-shared.la
+
+udevlibexec_PROGRAMS += \
+   nvme_id
+
+# 
--
accelerometer_SOURCES = \
src/udev/accelerometer/accelerometer.c

diff --git a/rules/60-persistent-storage.rules 
b/rules/60-persistent-storage.rules
index 25b44a5..d3368a5 100644
--- a/rules/60-persistent-storage.rules
+++ b/rules/60-persistent-storage.rules
@@ -42,6 +42,11 @@ KERNEL==cciss*, ENV{DEVTYPE}==disk, 
ENV{ID_SERIAL}!=?*, IMPORT{program}=s
KERNEL==sd*|sr*|cciss*, ENV{DEVTYPE}==disk, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/$env{ID_BUS}-$env{ID_SERIAL}
KERNEL==sd*|cciss*, ENV{DEVTYPE}==partition, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/$env{ID_BUS}-$env{ID_SERIAL}-part%n

+# NVMe
+KERNEL==nvme*, ENV{ID_SERIAL}!=?*, IMPORT{program}=nvme_id --export 
$devnode
+KERNEL==nvme*, ENV{DEVTYPE}==disk, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/nvme-$env{ID_SERIAL}
+KERNEL==nvme*, ENV{DEVTYPE}==partition, ENV{ID_SERIAL}==?*, 
SYMLINK+=disk/by-id/nvme-$env{ID_SERIAL}-part%n
+
# firewire
KERNEL==sd*[!0-9]|sr*, ATTRS{ieee1394_id}==?*, 
SYMLINK+=disk/by-id/ieee1394-$attr{ieee1394_id}
KERNEL==sd*[0-9], ATTRS{ieee1394_id}==?*, 
SYMLINK+=disk/by-id/ieee1394-$attr{ieee1394_id}-part%n
diff --git a/src/udev/nvme_id/nvme_id.c b/src/udev/nvme_id/nvme_id.c
new file mode 100644
index 000..aa0728b
--- /dev/null
+++ b/src/udev/nvme_id/nvme_id.c
@@ -0,0 +1,155 @@
+/*
+ * nvme_id - reads model/serial/firmware revision numbers from nvme drives
+ *
+ * Copyright (C) 2015 Per Bergqvist p...@bst.lu
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/.
+ */
+
+#include stdio.h
+#include stdlib.h
+#include stdint.h
+#include unistd.h
+#include fcntl.h
+#include ctype.h
+#include string.h
+#include errno.h
+#include getopt.h
+#include linux/nvme.h
+#include sys/ioctl.h
+#include sys/types.h
+#include sys/stat.h
+
+#include libudev.h
+#include libudev-private.h
+#include udev-util.h
+#include log.h
+
+static int nvme_identify(struct udev *udev,
+ int fd,
+ void *ptr)
+{
+   struct nvme_admin_cmd command;
+   memset(command, 0, sizeof(command));
+   
+   command.opcode = nvme_admin_identify;
+   command.nsid = 0;
+   command.addr = (unsigned long)ptr;
+   command.data_len = sizeof(struct nvme_id_ctrl);
+command.cdw10 = 1;
+
+return ioctl(fd, NVME_IOCTL_ADMIN_CMD, command);
+}
+
+int main(int argc, char *argv[])
+{
+_cleanup_udev_unref_ struct udev *udev = NULL;
+
+   struct nvme_id_ctrl nvme_ctrl;
+   
+char model[41];
+char model_enc[256];
+char serial[21];
+char revision[9];
+   
+const char *node = NULL;
+int export = 0;
+   
+_cleanup_close_ int fd = -1;
+
+static const struct option options[] = {
+{ export, no_argument, NULL, 'x' },
+{ help, no_argument, NULL, 'h' },
+{}
+};
+
+log_parse_environment();
+log_open();
+
+udev = udev_new();
+if (udev == NULL)
+return 0;
+
+while (1) {
+int option;
+
+option = getopt_long(argc, argv, xh, options, NULL);
+if (option == -1)
+break;
+
+switch (option) {
+case 'x':
+export = 1;
+break;
+case 'h':
+printf(Usage: nvme_id [--export] [--help] device\n
+

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Lennart Poettering 
On Fri, 15.05.15 20:53, Per Bergqvist (p...@bst.lu) wrote:

 Hej,
 
 Please (finally) find a patched for v219 to add a nvme_id utility
 and add support for NVMe disks in 60-persistent-storage.rules.

I figure Kay and Tom have to decide if this goes in, but here's a
quick code review, in case they are in favour:

 +static int nvme_identify(struct udev *udev,
 + int fd,
 + void *ptr)
 +{

Please see CODING_STYLE. We tend to place the opening bracket on the
same line as the function name. (yes some older code in the systemd
tree does not follow this rule, but please try to follow this for new
code).

 + struct nvme_admin_cmd command;
 + memset(command, 0, sizeof(command));

See CODING_STYLE, we tend to prefer initializing structures on the
stack via structure initializers, so that we don't need explicit
memset(). i.e.

struct nvme_admin_cmd command = {
.opcode = ...,
}

 + 
 + command.opcode = nvme_admin_identify;
 + command.nsid = 0;
 + command.addr = (unsigned long)ptr;
 + command.data_len = sizeof(struct nvme_id_ctrl);
 +command.cdw10 = 1;

Indenting is weird. Please strictly use 8ch space indenting.

 +return ioctl(fd, NVME_IOCTL_ADMIN_CMD, command);
 +}

We generally try to follow the rule to return kernel-style negative
errno error codes from the functions we define, even if the underlying
syscalls don't. Hence, please rewrite this as:

if (ioctl(...)  0)
return -errno;

return 0;

 +
 +int main(int argc, char *argv[])
 +{

The opening bracket on the same line as the function name please.

 +_cleanup_udev_unref_ struct udev *udev = NULL;
 +
 + struct nvme_id_ctrl nvme_ctrl;
 + 
 +char model[41];
 +char model_enc[256];
 +char serial[21];
 +char revision[9];

Weird indenting...

 +
 +while (1) {
 +int option;
 +
 +option = getopt_long(argc, argv, xh, options, NULL);
 +if (option == -1)
 +break;
 +
 +switch (option) {
 +case 'x':
 +export = 1;
 +break;
 +case 'h':
 +printf(Usage: nvme_id [--export] [--help] 
 device\n
 + -x,--exportprint values as environment 
 keys\n
 + -h,--help  print this help text\n\n);
 +return 0;
 +}
 +}
 +
 +node = argv[optind];
 +if (node == NULL) {
 +log_error(no node specified);
 +return 1;

Please use libc's EXIT_FAILURE define here.

 +}
 +
 +fd = open(node, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
 +if (fd  0) {
 +log_error(unable to open '%s', node);
 +return 1;
 +}

Similar here.

Also, please use use this log_error_errno() syntax if there's an errno
to report:

log_error_errno(errno, Unable to open '%s': %m, node);


 +
 + memset(nvme_ctrl, 0, sizeof(struct nvme_id_ctrl));

Please initialize at time of declaration.

 + 
 +if (nvme_identify(udev, fd, nvme_ctrl) == 0) {
 +int i;
 +memcpy (model, nvme_ctrl.mn, 40);

No space between function name and opening (, please, see CODING_STYLE.

 +for(i=39;i=0;i--) if (model[i]== ' ') model[i] = '\0';
 else break;

Please use strstrip() for this.

 +model[40] = '\0';
 +udev_util_encode_string(model, model_enc, sizeof(model_enc));
 +util_replace_whitespace((char *) nvme_ctrl.mn, model,
 40);

Hmm, use sizeof(model) instead of 40?

 +if (export) {
 +printf(ID_TYPE=nvme\n);
 +printf(ID_MODEL=%s\n, model);
 +printf(ID_MODEL_ENC=%s\n, model_enc);
 +printf(ID_REVISION=%s\n, revision);
 +if (serial[0] != '\0') {
 +printf(ID_SERIAL=%s_%s\n, model, serial);
 +printf(ID_SERIAL_SHORT=%s\n, serial);
 +} else {
 +printf(ID_SERIAL=%s\n, model);
 +}
 +
 +} else {
 +if (serial[0] != '\0')
 +printf(%s_%s\n, model, serial);
 +else
 +printf(%s\n, model);
 +}
 +
 +return 0;

Use EXIT_SUCCESS here...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Per Bergqvist 
OK, now in git-format-patch.BRPer

nvme_id.patch
Description: Binary data
On 15 May 2015, at 17:03 , Lennart Poettering lenn...@poettering.net wrote:On Fri, 15.05.15 15:56, Per Bergqvist (p...@bst.lu) wrote:Hej,Seems like nobody thinks using the scsi_id for NVMe drives is a good idea so the alternative must be to introduce a new nvme_id utility.Please find a patch (relative 2.19) to add this.Hmm, did you intend to attach something to your mail? You didn't...Also, please make sure to send git-format-patch formatted patches. THanks.Lennart-- Lennart Poettering, Red Hat


___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Lennart Poettering 
On Fri, 15.05.15 18:41, Per Bergqvist (p...@bst.lu) wrote:

 OK, now in git-format-patch.

There's no patch in this mails of yours...

 
 BR
 Per
 
 
 
 On 15 May 2015, at 17:03 , Lennart Poettering lenn...@poettering.net wrote:
 
  On Fri, 15.05.15 15:56, Per Bergqvist (p...@bst.lu) wrote:
  
  Hej,
  
  Seems like nobody thinks using the scsi_id for NVMe drives is a good idea 
  so the alternative must be to introduce a new nvme_id utility.
  Please find a patch (relative 2.19) to add this.
  
  Hmm, did you intend to attach something to your mail? You didn't...
  
  Also, please make sure to send git-format-patch formatted patches. THanks.
  
  Lennart
  
  -- 
  Lennart Poettering, Red Hat
 
 
 
 


Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Thomas H.P. Andersen 
On Fri, May 15, 2015 at 7:30 PM, Lennart Poettering
lenn...@poettering.net wrote:
 On Fri, 15.05.15 18:41, Per Bergqvist (p...@bst.lu) wrote:

 OK, now in git-format-patch.

 There's no patch in this mails of yours...

I see patches (nvme_id.patch) attached to Per's last two emails.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-15 Thread Lennart Poettering 
On Fri, 15.05.15 19:37, Thomas H.P. Andersen (pho...@gmail.com) wrote:

 On Fri, May 15, 2015 at 7:30 PM, Lennart Poettering
 lenn...@poettering.net wrote:
  On Fri, 15.05.15 18:41, Per Bergqvist (p...@bst.lu) wrote:
 
  OK, now in git-format-patch.
 
  There's no patch in this mails of yours...
 
 I see patches (nvme_id.patch) attached to Per's last two emails.

Oh, uhmm. Per's mailer is broken... It encoded the mail as
multipart/alternative, with one part text/plain, and one part
multipart/mixed which contained HTML mail with the patch. My mailer
showed the plain version which lacked the patch.

Per, your mailer is broken. Also, please don't send HTML mail to this
list anyway. In fact, most technical mailing lists are not too keen on
HTML mails...

Any chance you can resend as normal text/plain message?

Thanks,

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Keith Busch 
Kay Sievers kay at vrfy.org writes:
 On Thu, May 14, 2015 at 6:12 PM, Lennart Poettering
 lennart at poettering.net wrote:
  Well, nothing changed really: we'd like to remove scsi_id from
  systemd/udev upstream. Please ask sg_utils to pick it up
  instead. We'll continue to support the status quo for a while longer,
  but it can't stay this way, and we shouldn't add new features to
  it while it's in limbo.
 
 Yeah, not sure how much NVMe has to do with SCSI. It might make more
 sense to export the primary values to identify these devices by the
 kernel driver in /sys, instead of excapsulating/emulating the SCSI
 behavior. The kernel does that for other block device types like mmc
 too.

I also like native nvme identification over scsi translation. Is the mmc
way your preferred method for consuming these? If so, can you provide a
pointer in that driver for what they're doing? I can probably get it in
the nvme driver for 4.2.

Thanks,
Keith

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Per Bergqvist 
Hej,

The kernel driver in /sys provide little useful information with current 
releases.

Even if the nvme driver is extended in future kernels as proposed by Keith 
Busch this will not solve 
the problem with kernels used today.

NVMe drives are being deployed now (in mass) and it will only increase rapidly 
due to the supreme performance.

I think the most important issue is to define a naming scheme in 
/dev/disk/by-id to ensure consistent 
naming and how to resolve this naming is secondary.

If the kernel can provide the more useful information in 4.2 it is great but we 
need a scheme that is possible 
to create with currently deployed kernels including 2.6.* kernels.

The scsi_id on arch 4.0.1-1 returns a ID_SERIAL of:
ID_SERIAL=SNVMe_INTEL_SSDPEDMD40CVFT504600EE400BGN
or 
ID_VENDOR=NVMe
ID_MODEL=INTEL_SSDPEDMD40
ID_SCSI_SERIAL=CVFT504600EE400BGN

on the same kind of system with SuSE 12 (3.12.28) system what you would get 
(different unit and serial):

ID_SERIAL=365cd2e4080864356494e0001
or
ID_VENDOR=NVMe
ID_MODEL=INTEL_SSDPEDMD40
ID_SCSI_SERIAL=CVFT4324006Z400BGN

A possible scheme to achieve with both is (assuming scsi_id being used):

/dev/disk/by-id/nvme-$ENV{ID_MODEL}_$ENV{ID_SCSI_SERIAL}

Distributions could then add necessary patches to achieve the same result as 
long as we know what 
we want to achieve.
If additional support is added in future kernel releases, systemd/udev can move 
away from use of scsi_id.

BR
Per

On 14 May 2015, at 19:07 , Keith Busch keith.bu...@intel.com wrote:

 Kay Sievers kay at vrfy.org writes:
 On Thu, May 14, 2015 at 6:12 PM, Lennart Poettering
 lennart at poettering.net wrote:
 Well, nothing changed really: we'd like to remove scsi_id from
 systemd/udev upstream. Please ask sg_utils to pick it up
 instead. We'll continue to support the status quo for a while longer,
 but it can't stay this way, and we shouldn't add new features to
 it while it's in limbo.
 
 Yeah, not sure how much NVMe has to do with SCSI. It might make more
 sense to export the primary values to identify these devices by the
 kernel driver in /sys, instead of excapsulating/emulating the SCSI
 behavior. The kernel does that for other block device types like mmc
 too.
 
 I also like native nvme identification over scsi translation. Is the mmc
 way your preferred method for consuming these? If so, can you provide a
 pointer in that driver for what they're doing? I can probably get it in
 the nvme driver for 4.2.
 
 Thanks,
 Keith
 
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Best Regards
Per Bergqvist

Managing Director
Bergqvist Software Technologies 

Mobile: + 352 691 686 600






___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Kay Sievers 
On Thu, May 14, 2015 at 6:12 PM, Lennart Poettering
lenn...@poettering.net wrote:
 On Thu, 14.05.15 09:10, Per Bergqvist (p...@bst.lu) wrote:

 There was a request for addition of NVMe disks Feb 10 2014 by Harald Hoyer 
 which was
 sort of rejected by Kay Sievers by referring to “we should find out what to 
 do for nvme before
 adding new users of scsi_id”.

 Well, nothing changed really: we'd like to remove scsi_id from
 systemd/udev upstream. Please ask sg_utils to pick it up
 instead. We'll continue to support the status quo for a while longer,
 but it can't stay this way, and we shouldn't add new features to
 it while it's in limbo.

Yeah, not sure how much NVMe has to do with SCSI. It might make more
sense to export the primary values to identify these devices by the
kernel driver in /sys, instead of excapsulating/emulating the SCSI
behavior. The kernel does that for other block device types like mmc
too.

Kay
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Lennart Poettering 
On Thu, 14.05.15 09:10, Per Bergqvist (p...@bst.lu) wrote:

 Hej,
 
 There was a request for addition of NVMe disks Feb 10 2014 by Harald Hoyer 
 which was 
 sort of rejected by Kay Sievers by referring to “we should find out what to 
 do for nvme before
 adding new users of scsi_id”.

Well, nothing changed really: we'd like to remove scsi_id from
systemd/udev upstream. Please ask sg_utils to pick it up
instead. We'll continue to support the status quo for a while longer,
but it can't stay this way, and we shouldn't add new features to
it while it's in limbo.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Keith Busch 

On Thu, 14 May 2015, Dimitri John Ledkov wrote:

On 14 May 2015 at 08:10, Per Bergqvist p...@bst.lu wrote:

+# NVMe
+KERNEL==nvme*, ENV{ID_SCSI_SERIAL}!=?*, IMPORT{program}=scsi_id
--export --whitelisted -d $devnode
+KERNEL==nvme*, ENV{DEVTYPE}==disk, ENV{ID_SCSI_SERIAL}==?*,
SYMLINK+=disk/by-id/nvme-$env{ID_SCSI_SERIAL}
+KERNEL==nvme*, ENV{DEVTYPE}==partition, ENV{ID_SCSI_SERIAL}==?*,
SYMLINK+=disk/by-id/nvme-$env{ID_SCSI_SERIAL}-part%n
+



This looks odd to me, from NVMe perspective. I would have thought the
stable names would be generated based on the NVMe Namespace UIDs
rather than anything else.

Keith - could you comment on above ^ ?


Hi,

It depends on the nvme controller revision. 1.1 compatible and higher
namespaces export a unique id in EUI64 format. 1.0 devices don't have
this; the serial is as unique as it gets but will create collisions
if the device exports multiple namespaces so we have to append the
namespace identifier.

In either case, the driver provides a scsi translation to obtain
persistent and consistent unique identifiers using inquiry vpd 83h
so I think you want to use ID_SERIAL instead, but it appears scsi_id
doesn't use our vpd 83 with the SCSI string designator type used for
1.0 devices. Do I need to change this?
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Per Bergqvist 
Hej,

There was a request for addition of NVMe disks Feb 10 2014 by Harald Hoyer 
which was 
sort of rejected by Kay Sievers by referring to “we should find out what to do 
for nvme before
adding new users of scsi_id”.

More than a year later nothing has happened and recent commits actually branch 
out early
for nvme drives.

I have been using similar patches to Hal's albeit a little bit different.

+# NVMe
+KERNEL==nvme*, ENV{ID_SCSI_SERIAL}!=?*, IMPORT{program}=scsi_id --export 
--whitelisted -d $devnode
+KERNEL==nvme*, ENV{DEVTYPE}==disk, ENV{ID_SCSI_SERIAL}==?*, 
SYMLINK+=disk/by-id/nvme-$env{ID_SCSI_SERIAL}
+KERNEL==nvme*, ENV{DEVTYPE}==partition, ENV{ID_SCSI_SERIAL}==?*, 
SYMLINK+=disk/by-id/nvme-$env{ID_SCSI_SERIAL}-part%n
+

I have found that the ID_SCSI_SERIAL is the most reliable since ID_SERIAL 
varies depending on systemd/driver/kernel versions.
(My primary use for the NVMe drives is zfs log and cache devices. I test the 
zfs pool with different OS:es so I need
the naming to be consistent over all targets OS:es).

One may argue as Kay that it is not a scsi bus, but I think that the NVMe is 
related close enough to use the
scsi_id, especially if considering the “NVM Express: SCSI Translation 
Reference” 
(http://www.nvmexpress.org/wp-content/uploads/NVM_Express_-_SCSI_Translation_Reference-1_4_20150116_Gold2.pdf).

There is definitely a need for nvme support in udev and I think 
60-persistent-storage.rules is the correct place.

The only issue I can see is wether or not to use scsi_id or to introduce a new 
nvme_id utility.

BR
Per





___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Robert Milasan 
On Thu, 14 May 2015 09:10:40 +0200
Per Bergqvist p...@bst.lu wrote:
 
 +KERNEL==nvme*,ENV{DEVTYPE}==disk, ENV{ID_SCSI_SERIAL}==?*,

Wouldn't this make sense: KERNEL==nvme*[!0-9] (this is disk)

 +KERNEL==nvme*,ENV{DEVTYPE}==partition, ENV{ID_SCSI_SERIAL}==?*,

and this KERNEL==nvme*[0-9] (this is partition) ?


-- 
Robert Milasan

L3 Support Engineer
SUSE Linux (http://www.suse.com)
email: rmila...@suse.com
GPG fingerprint: B6FE F4A8 0FA3 3040 3402  6FE7 2F64 167C 1909 6D1A
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Per Bergqvist 
Not really the point.

I do however prefer to trust an explicit indication in ENV{DEVTYPE} rather than 
to guess device type based on device naming.
And so does the standard scsi rules.

BR
Per

On 14 May 2015, at 9:20 , Robert Milasan rmila...@suse.com wrote:

 On Thu, 14 May 2015 09:10:40 +0200
 Per Bergqvist p...@bst.lu wrote:
 
 +KERNEL==nvme*,ENV{DEVTYPE}==disk, ENV{ID_SCSI_SERIAL}==?*,
 
 Wouldn't this make sense: KERNEL==nvme*[!0-9] (this is disk)
 
 +KERNEL==nvme*,ENV{DEVTYPE}==partition, ENV{ID_SCSI_SERIAL}==?*,
 
 and this KERNEL==nvme*[0-9] (this is partition) ?
 
 
 -- 
 Robert Milasan
 
 L3 Support Engineer
 SUSE Linux (http://www.suse.com)
 email: rmila...@suse.com
 GPG fingerprint: B6FE F4A8 0FA3 3040 3402  6FE7 2F64 167C 1909 6D1A






___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] 60-persistent-storage.rules: add NVMe disks and partitions (again)

2015-05-14 Thread Dimitri John Ledkov 
Hello,

On 14 May 2015 at 08:10, Per Bergqvist p...@bst.lu wrote:
 Hej,

 There was a request for addition of NVMe disks Feb 10 2014 by Harald Hoyer
 which was
 sort of rejected by Kay Sievers by referring to “we should find out what to
 do for nvme before
 adding new users of scsi_id”.

 More than a year later nothing has happened and recent commits actually
 branch out early
 for nvme drives.

 I have been using similar patches to Hal's albeit a little bit different.

 +# NVMe
 +KERNEL==nvme*, ENV{ID_SCSI_SERIAL}!=?*, IMPORT{program}=scsi_id
 --export --whitelisted -d $devnode
 +KERNEL==nvme*, ENV{DEVTYPE}==disk, ENV{ID_SCSI_SERIAL}==?*,
 SYMLINK+=disk/by-id/nvme-$env{ID_SCSI_SERIAL}
 +KERNEL==nvme*, ENV{DEVTYPE}==partition, ENV{ID_SCSI_SERIAL}==?*,
 SYMLINK+=disk/by-id/nvme-$env{ID_SCSI_SERIAL}-part%n
 +


This looks odd to me, from NVMe perspective. I would have thought the
stable names would be generated based on the NVMe Namespace UIDs
rather than anything else.

Keith - could you comment on above ^ ?

 I have found that the ID_SCSI_SERIAL is the most reliable since ID_SERIAL
 varies depending on systemd/driver/kernel versions.
 (My primary use for the NVMe drives is zfs log and cache devices. I test the
 zfs pool with different OS:es so I need
 the naming to be consistent over all targets OS:es).

 One may argue as Kay that it is not a scsi bus, but I think that the NVMe is
 related close enough to use the
 scsi_id, especially if considering the “NVM Express: SCSI Translation
 Reference”
 (http://www.nvmexpress.org/wp-content/uploads/NVM_Express_-_SCSI_Translation_Reference-1_4_20150116_Gold2.pdf).

 There is definitely a need for nvme support in udev and I think
 60-persistent-storage.rules is the correct place.

 The only issue I can see is wether or not to use scsi_id or to introduce a
 new nvme_id utility.

 BR
 Per






 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel




-- 
Regards,

Dimitri.
Pura Vida!

https://clearlinux.org
Open Source Technology Center
Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel