[RFC 0/2] nvme: add passthrough error logging opt-in

[RFC 0/2] nvme: add passthrough error logging opt-in

[Alan Adamson]

Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
logging for user passthrough commands.  This commit adds the ability to opt in
to passthrough error logging via debugfs.

Currently nvme uses debugfs for nvme fault injection. A change is needed to extend
the nvme debugfs implementation to go beyond error injection.

Move Fault Injection to nvme-debugfs.c.  Add new config parameter (CONFIG_NVME_FAULT_INJECTION_DEBUG_FS)
which enables the compilation of Fault Injection functionality.  Other consumers 
of nvme-debugfs can be added without requiring Fault Injection.

Add new consumer of nvme-debugfs (CONFIG_NVME_ERROR_LOGGING_DEBUG_FS) that provides the
ability of passthrough error logging.

To enable passthrough error logging:
	echo Y > /sys/kernel/debug/nvme0/error-logging

To disable passthrough error logging:
	echo N > /sys/kernel/debug/nvme0/error-logging

By default, passthrough error logging will remain disabled.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>

[RFC 1/2] nvme: move fault injector to nvme-debugfs.c

[Alan Adamson]

Move the nvme fault injector code associated with debugfs into
nvme-debugfs.c (new file) so all nvme debugfs associated can
reside in a common place.

CONFIG_NVME_FAULT_INJECTION_DEBUG_FS needs to be enabled to
be able to use fault injection.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
---
 drivers/nvme/host/Kconfig                     |  6 ++
 drivers/nvme/host/Makefile                    |  2 +-
 drivers/nvme/host/core.c                      | 11 ++--
 .../host/{fault_inject.c => nvme-debugfs.c}   | 56 +++++++++----------
 drivers/nvme/host/nvme.h                      | 31 +++++-----
 5 files changed, 59 insertions(+), 47 deletions(-)
 rename drivers/nvme/host/{fault_inject.c => nvme-debugfs.c} (60%)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 2f6a7f8c94e8..70f380c1ccae 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -32,6 +32,12 @@ config NVME_VERBOSE_ERRORS
 	   error translation table will grow the kernel image size by
 	   about 4 KB.
 
+config NVME_FAULT_INJECTION_DEBUG_FS
+	bool "NVMe Fault Injection"
+	depends on FAULT_INJECTION_DEBUG_FS
+	help
+	   This enables NVMe Fault Injection through debugfs.
+
 config NVME_HWMON
 	bool "NVMe hardware monitoring"
 	depends on (NVME_CORE=y && HWMON=y) || (NVME_CORE=m && HWMON)
diff --git a/drivers/nvme/host/Makefile b/drivers/nvme/host/Makefile
index e27202d22c7d..c232d7703b65 100644
--- a/drivers/nvme/host/Makefile
+++ b/drivers/nvme/host/Makefile
@@ -15,7 +15,7 @@ nvme-core-$(CONFIG_NVME_VERBOSE_ERRORS)	+= constants.o
 nvme-core-$(CONFIG_TRACING)		+= trace.o
 nvme-core-$(CONFIG_NVME_MULTIPATH)	+= multipath.o
 nvme-core-$(CONFIG_BLK_DEV_ZONED)	+= zns.o
-nvme-core-$(CONFIG_FAULT_INJECTION_DEBUG_FS)	+= fault_inject.o
+nvme-core-$(CONFIG_FAULT_INJECTION_DEBUG_FS)	+= nvme-debugfs.o
 nvme-core-$(CONFIG_NVME_HWMON)		+= hwmon.o
 nvme-core-$(CONFIG_NVME_AUTH)		+= auth.o
 
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2730b116dc6..cf5db19d50a5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4326,7 +4326,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
 		nvme_add_ns_cdev(ns);
 
 	nvme_mpath_add_disk(ns, info->anagrpid);
-	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
+
+	nvme_debugfs_init(&ns->debugfs, ns->disk->disk_name);
+	nvme_fault_inject_init(&ns->debugfs);
 
 	return;
 
@@ -4357,7 +4359,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
 	clear_bit(NVME_NS_READY, &ns->flags);
 	set_capacity(ns->disk, 0);
-	nvme_fault_inject_fini(&ns->fault_inject);
+	nvme_debugfs_remove(&ns->debugfs);
 
 	/*
 	 * Ensure that !NVME_NS_READY is seen by other threads to prevent
@@ -5054,7 +5056,7 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
 	nvme_hwmon_exit(ctrl);
-	nvme_fault_inject_fini(&ctrl->fault_inject);
+	nvme_debugfs_remove(&ctrl->debugfs);
 	dev_pm_qos_hide_latency_tolerance(ctrl->device);
 	cdev_device_del(&ctrl->cdev, ctrl->device);
 	nvme_put_ctrl(ctrl);
@@ -5179,7 +5181,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	dev_pm_qos_update_user_latency_tolerance(ctrl->device,
 		min(default_ps_max_latency_us, (unsigned long)S32_MAX));
 
-	nvme_fault_inject_init(&ctrl->fault_inject, dev_name(ctrl->device));
+	nvme_debugfs_init(&ctrl->debugfs, dev_name(ctrl->device));
+	nvme_fault_inject_init(&ctrl->debugfs);
 	nvme_mpath_init_ctrl(ctrl);
 	ret = nvme_auth_init_ctrl(ctrl);
 	if (ret)
diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/nvme-debugfs.c
similarity index 60%
rename from drivers/nvme/host/fault_inject.c
rename to drivers/nvme/host/nvme-debugfs.c
index 83d2e6860d38..87f78b864225 100644
--- a/drivers/nvme/host/fault_inject.c
+++ b/drivers/nvme/host/nvme-debugfs.c
@@ -1,13 +1,27 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * fault injection support for nvme.
- *
- * Copyright (c) 2018, Oracle and/or its affiliates
+ * Copyright (c) 2023, Oracle and/or its affiliates
  */
 
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/moduleparam.h>
 #include "nvme.h"
 
+#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
+void nvme_debugfs_init(struct nvme_debugfs *debugfs,
+		  const char *dev_name)
+{
+	/* create debugfs directory */
+	debugfs->parent = debugfs_create_dir(dev_name, NULL);
+}
+
+void nvme_debugfs_remove(struct nvme_debugfs *debugfs)
+{
+	/* remove debugfs directories */
+	debugfs_remove_recursive(debugfs->parent);
+}
+#ifdef CONFIG_NVME_FAULT_INJECTION_DEBUG_FS
 static DECLARE_FAULT_ATTR(fail_default_attr);
 /* optional fault injection attributes boot time option:
  * nvme_core.fail_request=<interval>,<probability>,<space>,<times>
@@ -15,31 +29,20 @@ static DECLARE_FAULT_ATTR(fail_default_attr);
 static char *fail_request;
 module_param(fail_request, charp, 0000);
 
-void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
-			    const char *dev_name)
+void nvme_fault_inject_init(struct nvme_debugfs *debugfs)
 {
-	struct dentry *dir, *parent;
+	struct nvme_fault_inject *fault_inj = &debugfs->fault_inject;
+	struct dentry *dir;
 	struct fault_attr *attr = &fault_inj->attr;
 
 	/* set default fault injection attribute */
 	if (fail_request)
 		setup_fault_attr(&fail_default_attr, fail_request);
 
-	/* create debugfs directory and attribute */
-	parent = debugfs_create_dir(dev_name, NULL);
-	if (!parent) {
-		pr_warn("%s: failed to create debugfs directory\n", dev_name);
-		return;
-	}
-
 	*attr = fail_default_attr;
-	dir = fault_create_debugfs_attr("fault_inject", parent, attr);
-	if (IS_ERR(dir)) {
-		pr_warn("%s: failed to create debugfs attr\n", dev_name);
-		debugfs_remove_recursive(parent);
+	dir = fault_create_debugfs_attr("fault_inject", debugfs->parent, attr);
+	if (IS_ERR(dir))
 		return;
-	}
-	fault_inj->parent = parent;
 
 	/* create debugfs for status code and dont_retry */
 	fault_inj->status = NVME_SC_INVALID_OPCODE;
@@ -48,12 +51,6 @@ void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
 	debugfs_create_bool("dont_retry", 0600, dir, &fault_inj->dont_retry);
 }
 
