[PATCHv3 0/8] nvmet: debugfs support

[PATCHv3 0/8] nvmet: debugfs support

[Hannes Reinecke]

Hi all,

taking up the original patchset for nvmet debugfs
I've improved upon that submission by modifying
the layout:

/dev/kernel/debug/nvmet
  <subsysnqn>
    ctrl<cntlid>
      port
      state
      hostnqn
      kato
      host_traddr
      queue<qnum>
	sqsize
	sqhead

The 'state' attribute is the value of the
CSTS register; one can trigger a controller reset
by writing 'fatal' into it (to set CSTS.CFS).

As usual, comments and reviews are welcome.

Changes to v2:
- Move the 'host_traddr' attribute to the
  controller directory
- Rename callback to 'host_traddr'

Hannes Reinecke (8):
  nvmet: add debugfs support
  nvmet: add 'host_traddr' callback for debugfs
  nvmet-tcp: implement host_traddr()
  nvmet-rdma: implement host_traddr()
  nvmet-fc: implement host_traddr()
  nvme-fcloop: implement 'host_traddr'
  lpfc_nvmet: implement 'host_traddr'
  nvmet: add debugfs support for queues

 drivers/nvme/target/Kconfig    |   9 ++
 drivers/nvme/target/Makefile   |   1 +
 drivers/nvme/target/core.c     |  32 ++++-
 drivers/nvme/target/debugfs.c  | 250 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/debugfs.h  |  50 +++++++
 drivers/nvme/target/fc.c       |  33 +++++
 drivers/nvme/target/fcloop.c   |  11 ++
 drivers/nvme/target/nvmet.h    |  15 +-
 drivers/nvme/target/rdma.c     |  12 ++
 drivers/nvme/target/tcp.c      |  14 ++
 drivers/scsi/lpfc/lpfc_nvmet.c |  11 ++
 include/linux/nvme-fc-driver.h |   4 +
 12 files changed, 439 insertions(+), 3 deletions(-)
 create mode 100644 drivers/nvme/target/debugfs.c
 create mode 100644 drivers/nvme/target/debugfs.h

-- 
2.35.3

[PATCH 1/8] nvmet: add debugfs support

[Hannes Reinecke]

Add a debugfs hierarchy to display the configured subsystems
and the controllers attached to the subsystems.

Suggested-by: Redouane BOUFENGHOUR <redouane.boufenghour@shadow.tech>
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/Kconfig   |   9 ++
 drivers/nvme/target/Makefile  |   1 +
 drivers/nvme/target/core.c    |  22 +++-
 drivers/nvme/target/debugfs.c | 182 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/debugfs.h |  42 ++++++++
 drivers/nvme/target/nvmet.h   |   8 +-
 6 files changed, 261 insertions(+), 3 deletions(-)
 create mode 100644 drivers/nvme/target/debugfs.c
 create mode 100644 drivers/nvme/target/debugfs.h

diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
index 872dd1a0acd8..5740893ace6d 100644
--- a/drivers/nvme/target/Kconfig
+++ b/drivers/nvme/target/Kconfig
@@ -18,6 +18,15 @@ config NVME_TARGET
 	  To configure the NVMe target you probably want to use the nvmetcli
 	  tool from http://git.infradead.org/users/hch/nvmetcli.git.
 
+config NVME_TARGET_DEBUGFS
+        bool "NVMe Target debugfs support"
+	depends on NVME_TARGET
+	help
+	  This enables debugfs support to display the connected controllers
+	  to each subsystem
+
+	  If unsure, say N.
+
 config NVME_TARGET_PASSTHRU
 	bool "NVMe Target Passthrough support"
 	depends on NVME_TARGET
diff --git a/drivers/nvme/target/Makefile b/drivers/nvme/target/Makefile
index c66820102493..c402c44350b2 100644
--- a/drivers/nvme/target/Makefile
+++ b/drivers/nvme/target/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NVME_TARGET_TCP)		+= nvmet-tcp.o
 
 nvmet-y		+= core.o configfs.o admin-cmd.o fabrics-cmd.o \
 			discovery.o io-cmd-file.o io-cmd-bdev.o
+nvmet-$(CONFIG_NVME_TARGET_DEBUGFS)	+= debugfs.o
 nvmet-$(CONFIG_NVME_TARGET_PASSTHRU)	+= passthru.o
 nvmet-$(CONFIG_BLK_DEV_ZONED)		+= zns.o
 nvmet-$(CONFIG_NVME_TARGET_AUTH)	+= fabrics-cmd-auth.o auth.o
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 1d1a7026e817..6cd1b92fc500 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -16,6 +16,7 @@
 #include "trace.h"
 
 #include "nvmet.h"
+#include "debugfs.h"
 
 struct kmem_cache *nvmet_bvec_cache;
 struct workqueue_struct *buffered_io_wq;
@@ -1463,6 +1464,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	mutex_lock(&subsys->lock);
 	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
 	nvmet_setup_p2p_ns_map(ctrl, req);
+	nvmet_debugfs_ctrl_setup(ctrl);
 	mutex_unlock(&subsys->lock);
 
 	*ctrlp = ctrl;
@@ -1497,6 +1499,8 @@ static void nvmet_ctrl_free(struct kref *ref)
 
 	nvmet_destroy_auth(ctrl);
 
+	nvmet_debugfs_ctrl_free(ctrl);
+
 	ida_free(&cntlid_ida, ctrl->cntlid);
 
 	nvmet_async_events_free(ctrl);
@@ -1610,8 +1614,14 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	INIT_LIST_HEAD(&subsys->ctrls);
 	INIT_LIST_HEAD(&subsys->hosts);
 
+	ret = nvmet_debugfs_subsys_setup(subsys);
+	if (ret)
+		goto free_subsysnqn;
+
 	return subsys;
 
