[PATCH V8 0/1] nvme: allow passthru cmd error logging

[PATCH V8 0/1] nvme: allow passthru cmd error logging

[Alan Adamson]

In nvme_end_req() we only log errors which are for non-passthru
commands. Add a helper function nvme_log_err_passthru() that allows us
to log error for passthru commands by decoding cdw10-cdw15 values of
nvme command.

Below is short testlog :-

* Admin Passsthru error log off, no errors are printed
* Admin Passsthru error log on, errors are printed
* IO Passsthru error log off, no errors are printed
* IO Passsthru error log on, errors are printed

v8:
- Add a logging_enabled flag to device structure.
- Add a passthru_err_log_enabled sysfs attribute for namespaces to
  allow logging of passthru IO commands. 

v7:
- Changed attribute/flag to passthru_err_log_enabled.
- Use kstrtobool rather than kstrtoint.

v6:
- Rebase, retest nvme-6.5 and add test log for admin and I/O
  passthru error log.

v5:
- Trim down code in the nvme_log_error_passthrough().
  Use following to get the disk name as an arg to
   pr_err_ratelimited() :-
	ns ? ns->disk->disk_name : dev_name(nr->ctrl->device),
  Use following to get the admin vs I/O opcode string as an arg to
  pr_err_ratelimited() :-
       	ns ? nvme_get_opcode_str(nr->cmd->common.opcode) :
       	     nvme_get_admin_opcode_str(nr->cmd->common.opcode),
- Rename nvme_log_error_passthrough() -> nvme_log_err_passthru().
- Remove else and return directly in nvme_passthru_err_log_show().
- Generate error on invalid values of the passthru_enable variable
  in nvme_passthru_log_store().
- Rename passthrough -> passthru.
- Rename sysfs attr from passthru_admin_err_logging -> passthru_log_err.

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.


Testing
-------
* Admin Passsthru error log off, no errors are printed :-

[root@localhost ~]# echo 0 > /sys/class/nvme/nvme0/passthru_err_log_enabled
[root@localhost ~]# nvme telemetry-log -o /tmp/test /dev/nvme0
[root@localhost ~]# dmesg
[root@localhost ~]


* Admin Passsthru error log on, errors are printed :-

[root@localhost ~]# echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
[root@localhost ~]# nvme telemetry-log -o /tmp/test /dev/nvme0
[root@localhost ~]# dmesg
[ 2364.008105] nvme0: Get Log Page(0x2), Invalid Field in Command (sct 0x0 / sc 0x2) DNR cdw10=0x7f0107 cdw11=0x0 cdw12=0x0 cdw13=0x0 cdw14=0x0 cdw15=0x0


* IO Passsthru error log off, errors are not printed :-

[root@localhost ~]# echo 0 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
[root@localhost ~]# nvme write-zeroes -n 1 -s 0x200000 -c 10 /dev/nvme0
NVMe status: LBA Out of Range: The command references an LBA that exceeds the size of the namespace(0x4080)
[root@localhost ~]# dmesg
[ 3131.602986] nvme nvme0: using deprecated NVME_IOCTL_IO_CMD ioctl on the char device!
[root@localhost ~]# 


* IO Passsthru error log on, errors are printed :-

[root@localhost ~]# echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
[root@localhost ~]# nvme write-zeroes -n 1 -s 0x200000 -c 10 /dev/nvme0
NVMe status: LBA Out of Range: The command references an LBA that exceeds the size of the namespace(0x4080)
[root@localhost ~]# dmesg
[ 2944.910393] nvme nvme0: using deprecated NVME_IOCTL_IO_CMD ioctl on the char device!
[ 2944.910423] nvme0n1: Write Zeroes(0x8), LBA Out of Range (sct 0x0 / sc 0x80) DNR cdw10=0x200000 cdw11=0x0 cdw12=0xa cdw13=0x0 cdw14=0x0 cdw15=0x0
[root@localhost ~]# 

Alan Adamson (1):
  nvme: allow passthru cmd error logging

 drivers/nvme/host/core.c  | 52 ++++++++++++++++++++++++++++++++++-----
 drivers/nvme/host/nvme.h  |  2 ++
 drivers/nvme/host/sysfs.c | 44 +++++++++++++++++++++++++++++++++
 include/linux/device.h    |  1 +
 4 files changed, 93 insertions(+), 6 deletions(-)

-- 
2.39.3

[PATCH V8 1/1] nvme: allow passthru cmd error logging

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

Add a helper function nvme_log_err_passthru() which allows us to log
error for passthru commands by decoding cdw10-cdw15 values of nvme
command.

Add a new sysfs attr passthru_err_log_enabled that allows user to conditionally
enable passthrough command logging for either passthrough Admin commands sent to
the controller or passthrough IO commands sent to a namespace.

By default, passthrough error logging is disabled.

To enable passthrough admin error logging:
        echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled

To disable passthrough admin error logging:
        echo 0 > /sys/class/nvme/nvme0/passthru_err_log_enabled

