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

[Belanger, Martin]

> >>>>> 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;
> +
> +

I just tried this and it works. 
Martin

>          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);
> --

Sagi, would you add this function to drivers/base/core.c?
Martin

Internal Use - Confidential