+free_subsysnqn:
+	kfree(subsys->subsysnqn);
 free_fr:
 	kfree(subsys->firmware_rev);
 free_mn:
@@ -1628,6 +1638,8 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	WARN_ON_ONCE(!xa_empty(&subsys->namespaces));
 
+	nvmet_debugfs_subsys_free(subsys);
+
 	xa_destroy(&subsys->namespaces);
 	nvmet_passthru_subsys_free(subsys);
 
@@ -1681,11 +1693,18 @@ static int __init nvmet_init(void)
 	if (error)
 		goto out_free_nvmet_work_queue;
 
-	error = nvmet_init_configfs();
+	error = nvmet_init_debugfs();
 	if (error)
 		goto out_exit_discovery;
+
+	error = nvmet_init_configfs();
+	if (error)
+		goto out_exit_debugfs;
+
 	return 0;
 
+out_exit_debugfs:
+	nvmet_exit_debugfs();
 out_exit_discovery:
 	nvmet_exit_discovery();
 out_free_nvmet_work_queue:
@@ -1702,6 +1721,7 @@ static int __init nvmet_init(void)
 static void __exit nvmet_exit(void)
 {
 	nvmet_exit_configfs();
+	nvmet_exit_debugfs();
 	nvmet_exit_discovery();
 	ida_destroy(&cntlid_ida);
 	destroy_workqueue(nvmet_wq);
diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
new file mode 100644
index 000000000000..4b9451ad0db9
--- /dev/null
+++ b/drivers/nvme/target/debugfs.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DebugFS interface for the NVMe target.
+ * Copyright (c) 2022-2024 Shadow
+ */
+
+#include <linux/debugfs.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+
+#include "nvmet.h"
+#include "debugfs.h"
+
+struct dentry *nvmet_debugfs;
+
+#define NVMET_DEBUGFS_ATTR(field) \
+	static int field##_open(struct inode *inode, struct file *file) \
+	{ return single_open(file, field##_show, inode->i_private); } \
+	\
+	static const struct file_operations field##_fops = { \
+		.open = field##_open, \
+		.read = seq_read, \
+		.release = single_release, \
+	}
+
+#define NVMET_DEBUGFS_RW_ATTR(field) \
+	static int field##_open(struct inode *inode, struct file *file) \
+	{ return single_open(file, field##_show, inode->i_private); } \
+	\
+	static const struct file_operations field##_fops = { \
+		.open = field##_open, \
+		.read = seq_read, \
+		.write = field##_write, \
+		.release = single_release, \
+	}
+
+static int nvmet_ctrl_hostnqn_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = (struct nvmet_ctrl *)m->private;
+
+	seq_printf(m, "%s\n", ctrl->hostnqn);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_hostnqn);
+
+static int nvmet_ctrl_kato_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = (struct nvmet_ctrl *)m->private;
+
+	seq_printf(m, "%d\n", ctrl->kato);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_kato);
+
+static int nvmet_ctrl_port_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = (struct nvmet_ctrl *)m->private;
+
+	seq_printf(m, "%d\n", le16_to_cpu(ctrl->port->disc_addr.portid));
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_port);
+
+static const char *const csts_state_names[] = {
+	[NVME_CSTS_RDY] = "ready",
+	[NVME_CSTS_CFS] = "fatal",
+	[NVME_CSTS_NSSRO] = "reset",
+	[NVME_CSTS_SHST_OCCUR] = "shutdown",
+	[NVME_CSTS_SHST_CMPLT] = "completed",
+	[NVME_CSTS_PP] = "paused",
+};
+
+static int nvmet_ctrl_state_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = (struct nvmet_ctrl *)m->private;
+	bool sep = false;
+	int i;
+
+	for (i = 0; i < 7; i++) {
+		int state = BIT(i);
+
+		if (!(ctrl->csts & state))
+			continue;
+		if (sep)
+			seq_puts(m, "|");
+		sep = true;
+		if (csts_state_names[state])
+			seq_puts(m, csts_state_names[state]);
+		else
+			seq_printf(m, "%d", state);
+	}
+	if (sep)
+		seq_printf(m, "\n");
+	return 0;
+}
+
+static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
+				      size_t count, loff_t *ppos)
+{
+	struct seq_file *m = file->private_data;
+	struct nvmet_ctrl *ctrl = m->private;
+	char reset[16];
+
+	if (count >= sizeof(reset))
+		return -EINVAL;
+	if (copy_from_user(reset, buf, count))
+		return -EFAULT;
+	if (!memcmp(reset, "fatal", 5))
+		nvmet_ctrl_fatal_error(ctrl);
+	else
+		return -EINVAL;
+	return count;
+}
+NVMET_DEBUGFS_RW_ATTR(nvmet_ctrl_state);
+
+int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
+{
+	char name[32];
+	struct dentry *parent = ctrl->subsys->debugfs_dir;
+	int ret;
+
+	if (!parent)
+		return -ENODEV;
+	snprintf(name, sizeof(name), "ctrl%d", ctrl->cntlid);
+	ctrl->debugfs_dir = debugfs_create_dir(name, parent);
+	if (IS_ERR(ctrl->debugfs_dir)) {
+		ret = PTR_ERR(ctrl->debugfs_dir);
+		ctrl->debugfs_dir = NULL;
+		return ret;
+	}
+	debugfs_create_file("port", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_port_fops);
+	debugfs_create_file("hostnqn", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_hostnqn_fops);
+	debugfs_create_file("kato", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_kato_fops);
+	debugfs_create_file("state", S_IRUSR | S_IWUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_state_fops);
+	return 0;
+}
+
+void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl)
+{
+	debugfs_remove_recursive(ctrl->debugfs_dir);
+}
+
+int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys)
+{
+	int ret = 0;
+
+	subsys->debugfs_dir = debugfs_create_dir(subsys->subsysnqn,
+						 nvmet_debugfs);
+	if (IS_ERR(subsys->debugfs_dir)) {
+		ret = PTR_ERR(subsys->debugfs_dir);
+		subsys->debugfs_dir = NULL;
+	}
+	return ret;
+}
+
+void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys)
+{
+	debugfs_remove_recursive(subsys->debugfs_dir);
+}
+
+int __init nvmet_init_debugfs(void)
+{
+	struct dentry *parent;
+
+	parent = debugfs_create_dir("nvmet", NULL);
+	if (IS_ERR(parent)) {
+		pr_warn("%s: failed to create debugfs directory\n", "nvmet");
+		return PTR_ERR(parent);
+	}
+	nvmet_debugfs = parent;
+	return 0;
+}
+
+void nvmet_exit_debugfs(void)
+{
+	debugfs_remove_recursive(nvmet_debugfs);
+}
diff --git a/drivers/nvme/target/debugfs.h b/drivers/nvme/target/debugfs.h
new file mode 100644
index 000000000000..ff09e5597614
--- /dev/null
+++ b/drivers/nvme/target/debugfs.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * DebugFS interface for the NVMe target.
+ * Copyright (c) 2022-2024 Shadow
+ * Copyright (c) 2024 SUSE LLC
+ */
+#ifndef NVMET_DEBUGFS_H
+#define NVMET_DEBUGFS_H
+
+#include <linux/types.h>
+
+#ifdef CONFIG_NVME_TARGET_DEBUGFS
+int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys);
+void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys);
+int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl);
+void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl);
+
+int __init nvmet_init_debugfs(void);
+void nvmet_exit_debugfs(void);
+#else
+static inline int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys)
+{
+	return 0;
+}
+static inline void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys){}
+
+static inline int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
+{
+	return 0;
+}
+static inline void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl) {}
+
+static inline int __init nvmet_init_debugfs(void)
+{
+    return 0;
+}
+
+static inline void nvmet_exit_debugfs(void) {}
+
+#endif
+
+#endif // NVMET_DEBUGFS_H
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 765da2b932c5..f0371163cb26 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -233,7 +233,9 @@ struct nvmet_ctrl {
 
 	struct device		*p2p_client;
 	struct radix_tree_root	p2p_ns_map;
-
+#ifdef CONFIG_NVME_TARGET_DEBUGFS
+	struct dentry		*debugfs_dir;
+#endif
 	spinlock_t		error_lock;
 	u64			err_counter;
 	struct nvme_error_slot	slots[NVMET_ERROR_LOG_SLOTS];
@@ -269,7 +271,9 @@ struct nvmet_subsys {
 
 	struct list_head	hosts;
 	bool			allow_any_host;
-
+#ifdef CONFIG_NVME_TARGET_DEBUGFS
+	struct dentry		*debugfs_dir;
+#endif
 	u16			max_qid;
 
 	u64			ver;
-- 
2.35.3

[PATCH 2/8] nvmet: add 'host_traddr' callback for debugfs

[Hannes Reinecke]

We want to display the transport address of the connected host
in debugfs, but this is a property of the transport.
So add a callback 'host_traddr' to allow the transport drivers
to fill in the data.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/core.c    |  8 ++++++++
 drivers/nvme/target/debugfs.c | 19 +++++++++++++++++++
 drivers/nvme/target/nvmet.h   |  4 ++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 6cd1b92fc500..4955819e4f5f 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1527,6 +1527,14 @@ void nvmet_ctrl_fatal_error(struct nvmet_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmet_ctrl_fatal_error);
 
+ssize_t nvmet_ctrl_host_traddr(struct nvmet_ctrl *ctrl,
+		char *traddr, size_t traddr_len)
+{
+	if (!ctrl->ops->host_traddr)
+		return -EOPNOTSUPP;
+	return ctrl->ops->host_traddr(ctrl, traddr, traddr_len);
+}
+
 static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 		const char *subsysnqn)
 {
diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
index 4b9451ad0db9..40eeac97157a 100644
--- a/drivers/nvme/target/debugfs.c
+++ b/drivers/nvme/target/debugfs.c
@@ -114,6 +114,23 @@ static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
 }
 NVMET_DEBUGFS_RW_ATTR(nvmet_ctrl_state);
 
+static int nvmet_ctrl_host_traddr_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = (struct nvmet_ctrl *)m->private;
+	ssize_t size;
+	char buf[NVMF_TRADDR_SIZE + 1];
+
+	size = nvmet_ctrl_host_traddr(ctrl, buf, NVMF_TRADDR_SIZE);
+	if (size < 0) {
+		buf[0] = '\0';
+		size = 0;
+	}
+	buf[size] = '\0';
+	seq_printf(m, "%s\n", buf);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_host_traddr);
+
 int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
 {
 	char name[32];
@@ -137,6 +154,8 @@ int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
 			    &nvmet_ctrl_kato_fops);
 	debugfs_create_file("state", S_IRUSR | S_IWUSR, ctrl->debugfs_dir, ctrl,
 			    &nvmet_ctrl_state_fops);
+	debugfs_create_file("host_traddr", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_host_traddr_fops);
 	return 0;
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index f0371163cb26..3b7ba6d656cf 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -361,6 +361,8 @@ struct nvmet_fabrics_ops {
 	void (*delete_ctrl)(struct nvmet_ctrl *ctrl);
 	void (*disc_traddr)(struct nvmet_req *req,
 			struct nvmet_port *port, char *traddr);
+	ssize_t (*host_traddr)(struct nvmet_ctrl *ctrl,
+			char *traddr, size_t traddr_len);
 	u16 (*install_queue)(struct nvmet_sq *nvme_sq);
 	void (*discovery_chg)(struct nvmet_port *port);
 	u8 (*get_mdts)(const struct nvmet_ctrl *ctrl);
@@ -509,6 +511,8 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
 				       struct nvmet_req *req);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
 u16 nvmet_check_ctrl_status(struct nvmet_req *req);
+ssize_t nvmet_ctrl_host_traddr(struct nvmet_ctrl *ctrl,
+		char *traddr, size_t traddr_len);
 
 struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		enum nvme_subsys_type type);
-- 
2.35.3

[PATCH 3/8] nvmet-tcp: implement host_traddr()

[Hannes Reinecke]

Implement callback to display the host transport address.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/tcp.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 4718d4d87a85..d890c1a393d8 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -2201,6 +2201,19 @@ static void nvmet_tcp_disc_port_addr(struct nvmet_req *req,
 	}
 }
 
