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

[PATCHv3 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>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.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 dd18861f77c0..b5e452aa3c0e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4229,6 +4229,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 };
@@ -4396,6 +4403,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

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

[John Meneghini]

On 2/8/22 14:33, 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>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.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 dd18861f77c0..b5e452aa3c0e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4229,6 +4229,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 };
> @@ -4396,6 +4403,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: John Meneghini <jmeneghi@redhat.com>

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

[Christoph Hellwig]

Thanks,

applied patches 1 and 2 to nvme-5.18.

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

[Martin Belanger]

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

TP8010 introduces the Discovery Controller Type attribute (dctype).
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. The dctype will
only be displayed for discovery controllers.

A note about the naming of this attribute:
Although TP8010 calls this attribute the Discovery Controller Type,
note that the dctype is now part of the response to the Identify
command for all controller types. I/O, Discovery, and Admin controllers
all share the same Identify response PDU structure. Non-discovery
controllers as well as pre-TP8010 discovery controllers will continue
to set this field to 0 (which has always been the default for reserved
bytes). Per TP8010, the value 0 now means "Discovery controller type is
not reported" instead of "Reserved". One could argue that this
definition is correct even for non-discovery controllers, and by
extension, exposing it in the sysfs for non-discovery controllers is
appropriate.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
---
 drivers/nvme/host/core.c | 39 +++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  3 +++
 include/linux/nvme.h     | 10 +++++++++-
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b5e452aa3c0e..bb6ec2759e3e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2990,6 +2990,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;
@@ -3523,6 +3526,40 @@ 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 cntrltype_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	static const char * const type[] = {
+		[NVME_CTRL_IO] = "io\n",
+		[NVME_CTRL_DISC] = "discovery\n",
+		[NVME_CTRL_ADMIN] = "admin\n",
+	};
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->cntrltype])
+		return sysfs_emit(buf, "reserved\n");
+
+	return sysfs_emit(buf, type[ctrl->cntrltype]);
+}
+static DEVICE_ATTR_RO(cntrltype);
+
+static ssize_t dctype_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	static const char * const type[] = {
+		[NVME_DCTYPE_NOT_REPORTED] = "none\n",
+		[NVME_DCTYPE_DDC] = "ddc\n",
+		[NVME_DCTYPE_CDC] = "cdc\n",
+	};
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (ctrl->dctype > NVME_DCTYPE_CDC || !type[ctrl->dctype])
+		return sysfs_emit(buf, "reserved\n");
+
+	return sysfs_emit(buf, type[ctrl->dctype]);
+}
+static DEVICE_ATTR_RO(dctype);
+
 static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reset_controller.attr,
 	&dev_attr_rescan_controller.attr,
@@ -3544,6 +3581,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
 };
 
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..21e92e97ca88 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: [PATCHv3 2/2] nvme: Expose cntrltype and dctype through sysfs

[Chaitanya Kulkarni]

On 2/8/22 11:33 AM, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> 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. The dctype will
> only be displayed for discovery controllers.
> 
> A note about the naming of this attribute:
> Although TP8010 calls this attribute the Discovery Controller Type,
> note that the dctype is now part of the response to the Identify
> command for all controller types. I/O, Discovery, and Admin controllers
> all share the same Identify response PDU structure. Non-discovery
> controllers as well as pre-TP8010 discovery controllers will continue
> to set this field to 0 (which has always been the default for reserved
> bytes). Per TP8010, the value 0 now means "Discovery controller type is
> not reported" instead of "Reserved". One could argue that this
> definition is correct even for non-discovery controllers, and by
> extension, exposing it in the sysfs for non-discovery controllers is
> appropriate.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---

Looks good.

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

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

[Chaitanya Kulkarni]

