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

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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