+static ssize_t nvmet_tcp_host_port_addr(struct nvmet_ctrl *ctrl,
+			char *traddr, size_t traddr_len)
+{
+	struct nvmet_sq *sq = ctrl->sqs[0];
+	struct nvmet_tcp_queue *queue =
+		container_of(sq, struct nvmet_tcp_queue, nvme_sq);
+
+	if (queue->sockaddr_peer.ss_family == AF_UNSPEC)
+		return -EINVAL;
+	return snprintf(traddr, traddr_len, "%pISc",
+			(struct sockaddr *)&queue->sockaddr_peer);
+}
+
 static const struct nvmet_fabrics_ops nvmet_tcp_ops = {
 	.owner			= THIS_MODULE,
 	.type			= NVMF_TRTYPE_TCP,
@@ -2211,6 +2224,7 @@ static const struct nvmet_fabrics_ops nvmet_tcp_ops = {
 	.delete_ctrl		= nvmet_tcp_delete_ctrl,
 	.install_queue		= nvmet_tcp_install_queue,
 	.disc_traddr		= nvmet_tcp_disc_port_addr,
+	.host_traddr		= nvmet_tcp_host_port_addr,
 };
 
 static int __init nvmet_tcp_init(void)
-- 
2.35.3

[PATCH 4/8] nvmet-rdma: implement host_traddr()

[Hannes Reinecke]

Implement callback to display the host transport address.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/rdma.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index f2bb9d95ecf4..99d98296d0ba 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -2014,6 +2014,17 @@ static void nvmet_rdma_disc_port_addr(struct nvmet_req *req,
 	}
 }
 
+static ssize_t nvmet_rdma_host_port_addr(struct nvmet_ctrl *ctrl,
+		char *traddr, size_t traddr_len)
+{
+	struct nvmet_sq *nvme_sq = ctrl->sqs[0];
+	struct nvmet_rdma_queue *queue =
+		container_of(nvme_sq, struct nvmet_rdma_queue, nvme_sq);
+
+	return snprintf(traddr, traddr_len, "%pISc",
+			(struct sockaddr *)&queue->cm_id->route.addr.dst_addr);
+}
+
 static u8 nvmet_rdma_get_mdts(const struct nvmet_ctrl *ctrl)
 {
 	if (ctrl->pi_support)
@@ -2038,6 +2049,7 @@ static const struct nvmet_fabrics_ops nvmet_rdma_ops = {
 	.queue_response		= nvmet_rdma_queue_response,
 	.delete_ctrl		= nvmet_rdma_delete_ctrl,
 	.disc_traddr		= nvmet_rdma_disc_port_addr,
+	.host_traddr		= nvmet_rdma_host_port_addr,
 	.get_mdts		= nvmet_rdma_get_mdts,
 	.get_max_queue_size	= nvmet_rdma_get_max_queue_size,
 };
-- 
2.35.3

[PATCH 5/8] nvmet-fc: implement host_traddr()

[Hannes Reinecke]

Implement callback to display the host transport address by
adding a callback 'host_traddr' for nvmet_fc_target_template.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/fc.c       | 33 +++++++++++++++++++++++++++++++++
 include/linux/nvme-fc-driver.h |  4 ++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index fd229f310c93..b4706a8a9837 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -2931,6 +2931,38 @@ nvmet_fc_discovery_chg(struct nvmet_port *port)
 		tgtport->ops->discovery_event(&tgtport->fc_target_port);
 }
 
