[PATCH] nvme: move passthrough logging attribute to head

[PATCH] nvme: move passthrough logging attribute to head

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The namespace does not have attributes, but the head does. Move the new
attribute to that structure instead of dereferncing the wrong type.

And while we're here, fix the reverse-tree coding style.

Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
Cc: Alan Adamson <alan.adamson@oracle.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c  |  3 +--
 drivers/nvme/host/nvme.h  |  2 +-
 drivers/nvme/host/sysfs.c | 30 +++++++++++++++---------------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 75927e8579857..786bebcfa4f77 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -713,7 +713,7 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
 	if (req->q->queuedata) {
 		struct nvme_ns *ns = req->q->disk->private_data;
 
-		logging_enabled = ns->passthru_err_log_enabled;
+		logging_enabled = ns->head->passthru_err_log_enabled;
 		req->timeout = NVME_IO_TIMEOUT;
 	} else { /* no queuedata implies admin queue */
 		logging_enabled = nr->ctrl->passthru_err_log_enabled;
@@ -3700,7 +3700,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
 
 	ns->disk = disk;
 	ns->queue = disk->queue;
-	ns->passthru_err_log_enabled = false;
 
 	if (ctrl->opts && ctrl->opts->data_digest)
 		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 3897334e3950d..7b87763e2f8a6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -455,6 +455,7 @@ struct nvme_ns_head {
 	struct list_head	entry;
 	struct kref		ref;
 	bool			shared;
+	bool			passthru_err_log_enabled;
 	int			instance;
 	struct nvme_effects_log *effects;
 	u64			nuse;
@@ -523,7 +524,6 @@ struct nvme_ns {
 	struct device		cdev_device;
 
 	struct nvme_fault_inject fault_inject;
-	bool			passthru_err_log_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 d099218e494a8..f2832f70e7e0a 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -48,8 +48,8 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	int err;
 	bool passthru_err_log_enabled;
+	int err;
 
 	err = kstrtobool(buf, &passthru_err_log_enabled);
 	if (err)
@@ -60,25 +60,34 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
 	return count;
 }
 
+static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+
+	if (nvme_disk_is_ns_head(disk))
+		return disk->private_data;
+	return nvme_get_ns_from_dev(dev)->head;
+}
+
 static ssize_t nvme_io_passthru_err_log_enabled_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct nvme_ns *n = dev_get_drvdata(dev);
+	struct nvme_ns_head *head = dev_to_ns_head(dev);
 
-	return sysfs_emit(buf, n->passthru_err_log_enabled ? "on\n" : "off\n");
+	return sysfs_emit(buf, head->passthru_err_log_enabled ? "on\n" : "off\n");
 }
 
 static ssize_t nvme_io_passthru_err_log_enabled_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct nvme_ns *ns = dev_get_drvdata(dev);
-	int err;
+	struct nvme_ns_head *head = dev_to_ns_head(dev);
 	bool passthru_err_log_enabled;
+	int err;
 
 	err = kstrtobool(buf, &passthru_err_log_enabled);
 	if (err)
 		return -EINVAL;
-	ns->passthru_err_log_enabled = passthru_err_log_enabled;
+	head->passthru_err_log_enabled = passthru_err_log_enabled;
 
 	return count;
 }
@@ -91,15 +100,6 @@ 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);
 