> +static ssize_t cntrltype_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	static const char * const type[] = {

nit:- static cost char *const type[] ...

> +		[NVME_CTRL_IO] = "io\n",
> +		[NVME_CTRL_DISC] = "discovery\n",
> +		[NVME_CTRL_ADMIN] = "admin\n",
> +	};
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->cntrltype])
> +		return sysfs_emit(buf, "reserved\n");
> +
> +	return sysfs_emit(buf, type[ctrl->cntrltype]);
> +}
> +static DEVICE_ATTR_RO(cntrltype);
> +
> +static ssize_t dctype_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	static const char * const type[] = {

nit:- static cost char *const typep[] ...

> +		[NVME_DCTYPE_NOT_REPORTED] = "none\n",
> +		[NVME_DCTYPE_DDC] = "ddc\n",
> +		[NVME_DCTYPE_CDC] = "cdc\n",
> +	};
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (ctrl->dctype > NVME_DCTYPE_CDC || !type[ctrl->dctype])
> +		return sysfs_emit(buf, "reserved\n");
> +
> +	return sysfs_emit(buf, type[ctrl->dctype]);
> +}
> +static DEVICE_ATTR_RO(dctype);
> +
>   static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reset_controller.attr,
>   	&dev_attr_rescan_controller.attr,
> @@ -3544,6 +3581,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
>   };
>   
> 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..21e92e97ca88 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];
>   };
> 

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

[Belanger, Martin]

> > +static ssize_t cntrltype_show(struct device *dev,
> > +			      struct device_attribute *attr, char *buf) {
> > +	static const char * const type[] = {
> 
> nit:- static cost char *const type[] ...

Hi Chaitanya. I'm afraid I don’t know what you mean by "nit". 
The syntax "static const char * const type[] =" was suggested 
by the checkpatch.pl script.

Regards,
Martin

> 
> > +		[NVME_CTRL_IO] = "io\n",
> > +		[NVME_CTRL_DISC] = "discovery\n",
> > +		[NVME_CTRL_ADMIN] = "admin\n",
> > +	};
> > +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> > +
> > +	if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->cntrltype])
> > +		return sysfs_emit(buf, "reserved\n");
> > +
> > +	return sysfs_emit(buf, type[ctrl->cntrltype]); } static
> > +DEVICE_ATTR_RO(cntrltype);
> > +
> > +static ssize_t dctype_show(struct device *dev,
> > +			   struct device_attribute *attr, char *buf) {
> > +	static const char * const type[] = {
> 
> nit:- static cost char *const typep[] ...
> 
> > +		[NVME_DCTYPE_NOT_REPORTED] = "none\n",
> > +		[NVME_DCTYPE_DDC] = "ddc\n",
> > +		[NVME_DCTYPE_CDC] = "cdc\n",
> > +	};
> > +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> > +
> > +	if (ctrl->dctype > NVME_DCTYPE_CDC || !type[ctrl->dctype])
> > +		return sysfs_emit(buf, "reserved\n");
> > +
> > +	return sysfs_emit(buf, type[ctrl->dctype]); } static
> > +DEVICE_ATTR_RO(dctype);
> > +
> >   static struct attribute *nvme_dev_attrs[] = {
> >   	&dev_attr_reset_controller.attr,
> >   	&dev_attr_rescan_controller.attr,
> > @@ -3544,6 +3581,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
> >   };
> >
> > 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..21e92e97ca88 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];
> >   };
> >

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

[Chaitanya Kulkarni]