To enable passthrough io error logging:
        echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled

To disable passthrough io error logging:
        echo 0 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
[kch] fix sevaral nits and trim down code, details in cover-letter.
Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c  | 52 ++++++++++++++++++++++++++++++++++-----
 drivers/nvme/host/nvme.h  |  2 ++
 drivers/nvme/host/sysfs.c | 44 +++++++++++++++++++++++++++++++++
 include/linux/device.h    |  1 +
 4 files changed, 93 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 60f14019f981..9bca08bf9e67 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -337,6 +337,30 @@ static void nvme_log_error(struct request *req)
 			   nr->status & NVME_SC_DNR  ? "DNR "  : "");
 }
 
+static void nvme_log_err_passthru(struct request *req)
+{
+	struct nvme_ns *ns = req->q->queuedata;
+	struct nvme_request *nr = nvme_req(req);
+
+	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 ? ns->disk->disk_name : dev_name(nr->ctrl->device),
+		ns ? nvme_get_opcode_str(nr->cmd->common.opcode) :
+		     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 +405,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_err_passthru(req);
+		else
+			nvme_log_error(req);
+	}
 	nvme_end_req_zoned(req);
 	nvme_trace_bio_complete(req);
 	if (req->cmd_flags & REQ_NVME_MPATH)
@@ -675,10 +703,19 @@ 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)
 {
-	if (req->q->queuedata)
+	struct nvme_request *nr = nvme_req(req);
+	struct device *device;
+
+	if (req->q->queuedata) {
 		req->timeout = NVME_IO_TIMEOUT;
-	else /* no queuedata implies admin queue */
+		device = disk_to_dev(req->q->disk);
+	} else { /* no queuedata implies admin queue */
 		req->timeout = NVME_ADMIN_TIMEOUT;
+		device = nr->ctrl->device;
+	}
+
+	if (!device->logging_enabled)
+		req->rq_flags |= RQF_QUIET;
 
 	/* passthru commands should let the driver set the SGL flags */
 	cmd->common.flags &= ~NVME_CMD_SGL_ALL;
@@ -687,8 +724,7 @@ 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);
 
@@ -3682,6 +3718,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
 
 	nvme_mpath_add_disk(ns, info->anagrpid);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
+	nvme_sysfs_add_passthru_err_log(disk_to_dev(ns->disk));
 
 	return;
 
@@ -3712,6 +3749,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
 	clear_bit(NVME_NS_READY, &ns->flags);
 	set_capacity(ns->disk, 0);
