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

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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