[PATCH 1/2] nvme: send uevent on connection up

[PATCH 1/2] nvme: send uevent on connection up

[Martin Belanger]

From: Martin Belanger <martin.belanger@dell.com>

When connectivity with a controller is lost, the driver will keep trying
to reconnect once every 10 sec. When connection is restored, user-space
apps need to be informed so that they can take proper action. For
example, TP8010 introduces the DIM PDU, which is used to register with
a discovery controller (DC). The DIM PDU is sent from user-space.  The
DIM PDU must be sent every time a connection is established with a DC.
Therefore, the kernel must tell user-space apps when connection is
restored so that registration can happen.

The uevent sent is a "change" uevent with environmental data
set to: "NVME_EVENT=connected".

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 961a5f8a44d2..872c389dc6d3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4231,6 +4231,13 @@ static int nvme_class_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return ret;
 }
 
+static void nvme_change_uevent(struct nvme_ctrl *ctrl, char *envdata)
+{
+	char *envp[2] = { envdata, NULL };
+
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+
 static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
 {
 	char *envp[2] = { NULL, NULL };
@@ -4405,6 +4412,8 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 		nvme_queue_scan(ctrl);
 		nvme_start_queues(ctrl);
 	}
+
+	nvme_change_uevent(ctrl, "NVME_EVENT=connected");
 }
 EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 
-- 
2.34.1

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

[Martin Belanger]

From: Martin Belanger <martin.belanger@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@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");
+	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");
+	case NVME_DCTYPE_DDC:		return sysfs_emit(buf, "ddc\n");
+	case NVME_DCTYPE_CDC:		return sysfs_emit(buf, "cdc\n");
+	default:			return sysfs_emit(buf, "reserved\n");
+	}
+}
+static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);
+
 static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reset_controller.attr,
 	&dev_attr_rescan_controller.attr,
@@ -3546,6 +3577,8 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reconnect_delay.attr,
 	&dev_attr_fast_io_fail_tmo.attr,
 	&dev_attr_kato.attr,
+	&dev_attr_cntrltype.attr,
+	&dev_attr_dctype.attr,
 	NULL
 };
 
@@ -3569,6 +3602,8 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
 		return 0;
 	if (a == &dev_attr_fast_io_fail_tmo.attr && !ctrl->opts)
 		return 0;
+	if (a == &dev_attr_dctype.attr && ctrl->cntrltype == NVME_CTRL_DISC)
+		return 0;
 
 	return a->mode;
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a162f6c6da6e..e30626c00791 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -349,6 +349,9 @@ struct nvme_ctrl {
 	unsigned long discard_page_busy;
 
 	struct nvme_fault_inject fault_inject;
+
+	enum nvme_ctrl_type cntrltype;
+	enum nvme_dctype dctype;
 };
 
 enum nvme_iopolicy {
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 855dd9b3e84b..af1494d83a52 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -43,6 +43,12 @@ enum nvme_ctrl_type {
 	NVME_CTRL_ADMIN	= 3,		/* Administrative controller */
 };
 
+enum nvme_dctype {
+	NVME_DCTYPE_NOT_REPORTED = 0,
+	NVME_DCTYPE_DDC = 1,		/* Direct Discovery Controller */
+	NVME_DCTYPE_CDC = 2,		/* Central Discovery Controller */
+};
+
 /* Address Family codes for Discovery Log Page entry ADRFAM field */
 enum {
 	NVMF_ADDR_FAMILY_PCI	= 0,	/* PCIe */
@@ -320,7 +326,9 @@ struct nvme_id_ctrl {
 	__le16			icdoff;
 	__u8			ctrattr;
 	__u8			msdbd;
-	__u8			rsvd1804[244];
+	__u8			rsvd1804[2];
+	__u8			dctype;
+	__u8			rsvd1807[241];
 	struct nvme_id_power_state	psd[32];
 	__u8			vs[1024];
 };
-- 
2.34.1

Re: [PATCH 1/2] nvme: send uevent on connection up

[Hannes Reinecke]

On 2/2/22 22:34, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> When connectivity with a controller is lost, the driver will keep trying
> to reconnect once every 10 sec. When connection is restored, user-space
> apps need to be informed so that they can take proper action. For
> example, TP8010 introduces the DIM PDU, which is used to register with
> a discovery controller (DC). The DIM PDU is sent from user-space.  The
> DIM PDU must be sent every time a connection is established with a DC.
> Therefore, the kernel must tell user-space apps when connection is
> restored so that registration can happen.
> 
> The uevent sent is a "change" uevent with environmental data
> set to: "NVME_EVENT=connected".
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 961a5f8a44d2..872c389dc6d3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4231,6 +4231,13 @@ static int nvme_class_uevent(struct device *dev, struct kobj_uevent_env *env)
>   	return ret;
>   }
>   
> +static void nvme_change_uevent(struct nvme_ctrl *ctrl, char *envdata)
> +{
> +	char *envp[2] = { envdata, NULL };
> +
> +	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
> +}
> +
>   static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
>   {
>   	char *envp[2] = { NULL, NULL };
> @@ -4405,6 +4412,8 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>   		nvme_queue_scan(ctrl);
>   		nvme_start_queues(ctrl);
>   	}
> +
> +	nvme_change_uevent(ctrl, "NVME_EVENT=connected");
>   }
>   EXPORT_SYMBOL_GPL(nvme_start_ctrl);
>   
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