+	nvme_sysfs_remove_passthru_err_log(disk_to_dev(ns->disk));
 	nvme_fault_inject_fini(&ns->fault_inject);
 
 	/*
@@ -4423,6 +4461,7 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
 	nvme_hwmon_exit(ctrl);
+	nvme_sysfs_remove_passthru_err_log(ctrl->device);
 	nvme_fault_inject_fini(&ctrl->fault_inject);
 	dev_pm_qos_hide_latency_tolerance(ctrl->device);
 	cdev_device_del(&ctrl->cdev, ctrl->device);
@@ -4550,6 +4589,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 		min(default_ps_max_latency_us, (unsigned long)S32_MAX));
 
 	nvme_fault_inject_init(&ctrl->fault_inject, dev_name(ctrl->device));
+	nvme_sysfs_add_passthru_err_log(ctrl->device);
 	nvme_mpath_init_ctrl(ctrl);
 	ret = nvme_auth_init_ctrl(ctrl);
 	if (ret)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e7411dac00f7..7d09437f35b9 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1101,6 +1101,8 @@ void nvme_passthru_end(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u32 effects,
 struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
 struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
 void nvme_put_ns(struct nvme_ns *ns);
+int nvme_sysfs_add_passthru_err_log(struct device *dev);
+void nvme_sysfs_remove_passthru_err_log(struct device *dev);
 
 static inline bool nvme_multi_css(struct nvme_ctrl *ctrl)
 {
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index c6b7fbd4d34d..2128df9df0d4 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -35,6 +35,31 @@ 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_err_log_enabled_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, dev->logging_enabled ? "on" : "off");
+}
+
+static ssize_t nvme_passthru_err_log_enabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	int err;
+	bool passthru_err_log_enabled;
+
+	err = kstrtobool(buf, &passthru_err_log_enabled);
+	if (err)
+		return -EINVAL;
+
+	dev->logging_enabled = passthru_err_log_enabled;
+
+	return count;
+}
+
+static DEVICE_ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR,
+	nvme_passthru_err_log_enabled_show,
+	nvme_passthru_err_log_enabled_store);
+
 static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
@@ -167,6 +192,25 @@ const struct attribute_group *nvme_ns_id_attr_groups[] = {
 	NULL,
 };
 
+static struct attribute *nvme_log_attrs[] = {
+	&dev_attr_passthru_err_log_enabled.attr,
+	NULL,
+};
+
+static const struct attribute_group nvme_log_attr_group = {
+	.attrs		= nvme_log_attrs,
+};
+
+int nvme_sysfs_add_passthru_err_log(struct device *dev)
+{
+	return sysfs_create_group(&dev->kobj, &nvme_log_attr_group);
+}
+
+void nvme_sysfs_remove_passthru_err_log(struct device *dev)
+{
+	sysfs_remove_group(&dev->kobj, &nvme_log_attr_group);
+}
+
 #define nvme_show_str_function(field)						\
 static ssize_t  field##_show(struct device *dev,				\
 			    struct device_attribute *attr, char *buf)		\
diff --git a/include/linux/device.h b/include/linux/device.h
index 6c83294395ac..30162059756e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -805,6 +805,7 @@ struct device {
 #ifdef CONFIG_DMA_OPS_BYPASS
 	bool			dma_ops_bypass : 1;
 #endif
+	bool			logging_enabled:1;
 };
 
 /**
-- 
2.39.3

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Christoph Hellwig]

On Wed, Jan 10, 2024 at 04:08:55PM -0800, Alan Adamson wrote:
> +int nvme_sysfs_add_passthru_err_log(struct device *dev)
> +{
> +	return sysfs_create_group(&dev->kobj, &nvme_log_attr_group);
> +}
> +
> +void nvme_sysfs_remove_passthru_err_log(struct device *dev)
> +{
> +	sysfs_remove_group(&dev->kobj, &nvme_log_attr_group);
> +}

Isn't the normal sysfs convention to have an is_visible method
instead of dynamically adding/removing groups?

Otherwise this looks good to me.

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

>   
> +static ssize_t nvme_passthru_err_log_enabled_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, dev->logging_enabled ? "on" : "off");
> +}
> +

Can we use following on the top of this as most of the XXX_show()
functions in the host/sysfs.c are using "\n" ?

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 8215abaa68e5..ce021f4995eb 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -38,7 +38,7 @@ static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, 
nvme_sysfs_rescan);
  static ssize_t nvme_passthru_err_log_enabled_show(struct device *dev,
                 struct device_attribute *attr, char *buf)
  {
-       return sysfs_emit(buf, dev->logging_enabled ? "on" : "off");
+       return sysfs_emit(buf, dev->logging_enabled ? "on\n" : "off\n");
  }

  static ssize_t nvme_passthru_err_log_enabled_store(struct device *dev,

-ck

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

Hi all,

On 1/10/24 23:04, Christoph Hellwig wrote:
> On Wed, Jan 10, 2024 at 04:08:55PM -0800, Alan Adamson wrote:
>> +int nvme_sysfs_add_passthru_err_log(struct device *dev)
>> +{
>> +	return sysfs_create_group(&dev->kobj, &nvme_log_attr_group);
>> +}
>> +
>> +void nvme_sysfs_remove_passthru_err_log(struct device *dev)
>> +{
>> +	sysfs_remove_group(&dev->kobj, &nvme_log_attr_group);
>> +}
> Isn't the normal sysfs convention to have an is_visible method
> instead of dynamically adding/removing groups?
>
> Otherwise this looks good to me.
>

This patch adds a new member to struct device: logging_enabled.
There are no other users for that member as of now so I think
we should avoid that.

How about following on the top of this patch that adds
logging_enabled to struct nvme_ctrl and struct nvme_ns so we can
remove struct device:logging_enabled ?

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8971af7fcb2b..925d36ce8931 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -127,6 +127,11 @@ static void nvme_remove_invalid_namespaces(struct 
nvme_ctrl *ctrl,
  static void nvme_update_keep_alive(struct nvme_ctrl *ctrl,
                                    struct nvme_command *cmd);

+bool is_nvme_class(const struct class *class)
+{
+       return nvme_class == class;
+}
+
  void nvme_queue_scan(struct nvme_ctrl *ctrl)
  {
         /*
@@ -708,17 +713,19 @@ static inline void nvme_clear_nvme_request(struct 
request *req)
  void nvme_init_request(struct request *req, struct nvme_command *cmd)
  {
         struct nvme_request *nr = nvme_req(req);
-       struct device *device;
+       bool logging_enabled;

         if (req->q->queuedata) {
+               struct nvme_ns *ns = req->q->disk->private_data;
+
+               logging_enabled = ns->logging_enabled;
                 req->timeout = NVME_IO_TIMEOUT;
-               device = disk_to_dev(req->q->disk);
         } else { /* no queuedata implies admin queue */
                 req->timeout = NVME_ADMIN_TIMEOUT;
-               device = nr->ctrl->device;
+               logging_enabled = nr->ctrl->logging_enabled;
         }

-       if (!device->logging_enabled)
+       if (!logging_enabled)
                 req->rq_flags |= RQF_QUIET;

         /* passthru commands should let the driver set the SGL flags */
@@ -3718,6 +3725,13 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, 
struct nvme_ns_info *info)

         nvme_mpath_add_disk(ns, info->anagrpid);
         nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