-void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inject)
-{
-	/* remove debugfs directories */
-	debugfs_remove_recursive(fault_inject->parent);
-}
-
 void nvme_should_fail(struct request *req)
 {
 	struct gendisk *disk = req->q->disk;
@@ -64,12 +61,11 @@ void nvme_should_fail(struct request *req)
 		struct nvme_ns *ns = disk->private_data;
 
 		if (ns)
-			fault_inject = &ns->fault_inject;
+			fault_inject = &ns->debugfs.fault_inject;
 		else
 			WARN_ONCE(1, "No namespace found for request\n");
-	} else {
-		fault_inject = &nvme_req(req)->ctrl->fault_inject;
-	}
+	} else
+		fault_inject = &nvme_req(req)->ctrl->debugfs.fault_inject;
 
 	if (fault_inject && should_fail(&fault_inject->attr, 1)) {
 		/* inject status code and DNR bit */
@@ -80,3 +76,5 @@ void nvme_should_fail(struct request *req)
 	}
 }
 EXPORT_SYMBOL_GPL(nvme_should_fail);
+#endif /* CONFIG_NVME_FAULT_INJECTION_DEBUG_FS */
+#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bf46f122e9e1..53d61723ca59 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -231,12 +231,16 @@ enum nvme_ctrl_state {
 struct nvme_fault_inject {
 #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
 	struct fault_attr attr;
-	struct dentry *parent;
 	bool dont_retry;	/* DNR, do not retry */
 	u16 status;		/* status code */
 #endif
 };
 
+struct nvme_debugfs {
+	struct dentry *parent;
+	struct nvme_fault_inject fault_inject;
+};
+
 enum nvme_ctrl_flags {
 	NVME_CTRL_FAILFAST_EXPIRED	= 0,
 	NVME_CTRL_ADMIN_Q_STOPPED	= 1,
@@ -370,7 +374,7 @@ struct nvme_ctrl {
 	struct page *discard_page;
 	unsigned long discard_page_busy;
 
-	struct nvme_fault_inject fault_inject;
+	struct nvme_debugfs debugfs;
 
 	enum nvme_ctrl_type cntrltype;
 	enum nvme_dctype dctype;
@@ -496,8 +500,7 @@ struct nvme_ns {
 	struct cdev		cdev;
 	struct device		cdev_device;
 
-	struct nvme_fault_inject fault_inject;
-
+	struct nvme_debugfs debugfs;
 };
 
 /* NVMe ns supports metadata actions by the controller (generate/strip) */
@@ -598,21 +601,23 @@ static inline void nvme_print_device_info(struct nvme_ctrl *ctrl)
 }
 
 #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
-void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
-			    const char *dev_name);
-void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inject);
+void nvme_debugfs_init(struct nvme_debugfs *debugfs,
+		       const char *dev_name);
+void nvme_debugfs_remove(struct nvme_debugfs *debugfs);
+#else
+static inline void nvme_debugfs_init(struct nvme_debugfs *debugfs,
+			const char *dev_name) {}
+static inline void nvme_debugfs_remove(struct nvme_debugfs *debugfs) {}
+#endif
+#ifdef CONFIG_NVME_FAULT_INJECTION_DEBUG_FS
+void nvme_fault_inject_init(struct nvme_debugfs *debugfs);
 void nvme_should_fail(struct request *req);
 #else
-static inline void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
-					  const char *dev_name)
-{
-}
-static inline void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inj)
+static inline void nvme_fault_inject_init(struct nvme_debugfs *debugfs)
 {
 }
 static inline void nvme_should_fail(struct request *req) {}
 #endif
-
 bool nvme_wait_reset(struct nvme_ctrl *ctrl);
 int nvme_try_sched_reset(struct nvme_ctrl *ctrl);
 
-- 
2.31.1

[RFC 2/2] nvme: add error logging opt-in

[Alan Adamson]

Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
logging for user passthrough commands.  This commit adds the ability to opt-in
to passthrough error logging.

To enable passthrough error logging:
	echo Y > /sys/kernel/debug/nvme0/error-logging

To disable passthrough error logging:
	echo N > /sys/kernel/debug/nvme0/error-logging

By default, passthrough error logging will remain disabled.

