* [RFC v4 0/1] nvme: add passthrough admin error logging opt-in @ 2023-04-06 21:55 Alan Adamson 2023-04-06 21:55 ` [RFC v4 1/1] " Alan Adamson 2023-04-07 4:46 ` [RFC v4 0/1] " Chaitanya Kulkarni 0 siblings, 2 replies; 7+ messages in thread From: Alan Adamson @ 2023-04-06 21:55 UTC (permalink / raw) To: linux-nvme; +Cc: alan.adamson, kbusch, hch, sagi v4: - Change sysfs attribute to passthru_admin_err_logging - Only log passthrough admin commands. IO passthrough commands will always be logged. v3: - Log a passthrough specific message that dumps CDW* contents. - Enable/disable vis sysfs rather than debugfs. v2: - Included Pankaj Raghav's patch 'nvme: ignore starting sector while error logging for passthrough requests' with a couple changes. - Moved error_logging flag to nvme_ctrl structure - The entire nvme-debugfs.c does not need to be guarded by #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS. - Use IS_ENABLED((CONFIG_NVME_ERROR_LOGGING_DEBUG_FS)) to determine if error logging should be initialized. - Various other nits. Alan Adamson (1): nvme: add passthrough admin error logging opt-in drivers/nvme/host/core.c | 97 +++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/nvme.h | 1 + 2 files changed, 93 insertions(+), 5 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC v4 1/1] nvme: add passthrough admin error logging opt-in 2023-04-06 21:55 [RFC v4 0/1] nvme: add passthrough admin error logging opt-in Alan Adamson @ 2023-04-06 21:55 ` Alan Adamson 2023-04-07 4:45 ` Chaitanya Kulkarni 2023-04-07 4:46 ` [RFC v4 0/1] " Chaitanya Kulkarni 1 sibling, 1 reply; 7+ messages in thread From: Alan Adamson @ 2023-04-06 21:55 UTC (permalink / raw) To: linux-nvme; +Cc: alan.adamson, kbusch, hch, sagi 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 admin error logging. IO commands initiated as passthrough will always be logged. The logging output for passthrough commands (Admin and IO) has been changed to include CDWXX fields. nvme0n1: Read(0x2), LBA Out of Range (sct 0x0 / sc 0x80) DNR cdw10=0x0 cdw11=0x1 cdw12=0x70000 cdw13=0x0 cdw14=0x0 cdw15=0x0 To enable passthrough admin error logging: echo 1 > /sys/class/nvme/nvme0/passthru_admin_err_logging To disable passthrough admin error logging: echo 0 > /sys/class/nvme/nvme0/passthru_admin_err_logging By default, passthrough admin error logging will be disabled. Signed-off-by: Alan Adamson <alan.adamson@oracle.com> --- drivers/nvme/host/core.c | 97 +++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/nvme.h | 1 + 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 53ef028596c6..a17a24f4c8af 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -337,6 +337,49 @@ static void nvme_log_error(struct request *req) nr->status & NVME_SC_DNR ? "DNR " : ""); } +static void nvme_log_error_passthrough(struct request *req) +{ + struct nvme_ns *ns = req->q->queuedata; + struct nvme_request *nr = nvme_req(req); + + if (ns) { + pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" + "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", + ns->disk ? ns->disk->disk_name : "?", + nvme_get_opcode_str(nr->cmd->common.opcode), + nr->cmd->common.opcode, + nvme_get_error_status_str(nr->status), + nr->status >> 8 & 7, /* Status Code Type */ + nr->status & 0xff, /* Status Code */ + nr->status & NVME_SC_MORE ? "MORE " : "", + nr->status & NVME_SC_DNR ? "DNR " : "", + nr->cmd->common.cdw10, + nr->cmd->common.cdw11, + nr->cmd->common.cdw12, + nr->cmd->common.cdw13, + nr->cmd->common.cdw14, + nr->cmd->common.cdw14); + return; + } + + pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" + "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", + dev_name(nr->ctrl->device), + nvme_get_admin_opcode_str(nr->cmd->common.opcode), + nr->cmd->common.opcode, + nvme_get_error_status_str(nr->status), + nr->status >> 8 & 7, /* Status Code Type */ + nr->status & 0xff, /* Status Code */ + nr->status & NVME_SC_MORE ? "MORE " : "", + nr->status & NVME_SC_DNR ? "DNR " : "", + nr->cmd->common.cdw10, + nr->cmd->common.cdw11, + nr->cmd->common.cdw12, + nr->cmd->common.cdw13, + nr->cmd->common.cdw14, + nr->cmd->common.cdw14); +} + enum nvme_disposition { COMPLETE, RETRY, @@ -381,8 +424,12 @@ static inline void nvme_end_req(struct request *req) { blk_status_t status = nvme_error_status(nvme_req(req)->status); - if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET))) - nvme_log_error(req); + if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET))) { + if (blk_rq_is_passthrough(req)) + nvme_log_error_passthrough(req); + else + nvme_log_error(req); + } nvme_end_req_zoned(req); nvme_trace_bio_complete(req); if (req->cmd_flags & REQ_NVME_MPATH) @@ -666,10 +713,15 @@ 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 */ + else { /* no queuedata implies admin queue */ req->timeout = NVME_ADMIN_TIMEOUT; + if (!nr->ctrl->error_logging) + req->rq_flags |= RQF_QUIET; + } /* passthru commands should let the driver set the SGL flags */ cmd->common.flags &= ~NVME_CMD_SGL_ALL; @@ -678,8 +730,8 @@ 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)); + + memcpy(nr->cmd, cmd, sizeof(*cmd)); } EXPORT_SYMBOL_GPL(nvme_init_request); @@ -3418,6 +3470,39 @@ static ssize_t nvme_sysfs_rescan(struct device *dev, } static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, nvme_sysfs_rescan); +static ssize_t nvme_passthru_admin_err_logging_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct nvme_ctrl *ctrl = dev_get_drvdata(dev); + + if (ctrl->error_logging) + return sysfs_emit(buf, "on\n"); + else + return sysfs_emit(buf, "off\n"); +} + +static ssize_t nvme_passthru_admin_err_logging_store(struct device *dev, + struct device_attribute *attr, const char *buf, size_t count) +{ + struct nvme_ctrl *ctrl = dev_get_drvdata(dev); + int passthrough_enable, err; + + err = kstrtoint(buf, 10, &passthrough_enable); + if (err) + return -EINVAL; + + if (passthrough_enable) + ctrl->error_logging = true; + else + ctrl->error_logging = false; + + return count; +} + +static DEVICE_ATTR(passthru_admin_err_logging, S_IRUGO | S_IWUSR, + nvme_passthru_admin_err_logging_show, + nvme_passthru_admin_err_logging_store); + static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev) { struct gendisk *disk = dev_to_disk(dev); @@ -3926,6 +4011,7 @@ static struct attribute *nvme_dev_attrs[] = { &dev_attr_dhchap_secret.attr, &dev_attr_dhchap_ctrl_secret.attr, #endif + &dev_attr_passthru_admin_err_logging.attr, NULL }; @@ -5125,6 +5211,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev, int ret; ctrl->state = NVME_CTRL_NEW; + ctrl->error_logging = false; clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags); spin_lock_init(&ctrl->lock); mutex_init(&ctrl->scan_lock); diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index bf46f122e9e1..dce5e6f7260c 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -248,6 +248,7 @@ struct nvme_ctrl { bool comp_seen; enum nvme_ctrl_state state; bool identified; + bool error_logging; spinlock_t lock; struct mutex scan_lock; const struct nvme_ctrl_ops *ops; -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC v4 1/1] nvme: add passthrough admin error logging opt-in 2023-04-06 21:55 ` [RFC v4 1/1] " Alan Adamson @ 2023-04-07 4:45 ` Chaitanya Kulkarni 2023-04-07 7:48 ` hch 0 siblings, 1 reply; 7+ messages in thread From: Chaitanya Kulkarni @ 2023-04-07 4:45 UTC (permalink / raw) To: Alan Adamson, linux-nvme@lists.infradead.org Cc: kbusch@kernel.org, hch@lst.de, sagi@grimberg.me On 4/6/23 14:55, 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 admin error logging. IO commands initiated as passthrough will always > be logged. > > The logging output for passthrough commands (Admin and IO) has been changed to include > CDWXX fields. > > nvme0n1: Read(0x2), LBA Out of Range (sct 0x0 / sc 0x80) DNR cdw10=0x0 cdw11=0x1 > cdw12=0x70000 cdw13=0x0 cdw14=0x0 cdw15=0x0 > > To enable passthrough admin error logging: > echo 1 > /sys/class/nvme/nvme0/passthru_admin_err_logging > > To disable passthrough admin error logging: > echo 0 > /sys/class/nvme/nvme0/passthru_admin_err_logging > > By default, passthrough admin error logging will be disabled. > > Signed-off-by: Alan Adamson <alan.adamson@oracle.com> > --- > drivers/nvme/host/core.c | 97 +++++++++++++++++++++++++++++++++++++--- > drivers/nvme/host/nvme.h | 1 + > 2 files changed, 93 insertions(+), 5 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 53ef028596c6..a17a24f4c8af 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -337,6 +337,49 @@ static void nvme_log_error(struct request *req) > nr->status & NVME_SC_DNR ? "DNR " : ""); > } > > +static void nvme_log_error_passthrough(struct request *req) > +{ > + struct nvme_ns *ns = req->q->queuedata; > + struct nvme_request *nr = nvme_req(req); > + > + if (ns) { > + pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" > + "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", > + ns->disk ? ns->disk->disk_name : "?", > + nvme_get_opcode_str(nr->cmd->common.opcode), > + nr->cmd->common.opcode, > + nvme_get_error_status_str(nr->status), > + nr->status >> 8 & 7, /* Status Code Type */ > + nr->status & 0xff, /* Status Code */ > + nr->status & NVME_SC_MORE ? "MORE " : "", > + nr->status & NVME_SC_DNR ? "DNR " : "", > + nr->cmd->common.cdw10, > + nr->cmd->common.cdw11, > + nr->cmd->common.cdw12, > + nr->cmd->common.cdw13, > + nr->cmd->common.cdw14, > + nr->cmd->common.cdw14); > + return; > + } > + > + pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" > + "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", > + dev_name(nr->ctrl->device), > + nvme_get_admin_opcode_str(nr->cmd->common.opcode), > + nr->cmd->common.opcode, > + nvme_get_error_status_str(nr->status), > + nr->status >> 8 & 7, /* Status Code Type */ > + nr->status & 0xff, /* Status Code */ > + nr->status & NVME_SC_MORE ? "MORE " : "", > + nr->status & NVME_SC_DNR ? "DNR " : "", > + nr->cmd->common.cdw10, > + nr->cmd->common.cdw11, > + nr->cmd->common.cdw12, > + nr->cmd->common.cdw13, > + nr->cmd->common.cdw14, > + nr->cmd->common.cdw14); > +} > + why not following ? it also fixes cdw15 which should be the expected last arg to pr_err_ratelimited() argument and not cdw14 and also saves code ? totally untested :- diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index a17a24f4c8af..d2ea214bb674 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -341,43 +341,28 @@ static void nvme_log_error_passthrough(struct request *req) { struct nvme_ns *ns = req->q->queuedata; struct nvme_request *nr = nvme_req(req); + const char *device_name = dev_name(nr->ctrl->device); - if (ns) { - pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" - "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", - ns->disk ? ns->disk->disk_name : "?", - nvme_get_opcode_str(nr->cmd->common.opcode), - nr->cmd->common.opcode, - nvme_get_error_status_str(nr->status), - nr->status >> 8 & 7, /* Status Code Type */ - nr->status & 0xff, /* Status Code */ - nr->status & NVME_SC_MORE ? "MORE " : "", - nr->status & NVME_SC_DNR ? "DNR " : "", - nr->cmd->common.cdw10, - nr->cmd->common.cdw11, - nr->cmd->common.cdw12, - nr->cmd->common.cdw13, - nr->cmd->common.cdw14, - nr->cmd->common.cdw14); - return; - } + if (ns) + device_name = ns->disk ? ns->disk->disk_name : "?"; pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" - "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", - dev_name(nr->ctrl->device), - nvme_get_admin_opcode_str(nr->cmd->common.opcode), - nr->cmd->common.opcode, - nvme_get_error_status_str(nr->status), - nr->status >> 8 & 7, /* Status Code Type */ - nr->status & 0xff, /* Status Code */ - nr->status & NVME_SC_MORE ? "MORE " : "", - nr->status & NVME_SC_DNR ? "DNR " : "", - nr->cmd->common.cdw10, - nr->cmd->common.cdw11, - nr->cmd->common.cdw12, - nr->cmd->common.cdw13, - nr->cmd->common.cdw14, - nr->cmd->common.cdw14); + "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", + device_name, + nvme_get_opcode_str(nr->cmd->common.opcode), + nr->cmd->common.opcode, + nvme_get_error_status_str(nr->status), + nr->status >> 8 & 7, /* Status Code Type */ + nr->status & 0xff, /* Status Code */ + nr->status & NVME_SC_MORE ? "MORE " : "", + nr->status & NVME_SC_DNR ? "DNR " : "", + nr->cmd->common.cdw10, + nr->cmd->common.cdw11, + nr->cmd->common.cdw12, + nr->cmd->common.cdw13, + nr->cmd->common.cdw14, + nr->cmd->common.cdw15); + } > enum nvme_disposition { > COMPLETE, > RETRY, > @@ -381,8 +424,12 @@ static inline void nvme_end_req(struct request *req) > { > blk_status_t status = nvme_error_status(nvme_req(req)->status); > > - if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET))) > - nvme_log_error(req); > + if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET))) { > + if (blk_rq_is_passthrough(req)) > + nvme_log_error_passthrough(req); > + else > + nvme_log_error(req); > + } > nvme_end_req_zoned(req); > nvme_trace_bio_complete(req); > if (req->cmd_flags & REQ_NVME_MPATH) > @@ -666,10 +713,15 @@ 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 */ > + else { /* no queuedata implies admin queue */ > req->timeout = NVME_ADMIN_TIMEOUT; > + if (!nr->ctrl->error_logging) > + req->rq_flags |= RQF_QUIET; > + } > > /* passthru commands should let the driver set the SGL flags */ > cmd->common.flags &= ~NVME_CMD_SGL_ALL; > @@ -678,8 +730,8 @@ 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)); > + > + memcpy(nr->cmd, cmd, sizeof(*cmd)); > } > EXPORT_SYMBOL_GPL(nvme_init_request); > > @@ -3418,6 +3470,39 @@ static ssize_t nvme_sysfs_rescan(struct device *dev, > } > static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, nvme_sysfs_rescan); > > +static ssize_t nvme_passthru_admin_err_logging_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct nvme_ctrl *ctrl = dev_get_drvdata(dev); > + > + if (ctrl->error_logging) > + return sysfs_emit(buf, "on\n"); > + else > + return sysfs_emit(buf, "off\n"); > +} > + how about this ? which avoids else since it's direct return at the end of the function, totally untested :- @@ -3477,8 +3462,8 @@ static ssize_t nvme_passthru_admin_err_logging_show(struct device *dev, if (ctrl->error_logging) return sysfs_emit(buf, "on\n"); - else - return sysfs_emit(buf, "off\n"); + + return sysfs_emit(buf, "off\n"); } static ssize_t nvme_passthru_admin_err_logging_store(struct device *dev, > +static ssize_t nvme_passthru_admin_err_logging_store(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ > + struct nvme_ctrl *ctrl = dev_get_drvdata(dev); > + int passthrough_enable, err; > + > + err = kstrtoint(buf, 10, &passthrough_enable); > + if (err) > + return -EINVAL; > + > + if (passthrough_enable) > + ctrl->error_logging = true; > + else > + ctrl->error_logging = false; > + > + return count; > +} > + how about this ? which also throws error if value is non-bool since we are using int type for passthrough_enable here, totally untested :- @@ -3491,10 +3476,16 @@ static ssize_t nvme_passthru_admin_err_logging_store(struct device *dev, if (err) return -EINVAL; - if (passthrough_enable) - ctrl->error_logging = true; - else - ctrl->error_logging = false; + switch (passthrough_enable) { + case true: + case false: + ctrl->error_logging = passthrough_enable; + break; + default: + pr_err("invlid value %d for admin error logging [on:1 off:0]\n", + passthrough_enable); + break; + } return count; } below is the entire patch for the code with all the above changes on the top of this version see if it makes sense, totally untested :- diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index a17a24f4c8af..cca5424bf9f0 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -341,43 +341,28 @@ static void nvme_log_error_passthrough(struct request *req) { struct nvme_ns *ns = req->q->queuedata; struct nvme_request *nr = nvme_req(req); + const char *device_name = dev_name(nr->ctrl->device); - if (ns) { - pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" - "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", - ns->disk ? ns->disk->disk_name : "?", - nvme_get_opcode_str(nr->cmd->common.opcode), - nr->cmd->common.opcode, - nvme_get_error_status_str(nr->status), - nr->status >> 8 & 7, /* Status Code Type */ - nr->status & 0xff, /* Status Code */ - nr->status & NVME_SC_MORE ? "MORE " : "", - nr->status & NVME_SC_DNR ? "DNR " : "", - nr->cmd->common.cdw10, - nr->cmd->common.cdw11, - nr->cmd->common.cdw12, - nr->cmd->common.cdw13, - nr->cmd->common.cdw14, - nr->cmd->common.cdw14); - return; - } + if (ns) + device_name = ns->disk ? ns->disk->disk_name : "?"; pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s" - "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", - dev_name(nr->ctrl->device), - nvme_get_admin_opcode_str(nr->cmd->common.opcode), - nr->cmd->common.opcode, - nvme_get_error_status_str(nr->status), - nr->status >> 8 & 7, /* Status Code Type */ - nr->status & 0xff, /* Status Code */ - nr->status & NVME_SC_MORE ? "MORE " : "", - nr->status & NVME_SC_DNR ? "DNR " : "", - nr->cmd->common.cdw10, - nr->cmd->common.cdw11, - nr->cmd->common.cdw12, - nr->cmd->common.cdw13, - nr->cmd->common.cdw14, - nr->cmd->common.cdw14); + "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n", + device_name, + nvme_get_opcode_str(nr->cmd->common.opcode), + nr->cmd->common.opcode, + nvme_get_error_status_str(nr->status), + nr->status >> 8 & 7, /* Status Code Type */ + nr->status & 0xff, /* Status Code */ + nr->status & NVME_SC_MORE ? "MORE " : "", + nr->status & NVME_SC_DNR ? "DNR " : "", + nr->cmd->common.cdw10, + nr->cmd->common.cdw11, + nr->cmd->common.cdw12, + nr->cmd->common.cdw13, + nr->cmd->common.cdw14, + nr->cmd->common.cdw15); + } enum nvme_disposition { @@ -3477,8 +3462,8 @@ static ssize_t nvme_passthru_admin_err_logging_show(struct device *dev, if (ctrl->error_logging) return sysfs_emit(buf, "on\n"); - else - return sysfs_emit(buf, "off\n"); + + return sysfs_emit(buf, "off\n"); } static ssize_t nvme_passthru_admin_err_logging_store(struct device *dev, @@ -3491,10 +3476,16 @@ static ssize_t nvme_passthru_admin_err_logging_store(struct device *dev, if (err) return -EINVAL; - if (passthrough_enable) - ctrl->error_logging = true; - else - ctrl->error_logging = false; + switch (passthrough_enable) { + case true: + case false: + ctrl->error_logging = passthrough_enable; + break; + default: + pr_err("invlid value %d for admin error logging [on:1 off:0]\n", + passthrough_enable); + break; + } return count; } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC v4 1/1] nvme: add passthrough admin error logging opt-in 2023-04-07 4:45 ` Chaitanya Kulkarni @ 2023-04-07 7:48 ` hch 2023-04-07 17:48 ` Chaitanya Kulkarni 2023-04-09 21:32 ` Chaitanya Kulkarni 0 siblings, 2 replies; 7+ messages in thread From: hch @ 2023-04-07 7:48 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: Alan Adamson, linux-nvme@lists.infradead.org, kbusch@kernel.org, hch@lst.de, sagi@grimberg.me On Fri, Apr 07, 2023 at 04:45:59AM +0000, Chaitanya Kulkarni wrote: > + const char *device_name = dev_name(nr->ctrl->device); > > + if (ns) > + device_name = ns->disk ? ns->disk->disk_name : "?"; I don't see how ns->disk could ever be NULL here. So this could be a simple ns ? ns->disk->disk->name : dev_name(nr->ctrl->device) in the parameter list. We'd still need to also call nvme_get_admin_opcode_str to get the actual command name as well. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC v4 1/1] nvme: add passthrough admin error logging opt-in 2023-04-07 7:48 ` hch @ 2023-04-07 17:48 ` Chaitanya Kulkarni 2023-04-09 21:32 ` Chaitanya Kulkarni 1 sibling, 0 replies; 7+ messages in thread From: Chaitanya Kulkarni @ 2023-04-07 17:48 UTC (permalink / raw) To: Alan Adamson Cc: hch@lst.de, linux-nvme@lists.infradead.org, kbusch@kernel.org, sagi@grimberg.me Alan, On 4/7/2023 12:48 AM, hch@lst.de wrote: > On Fri, Apr 07, 2023 at 04:45:59AM +0000, Chaitanya Kulkarni wrote: >> + const char *device_name = dev_name(nr->ctrl->device); >> >> + if (ns) >> + device_name = ns->disk ? ns->disk->disk_name : "?"; > > I don't see how ns->disk could ever be NULL here. So this could > be a simple > > ns ? ns->disk->disk->name : dev_name(nr->ctrl->device) > > in the parameter list. We'd still need to also call > nvme_get_admin_opcode_str to get the actual command name as well. > Can you plz fold above comment into next version ? -ck ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC v4 1/1] nvme: add passthrough admin error logging opt-in 2023-04-07 7:48 ` hch 2023-04-07 17:48 ` Chaitanya Kulkarni @ 2023-04-09 21:32 ` Chaitanya Kulkarni 1 sibling, 0 replies; 7+ messages in thread From: Chaitanya Kulkarni @ 2023-04-09 21:32 UTC (permalink / raw) To: hch@lst.de Cc: Alan Adamson, linux-nvme@lists.infradead.org, kbusch@kernel.org, sagi@grimberg.me On 4/7/23 00:48, hch@lst.de wrote: > On Fri, Apr 07, 2023 at 04:45:59AM +0000, Chaitanya Kulkarni wrote: >> + const char *device_name = dev_name(nr->ctrl->device); >> >> + if (ns) >> + device_name = ns->disk ? ns->disk->disk_name : "?"; > I don't see how ns->disk could ever be NULL here. So this could > be a simple > > ns ? ns->disk->disk->name : dev_name(nr->ctrl->device) > > in the parameter list. We'd still need to also call > nvme_get_admin_opcode_str to get the actual command name as well. > sent out a tested V5 with all the fixes I mentioned and your suggestion... -ck ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC v4 0/1] nvme: add passthrough admin error logging opt-in 2023-04-06 21:55 [RFC v4 0/1] nvme: add passthrough admin error logging opt-in Alan Adamson 2023-04-06 21:55 ` [RFC v4 1/1] " Alan Adamson @ 2023-04-07 4:46 ` Chaitanya Kulkarni 1 sibling, 0 replies; 7+ messages in thread From: Chaitanya Kulkarni @ 2023-04-07 4:46 UTC (permalink / raw) To: Alan Adamson, linux-nvme@lists.infradead.org Cc: kbusch@kernel.org, hch@lst.de, sagi@grimberg.me On 4/6/23 14:55, Alan Adamson wrote: > v4: > - Change sysfs attribute to passthru_admin_err_logging > - Only log passthrough admin commands. IO passthrough commands will always be logged. > > v3: > - Log a passthrough specific message that dumps CDW* contents. > - Enable/disable vis sysfs rather than debugfs. > > v2: > - Included Pankaj Raghav's patch 'nvme: ignore starting sector while error logging for passthrough requests' > with a couple changes. > - Moved error_logging flag to nvme_ctrl structure > - The entire nvme-debugfs.c does not need to be guarded by #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS. > - Use IS_ENABLED((CONFIG_NVME_ERROR_LOGGING_DEBUG_FS)) to determine if error logging should be > initialized. > - Various other nits. > > Alan Adamson (1): > nvme: add passthrough admin error logging opt-in > > drivers/nvme/host/core.c | 97 +++++++++++++++++++++++++++++++++++++--- > drivers/nvme/host/nvme.h | 1 + > 2 files changed, 93 insertions(+), 5 deletions(-) > I think it is safe to drop RFC for V5 :). -ck ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-04-09 21:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-06 21:55 [RFC v4 0/1] nvme: add passthrough admin error logging opt-in Alan Adamson 2023-04-06 21:55 ` [RFC v4 1/1] " Alan Adamson 2023-04-07 4:45 ` Chaitanya Kulkarni 2023-04-07 7:48 ` hch 2023-04-07 17:48 ` Chaitanya Kulkarni 2023-04-09 21:32 ` Chaitanya Kulkarni 2023-04-07 4:46 ` [RFC v4 0/1] " Chaitanya Kulkarni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox