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

[John Meneghini <jmeneghi@redhat.com>]

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