CONFIG_NVME_ERROR_LOGGING_DEBUG_FS needs to be enabled to
to enable passthrough error logging.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
---
 drivers/nvme/host/Kconfig        |  6 ++++++
 drivers/nvme/host/core.c         | 13 +++++++++++--
 drivers/nvme/host/ioctl.c        |  1 +
 drivers/nvme/host/nvme-debugfs.c |  6 ++++++
 drivers/nvme/host/nvme.h         |  8 ++++++++
 5 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 70f380c1ccae..5677292727bf 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -38,6 +38,12 @@ config NVME_FAULT_INJECTION_DEBUG_FS
 	help
 	   This enables NVMe Fault Injection through debugfs.
 
+config NVME_ERROR_LOGGING_DEBUG_FS
+	bool "NVMe Pass Thru Error Logging"
+	depends on FAULT_INJECTION_DEBUG_FS
+	help
+	   This enables NVMe Pass Thru command error logging.
+
 config NVME_HWMON
 	bool "NVMe hardware monitoring"
 	depends on (NVME_CORE=y && HWMON=y) || (NVME_CORE=m && HWMON)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cf5db19d50a5..eb8d7caf2887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -666,6 +666,8 @@ static inline void nvme_clear_nvme_request(struct request *req)
 /* initialize a passthrough request */
 void nvme_init_request(struct request *req, struct nvme_command *cmd)
 {
+	struct nvme_request *nr = nvme_req(req);
+
 	if (req->q->queuedata)
 		req->timeout = NVME_IO_TIMEOUT;
 	else /* no queuedata implies admin queue */
@@ -678,8 +680,11 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
 	if (req->mq_hctx->type == HCTX_TYPE_POLL)
 		req->cmd_flags |= REQ_POLLED;
 	nvme_clear_nvme_request(req);
-	req->rq_flags |= RQF_QUIET;
-	memcpy(nvme_req(req)->cmd, cmd, sizeof(*cmd));
+
+	if (!nr->ctrl->debugfs.error_logging)
+		req->rq_flags |= RQF_QUIET;
+
+	memcpy(nr->cmd, cmd, sizeof(*cmd));
 }
 EXPORT_SYMBOL_GPL(nvme_init_request);
 
@@ -5183,6 +5188,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 
 	nvme_debugfs_init(&ctrl->debugfs, dev_name(ctrl->device));
 	nvme_fault_inject_init(&ctrl->debugfs);
+
+	ctrl->debugfs.error_logging = false;
+	nvme_error_logging_init(&ctrl->debugfs);
+
 	nvme_mpath_init_ctrl(ctrl);
 	ret = nvme_auth_init_ctrl(ctrl);
 	if (ret)
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 723e7d5b778f..55d3f86524bf 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -243,6 +243,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	ctrl = nvme_req(req)->ctrl;
 
 	effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
+
 	ret = nvme_execute_rq(req, false);
 	if (result)
 		*result = le64_to_cpu(nvme_req(req)->result.u64);
diff --git a/drivers/nvme/host/nvme-debugfs.c b/drivers/nvme/host/nvme-debugfs.c
index 87f78b864225..09953626a1dd 100644
--- a/drivers/nvme/host/nvme-debugfs.c
+++ b/drivers/nvme/host/nvme-debugfs.c
@@ -77,4 +77,10 @@ void nvme_should_fail(struct request *req)
 }
 EXPORT_SYMBOL_GPL(nvme_should_fail);
 #endif /* CONFIG_NVME_FAULT_INJECTION_DEBUG_FS */
+#ifdef CONFIG_NVME_ERROR_LOGGING_DEBUG_FS
+void nvme_error_logging_init(struct nvme_debugfs *debugfs)
+{
+	debugfs_create_bool("error-logging", 0600, debugfs->parent, &debugfs->error_logging);
+}
+#endif /* CONFIG_NVME_ERROR_LOGGING_DEBUG_FS */
 #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 53d61723ca59..74bdf5fd9abd 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -239,6 +239,7 @@ struct nvme_fault_inject {
 struct nvme_debugfs {
 	struct dentry *parent;
 	struct nvme_fault_inject fault_inject;
+	bool error_logging;
 };
 
 enum nvme_ctrl_flags {
@@ -618,6 +619,13 @@ static inline void nvme_fault_inject_init(struct nvme_debugfs *debugfs)
 }
 static inline void nvme_should_fail(struct request *req) {}
 #endif
