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