nvme: Scan NVMe topology without nvme identify namespace

[ikegami-t]

Since the show-regs command needed this to scan for PCI.

[ikegami-t]

Depended on the libnvme PR changes linux-nvme/libnvme#790 and this can resolved the issue #2048 also.

[igaw]

Why is this necessary? What is the problem you are trying to solve?

[ikegami-t]

Since the show-regs command hanged up when CC.EN set to zero then reset the controller by timeout so the register values not able to print before the reset. This caused by the nvme identify namespace but not necessary for the command output. Later will do include the reason into the commit message.

[hreinecke]

I'd phrase it differently: why do we need to call 'nvme_scan()' here? It feels a bit too heavy-handed to me. We should just drop it and use the device name directly, much like we already do eg for 'nvme reset'.

[igaw]

I suspect the scan is needed if we want to list the fabrics properties. Still we could just do the scan only for the fabric devices and not for PCI devices.

[hreinecke]

For get_regs ? Does that even work on fabrics devices?

[igaw]

I thought that nvme_get_properties might rely on nvme_root_t but nope. I think we don't need the scan at all.

[igaw]

It's mmap_registers which wants nvme_root_t. Could we get away without it?

[hreinecke]

Like this?

diff --git a/nvme.c b/nvme.c
index 40496b54..8ffce55b 100644
--- a/nvme.c
+++ b/nvme.c
@@ -5249,32 +5249,10 @@ static int nvme_get_properties(int fd, void **pbar)
        return err;
 }
 
-static void *mmap_registers(nvme_root_t r, struct nvme_dev *dev)
+static void *mmap_registers(int fd)
 {
-       nvme_ctrl_t c = NULL;
-       nvme_ns_t n = NULL;
-
-       char path[512];
        void *membase;
-       int fd;
-
-       c = nvme_scan_ctrl(r, dev->name);
-       if (c) {
-               snprintf(path, sizeof(path), "%s/device/resource0",
-                       nvme_ctrl_get_sysfs_dir(c));
-               nvme_free_ctrl(c);
-       } else {
-               n = nvme_scan_namespace(dev->name);
-               if (!n) {
-                       nvme_show_error("Unable to find %s", dev->name);
-                       return NULL;
-               }
-               snprintf(path, sizeof(path), "%s/device/device/resource0",
-                        nvme_ns_get_sysfs_dir(n));
-               nvme_free_ns(n);
-       }
 
-       fd = open(path, O_RDONLY);
        if (fd < 0) {
                if (log_level >= LOG_DEBUG)
                        nvme_show_error("%s did not find a pci resource, open failed %s",
@@ -5290,8 +5268,6 @@ static void *mmap_registers(nvme_root_t r, struct nvme_dev *dev)
                }
                membase = NULL;
        }
-
-       close(fd);
        return membase;
 }
 
@@ -5334,7 +5310,7 @@ static int show_registers(int argc, char **argv, struct command *cmd, struct plu
        if (cfg.human_readable)
                flags |= VERBOSE;
 
-       bar = mmap_registers(r, dev);
+       bar = mmap_registers(dev_fd(dev));
        if (!bar) {
                err = nvme_get_properties(dev_fd(dev), &bar);
                if (err)

[ikegami-t]

Yes the nvme_scan only used for the mmap_registers to get the path so will do consider to get the path from libnvme without the scan. I think still the path information required for the mmap as mentioned by #2048 (comment) also.

[igaw]

Like this?

I can't really remember the exact reason for the resource0 path lookups. We added it for some reason. I'll try to figure out why we added it initially.

[igaw]

Ah after digging into it, I think we can do something very simple:

sprintf(path, "/sys/class/nvme/%s/device/resource0", dev->name);

because the fallback (via namespace) was needed prio v4.0 but since then the resource0 file is available via the /sys/class/nvme path. So no need to be fancy here. v4.0 was released 2015 and nvme-cli 2.x requires >= 4.15 anyway.

d902032 ("Register read fix for kernel 4.0")
eb5d180 ("Fixed versioning for pre 1.2 devices")
08290ca ("NVMe: Add show registers command")