[Hannes Reinecke]

On 2/2/22 22:34, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@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@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'?)

> +	case NVME_DCTYPE_DDC:		return sysfs_emit(buf, "ddc\n");
> +	case NVME_DCTYPE_CDC:		return sysfs_emit(buf, "cdc\n");
> +	default:			return sysfs_emit(buf, "reserved\n");
> +	}
> +}
> +static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);
> +
>   static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reset_controller.attr,
>   	&dev_attr_rescan_controller.attr,
> @@ -3546,6 +3577,8 @@ static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reconnect_delay.attr,
>   	&dev_attr_fast_io_fail_tmo.attr,
>   	&dev_attr_kato.attr,
> +	&dev_attr_cntrltype.attr,
> +	&dev_attr_dctype.attr,
>   	NULL
>   };
>   
> @@ -3569,6 +3602,8 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
>   		return 0;
>   	if (a == &dev_attr_fast_io_fail_tmo.attr && !ctrl->opts)
>   		return 0;
> +	if (a == &dev_attr_dctype.attr && ctrl->cntrltype == NVME_CTRL_DISC)
> +		return 0;
>   
>   	return a->mode;
>   }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index a162f6c6da6e..e30626c00791 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -349,6 +349,9 @@ struct nvme_ctrl {
>   	unsigned long discard_page_busy;
>   
>   	struct nvme_fault_inject fault_inject;
> +
> +	enum nvme_ctrl_type cntrltype;
> +	enum nvme_dctype dctype;
>   };
>   
>   enum nvme_iopolicy {
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 855dd9b3e84b..af1494d83a52 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -43,6 +43,12 @@ enum nvme_ctrl_type {
>   	NVME_CTRL_ADMIN	= 3,		/* Administrative controller */
>   };
>   
> +enum nvme_dctype {
> +	NVME_DCTYPE_NOT_REPORTED = 0,
> +	NVME_DCTYPE_DDC = 1,		/* Direct Discovery Controller */
> +	NVME_DCTYPE_CDC = 2,		/* Central Discovery Controller */
> +};
> +
>   /* Address Family codes for Discovery Log Page entry ADRFAM field */
>   enum {
>   	NVMF_ADDR_FAMILY_PCI	= 0,	/* PCIe */
> @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
>   	__le16			icdoff;
>   	__u8			ctrattr;
>   	__u8			msdbd;
> -	__u8			rsvd1804[244];
> +	__u8			rsvd1804[2];
> +	__u8			dctype;
> +	__u8			rsvd1807[241];
>   	struct nvme_id_power_state	psd[32];
>   	__u8			vs[1024];
>   };

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/2] nvme: send uevent on connection up

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[Belanger, Martin]

> On 2/2/22 22:34, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger@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@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?

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?

> 
> > +	case NVME_DCTYPE_DDC:		return sysfs_emit(buf,
> "ddc\n");
> > +	case NVME_DCTYPE_CDC:		return sysfs_emit(buf,
> "cdc\n");
> > +	default:			return sysfs_emit(buf, "reserved\n");
> > +	}
> > +}
> > +static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);
> > +
> >   static struct attribute *nvme_dev_attrs[] = {
> >   	&dev_attr_reset_controller.attr,
> >   	&dev_attr_rescan_controller.attr,
> > @@ -3546,6 +3577,8 @@ static struct attribute *nvme_dev_attrs[] = {
> >   	&dev_attr_reconnect_delay.attr,
> >   	&dev_attr_fast_io_fail_tmo.attr,
> >   	&dev_attr_kato.attr,
> > +	&dev_attr_cntrltype.attr,
> > +	&dev_attr_dctype.attr,
> >   	NULL
> >   };
> >
> > @@ -3569,6 +3602,8 @@ static umode_t
> nvme_dev_attrs_are_visible(struct kobject *kobj,
> >   		return 0;
> >   	if (a == &dev_attr_fast_io_fail_tmo.attr && !ctrl->opts)
> >   		return 0;
> > +	if (a == &dev_attr_dctype.attr && ctrl->cntrltype ==
> NVME_CTRL_DISC)
> > +		return 0;
> >
> >   	return a->mode;
> >   }
> > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index
> > a162f6c6da6e..e30626c00791 100644
> > --- a/drivers/nvme/host/nvme.h
> > +++ b/drivers/nvme/host/nvme.h
> > @@ -349,6 +349,9 @@ struct nvme_ctrl {
> >   	unsigned long discard_page_busy;
> >
> >   	struct nvme_fault_inject fault_inject;
> > +
> > +	enum nvme_ctrl_type cntrltype;
> > +	enum nvme_dctype dctype;
> >   };
> >
> >   enum nvme_iopolicy {
> > diff --git a/include/linux/nvme.h b/include/linux/nvme.h index
> > 855dd9b3e84b..af1494d83a52 100644
> > --- a/include/linux/nvme.h
> > +++ b/include/linux/nvme.h
> > @@ -43,6 +43,12 @@ enum nvme_ctrl_type {
> >   	NVME_CTRL_ADMIN	= 3,		/* Administrative controller
> */
> >   };
> >
> > +enum nvme_dctype {
> > +	NVME_DCTYPE_NOT_REPORTED = 0,
> > +	NVME_DCTYPE_DDC = 1,		/* Direct Discovery Controller
> */
> > +	NVME_DCTYPE_CDC = 2,		/* Central Discovery
> Controller */
> > +};
> > +
> >   /* Address Family codes for Discovery Log Page entry ADRFAM field */
> >   enum {
> >   	NVMF_ADDR_FAMILY_PCI	= 0,	/* PCIe */
> > @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
> >   	__le16			icdoff;
> >   	__u8			ctrattr;
> >   	__u8			msdbd;
> > -	__u8			rsvd1804[244];
> > +	__u8			rsvd1804[2];
> > +	__u8			dctype;
> > +	__u8			rsvd1807[241];
> >   	struct nvme_id_power_state	psd[32];
> >   	__u8			vs[1024];
> >   };
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke                Kernel Storage Architect
> hare@suse.de                              +49 911 74053 688
> SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809
> (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Internal Use - Confidential

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

[Hannes Reinecke]

On 2/3/22 12:59, Belanger, Martin wrote:
>> On 2/2/22 22:34, Martin Belanger wrote:
>>> From: Martin Belanger <martin.belanger@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@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?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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

Re: [PATCH 1/2] nvme: send uevent on connection up

[Chaitanya Kulkarni]

On 2/2/22 13:34, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> When connectivity with a controller is lost, the driver will keep trying
> to reconnect once every 10 sec. When connection is restored, user-space
> apps need to be informed so that they can take proper action. For
> example, TP8010 introduces the DIM PDU, which is used to register with
> a discovery controller (DC). The DIM PDU is sent from user-space.  The
> DIM PDU must be sent every time a connection is established with a DC.
> Therefore, the kernel must tell user-space apps when connection is
> restored so that registration can happen.
> 
> The uevent sent is a "change" uevent with environmental data
> set to: "NVME_EVENT=connected".
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

RE: [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@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@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@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

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

[Sagi Grimberg]

>>>> From: Martin Belanger <martin.belanger@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@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?

We should really move the ctrl device node creation to
nvme_init_ctrl_finish. I think this came up before...

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

[Chaitanya Kulkarni]

On 2/2/22 13:34, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@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.
> 

above can be re aligned to 72 char per line :-

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@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");
> +	case NVME_CTRL_ADMIN:	return sysfs_emit(buf, "admin\n");
> +	default:		return sysfs_emit(buf, "reserved\n");

please keep the return on new line ...

> +	}
> +}

with newline added to the switch for each new case we will be adding new 
case statement and a sysfs_emit() call, following style avoids current 
and future repetations of case ... sysfs_emit() calls and brings down to 
only two calls including any future ctrl type value (2 lines for each 
new value pf ctrltype vs 1 line in the array of string :-

static ssize_t nvme_ctrl_cntrltype_show(struct device *dev,
                                      struct device_attribute *attr, 
char *buf)
{
         const char *type[] = {
             [NVME_CTRL_IO], "io\n",
             [NVME_CTRL_DISC], "disc\n",
             [NVME_CTRL_ADMIN], "admin\n",
         };

         if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->ctrltype])
             return sysfs_emit(buf, "reserved\n");

         return sysfs_emit(buf, type[ctrl->ctrltype]);
}




> +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");
> +	case NVME_DCTYPE_DDC:		return sysfs_emit(buf, "ddc\n");
> +	case NVME_DCTYPE_CDC:		return sysfs_emit(buf, "cdc\n");
> +	default:			return sysfs_emit(buf, "reserved\n");
> +	}
> +}

