[PATCH 2/2] nvme: Expose cntrltype and dctype through sysfs.

[Sagi Grimberg]

>>>>> From: Martin Belanger <martin.belanger at dell.com>
>>>>>
>>>>> TP8010 introduces the Discovery Controller Type (dctype) attribute.
>>>>> The dctype is returned in the response to the Identify command. This
>>>>> patch exposes the dctype through the sysfs. Since the dctype depends
>>>>> on the Controller Type (cntrltype), another attribute of the
>>>>> Identify response, the patch also exposes the cntrltype as well.
>>>>>
>>>>> Signed-off-by: Martin Belanger <martin.belanger at dell.com>
>>>>> ---
>>>>>     drivers/nvme/host/core.c | 35
>>>> +++++++++++++++++++++++++++++++++++
>>>>>     drivers/nvme/host/nvme.h |  3 +++
>>>>>     include/linux/nvme.h     | 10 +++++++++-
>>>>>     3 files changed, 47 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>> index
>>>>> 872c389dc6d3..c65d44fb65c4 100644
>>>>> --- a/drivers/nvme/host/core.c
>>>>> +++ b/drivers/nvme/host/core.c
>>>>> @@ -2992,6 +2992,9 @@ static int nvme_init_identify(struct nvme_ctrl
>>>> *ctrl)
>>>>>     	ctrl->max_namespaces = le32_to_cpu(id->mnan);
>>>>>     	ctrl->ctratt = le32_to_cpu(id->ctratt);
>>>>>
>>>>> +	ctrl->cntrltype = id->cntrltype;
>>>>> +	ctrl->dctype = id->dctype;
>>>>> +
>>>>>     	if (id->rtd3e) {
>>>>>     		/* us -> s */
>>>>>     		u32 transition_time = le32_to_cpu(id->rtd3e) /
>>>> USEC_PER_SEC; @@
>>>>> -3525,6 +3528,34 @@ static ssize_t
>>>> nvme_ctrl_fast_io_fail_tmo_store(struct device *dev,
>>>>>     static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,
>>>>>     	nvme_ctrl_fast_io_fail_tmo_show,
>>>> nvme_ctrl_fast_io_fail_tmo_store);
>>>>>
>>>>> +static ssize_t nvme_ctrl_cntrltype_show(struct device *dev,
>>>>> +				     struct device_attribute *attr, char *buf) {
>>>>> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>>>> +
>>>>> +	switch (ctrl->cntrltype) {
>>>>> +	case NVME_CTRL_IO:	return sysfs_emit(buf, "io\n");
>>>>> +	case NVME_CTRL_DISC:	return sysfs_emit(buf, "disc\n");
>>>>
>>>> 'discovery', please. 'disc' is misleading here.
>>>>
>>>>> +	case NVME_CTRL_ADMIN:	return sysfs_emit(buf, "admin\n");
>>>>> +	default:		return sysfs_emit(buf, "reserved\n");
>>>>> +	}
>>>>> +}
>>>>> +static DEVICE_ATTR(cntrltype, S_IRUGO, nvme_ctrl_cntrltype_show,
>>>>> +NULL);
>>>>> +
>>>>> +static ssize_t nvme_ctrl_dctype_show(struct device *dev,
>>>>> +				     struct device_attribute *attr, char *buf) {
>>>>> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>>>> +
>>>>> +	switch (ctrl->dctype) {
>>>>> +	case NVME_DCTYPE_NOT_REPORTED:	return sysfs_emit(buf,
>>>> "nr\n");
>>>>
>>>> Please use 'none' here; 'nr' is ambiguous ('Number'?)
>>>
>>> Thanks for the review. I will apply the changes.
>>>
>>> Question: Since this was part of a set of patches, do I need to
>>> resubmit the whole set or can I just resubmit this one patch?
>>>
>> Whole series, please. And gather up the reviews when resubmitting.
>>
>>> Also, Sagi mentioned that the dctype should only be exposed for
>>> Discovery Controllers, which makes sense. However, I found that when
>>> nvme_dev_attrs_are_visible() is invoked the ctrl object is not fully
>>> configured yet (i.e. the controller is not yet identified).
>>> Is there any other way to determine the controller type prior to
>>> receiving the response to the Identify command?
>>>
>> Nope, unfortunately.
>>
>> But maybe we can lump both attributes together in to a single ctrltype; then
>> we would have 'io', 'admin', 'discovery', 'discovery-cdc', and 'discovery-ddc'.
>>
>> Hmm?
> 
> I like that. I shall combine them into a single attribute.

I don't think this is a good approach.

There is already an interface to update sysfs groups, so in theory
this should do the trick:
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c11cd3a814fd..d86828cf7720 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3115,6 +3115,11 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
                         return ret;
         }

+       ret = sysfs_update_groups(&ctrl->device->kobj, 
ctrl->device->groups);
+       if (ret)
+               return ret;
+
+
         ctrl->identified = true;

         return 0;
--

But probably we want add a nicer interface like
--
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c11cd3a814fd..8fb94cb15fb0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3115,6 +3115,11 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
                         return ret;
         }

+       ret = device_update_groups(&ctrl->device);
+       if (ret)
+               return ret;
+
+
         ctrl->identified = true;

         return 0;
--

Where device_update_groups would be something like:
--
int device_update_groups(struct device *dev)
{
         struct class *class = dev->class;
         const struct device_type *type = dev->type;
         int error;

         if (class) {
                 error = device_update_groups(dev, class->dev_groups);
                 if (error)
                         return error;
         }

         if (type) {
                 error = device_update_groups(dev, type->groups);
                 if (error)
                         goto err_remove_class_groups;
         }

         error = device_update_groups(dev, dev->groups);
         if (error)
                 goto err_remove_type_groups;
}
EXPORT_SYMBOL_GPL(device_update_groups);
--