+
+       /*
+        * Set ns->disk->device->driver_data to ns so we can access
+        * ns->logging_enabled in nvme_passthru_err_log_enabled_store() and
+        * nvme_passthru_err_log_enabled_show().
+        */
+       dev_set_drvdata(disk_to_dev(ns->disk), ns);
         nvme_sysfs_add_passthru_err_log(disk_to_dev(ns->disk));

         return;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index df0e90d3e4b5..33f9d636a576 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -20,6 +20,8 @@

  #include <trace/events/block.h>

+extern bool is_nvme_class(const struct class *class);
+
  extern const struct pr_ops nvme_pr_ops;

  extern unsigned int nvme_io_timeout;
@@ -385,6 +387,7 @@ struct nvme_ctrl {

         enum nvme_ctrl_type cntrltype;
         enum nvme_dctype dctype;
+       bool logging_enabled;
  };

  enum nvme_iopolicy {
@@ -511,7 +514,7 @@ struct nvme_ns {
         struct device           cdev_device;

         struct nvme_fault_inject fault_inject;
-
+       bool logging_enabled;
  };

  /* NVMe ns supports metadata actions by the controller (generate/strip) */
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 8215abaa68e5..db6d4a3d7c4a 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -38,20 +38,36 @@ static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, 
nvme_sysfs_rescan);
  static ssize_t nvme_passthru_err_log_enabled_show(struct device *dev,
                 struct device_attribute *attr, char *buf)
  {
-       return sysfs_emit(buf, dev->logging_enabled ? "on" : "off");
+       if (is_nvme_class(dev->class)) {
+               struct nvme_ctrl *c = dev_get_drvdata(dev);
+
+               return sysfs_emit(buf, c->logging_enabled ? "on\n" : 
"off\n");
+       } else {
+               struct nvme_ns *n = dev_get_drvdata(dev);
+
+               return sysfs_emit(buf, n->logging_enabled ? "on\n" : 
"off\n");
+       }
  }

  static ssize_t nvme_passthru_err_log_enabled_store(struct device *dev,
                 struct device_attribute *attr, const char *buf, size_t 
count)
  {
-       int err;
         bool passthru_err_log_enabled;
+       int err;

         err = kstrtobool(buf, &passthru_err_log_enabled);
         if (err)
                 return -EINVAL;

-       dev->logging_enabled = passthru_err_log_enabled;
+       if (is_nvme_class(dev->class)) {
+               struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+               ctrl->logging_enabled = passthru_err_log_enabled;
+       } else {
+               struct nvme_ns *ns = dev_get_drvdata(dev);
+
+               ns->logging_enabled = passthru_err_log_enabled;
+       }

         return count;
  }
diff --git a/include/linux/device.h b/include/linux/device.h
index 3fedff698349..d7a72a8749ea 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -805,7 +805,6 @@ struct device {
  #ifdef CONFIG_DMA_OPS_BYPASS
         bool                    dma_ops_bypass : 1;
  #endif
-       bool                    logging_enabled:1;
  };

  /**


-ck

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Sagi Grimberg]

On 1/17/24 05:46, Chaitanya Kulkarni wrote:
> Hi all,
> 
> On 1/10/24 23:04, Christoph Hellwig wrote:
>> On Wed, Jan 10, 2024 at 04:08:55PM -0800, Alan Adamson wrote:
>>> +int nvme_sysfs_add_passthru_err_log(struct device *dev)
>>> +{
>>> +	return sysfs_create_group(&dev->kobj, &nvme_log_attr_group);
>>> +}
>>> +
>>> +void nvme_sysfs_remove_passthru_err_log(struct device *dev)
>>> +{
>>> +	sysfs_remove_group(&dev->kobj, &nvme_log_attr_group);
>>> +}
>> Isn't the normal sysfs convention to have an is_visible method
>> instead of dynamically adding/removing groups?
>>
>> Otherwise this looks good to me.
>>
> 
> This patch adds a new member to struct device: logging_enabled.
> There are no other users for that member as of now so I think
> we should avoid that.

We definitely should.

> 
> How about following on the top of this patch that adds
> logging_enabled to struct nvme_ctrl and struct nvme_ns so we can
> remove struct device:logging_enabled ?
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8971af7fcb2b..925d36ce8931 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -127,6 +127,11 @@ static void nvme_remove_invalid_namespaces(struct
> nvme_ctrl *ctrl,
>    static void nvme_update_keep_alive(struct nvme_ctrl *ctrl,
>                                      struct nvme_command *cmd);
> 
> +bool is_nvme_class(const struct class *class)
> +{
> +       return nvme_class == class;
> +}
> +
>    void nvme_queue_scan(struct nvme_ctrl *ctrl)
>    {
>           /*
> @@ -708,17 +713,19 @@ static inline void nvme_clear_nvme_request(struct
> request *req)
>    void nvme_init_request(struct request *req, struct nvme_command *cmd)
>    {
>           struct nvme_request *nr = nvme_req(req);
> -       struct device *device;
> +       bool logging_enabled;
> 
>           if (req->q->queuedata) {
> +               struct nvme_ns *ns = req->q->disk->private_data;
> +
> +               logging_enabled = ns->logging_enabled;
>                   req->timeout = NVME_IO_TIMEOUT;
> -               device = disk_to_dev(req->q->disk);
>           } else { /* no queuedata implies admin queue */
>                   req->timeout = NVME_ADMIN_TIMEOUT;
> -               device = nr->ctrl->device;
> +               logging_enabled = nr->ctrl->logging_enabled;
>           }
> 
> -       if (!device->logging_enabled)
> +       if (!logging_enabled)
>                   req->rq_flags |= RQF_QUIET;
> 
>           /* passthru commands should let the driver set the SGL flags */
> @@ -3718,6 +3725,13 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl,
> struct nvme_ns_info *info)
> 
>           nvme_mpath_add_disk(ns, info->anagrpid);
>           nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> +
> +       /*
> +        * Set ns->disk->device->driver_data to ns so we can access
> +        * ns->logging_enabled in nvme_passthru_err_log_enabled_store() and
> +        * nvme_passthru_err_log_enabled_show().
> +        */
> +       dev_set_drvdata(disk_to_dev(ns->disk), ns);