same comment as above to use array of string unless there is any reason
we should repeat the sysfs_emit() calls.

> +static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);
> +
>   static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reset_controller.attr,
>   	&dev_attr_rescan_controller.attr,
> @@ -3546,6 +3577,8 @@ static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reconnect_delay.attr,
>   	&dev_attr_fast_io_fail_tmo.attr,
>   	&dev_attr_kato.attr,
> +	&dev_attr_cntrltype.attr,
> +	&dev_attr_dctype.attr,
>   	NULL
>   };
>   
> @@ -3569,6 +3602,8 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
>   		return 0;
>   	if (a == &dev_attr_fast_io_fail_tmo.attr && !ctrl->opts)
>   		return 0;
> +	if (a == &dev_attr_dctype.attr && ctrl->cntrltype == NVME_CTRL_DISC)
> +		return 0;
>   
>   	return a->mode;
>   }
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index a162f6c6da6e..e30626c00791 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -349,6 +349,9 @@ struct nvme_ctrl {
>   	unsigned long discard_page_busy;
>   
>   	struct nvme_fault_inject fault_inject;
> +
> +	enum nvme_ctrl_type cntrltype;
> +	enum nvme_dctype dctype;
>   };
>   
>   enum nvme_iopolicy {
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index 855dd9b3e84b..af1494d83a52 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -43,6 +43,12 @@ enum name_ctrl_type {
>   	NVME_CTRL_ADMIN	= 3,		/* Administrative controller */
>   };
>   
> +enum nvme_dctype {
> +	NVME_DCTYPE_NOT_REPORTED = 0,
> +	NVME_DCTYPE_DDC = 1,		/* Direct Discovery Controller */
> +	NVME_DCTYPE_CDC = 2,		/* Central Discovery Controller */
> +};
> +

consider follwing with aligned initial values that also matches what
we have in the include/linux/nvme.h for enums initialization :-
enum nvme_dctype {
         NVME_DCTYPE_NOT_REPORTED        = 0,
         NVME_DCTYPE_DDC                 = 1, /* Direct Discovery 
Controller */
         NVME_DCTYPE_CDC                 = 2, /* Central Discovery 
Controller */
};

>   /* Address Family codes for Discovery Log Page entry ADRFAM field */
>   enum {
>   	NVMF_ADDR_FAMILY_PCI	= 0,	/* PCIe */
> @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
>   	__le16			icdoff;
>   	__u8			ctrattr;
>   	__u8			msdbd;
> -	__u8			rsvd1804[244];
> +	__u8			rsvd1804[2];
> +	__u8			dctype;
> +	__u8			rsvd1807[241];
>   	struct nvme_id_power_state	psd[32];
>   	__u8			vs[1024];
>   };
> 

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

[Sagi Grimberg]

>>>>> From: Martin Belanger <martin.belanger@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@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);
--

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

