[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 at 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