Is this needed?

>           nvme_sysfs_add_passthru_err_log(disk_to_dev(ns->disk));
> 
>           return;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index df0e90d3e4b5..33f9d636a576 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -20,6 +20,8 @@
> 
>    #include <trace/events/block.h>
> 
> +extern bool is_nvme_class(const struct class *class);
> +
>    extern const struct pr_ops nvme_pr_ops;
> 
>    extern unsigned int nvme_io_timeout;
> @@ -385,6 +387,7 @@ struct nvme_ctrl {
> 
>           enum nvme_ctrl_type cntrltype;
>           enum nvme_dctype dctype;
> +       bool logging_enabled;
>    };
> 
>    enum nvme_iopolicy {
> @@ -511,7 +514,7 @@ struct nvme_ns {
>           struct device           cdev_device;
> 
>           struct nvme_fault_inject fault_inject;
> -
> +       bool logging_enabled;
>    };
> 
>    /* NVMe ns supports metadata actions by the controller (generate/strip) */
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 8215abaa68e5..db6d4a3d7c4a 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -38,20 +38,36 @@ static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL,
> nvme_sysfs_rescan);
>    static ssize_t nvme_passthru_err_log_enabled_show(struct device *dev,
>                   struct device_attribute *attr, char *buf)
>    {
> -       return sysfs_emit(buf, dev->logging_enabled ? "on" : "off");
> +       if (is_nvme_class(dev->class)) {
> +               struct nvme_ctrl *c = dev_get_drvdata(dev);
> +
> +               return sysfs_emit(buf, c->logging_enabled ? "on\n" :
> "off\n");
> +       } else {
> +               struct nvme_ns *n = dev_get_drvdata(dev);
> +
> +               return sysfs_emit(buf, n->logging_enabled ? "on\n" :
> "off\n");
> +       }
>    }
> 
>    static ssize_t nvme_passthru_err_log_enabled_store(struct device *dev,
>                   struct device_attribute *attr, const char *buf, size_t
> count)
>    {
> -       int err;
>           bool passthru_err_log_enabled;
> +       int err;
> 
>           err = kstrtobool(buf, &passthru_err_log_enabled);
>           if (err)
>                   return -EINVAL;
> 
> -       dev->logging_enabled = passthru_err_log_enabled;
> +       if (is_nvme_class(dev->class)) {
> +               struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +               ctrl->logging_enabled = passthru_err_log_enabled;
> +       } else {
> +               struct nvme_ns *ns = dev_get_drvdata(dev);
> +
> +               ns->logging_enabled = passthru_err_log_enabled;
> +       }

Why mix ctrl with ns sysfs handlers?

> 
>           return count;
>    }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 3fedff698349..d7a72a8749ea 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -805,7 +805,6 @@ struct device {
>    #ifdef CONFIG_DMA_OPS_BYPASS
>           bool                    dma_ops_bypass : 1;
>    #endif
> -       bool                    logging_enabled:1;
>    };
> 
>    /**
> 
> 
> -ck
> 
> 

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

Sagi,

>> nvme_mpath_add_disk(ns, info->anagrpid);
>>           nvme_fault_inject_init(&ns->fault_inject, 
>> ns->disk->disk_name);
>> +
>> +       /*
>> +        * Set ns->disk->device->driver_data to ns so we can access
>> +        * ns->logging_enabled in 
>> nvme_passthru_err_log_enabled_store() and
>> +        * nvme_passthru_err_log_enabled_show().
>> +        */
>> +       dev_set_drvdata(disk_to_dev(ns->disk), ns);
>
> Is this needed?

Yes see explanation below ..

[...]