[Chaitanya Kulkarni]

Sagi,

>> 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?
> 
> We should really move the ctrl device node creation to
> nvme_init_ctrl_finish. I think this came up before...
> 

Unless I miss something all the transport rdma/pci/tcp/fc set ctrl
state to NVME_CTRL_CONNECTING before calling nvme_init_ctrl_finish().

Are you referring to the call of cdev_device_add() we have in the
nvme_init_ctrl() that needs to be moved into the
nvme_init_ctrl_finish() so instead of ctrl holding NVME_CTRL_NEW state
it will have NVME_CTRL_CONNECTING state ?

-ck

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

[Belanger, Martin]

> >>>>> From: Martin Belanger <martin.belanger@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@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

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

[Sagi Grimberg]

>> 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.

Makes sense.

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

This last hunk should probably be:
	return device_update_groups(dev, dev->groups);

>> }
>> EXPORT_SYMBOL_GPL(device_update_groups);
>> --
> 
> Sagi, would you add this function to drivers/base/core.c?
> Martin

Yes. This would need some more eyeballs though, maybe this
should be extended to device_update_attrs?

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

[Belanger, Martin]

> On 2/2/22 13:34, Martin Belanger wrote:
> > From: Martin Belanger <martin.belanger@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.
> >
> 
> above can be re aligned to 72 char per line :-
> 
> 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@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");
> > +	case NVME_CTRL_ADMIN:	return sysfs_emit(buf, "admin\n");
> > +	default:		return sysfs_emit(buf, "reserved\n");
> 
> please keep the return on new line ...

Hi Chaitanya,
Ok, I will keep the return on a separate line. BTW, is that specific to the 
NVMe driver. I see that in many places in the kernel they put the 
return on the same line as the case statement, as shown here:

$ git grep -E "case .*: *return" | wc -l
1676

