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

[Belanger, Martin]

> On 2/3/22 12:59, Belanger, Martin wrote:
> >> On 2/2/22 22:34, Martin Belanger wrote:
> >>> 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. 
Thanks,
Martin

> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke		           Kernel Storage Architect
> hare at suse.de			                  +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Internal Use - Confidential