>
>>
>>    static ssize_t nvme_passthru_err_log_enabled_store(struct device 
>> *dev,
>>                   struct device_attribute *attr, const char *buf, size_t
>> count)
>>    {
>> -       int err;
>>           bool passthru_err_log_enabled;
>> +       int err;
>>
>>           err = kstrtobool(buf, &passthru_err_log_enabled);
>>           if (err)
>>                   return -EINVAL;
>>
>> -       dev->logging_enabled = passthru_err_log_enabled;
>> +       if (is_nvme_class(dev->class)) {
>> +               struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> +
>> +               ctrl->logging_enabled = passthru_err_log_enabled;
>> +       } else {
>> +               struct nvme_ns *ns = dev_get_drvdata(dev);
>> +
>> +               ns->logging_enabled = passthru_err_log_enabled;
>> +       }
>
> Why mix ctrl with ns sysfs handlers?

sorry I didn't understand your question clearly ...

In the original implementation we get two different struct device objects
for following commands in sysfs, that sets struct device:logging_enabled 
flag,
which is also used to determine the logging in nvme_init_request() :-

echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled

nvme_init_ctrl() already sets dev_set_drvdata(ctrl->device, ctrl). In this
proposal above change in nvme_alloc_ns() sets the
dev_set_drvdata(disk_to_dev(ns->disk), ns).

This allows us to get the ctrl or ns object associated with the struct 
device
we get in the sysfs, then based on the device class we update 
logging_enabled
flags for either ctrl or ns respectively. In nvme_init_request() I use
ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.

-ck

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Sagi Grimberg]