-static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
-{
-	struct gendisk *disk = dev_to_disk(dev);
-
-	if (nvme_disk_is_ns_head(disk))
-		return disk->private_data;
-	return nvme_get_ns_from_dev(dev)->head;
-}
-
 static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
 		char *buf)
 {
-- 
2.34.1

Re: [PATCH] nvme: move passthrough logging attribute to head

[Chaitanya Kulkarni]

On 2/6/24 09:50, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

BTW I've also posted comment correction patch with Alan's review in this
area, please have a look.

-ck

Re: [PATCH] nvme: move passthrough logging attribute to head

[Keith Busch]

On Wed, Feb 07, 2024 at 04:02:42AM +0000, Chaitanya Kulkarni wrote:
> On 2/6/24 09:50, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> >
> > The namespace does not have attributes, but the head does. Move the new
> > attribute to that structure instead of dereferncing the wrong type.
> >
> > And while we're here, fix the reverse-tree coding style.
> >
> > Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> > Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> > Cc: Alan Adamson <alan.adamson@oracle.com>
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >
> 
> Looks good.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 
> BTW I've also posted comment correction patch with Alan's review in this
> area, please have a look.

Right, I saw that but the comments would need to be updated again after
this fix. I'll take another look tomorrow and maybe just change it when
applying.

Re: [PATCH] nvme: move passthrough logging attribute to head

[Tasmiya Nalatwad]

Thanks for the Patch. I have tested the patch and the issue is fixed 
with the patch.

Tested-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>

On 2/6/24 23:20, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c  |  3 +--
>   drivers/nvme/host/nvme.h  |  2 +-
>   drivers/nvme/host/sysfs.c | 30 +++++++++++++++---------------
>   3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 75927e8579857..786bebcfa4f77 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -713,7 +713,7 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
>   	if (req->q->queuedata) {
>   		struct nvme_ns *ns = req->q->disk->private_data;
>   
> -		logging_enabled = ns->passthru_err_log_enabled;
> +		logging_enabled = ns->head->passthru_err_log_enabled;
>   		req->timeout = NVME_IO_TIMEOUT;
>   	} else { /* no queuedata implies admin queue */
>   		logging_enabled = nr->ctrl->passthru_err_log_enabled;
> @@ -3700,7 +3700,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
>   
>   	ns->disk = disk;
>   	ns->queue = disk->queue;
> -	ns->passthru_err_log_enabled = false;
>   
>   	if (ctrl->opts && ctrl->opts->data_digest)
>   		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 3897334e3950d..7b87763e2f8a6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -455,6 +455,7 @@ struct nvme_ns_head {
>   	struct list_head	entry;
>   	struct kref		ref;
>   	bool			shared;
> +	bool			passthru_err_log_enabled;
>   	int			instance;
>   	struct nvme_effects_log *effects;
>   	u64			nuse;
> @@ -523,7 +524,6 @@ struct nvme_ns {
>   	struct device		cdev_device;
>   
>   	struct nvme_fault_inject fault_inject;
> -	bool			passthru_err_log_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 d099218e494a8..f2832f70e7e0a 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -48,8 +48,8 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	int err;
>   	bool passthru_err_log_enabled;
> +	int err;
>   
>   	err = kstrtobool(buf, &passthru_err_log_enabled);
>   	if (err)
> @@ -60,25 +60,34 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
>   	return count;
>   }
>   
> +static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> +{
> +	struct gendisk *disk = dev_to_disk(dev);
> +
> +	if (nvme_disk_is_ns_head(disk))
> +		return disk->private_data;
> +	return nvme_get_ns_from_dev(dev)->head;
> +}
> +
>   static ssize_t nvme_io_passthru_err_log_enabled_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
>   {
> -	struct nvme_ns *n = dev_get_drvdata(dev);
> +	struct nvme_ns_head *head = dev_to_ns_head(dev);
>   
> -	return sysfs_emit(buf, n->passthru_err_log_enabled ? "on\n" : "off\n");
> +	return sysfs_emit(buf, head->passthru_err_log_enabled ? "on\n" : "off\n");
>   }
>   
>   static ssize_t nvme_io_passthru_err_log_enabled_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
> -	struct nvme_ns *ns = dev_get_drvdata(dev);
> -	int err;
> +	struct nvme_ns_head *head = dev_to_ns_head(dev);
>   	bool passthru_err_log_enabled;
> +	int err;
>   
>   	err = kstrtobool(buf, &passthru_err_log_enabled);
>   	if (err)
>   		return -EINVAL;
> -	ns->passthru_err_log_enabled = passthru_err_log_enabled;
> +	head->passthru_err_log_enabled = passthru_err_log_enabled;
>   
>   	return count;
>   }
> @@ -91,15 +100,6 @@ 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);
>   
> -static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> -{
> -	struct gendisk *disk = dev_to_disk(dev);
> -
> -	if (nvme_disk_is_ns_head(disk))
> -		return disk->private_data;
> -	return nvme_get_ns_from_dev(dev)->head;
> -}
> -
>   static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
>   		char *buf)
>   {

-- 
Regards,
Tasmiya Nalatwad
IBM Linux Technology Center

Re: [PATCH] nvme: move passthrough logging attribute to head

[alan.adamson]

On 2/6/24 9:50 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good.

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

I tested it with blktest tests I'm putting together for passthrough errors.

Alan

Re: [PATCH] nvme: move passthrough logging attribute to head

[Keith Busch]

Applied to nvme-6.8.

Re: [PATCH] nvme: move passthrough logging attribute to head

[Tasmiya Nalatwad]

Thanks for the Patch. I tested the patch and the reported issue is fixed 
with this patch. Thank you

Tested-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>


On 2/6/24 23:20, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c  |  3 +--
>   drivers/nvme/host/nvme.h  |  2 +-
>   drivers/nvme/host/sysfs.c | 30 +++++++++++++++---------------
>   3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 75927e8579857..786bebcfa4f77 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -713,7 +713,7 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
>   	if (req->q->queuedata) {
>   		struct nvme_ns *ns = req->q->disk->private_data;
>   
> -		logging_enabled = ns->passthru_err_log_enabled;
> +		logging_enabled = ns->head->passthru_err_log_enabled;
>   		req->timeout = NVME_IO_TIMEOUT;
>   	} else { /* no queuedata implies admin queue */
>   		logging_enabled = nr->ctrl->passthru_err_log_enabled;
> @@ -3700,7 +3700,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
>   
>   	ns->disk = disk;
>   	ns->queue = disk->queue;
> -	ns->passthru_err_log_enabled = false;
>   
>   	if (ctrl->opts && ctrl->opts->data_digest)
>   		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 3897334e3950d..7b87763e2f8a6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -455,6 +455,7 @@ struct nvme_ns_head {
>   	struct list_head	entry;
>   	struct kref		ref;
>   	bool			shared;
> +	bool			passthru_err_log_enabled;
>   	int			instance;
>   	struct nvme_effects_log *effects;
>   	u64			nuse;
> @@ -523,7 +524,6 @@ struct nvme_ns {
>   	struct device		cdev_device;
>   
>   	struct nvme_fault_inject fault_inject;
> -	bool			passthru_err_log_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 d099218e494a8..f2832f70e7e0a 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -48,8 +48,8 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	int err;
>   	bool passthru_err_log_enabled;
> +	int err;
>   
>   	err = kstrtobool(buf, &passthru_err_log_enabled);
>   	if (err)
> @@ -60,25 +60,34 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
>   	return count;
>   }
>   
> +static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> +{
> +	struct gendisk *disk = dev_to_disk(dev);
> +
> +	if (nvme_disk_is_ns_head(disk))
> +		return disk->private_data;
> +	return nvme_get_ns_from_dev(dev)->head;
> +}
> +
>   static ssize_t nvme_io_passthru_err_log_enabled_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
>   {
> -	struct nvme_ns *n = dev_get_drvdata(dev);
> +	struct nvme_ns_head *head = dev_to_ns_head(dev);
>   
> -	return sysfs_emit(buf, n->passthru_err_log_enabled ? "on\n" : "off\n");
> +	return sysfs_emit(buf, head->passthru_err_log_enabled ? "on\n" : "off\n");
>   }
>   
>   static ssize_t nvme_io_passthru_err_log_enabled_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
> -	struct nvme_ns *ns = dev_get_drvdata(dev);
> -	int err;
> +	struct nvme_ns_head *head = dev_to_ns_head(dev);
>   	bool passthru_err_log_enabled;
> +	int err;
>   
>   	err = kstrtobool(buf, &passthru_err_log_enabled);
>   	if (err)
>   		return -EINVAL;
> -	ns->passthru_err_log_enabled = passthru_err_log_enabled;
> +	head->passthru_err_log_enabled = passthru_err_log_enabled;
>   
>   	return count;
>   }
> @@ -91,15 +100,6 @@ 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);
>   
> -static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> -{
> -	struct gendisk *disk = dev_to_disk(dev);
> -
> -	if (nvme_disk_is_ns_head(disk))
> -		return disk->private_data;
> -	return nvme_get_ns_from_dev(dev)->head;
> -}
> -
>   static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
>   		char *buf)
>   {

-- 
Regards,
Tasmiya Nalatwad
IBM Linux Technology Center

Re: [PATCH] nvme: move passthrough logging attribute to head

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>