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