> On Feb 8, 2022, at 3:54 PM, Belanger, Martin <Martin.Belanger@dell.com> wrote:
> 
> 
>> 
>>> +static ssize_t cntrltype_show(struct device *dev,
>>> +                  struct device_attribute *attr, char *buf) {
>>> +    static const char * const type[] = {
>> 
>> nit:- static cost char *const type[] ...
> 
> Hi Chaitanya. I'm afraid I don’t know what you mean by "nit". 
> The syntax "static const char * const type[] =" was suggested 
> by the checkpatch.pl script.
> 
> Regards,
> Martin

In that case ignore my comment. 

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

[Hannes Reinecke]

On 2/8/22 20:33, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> 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. The dctype will
> only be displayed for discovery controllers.
> 
> A note about the naming of this attribute:
> Although TP8010 calls this attribute the Discovery Controller Type,
> note that the dctype is now part of the response to the Identify
> command for all controller types. I/O, Discovery, and Admin controllers
> all share the same Identify response PDU structure. Non-discovery
> controllers as well as pre-TP8010 discovery controllers will continue
> to set this field to 0 (which has always been the default for reserved
> bytes). Per TP8010, the value 0 now means "Discovery controller type is
> not reported" instead of "Reserved". One could argue that this
> definition is correct even for non-discovery controllers, and by
> extension, exposing it in the sysfs for non-discovery controllers is
> appropriate.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c | 39 +++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h |  3 +++
>   include/linux/nvme.h     | 10 +++++++++-
>   3 files changed, 51 insertions(+), 1 deletion(-)
> 
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: [PATCHv3 2/2] nvme: Expose cntrltype and dctype through sysfs

[Hannes Reinecke]

On 2/8/22 20:33, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> 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. The dctype will
> only be displayed for discovery controllers.
> 
> A note about the naming of this attribute:
> Although TP8010 calls this attribute the Discovery Controller Type,
> note that the dctype is now part of the response to the Identify
> command for all controller types. I/O, Discovery, and Admin controllers
> all share the same Identify response PDU structure. Non-discovery
> controllers as well as pre-TP8010 discovery controllers will continue
> to set this field to 0 (which has always been the default for reserved
> bytes). Per TP8010, the value 0 now means "Discovery controller type is
> not reported" instead of "Reserved". One could argue that this
> definition is correct even for non-discovery controllers, and by
> extension, exposing it in the sysfs for non-discovery controllers is
> appropriate.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c | 39 +++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h |  3 +++
>   include/linux/nvme.h     | 10 +++++++++-
>   3 files changed, 51 insertions(+), 1 deletion(-)
> 
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: [PATCHv3 2/2] nvme: Expose cntrltype and dctype through sysfs

[John Meneghini]

Looks good.

Reviewed-by: John Meneghini <jmeneghi@redhat.com>

On 2/8/22 14:33, Martin Belanger wrote:
> From: Martin Belanger <martin.belanger@dell.com>
> 
> TP8010 introduces the Discovery Controller Type attribute (dctype).
> 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. The dctype will
> only be displayed for discovery controllers.
> 
> A note about the naming of this attribute:
> Although TP8010 calls this attribute the Discovery Controller Type,
> note that the dctype is now part of the response to the Identify
> command for all controller types. I/O, Discovery, and Admin controllers
> all share the same Identify response PDU structure. Non-discovery
> controllers as well as pre-TP8010 discovery controllers will continue
> to set this field to 0 (which has always been the default for reserved
> bytes). Per TP8010, the value 0 now means "Discovery controller type is
> not reported" instead of "Reserved". One could argue that this
> definition is correct even for non-discovery controllers, and by
> extension, exposing it in the sysfs for non-discovery controllers is
> appropriate.
> 
> Signed-off-by: Martin Belanger <martin.belanger@dell.com>
> ---
>   drivers/nvme/host/core.c | 39 +++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/nvme.h |  3 +++
>   include/linux/nvme.h     | 10 +++++++++-
>   3 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index b5e452aa3c0e..bb6ec2759e3e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2990,6 +2990,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;
> @@ -3523,6 +3526,40 @@ 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 cntrltype_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	static const char * const type[] = {
> +		[NVME_CTRL_IO] = "io\n",
> +		[NVME_CTRL_DISC] = "discovery\n",
> +		[NVME_CTRL_ADMIN] = "admin\n",
> +	};
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (ctrl->cntrltype > NVME_CTRL_ADMIN || !type[ctrl->cntrltype])
> +		return sysfs_emit(buf, "reserved\n");
> +
> +	return sysfs_emit(buf, type[ctrl->cntrltype]);
> +}
> +static DEVICE_ATTR_RO(cntrltype);
> +
> +static ssize_t dctype_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	static const char * const type[] = {
> +		[NVME_DCTYPE_NOT_REPORTED] = "none\n",
> +		[NVME_DCTYPE_DDC] = "ddc\n",
> +		[NVME_DCTYPE_CDC] = "cdc\n",
> +	};
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (ctrl->dctype > NVME_DCTYPE_CDC || !type[ctrl->dctype])
> +		return sysfs_emit(buf, "reserved\n");
> +
> +	return sysfs_emit(buf, type[ctrl->dctype]);
> +}
> +static DEVICE_ATTR_RO(dctype);
> +
>   static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_reset_controller.attr,
>   	&dev_attr_rescan_controller.attr,
> @@ -3544,6 +3581,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
>   };
>   
> 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..21e92e97ca88 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];
>   };