+static ssize_t
+nvmet_fc_host_traddr(struct nvmet_ctrl *ctrl,
+		char *traddr, size_t traddr_size)
+{
+	struct nvmet_sq *sq = ctrl->sqs[0];
+	struct nvmet_fc_tgt_queue *queue =
+		container_of(sq, struct nvmet_fc_tgt_queue, nvme_sq);
+	struct nvmet_fc_tgtport *tgtport = queue->assoc ? queue->assoc->tgtport : NULL;
+	struct nvmet_fc_hostport *hostport = queue->assoc ? queue->assoc->hostport : NULL;
+	u64 wwnn, wwpn;
+	ssize_t ret = 0;
+
+	if (!tgtport || !nvmet_fc_tgtport_get(tgtport))
+		return -ENODEV;
+	if (!hostport || !nvmet_fc_hostport_get(hostport)) {
+		ret = -ENODEV;
+		goto out_put;
+	}
+
+	if (tgtport->ops->host_traddr) {
+		ret = tgtport->ops->host_traddr(hostport->hosthandle, &wwnn, &wwpn);
+		if (ret)
+			goto out_put_host;
+		ret = snprintf(traddr, traddr_size, "nn-0x%llx:pn-0x%llx", wwnn, wwpn);
+	}
+out_put_host:
+	nvmet_fc_hostport_put(hostport);
+out_put:
+	nvmet_fc_tgtport_put(tgtport);
+	return ret;
+}
+
 static const struct nvmet_fabrics_ops nvmet_fc_tgt_fcp_ops = {
 	.owner			= THIS_MODULE,
 	.type			= NVMF_TRTYPE_FC,
@@ -2940,6 +2972,7 @@ static const struct nvmet_fabrics_ops nvmet_fc_tgt_fcp_ops = {
 	.queue_response		= nvmet_fc_fcp_nvme_cmd_done,
 	.delete_ctrl		= nvmet_fc_delete_ctrl,
 	.discovery_chg		= nvmet_fc_discovery_chg,
+	.host_traddr		= nvmet_fc_host_traddr,
 };
 
 static int __init nvmet_fc_init_module(void)
diff --git a/include/linux/nvme-fc-driver.h b/include/linux/nvme-fc-driver.h
index 4109f1bd6128..89ea1ebd975a 100644
--- a/include/linux/nvme-fc-driver.h
+++ b/include/linux/nvme-fc-driver.h
@@ -920,6 +920,9 @@ struct nvmet_fc_target_port {
  *       further references to hosthandle.
  *       Entrypoint is Mandatory if the lldd calls nvmet_fc_invalidate_host().
  *
+ * @host_traddr: called by the transport to retrieve the node name and
+ *       port name of the host port address.
+ *
  * @max_hw_queues:  indicates the maximum number of hw queues the LLDD
  *       supports for cpu affinitization.
  *       Value is Mandatory. Must be at least 1.
@@ -975,6 +978,7 @@ struct nvmet_fc_target_template {
 	void (*ls_abort)(struct nvmet_fc_target_port *targetport,
 				void *hosthandle, struct nvmefc_ls_req *lsreq);
 	void (*host_release)(void *hosthandle);
+	int (*host_traddr)(void *hosthandle, u64 *wwnn, u64 *wwpn);
 
 	u32	max_hw_queues;
 	u16	max_sgl_segments;
-- 
2.35.3

[PATCH 6/8] nvme-fcloop: implement 'host_traddr'

[Hannes Reinecke]

Implement the 'host_traddr' callback to display the host transport
address for nvmet debugfs.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/fcloop.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 913cd2ec7a6f..e1abb27927ff 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -492,6 +492,16 @@ fcloop_t2h_host_release(void *hosthandle)
 	/* host handle ignored for now */
 }
 
+static int
+fcloop_t2h_host_traddr(void *hosthandle, u64 *wwnn, u64 *wwpn)
+{
+	struct fcloop_rport *rport = hosthandle;
+
+	*wwnn = rport->lport->localport->node_name;
+	*wwpn = rport->lport->localport->port_name;
+	return 0;
+}
+
 /*
  * Simulate reception of RSCN and converting it to a initiator transport
  * call to rescan a remote port.
@@ -1074,6 +1084,7 @@ static struct nvmet_fc_target_template tgttemplate = {
 	.ls_req			= fcloop_t2h_ls_req,
 	.ls_abort		= fcloop_t2h_ls_abort,
 	.host_release		= fcloop_t2h_host_release,
+	.host_traddr		= fcloop_t2h_host_traddr,
 	.max_hw_queues		= FCLOOP_HW_QUEUES,
 	.max_sgl_segments	= FCLOOP_SGL_SEGS,
 	.max_dif_sgl_segments	= FCLOOP_SGL_SEGS,
-- 
2.35.3

[PATCH 7/8] lpfc_nvmet: implement 'host_traddr'

[Hannes Reinecke]

Implement the 'host_traddr' callback to display the host transport
address for nvmet debugfs.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/scsi/lpfc/lpfc_nvmet.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index 425328d9c2d8..a315ab03f721 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -1363,6 +1363,16 @@ lpfc_nvmet_ls_abort(struct nvmet_fc_target_port *targetport,
 		atomic_inc(&lpfc_nvmet->xmt_ls_abort);
 }
 
+static int
+lpfc_nvmet_host_traddr(void *hosthandle, u64 *wwnn, u64 *wwpn)
+{
+	struct lpfc_nodelist *ndlp = hosthandle;
+
+	*wwnn = wwn_to_u64(ndlp->nlp_nodename.u.wwn);
+	*wwpn = wwn_to_u64(ndlp->nlp_portname.u.wwn);
+	return 0;
+}
+
 static void
 lpfc_nvmet_host_release(void *hosthandle)
 {
@@ -1413,6 +1423,7 @@ static struct nvmet_fc_target_template lpfc_tgttemplate = {
 	.ls_req         = lpfc_nvmet_ls_req,
 	.ls_abort       = lpfc_nvmet_ls_abort,
 	.host_release   = lpfc_nvmet_host_release,
+	.host_traddr    = lpfc_nvmet_host_traddr,
 
 	.max_hw_queues  = 1,
 	.max_sgl_segments = LPFC_NVMET_DEFAULT_SEGS,
-- 
2.35.3

[PATCH 8/8] nvmet: add debugfs support for queues

[Hannes Reinecke]

Add debugfs entries to display the status of the controller
queues.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/core.c    |  2 ++
 drivers/nvme/target/debugfs.c | 49 +++++++++++++++++++++++++++++++++++
 drivers/nvme/target/debugfs.h |  8 ++++++
 drivers/nvme/target/nvmet.h   |  3 +++
 4 files changed, 62 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 4955819e4f5f..694e91cdfc8d 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -791,6 +791,7 @@ void nvmet_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq,
 	sq->size = size;
 
 	ctrl->sqs[qid] = sq;
+	nvmet_debugfs_queue_setup(ctrl, sq);
 }
 
 static void nvmet_confirm_sq(struct percpu_ref *ref)
@@ -815,6 +816,7 @@ void nvmet_sq_destroy(struct nvmet_sq *sq)
 	wait_for_completion(&sq->free_done);
 	percpu_ref_exit(&sq->ref);
 	nvmet_auth_sq_free(sq);
+	nvmet_debugfs_queue_free(sq);
 
 	if (ctrl) {
 		/*
diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
index 40eeac97157a..a8fec34f36d4 100644
--- a/drivers/nvme/target/debugfs.c
+++ b/drivers/nvme/target/debugfs.c
@@ -35,6 +35,55 @@ struct dentry *nvmet_debugfs;
 		.release = single_release, \
 	}
 
+static int nvmet_queue_sqsize_show(struct seq_file *m, void *p)
+{
+	struct nvmet_sq *sq = (struct nvmet_sq *)m->private;
+
+	seq_printf(m, "%d\n", sq->size);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_queue_sqsize);
+
+static int nvmet_queue_sqhead_show(struct seq_file *m, void *p)
+{
+	struct nvmet_sq *sq = (struct nvmet_sq *)m->private;
+	u32 sqhd = READ_ONCE(sq->sqhd);
+
+	if (sq->sqhd_disabled)
+		seq_puts(m, "disabled\n");
+	else
+		seq_printf(m, "%u\n", sqhd & 0x0000FFFF);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_queue_sqhead);
+
+int nvmet_debugfs_queue_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
+{
+	char name[32];
+	struct dentry *parent = ctrl->debugfs_dir, *queue_dir;
+
+	if (!parent)
+		return -ENODEV;
+	if (!sq)
+		return -EINVAL;
+	snprintf(name, sizeof(name), "queue%d", sq->qid);
+	queue_dir = debugfs_create_dir(name, parent);
+	if (IS_ERR(queue_dir))
+		return PTR_ERR(queue_dir);
+	sq->debugfs_dir = queue_dir;
+	debugfs_create_file("sqsize", S_IRUSR, queue_dir,
+			    sq, &nvmet_queue_sqsize_fops);
+	debugfs_create_file("sqhead", S_IRUSR, queue_dir,
+			    sq, &nvmet_queue_sqhead_fops);
+	return 0;
+}
+
+void nvmet_debugfs_queue_free(struct nvmet_sq *sq)
+{
+	debugfs_remove_recursive(sq->debugfs_dir);
+	sq->debugfs_dir = NULL;
+}
+
 static int nvmet_ctrl_hostnqn_show(struct seq_file *m, void *p)
 {
 	struct nvmet_ctrl *ctrl = (struct nvmet_ctrl *)m->private;
diff --git a/drivers/nvme/target/debugfs.h b/drivers/nvme/target/debugfs.h
index ff09e5597614..5192f8012b79 100644
--- a/drivers/nvme/target/debugfs.h
+++ b/drivers/nvme/target/debugfs.h
@@ -14,6 +14,8 @@ int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys);
 void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys);
 int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl);
 void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl);
+int nvmet_debugfs_queue_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq);
+void nvmet_debugfs_queue_free(struct nvmet_sq *sq);
 
 int __init nvmet_init_debugfs(void);
 void nvmet_exit_debugfs(void);
@@ -30,6 +32,12 @@ static inline int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
 }
 static inline void nvmet_debugfs_ctrl_free(struct nvmet_ctrl *ctrl) {}
 
+static inline int nvmet_debugfs_sq_setup(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
+{
+	return 0;
+}
+static inline void nvmet_debugfs_sq_free(struct nvmet_sq *sq) {}
+
 static inline int __init nvmet_init_debugfs(void)
 {
     return 0;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 3b7ba6d656cf..a6c2b106ba1c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -109,6 +109,9 @@ struct nvmet_sq {
 	u16			size;
 	u32			sqhd;
 	bool			sqhd_disabled;
+#ifdef CONFIG_NVME_TARGET_DEBUGFS
+	struct dentry		*debugfs_dir;
+#endif
 #ifdef CONFIG_NVME_TARGET_AUTH
 	bool			authenticated;
 	struct delayed_work	auth_expired_work;
-- 
2.35.3

Re: [PATCH 1/8] nvmet: add debugfs support

[Sagi Grimberg]

> +int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys)
> +{
> +	int ret = 0;
> +
> +	subsys->debugfs_dir = debugfs_create_dir(subsys->subsysnqn,
> +						 nvmet_debugfs);
> +	if (IS_ERR(subsys->debugfs_dir)) {
> +		ret = PTR_ERR(subsys->debugfs_dir);
> +		subsys->debugfs_dir = NULL;
> +	}
> +	return ret;
> +}
> +
> +void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys)
> +{
> +	debugfs_remove_recursive(subsys->debugfs_dir);
> +}

The subsys setup/free look trivial enough to just open-code
in the call-sites.

The rest looks fine to me.

Re: [PATCH 2/8] nvmet: add 'host_traddr' callback for debugfs

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 3/8] nvmet-tcp: implement host_traddr()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 4/8] nvmet-rdma: implement host_traddr()

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 8/8] nvmet: add debugfs support for queues

[Sagi Grimberg]

On 22/03/2024 9:03, Hannes Reinecke wrote:
> Add debugfs entries to display the status of the controller
> queues.

I'm questioning the value of this. We can possibly see lots and lots of
queues and I wonder how a queue entry will this actually help.

Re: [PATCH 8/8] nvmet: add debugfs support for queues

[Hannes Reinecke]

On 3/23/24 21:31, Sagi Grimberg wrote:
> 
> 
> On 22/03/2024 9:03, Hannes Reinecke wrote:
>> Add debugfs entries to display the status of the controller
>> queues.
> 
> I'm questioning the value of this. We can possibly see lots and lots of
> queues and I wonder how a queue entry will this actually help.

One can inspect I/O progress via the sqhead attribute; also one would
want to know the number of queues created.
But we can leave it for now; main part of this patchset is the 
controller entries and the possibility to trigger a controller reset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 8/8] nvmet: add debugfs support for queues

[Sagi Grimberg]

On 25/03/2024 9:25, Hannes Reinecke wrote:
> On 3/23/24 21:31, Sagi Grimberg wrote:
>>
>>
>> On 22/03/2024 9:03, Hannes Reinecke wrote:
>>> Add debugfs entries to display the status of the controller
>>> queues.
>>
>> I'm questioning the value of this. We can possibly see lots and lots of
>> queues and I wonder how a queue entry will this actually help.
>
> One can inspect I/O progress via the sqhead attribute; also one would
> want to know the number of queues created.
> But we can leave it for now; main part of this patchset is the 
> controller entries and the possibility to trigger a controller reset.

Lets drop the queue debug stats for now

Re: [PATCH 1/8] nvmet: add debugfs support

[Daniel Wagner]

On Fri, Mar 22, 2024 at 08:03:26AM +0100, Hannes Reinecke wrote:
> +static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
> +				      size_t count, loff_t *ppos)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct nvmet_ctrl *ctrl = m->private;
> +	char reset[16];
> +
> +	if (count >= sizeof(reset))
> +		return -EINVAL;
> +	if (copy_from_user(reset, buf, count))
> +		return -EFAULT;
> +	if (!memcmp(reset, "fatal", 5))
> +		nvmet_ctrl_fatal_error(ctrl);

I'd like to use this also to trigger a reset. This allows to test the
auth code a bit better, e.g. after changing the ctrl key on the target
nothing will happen until a reconnect happens. Currently, I have to
set the max queue count which triggers a reset. Also it would make the
'support real hardware' series for blktests why more generic.

This here should does the trick in my local testing:

--- a/drivers/nvme/target/debugfs.c
+++ b/drivers/nvme/target/debugfs.c
@@ -157,6 +157,8 @@ static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
                return -EFAULT;
        if (!memcmp(reset, "fatal", 5))
                nvmet_ctrl_fatal_error(ctrl);
+       else if (!memcmp(reset, "reset", 5))
+               ctrl->ops->delete_ctrl(ctrl);
        else
                return -EINVAL;
        return count;

Re: [PATCH 1/8] nvmet: add debugfs support

[Daniel Wagner]

On Mon, Mar 25, 2024 at 07:18:52PM +0100, Daniel Wagner wrote:
> On Fri, Mar 22, 2024 at 08:03:26AM +0100, Hannes Reinecke wrote:
> > +static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
> > +				      size_t count, loff_t *ppos)
> > +{
> > +	struct seq_file *m = file->private_data;
> > +	struct nvmet_ctrl *ctrl = m->private;
> > +	char reset[16];
> > +
> > +	if (count >= sizeof(reset))
> > +		return -EINVAL;
> > +	if (copy_from_user(reset, buf, count))
> > +		return -EFAULT;
> > +	if (!memcmp(reset, "fatal", 5))
> > +		nvmet_ctrl_fatal_error(ctrl);
> 
> I'd like to use this also to trigger a reset. This allows to test the
> auth code a bit better, e.g. after changing the ctrl key on the target
> nothing will happen until a reconnect happens. Currently, I have to
> set the max queue count which triggers a reset. Also it would make the
> 'support real hardware' series for blktests why more generic.
> 
> This here should does the trick in my local testing:
> 
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -157,6 +157,8 @@ static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
>                 return -EFAULT;
>         if (!memcmp(reset, "fatal", 5))
>                 nvmet_ctrl_fatal_error(ctrl);
> +       else if (!memcmp(reset, "reset", 5))
> +               ctrl->ops->delete_ctrl(ctrl);
>         else
>                 return -EINVAL;
>         return count;
> 

and here the corresponding test extension (nvme/045)

	echo "Renew host key on the controller and force reconnect"

	new_hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"

	_set_nvmet_hostkey "${def_hostnqn}" "${new_hostkey}"

	# Force a reconnect
	nvmedev=$(_find_nvme_dev "${def_subsysnqn}")
	cntlid="$(nvme id-ctrl -o json "/dev/${nvmedev}" | jq .cntlid)"
	echo "reset" > /sys/kernel/debug/nvmet/"${def_subsysnqn}/ctrl${cntlid}"/state
	nvmf_wait_for_ctrl_delete "${nvmedev}"

Re: [PATCH 1/8] nvmet: add debugfs support

[Sagi Grimberg]

On 25/03/2024 20:18, Daniel Wagner wrote:
> On Fri, Mar 22, 2024 at 08:03:26AM +0100, Hannes Reinecke wrote:
>> +static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
>> +				      size_t count, loff_t *ppos)
>> +{
>> +	struct seq_file *m = file->private_data;
>> +	struct nvmet_ctrl *ctrl = m->private;
>> +	char reset[16];
>> +
>> +	if (count >= sizeof(reset))
>> +		return -EINVAL;
>> +	if (copy_from_user(reset, buf, count))
>> +		return -EFAULT;
>> +	if (!memcmp(reset, "fatal", 5))
>> +		nvmet_ctrl_fatal_error(ctrl);
> I'd like to use this also to trigger a reset. This allows to test the
> auth code a bit better, e.g. after changing the ctrl key on the target
> nothing will happen until a reconnect happens. Currently, I have to
> set the max queue count which triggers a reset. Also it would make the
> 'support real hardware' series for blktests why more generic.
>
> This here should does the trick in my local testing:
>
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -157,6 +157,8 @@ static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
>                  return -EFAULT;
>          if (!memcmp(reset, "fatal", 5))
>                  nvmet_ctrl_fatal_error(ctrl);
> +       else if (!memcmp(reset, "reset", 5))
> +               ctrl->ops->delete_ctrl(ctrl);
>          else
>                  return -EINVAL;
>          return count;

What is the difference between fatal and reset?

Re: [PATCH 1/8] nvmet: add debugfs support

[Hannes Reinecke]

On 3/25/24 19:18, Daniel Wagner wrote:
> On Fri, Mar 22, 2024 at 08:03:26AM +0100, Hannes Reinecke wrote:
>> +static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
>> +				      size_t count, loff_t *ppos)
>> +{
>> +	struct seq_file *m = file->private_data;
>> +	struct nvmet_ctrl *ctrl = m->private;
>> +	char reset[16];
>> +
>> +	if (count >= sizeof(reset))
>> +		return -EINVAL;
>> +	if (copy_from_user(reset, buf, count))
>> +		return -EFAULT;
>> +	if (!memcmp(reset, "fatal", 5))
>> +		nvmet_ctrl_fatal_error(ctrl);
> 
> I'd like to use this also to trigger a reset. This allows to test the
> auth code a bit better, e.g. after changing the ctrl key on the target
> nothing will happen until a reconnect happens. Currently, I have to
> set the max queue count which triggers a reset. Also it would make the
> 'support real hardware' series for blktests why more generic.
> 
> This here should does the trick in my local testing:
> 
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -157,6 +157,8 @@ static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
>                  return -EFAULT;
>          if (!memcmp(reset, "fatal", 5))
>                  nvmet_ctrl_fatal_error(ctrl);
> +       else if (!memcmp(reset, "reset", 5))
> +               ctrl->ops->delete_ctrl(ctrl);
>          else
>                  return -EINVAL;
>          return count;
> 

Weelll ... guess what the 'fatal' status does ...
I can rename it to 'reset' if you'd like, but 'fatal' corresponds to one 
of the controller states, whereas 'reset' doesn't.
But I can make it an alias if you really want to write 'reset' :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 1/8] nvmet: add debugfs support

[Daniel Wagner]

On Tue, Mar 26, 2024 at 12:39:16AM +0200, Sagi Grimberg wrote:
> 
> 
> On 25/03/2024 20:18, Daniel Wagner wrote:
> > On Fri, Mar 22, 2024 at 08:03:26AM +0100, Hannes Reinecke wrote:
> > > +static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
> > > +				      size_t count, loff_t *ppos)
> > > +{
> > > +	struct seq_file *m = file->private_data;
> > > +	struct nvmet_ctrl *ctrl = m->private;
> > > +	char reset[16];
> > > +
> > > +	if (count >= sizeof(reset))
> > > +		return -EINVAL;
> > > +	if (copy_from_user(reset, buf, count))
> > > +		return -EFAULT;
> > > +	if (!memcmp(reset, "fatal", 5))
> > > +		nvmet_ctrl_fatal_error(ctrl);
> > I'd like to use this also to trigger a reset. This allows to test the
> > auth code a bit better, e.g. after changing the ctrl key on the target
> > nothing will happen until a reconnect happens. Currently, I have to
> > set the max queue count which triggers a reset. Also it would make the
> > 'support real hardware' series for blktests why more generic.
> > 
> > This here should does the trick in my local testing:
> > 
> > --- a/drivers/nvme/target/debugfs.c
> > +++ b/drivers/nvme/target/debugfs.c
> > @@ -157,6 +157,8 @@ static ssize_t nvmet_ctrl_state_write(struct file *file, const char __user *buf,
> >                  return -EFAULT;
> >          if (!memcmp(reset, "fatal", 5))
> >                  nvmet_ctrl_fatal_error(ctrl);
> > +       else if (!memcmp(reset, "reset", 5))
> > +               ctrl->ops->delete_ctrl(ctrl);
> >          else
> >                  return -EINVAL;
> >          return count;
> 
> What is the difference between fatal and reset?

nvmet_ctrl_fatal_error() also calls ctrl->ops->delete_ctrl() eventually
but also logs 'fatal error occurred'. This works also just fine for what
I had in mind.

Re: [PATCH 1/8] nvmet: add debugfs support

[Daniel Wagner]

On Tue, Mar 26, 2024 at 08:14:28AM +0100, Hannes Reinecke wrote:
> Weelll ... guess what the 'fatal' status does ...
> I can rename it to 'reset' if you'd like, but 'fatal' corresponds to one of
> the controller states, whereas 'reset' doesn't.
> But I can make it an alias if you really want to write 'reset' :-)

I am fine with 'fatal'.

Re: [PATCH 1/8] nvmet: add debugfs support

[Hannes Reinecke]

On 3/23/24 21:25, Sagi Grimberg wrote:
>> +int nvmet_debugfs_subsys_setup(struct nvmet_subsys *subsys)
>> +{
>> +    int ret = 0;
>> +
>> +    subsys->debugfs_dir = debugfs_create_dir(subsys->subsysnqn,
>> +                         nvmet_debugfs);
>> +    if (IS_ERR(subsys->debugfs_dir)) {
>> +        ret = PTR_ERR(subsys->debugfs_dir);
>> +        subsys->debugfs_dir = NULL;
>> +    }
>> +    return ret;
>> +}
>> +
>> +void nvmet_debugfs_subsys_free(struct nvmet_subsys *subsys)
>> +{
>> +    debugfs_remove_recursive(subsys->debugfs_dir);
>> +}
> 
> The subsys setup/free look trivial enough to just open-code
> in the call-sites.
> 
> The rest looks fine to me.

Agreed for the 'free' call, the setup call has to stay as it's
referring to the static dentry 'nvmet_debugfs', which I'd like
to keep private to 'debugfs.c'.

Cheers,

Hannes