On 1/18/24 05:31, Chaitanya Kulkarni wrote:
> Sagi,
> 
>>> nvme_mpath_add_disk(ns, info->anagrpid);
>>>            nvme_fault_inject_init(&ns->fault_inject,
>>> ns->disk->disk_name);
>>> +
>>> +       /*
>>> +        * Set ns->disk->device->driver_data to ns so we can access
>>> +        * ns->logging_enabled in
>>> nvme_passthru_err_log_enabled_store() and
>>> +        * nvme_passthru_err_log_enabled_show().
>>> +        */
>>> +       dev_set_drvdata(disk_to_dev(ns->disk), ns);
>>
>> Is this needed?
> 
> Yes see explanation below ..
> 
> [...]
> 
>>
>>>
>>>     static ssize_t nvme_passthru_err_log_enabled_store(struct device
>>> *dev,
>>>                    struct device_attribute *attr, const char *buf, size_t
>>> count)
>>>     {
>>> -       int err;
>>>            bool passthru_err_log_enabled;
>>> +       int err;
>>>
>>>            err = kstrtobool(buf, &passthru_err_log_enabled);
>>>            if (err)
>>>                    return -EINVAL;
>>>
>>> -       dev->logging_enabled = passthru_err_log_enabled;
>>> +       if (is_nvme_class(dev->class)) {
>>> +               struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>> +
>>> +               ctrl->logging_enabled = passthru_err_log_enabled;
>>> +       } else {
>>> +               struct nvme_ns *ns = dev_get_drvdata(dev);
>>> +
>>> +               ns->logging_enabled = passthru_err_log_enabled;
>>> +       }
>>
>> Why mix ctrl with ns sysfs handlers?
> 
> sorry I didn't understand your question clearly ...
> 
> In the original implementation we get two different struct device objects
> for following commands in sysfs, that sets struct device:logging_enabled
> flag,
> which is also used to determine the logging in nvme_init_request() :-
> 
> echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
> echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
> 
> nvme_init_ctrl() already sets dev_set_drvdata(ctrl->device, ctrl). In this
> proposal above change in nvme_alloc_ns() sets the
> dev_set_drvdata(disk_to_dev(ns->disk), ns).
> 
> This allows us to get the ctrl or ns object associated with the struct
> device
> we get in the sysfs, then based on the device class we update
> logging_enabled
> flags for either ctrl or ns respectively. In nvme_init_request() I use
> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.

I was asking why should we have a show/store that operate on both ns and
ctrl?

Why not have a show/store in nvme_dev_attrs and a separate one in
nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
the ns attrs can access the ns in a normal way like the rest? Or am
I missing something?

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[alan.adamson]

>> This allows us to get the ctrl or ns object associated with the struct
>> device
>> we get in the sysfs, then based on the device class we update
>> logging_enabled
>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>
> I was asking why should we have a show/store that operate on both ns and
> ctrl?
>
> Why not have a show/store in nvme_dev_attrs and a separate one in
> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
> the ns attrs can access the ns in a normal way like the rest? Or am
> I missing something?

I did have a version that used nvme_ns_id_attrs but didn't think it was 
an appropriate place for it.  I can go back to that.

Alan

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

On 1/23/24 03:33, Sagi Grimberg wrote:
>
>
> On 1/18/24 05:31, Chaitanya Kulkarni wrote:
>> Sagi,
>>
>>>> nvme_mpath_add_disk(ns, info->anagrpid);
>>>> nvme_fault_inject_init(&ns->fault_inject,
>>>> ns->disk->disk_name);
>>>> +
>>>> +       /*
>>>> +        * Set ns->disk->device->driver_data to ns so we can access
>>>> +        * ns->logging_enabled in
>>>> nvme_passthru_err_log_enabled_store() and
>>>> +        * nvme_passthru_err_log_enabled_show().
>>>> +        */
>>>> +       dev_set_drvdata(disk_to_dev(ns->disk), ns);
>>>
>>> Is this needed?
>>
>> Yes see explanation below ..
>>
>> [...]
>>
>>>
>>>>
>>>>     static ssize_t nvme_passthru_err_log_enabled_store(struct device
>>>> *dev,
>>>>                    struct device_attribute *attr, const char *buf, 
>>>> size_t
>>>> count)
>>>>     {
>>>> -       int err;
>>>>            bool passthru_err_log_enabled;
>>>> +       int err;
>>>>
>>>>            err = kstrtobool(buf, &passthru_err_log_enabled);
>>>>            if (err)
>>>>                    return -EINVAL;
>>>>
>>>> -       dev->logging_enabled = passthru_err_log_enabled;
>>>> +       if (is_nvme_class(dev->class)) {
>>>> +               struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>>> +
>>>> +               ctrl->logging_enabled = passthru_err_log_enabled;
>>>> +       } else {
>>>> +               struct nvme_ns *ns = dev_get_drvdata(dev);
>>>> +
>>>> +               ns->logging_enabled = passthru_err_log_enabled;
>>>> +       }
>>>
>>> Why mix ctrl with ns sysfs handlers?
>>
>> sorry I didn't understand your question clearly ...
>>
>> In the original implementation we get two different struct device 
>> objects
>> for following commands in sysfs, that sets struct device:logging_enabled
>> flag,
>> which is also used to determine the logging in nvme_init_request() :-
>>
>> echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
>> echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>>
>> nvme_init_ctrl() already sets dev_set_drvdata(ctrl->device, ctrl). In 
>> this
>> proposal above change in nvme_alloc_ns() sets the
>> dev_set_drvdata(disk_to_dev(ns->disk), ns).
>>
>> This allows us to get the ctrl or ns object associated with the struct
>> device
>> we get in the sysfs, then based on the device class we update
>> logging_enabled
>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>
> I was asking why should we have a show/store that operate on both ns and
> ctrl?
>
> Why not have a show/store in nvme_dev_attrs and a separate one in
> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
> the ns attrs can access the ns in a normal way like the rest? Or am
> I missing something?

no you are not, I was just trying to minimize the changes in the posted
patch guess that is a right way to go forward ...

-ck

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

Alan,

On 1/23/24 09:11, alan.adamson@oracle.com wrote:
>
>>> This allows us to get the ctrl or ns object associated with the struct
>>> device
>>> we get in the sysfs, then based on the device class we update
>>> logging_enabled
>>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>>
>> I was asking why should we have a show/store that operate on both ns and
>> ctrl?
>>
>> Why not have a show/store in nvme_dev_attrs and a separate one in
>> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
>> the ns attrs can access the ns in a normal way like the rest? Or am
>> I missing something?
>
> I did have a version that used nvme_ns_id_attrs but didn't think it 
> was an appropriate place for it.  I can go back to that.
>
> Alan
>

can you please remove the struct device:logging_enabled and use Sagi's
suggestion and post a new version ?

There is a also comment from Christoph to use is_visible method instead
of dynamically adding and removing groups, can you also add that to next
version ? unless that cannot be done for some reason ...

-ck

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Christoph Hellwig]

On Tue, Jan 23, 2024 at 09:11:32AM -0800, alan.adamson@oracle.com wrote:
>>> This allows us to get the ctrl or ns object associated with the struct
>>> device
>>> we get in the sysfs, then based on the device class we update
>>> logging_enabled
>>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>>
>> I was asking why should we have a show/store that operate on both ns and
>> ctrl?
>>
>> Why not have a show/store in nvme_dev_attrs and a separate one in
>> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
>> the ns attrs can access the ns in a normal way like the rest? Or am
>> I missing something?
>
> I did have a version that used nvme_ns_id_attrs but didn't think it was an 
> appropriate place for it.  I can go back to that.

nvme_ns_id_attrs has been renamed now that we're using it for more than
just ids, so adding more attributes if they fit is fine.

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[alan.adamson]

On 1/23/24 7:41 PM, Chaitanya Kulkarni wrote:
> Alan,
>
> On 1/23/24 09:11, alan.adamson@oracle.com wrote:
>>>> This allows us to get the ctrl or ns object associated with the struct
>>>> device
>>>> we get in the sysfs, then based on the device class we update
>>>> logging_enabled
>>>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>>>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>>> I was asking why should we have a show/store that operate on both ns and
>>> ctrl?
>>>
>>> Why not have a show/store in nvme_dev_attrs and a separate one in
>>> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
>>> the ns attrs can access the ns in a normal way like the rest? Or am
>>> I missing something?
>> I did have a version that used nvme_ns_id_attrs but didn't think it
>> was an appropriate place for it.  I can go back to that.
>>
>> Alan
>>
> can you please remove the struct device:logging_enabled and use Sagi's
> suggestion and post a new version ?
>
> There is a also comment from Christoph to use is_visible method instead
> of dynamically adding and removing groups, can you also add that to next
> version ? unless that cannot be done for some reason ...

Yes, I'll post a new version. I don't think the is_visible method is 
necessary since we will not need to create a new group, just use the 
existing groups (nvme_ns_id and nvme_dev).


Alan

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[alan.adamson]

>> sorry I didn't understand your question clearly ...
>>
>> In the original implementation we get two different struct device 
>> objects
>> for following commands in sysfs, that sets struct device:logging_enabled
>> flag,
>> which is also used to determine the logging in nvme_init_request() :-
>>
>> echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
>> echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>>
>> nvme_init_ctrl() already sets dev_set_drvdata(ctrl->device, ctrl). In 
>> this
>> proposal above change in nvme_alloc_ns() sets the
>> dev_set_drvdata(disk_to_dev(ns->disk), ns).
>>
>> This allows us to get the ctrl or ns object associated with the struct
>> device
>> we get in the sysfs, then based on the device class we update
>> logging_enabled
>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>
> I was asking why should we have a show/store that operate on both ns and
> ctrl?
>
> Why not have a show/store in nvme_dev_attrs and a separate one in
> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
> the ns attrs can access the ns in a normal way like the rest? Or am
> I missing something?

If we create separate show/stores for the ctrl (nvme_dev_attrs) and ns 
(nvme_ns_attrs), using the exiting macro: DEVICE_ATTR(), the attribute 
name (passthru_err_log_enabled) can not be used for both nvme_dev_attrs 
and nvme_ns_attrs.

/sys/class/nvme/nvme0/adm_passthru_err_log_enabled

/sys/class/nvme/nvme0/nvme0n1/io_passthru_err_log_enabled


If I setup the attributes like:

static struct device_attribute dev_attr_adm_passthru_err_log_enabled = \
         __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
         nvme_adm_passthru_err_log_enabled_show, 
nvme_adm_passthru_err_log_enabled_store);