+#ifdef CONFIG_NVME_ERROR_LOGGING_DEBUG_FS
+void nvme_error_logging_init(struct nvme_debugfs *debugfs);
+#else
+static inline void nvme_error_logging_init(struct nvme_debugfs *debugfs)
+{
+}
+#endif /* CONFIG_NVME_ERROR_LOGGING_DEBUG_FS */
 bool nvme_wait_reset(struct nvme_ctrl *ctrl);
 int nvme_try_sched_reset(struct nvme_ctrl *ctrl);
 
-- 
2.31.1

Re: [RFC 2/2] nvme: add error logging opt-in

[Chaitanya Kulkarni]

On 3/16/2023 4:49 PM, Alan Adamson wrote:
> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
> logging for user passthrough commands.  This commit adds the ability to opt-in
> to passthrough error logging.
> 
> To enable passthrough error logging:
> 	echo Y > /sys/kernel/debug/nvme0/error-logging
> 
can we use 0/1 based enabling ? Y and N seems a bit odd,
unless we can't for some reason ...
> To disable passthrough error logging:
> 	echo N > /sys/kernel/debug/nvme0/error-logging
> 
> By default, passthrough error logging will remain disabled.
> 
> CONFIG_NVME_ERROR_LOGGING_DEBUG_FS needs to be enabled to
> to enable passthrough error logging.
> 
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
> ---
>   drivers/nvme/host/Kconfig        |  6 ++++++
>   drivers/nvme/host/core.c         | 13 +++++++++++--
>   drivers/nvme/host/ioctl.c        |  1 +
>   drivers/nvme/host/nvme-debugfs.c |  6 ++++++
>   drivers/nvme/host/nvme.h         |  8 ++++++++
>   5 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index 70f380c1ccae..5677292727bf 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -38,6 +38,12 @@ config NVME_FAULT_INJECTION_DEBUG_FS
>   	help
>   	   This enables NVMe Fault Injection through debugfs.
>   
> +config NVME_ERROR_LOGGING_DEBUG_FS
> +	bool "NVMe Pass Thru Error Loggigng"

/Pass Thru/Passthrough/s ?

> +	depends on FAULT_INJECTION_DEBUG_FS
> +	help
> +	   This enables NVMe Pass Thru command error logging.
> +
>   config NVME_HWMON
>   	bool "NVMe hardware monitoring"
>   	depends on (NVME_CORE=y && HWMON=y) || (NVME_CORE=m && HWMON)
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cf5db19d50a5..eb8d7caf2887 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -666,6 +666,8 @@ static inline void nvme_clear_nvme_request(struct request *req)
>   /* initialize a passthrough request */
>   void nvme_init_request(struct request *req, struct nvme_command *cmd)
>   {
> +	struct nvme_request *nr = nvme_req(req);
> +
>   	if (req->q->queuedata)
>   		req->timeout = NVME_IO_TIMEOUT;
>   	else /* no queuedata implies admin queue */
> @@ -678,8 +680,11 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
>   	if (req->mq_hctx->type == HCTX_TYPE_POLL)
>   		req->cmd_flags |= REQ_POLLED;
>   	nvme_clear_nvme_request(req);
> -	req->rq_flags |= RQF_QUIET;
> -	memcpy(nvme_req(req)->cmd, cmd, sizeof(*cmd));
> +
> +	if (!nr->ctrl->debugfs.error_logging)
> +		req->rq_flags |= RQF_QUIET;
> +
> +	memcpy(nr->cmd, cmd, sizeof(*cmd));
>   }
>   EXPORT_SYMBOL_GPL(nvme_init_request);
>   
> @@ -5183,6 +5188,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>   
>   	nvme_debugfs_init(&ctrl->debugfs, dev_name(ctrl->device));
>   	nvme_fault_inject_init(&ctrl->debugfs);
> +
> +	ctrl->debugfs.error_logging = false;
> +	nvme_error_logging_init(&ctrl->debugfs);
> +
>   	nvme_mpath_init_ctrl(ctrl);
>   	ret = nvme_auth_init_ctrl(ctrl);
>   	if (ret)
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 723e7d5b778f..55d3f86524bf 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -243,6 +243,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   	ctrl = nvme_req(req)->ctrl;
>   
>   	effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
> +

no white in patch please ..

>   	ret = nvme_execute_rq(req, false);
>   	if (result)
>   		*result = le64_to_cpu(nvme_req(req)->result.u64);
> diff --git a/drivers/nvme/host/nvme-debugfs.c b/drivers/nvme/host/nvme-debugfs.c
> index 87f78b864225..09953626a1dd 100644
> --- a/drivers/nvme/host/nvme-debugfs.c
> +++ b/drivers/nvme/host/nvme-debugfs.c
> @@ -77,4 +77,10 @@ void nvme_should_fail(struct request *req)
>   }
>   EXPORT_SYMBOL_GPL(nvme_should_fail);
>   #endif /* CONFIG_NVME_FAULT_INJECTION_DEBUG_FS */
> +#ifdef CONFIG_NVME_ERROR_LOGGING_DEBUG_FS
> +void nvme_error_logging_init(struct nvme_debugfs *debugfs)
> +{
> +	debugfs_create_bool("error-logging", 0600, debugfs->parent, &debugfs->error_logging);

iff possible keep it under 80 char..

> +}
> +#endif /* CONFIG_NVME_ERROR_LOGGING_DEBUG_FS */
>   #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 53d61723ca59..74bdf5fd9abd 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -239,6 +239,7 @@ struct nvme_fault_inject {
>   struct nvme_debugfs {
>   	struct dentry *parent;
>   	struct nvme_fault_inject fault_inject;
> +	bool error_logging;
>   };
>   
>   enum nvme_ctrl_flags {
> @@ -618,6 +619,13 @@ static inline void nvme_fault_inject_init(struct nvme_debugfs *debugfs)
>   }
>   static inline void nvme_should_fail(struct request *req) {}
>   #endif
> +#ifdef CONFIG_NVME_ERROR_LOGGING_DEBUG_FS
> +void nvme_error_logging_init(struct nvme_debugfs *debugfs);
> +#else
> +static inline void nvme_error_logging_init(struct nvme_debugfs *debugfs)
> +{
> +}

since there is only one caller for above function can we use
if (IS_ENABLED(....)) ?

> +#endif /* CONFIG_NVME_ERROR_LOGGING_DEBUG_FS */
>   bool nvme_wait_reset(struct nvme_ctrl *ctrl);
>   int nvme_try_sched_reset(struct nvme_ctrl *ctrl);
>  

-ck

Re: [RFC 2/2] nvme: add error logging opt-in

[Pankaj Raghav]

On Thu, Mar 16, 2023 at 04:49:58PM -0700, Alan Adamson wrote:
>  
> @@ -5183,6 +5188,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>  
>  	nvme_debugfs_init(&ctrl->debugfs, dev_name(ctrl->device));
>  	nvme_fault_inject_init(&ctrl->debugfs);
> +
> +	ctrl->debugfs.error_logging = false;
Can we wrap the above statement into nvme_error_logging_init function?
static inline void nvme_error_logging_init(struct nvme_debugfs *debugfs)
{
	debugfs->error_logging = false;
}
> +	nvme_error_logging_init(&ctrl->debugfs);
> +
>  	nvme_mpath_init_ctrl(ctrl);
>  	ret = nvme_auth_init_ctrl(ctrl);
>  	if (ret)
> +#ifdef CONFIG_NVME_ERROR_LOGGING_DEBUG_FS
> +void nvme_error_logging_init(struct nvme_debugfs *debugfs);
> +#else
> +static inline void nvme_error_logging_init(struct nvme_debugfs *debugfs)
> +{
> +}
> +#endif /* CONFIG_NVME_ERROR_LOGGING_DEBUG_FS */

Re: [RFC 2/2] nvme: add error logging opt-in

[alan.adamson]

On 3/17/23 1:52 AM, Pankaj Raghav wrote:
> On Thu, Mar 16, 2023 at 04:49:58PM -0700, Alan Adamson wrote:
>>   
>> @@ -5183,6 +5188,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>>   
>>   	nvme_debugfs_init(&ctrl->debugfs, dev_name(ctrl->device));
>>   	nvme_fault_inject_init(&ctrl->debugfs);
>> +
>> +	ctrl->debugfs.error_logging = false;
> Can we wrap the above statement into nvme_error_logging_init function?
> static inline void nvme_error_logging_init(struct nvme_debugfs *debugfs)
> {
> 	debugfs->error_logging = false;
> }

Since the error_logging flag is being used outside the debugfs 
environment, it will need to be initialized to false even if Pass 
Through errors is not configured.  I will move the flag to the nvme_ctrl.

Thanks,

Alan