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

[Sagi Grimberg]

>>> 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?
> 
> If we create separate show/stores for the ctrl (nvme_dev_attrs) and ns 
> (nvme_ns_attrs), using the exiting macro: DEVICE_ATTR(), the attribute 
> name (passthru_err_log_enabled) can not be used for both nvme_dev_attrs 
> and nvme_ns_attrs.
> 
> /sys/class/nvme/nvme0/adm_passthru_err_log_enabled
> 
> /sys/class/nvme/nvme0/nvme0n1/io_passthru_err_log_enabled
> 
> 
> If I setup the attributes like:
> 
> static struct device_attribute dev_attr_adm_passthru_err_log_enabled = \
>          __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
>          nvme_adm_passthru_err_log_enabled_show, 
> nvme_adm_passthru_err_log_enabled_store);
> 
> 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);
> 
> 
> Then both attributes can be the same.
> 
> /sys/class/nvme/nvme0/passthru_err_log_enabled
> 
> /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
> 
> What do we prefer?

The second one looks fine to me.