static struct device_attribute dev_attr_io_passthru_err_log_enabled = \
         __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
         nvme_io_passthru_err_log_enabled_show, 
nvme_io_passthru_err_log_enabled_store);


Then both attributes can be the same.

/sys/class/nvme/nvme0/passthru_err_log_enabled

/sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled

What do we prefer?

Alan

Re: [PATCH V8 1/1] nvme: allow passthru cmd error logging

[Sagi Grimberg]

>>> sorry I didn't understand your question clearly ...
>>>
>>> In the original implementation we get two different struct device 
>>> objects
>>> for following commands in sysfs, that sets struct device:logging_enabled
>>> flag,
>>> which is also used to determine the logging in nvme_init_request() :-
>>>
>>> echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
>>> echo 1 > /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>>>
>>> nvme_init_ctrl() already sets dev_set_drvdata(ctrl->device, ctrl). In 
>>> this
>>> proposal above change in nvme_alloc_ns() sets the
>>> dev_set_drvdata(disk_to_dev(ns->disk), ns).
>>>
>>> This allows us to get the ctrl or ns object associated with the struct
>>> device
>>> we get in the sysfs, then based on the device class we update
>>> logging_enabled
>>> flags for either ctrl or ns respectively. In nvme_init_request() I use
>>> ctrl->logging_enabled and ns->logging_enabled based on admin or io cmd.
>>
>> I was asking why should we have a show/store that operate on both ns and
>> ctrl?
>>
>> Why not have a show/store in nvme_dev_attrs and a separate one in
>> nvme_ns_id_attrs ? Then you don't need the awkward is_nvme_class() ?
>> the ns attrs can access the ns in a normal way like the rest? Or am
>> I missing something?
> 
> If we create separate show/stores for the ctrl (nvme_dev_attrs) and ns 
> (nvme_ns_attrs), using the exiting macro: DEVICE_ATTR(), the attribute 
> name (passthru_err_log_enabled) can not be used for both nvme_dev_attrs 
> and nvme_ns_attrs.
> 
> /sys/class/nvme/nvme0/adm_passthru_err_log_enabled
> 
> /sys/class/nvme/nvme0/nvme0n1/io_passthru_err_log_enabled
> 
> 
> If I setup the attributes like:
> 
> static struct device_attribute dev_attr_adm_passthru_err_log_enabled = \
>          __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
>          nvme_adm_passthru_err_log_enabled_show, 
> nvme_adm_passthru_err_log_enabled_store);
> 
> static struct device_attribute dev_attr_io_passthru_err_log_enabled = \
>          __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
>          nvme_io_passthru_err_log_enabled_show, 
> nvme_io_passthru_err_log_enabled_store);
> 
> 
> Then both attributes can be the same.
> 
> /sys/class/nvme/nvme0/passthru_err_log_enabled
> 
> /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
> 
> What do we prefer?

The second one looks fine to me.