> 
> > +	}
> > +}
> 
> with newline added to the switch for each new case we will be adding new
> case statement and a sysfs_emit() call, following style avoids current and
> future repetations of case ... sysfs_emit() calls and brings down to only two
> calls including any future ctrl type value (2 lines for each new value pf ctrltype
> vs 1 line in the array of string :-
> 
> static ssize_t nvme_ctrl_cntrltype_show(struct device *dev,
>                                       struct device_attribute *attr, char *buf) {
>          const char *type[] = {
>              [NVME_CTRL_IO], "io\n",
>              [NVME_CTRL_DISC], "disc\n",
>              [NVME_CTRL_ADMIN], "admin\n",
>          };
> 
>          if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->ctrltype])
>              return sysfs_emit(buf, "reserved\n");
> 
>          return sysfs_emit(buf, type[ctrl->ctrltype]); }
> 
> 
> 
> 
> > +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");
> > +	case NVME_DCTYPE_DDC:		return sysfs_emit(buf,
> "ddc\n");
> > +	case NVME_DCTYPE_CDC:		return sysfs_emit(buf,
> "cdc\n");
> > +	default:			return sysfs_emit(buf, "reserved\n");
> > +	}
> > +}
> 
> same comment as above to use array of string unless there is any reason we
> should repeat the sysfs_emit() calls.
> 
> > +static DEVICE_ATTR(dctype, S_IRUGO, nvme_ctrl_dctype_show, NULL);
> > +
> >   static struct attribute *nvme_dev_attrs[] = {
> >   	&dev_attr_reset_controller.attr,
> >   	&dev_attr_rescan_controller.attr,
> > @@ -3546,6 +3577,8 @@ static struct attribute *nvme_dev_attrs[] = {
> >   	&dev_attr_reconnect_delay.attr,
> >   	&dev_attr_fast_io_fail_tmo.attr,
> >   	&dev_attr_kato.attr,
> > +	&dev_attr_cntrltype.attr,
> > +	&dev_attr_dctype.attr,
> >   	NULL
> >   };
> >
> > @@ -3569,6 +3602,8 @@ static umode_t
> nvme_dev_attrs_are_visible(struct kobject *kobj,
> >   		return 0;
> >   	if (a == &dev_attr_fast_io_fail_tmo.attr && !ctrl->opts)
> >   		return 0;
> > +	if (a == &dev_attr_dctype.attr && ctrl->cntrltype ==
> NVME_CTRL_DISC)
> > +		return 0;
> >
> >   	return a->mode;
> >   }
> > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index
> > a162f6c6da6e..e30626c00791 100644
> > --- a/drivers/nvme/host/nvme.h
> > +++ b/drivers/nvme/host/nvme.h
> > @@ -349,6 +349,9 @@ struct nvme_ctrl {
> >   	unsigned long discard_page_busy;
> >
> >   	struct nvme_fault_inject fault_inject;
> > +
> > +	enum nvme_ctrl_type cntrltype;
> > +	enum nvme_dctype dctype;
> >   };
> >
> >   enum nvme_iopolicy {
> > diff --git a/include/linux/nvme.h b/include/linux/nvme.h index
> > 855dd9b3e84b..af1494d83a52 100644
> > --- a/include/linux/nvme.h
> > +++ b/include/linux/nvme.h
> > @@ -43,6 +43,12 @@ enum name_ctrl_type {
> >   	NVME_CTRL_ADMIN	= 3,		/* Administrative controller
> */
> >   };
> >
> > +enum nvme_dctype {
> > +	NVME_DCTYPE_NOT_REPORTED = 0,
> > +	NVME_DCTYPE_DDC = 1,		/* Direct Discovery Controller
> */
> > +	NVME_DCTYPE_CDC = 2,		/* Central Discovery
> Controller */
> > +};
> > +
> 
> consider follwing with aligned initial values that also matches what we have in
> the include/linux/nvme.h for enums initialization :- enum nvme_dctype {
>          NVME_DCTYPE_NOT_REPORTED        = 0,
>          NVME_DCTYPE_DDC                 = 1, /* Direct Discovery
> Controller */
>          NVME_DCTYPE_CDC                 = 2, /* Central Discovery
> Controller */
> };
> 
> >   /* Address Family codes for Discovery Log Page entry ADRFAM field */
> >   enum {
> >   	NVMF_ADDR_FAMILY_PCI	= 0,	/* PCIe */
> > @@ -320,7 +326,9 @@ struct nvme_id_ctrl {
> >   	__le16			icdoff;
> >   	__u8			ctrattr;
> >   	__u8			msdbd;
> > -	__u8			rsvd1804[244];
> > +	__u8			rsvd1804[2];
> > +	__u8			dctype;
> > +	__u8			rsvd1807[241];
> >   	struct nvme_id_power_state	psd[32];
> >   	__u8			vs[1024];
> >   };
> >

Internal Use - Confidential

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

[Belanger, Martin]

> >> 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.
> 
> Makes sense.
> 
> > 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;
> 
> This last hunk should probably be:
> 	return device_update_groups(dev, dev->groups);
> 
> >> }
> >> EXPORT_SYMBOL_GPL(device_update_groups);
> >> --
> >
> > Sagi, would you add this function to drivers/base/core.c?
> > Martin
> 
> Yes. This would need some more eyeballs though, maybe this should be
> extended to device_update_attrs?

Is the linux-nvme repo even the right place to submit this change? 
This is a core kernel file and not just the nvme module.

Submitting this change will probably delay what I'm trying 
to release by weeks. Is it OK if I just submit the small change
and leave this bigger change to someone that has more 
experience with the kernel?

Martin

Internal Use - Confidential

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

[Keith Busch]

On Thu, Feb 03, 2022 at 11:59:33AM +0000, Belanger, Martin wrote:
> 
> Internal Use - Confidential

This is not appropriate for public mailing lists. I assume it's some IT
policy auto-appending it, but I'm sure there must be ways to remove it.

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

[Sagi Grimberg]

>>> Sagi, would you add this function to drivers/base/core.c?
>>> Martin
>>
>> Yes. This would need some more eyeballs though, maybe this should be
>> extended to device_update_attrs?
> 
> Is the linux-nvme repo even the right place to submit this change?
> This is a core kernel file and not just the nvme module.
> 
> Submitting this change will probably delay what I'm trying
> to release by weeks. Is it OK if I just submit the small change
> and leave this bigger change to someone that has more
> experience with the kernel?

I don't think it will take a long time we still have time for 5.18 which
is where this is headed anyways.

I suggest to send a new patchset, make sure to cc Greg-kh and rafael
(see MAINTAINERS file) and let's see what feedback comes.