[PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[Gulam Mohamed]

This patch will implement the REPORT_IDENTIFICATION_INFORMATION using the
configfs file pd_text_id_info in target core module. The configfs file is
created in /sys/kernel/config/target/core/<backend type>/
<backing_store_name>/wwn/. The user can set the peripheral device text
identification string to the file pd_text_id_info. An emulation function
"spc_emulate_report_id_info()" is defined in target_core_spc.c which
returns the device text id whenever the user requests the same.

Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
---
 drivers/target/target_core_configfs.c | 55 ++++++++++++++++++
 drivers/target/target_core_spc.c      | 82 +++++++++++++++++++++++++++
 include/target/target_core_base.h     |  4 ++
 3 files changed, 141 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index b19acd662726..ac78a106e0f3 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1726,6 +1726,59 @@ static ssize_t target_wwn_vpd_protocol_identifier_show(struct config_item *item,
 	return len;
 }
 
+static ssize_t target_wwn_pd_text_id_info_show(struct config_item *item,
+		char *page)
+{
+	return sprintf(page, "%s\n", &to_t10_wwn(item)->pd_text_id_info[0]);
+}
+
+static ssize_t target_wwn_pd_text_id_info_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct t10_wwn *t10_wwn = to_t10_wwn(item);
+	struct se_device *dev = t10_wwn->t10_dev;
+
+	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
+	unsigned char buf[PD_TEXT_ID_INFO_LEN + 2];
+	char *stripped = NULL;
+	ssize_t len;
+
+	len = strscpy(buf, page, sizeof(buf));
+	if (len > 0) {
+		/* Strip any newline added from userspace. */
+		stripped = strstrip(buf);
+		len = strlen(stripped);
+	}
+
+	if (len < 0 || len >= PD_TEXT_ID_INFO_LEN) {
+		pr_err("Emulated peripheral device text id info exceeds"
+			" PD_TEXT_ID_INFO_LEN: " __stringify(PD_TEXT_ID_INFO_LEN)
+			"\n");
+		return -EOVERFLOW;
+	}
+
+	/*
+	 * Check to see if any active exports exist.  If they do exist, fail
+	 * here as changing this information on the fly (underneath the
+	 * initiator side OS dependent multipath code) could cause negative
+	 * effects.
+	 */
+	if (dev->export_count) {
+		pr_err("Unable to set the peripheral device text id info while"
+			" active %d exports exist\n", dev->export_count);
+		return -EINVAL;
+	}
+
+	BUILD_BUG_ON(sizeof(dev->t10_wwn.pd_text_id_info) != PD_TEXT_ID_INFO_LEN);
+	strscpy(dev->t10_wwn.pd_text_id_info, stripped,
+	       sizeof(dev->t10_wwn.pd_text_id_info));
+
+	pr_debug("Target_Core_ConfigFS: Set emulated peripheral dev text id info:"
+		  " %s\n", dev->t10_wwn.pd_text_id_info);
+
+	return count;
+}
+
 /*
  * Generic wrapper for dumping VPD identifiers by association.
  */
@@ -1782,6 +1835,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
 CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
+CONFIGFS_ATTR(target_wwn_, pd_text_id_info);
 
 static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
 	&target_wwn_attr_vendor_id,
@@ -1793,6 +1847,7 @@ static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
 	&target_wwn_attr_vpd_assoc_logical_unit,
 	&target_wwn_attr_vpd_assoc_target_port,
 	&target_wwn_attr_vpd_assoc_scsi_target_device,
+	&target_wwn_attr_pd_text_id_info,
 	NULL,
 };
 
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index aad0096afa21..9fa84202ee4b 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -1963,6 +1963,18 @@ static const struct target_opcode_descriptor tcm_opcode_report_supp_opcodes = {
 	.enabled = spc_rsoc_enabled,
 };
 
+static struct target_opcode_descriptor tcm_opcode_report_identifying_information = {
+	.support = SCSI_SUPPORT_FULL,
+	.serv_action_valid = 1,
+	.opcode = MAINTENANCE_IN,
+	.service_action = MI_REPORT_IDENTIFYING_INFORMATION,
+	.cdb_size = 12,
+	.usage_bits = {MAINTENANCE_IN, MI_REPORT_IDENTIFYING_INFORMATION,
+		       0x00, 0x00,
+		       0x00, 0x00, 0xff, 0xff,
+		       0xff, 0xff, 0xff, SCSI_CONTROL_MASK},
+};
+
 static bool tcm_is_set_tpg_enabled(const struct target_opcode_descriptor *descr,
 				   struct se_cmd *cmd)
 {
@@ -2049,6 +2061,7 @@ static const struct target_opcode_descriptor *tcm_supported_opcodes[] = {
 	&tcm_opcode_report_target_pgs,
 	&tcm_opcode_report_supp_opcodes,
 	&tcm_opcode_set_tpg,
+	&tcm_opcode_report_identifying_information,
 };
 
 static int
@@ -2266,6 +2279,70 @@ spc_emulate_report_supp_op_codes(struct se_cmd *cmd)
 	return ret;
 }
 
+static sense_reason_t
+spc_fill_pd_text_id_info(struct se_cmd *cmd, u8 *cdb)
+{
+	struct se_device *dev = cmd->se_dev;
+	unsigned char *buf;
+	unsigned char *rbuf;
+	u32 header_len = 4;
+	u32 actual_data_len;
+	u32 buf_len;
+	u16 id_len;
+
+	buf_len = get_unaligned_be32(&cdb[6]);
+	if (buf_len < header_len)
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+
+	id_len = strlen(dev->t10_wwn.pd_text_id_info);
+	if (id_len > 0)
+		/* trailing null */
+		id_len += 1;
+
+	actual_data_len = id_len + header_len;
+
+	if (actual_data_len < buf_len)
+		buf_len = actual_data_len;
+
+	buf = kzalloc(buf_len, GFP_KERNEL);
+	if (!buf) {
+		pr_err("Unable to allocate response buffer for IDENTITY INFO\n");
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
+
+	scnprintf(&buf[header_len], buf_len - header_len, "%s",
+		 dev->t10_wwn.pd_text_id_info);
+
+	put_unaligned_be16(id_len, &buf[2]);
+
+	rbuf = transport_kmap_data_sg(cmd);
+	if (rbuf) {
+		memcpy(rbuf, buf, buf_len);
+		transport_kunmap_data_sg(cmd);
+	}
+	kfree(buf);
+
+	target_complete_cmd_with_length(cmd, SAM_STAT_GOOD, buf_len);
+	return TCM_NO_SENSE;
+}
+
+static sense_reason_t
+spc_emulate_report_id_info(struct se_cmd *cmd)
+{
+	u8 *cdb = cmd->t_task_cdb;
+	sense_reason_t rc;
+
+	switch ((cdb[10] >> 1)) {
+	case 2:
+		rc = spc_fill_pd_text_id_info(cmd, cdb);
+		break;
+	default:
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	return rc;
+}
+
 sense_reason_t
 spc_parse_cdb(struct se_cmd *cmd, unsigned int *size)
 {
@@ -2405,6 +2482,11 @@ spc_parse_cdb(struct se_cmd *cmd, unsigned int *size)
 			    MI_REPORT_SUPPORTED_OPERATION_CODES)
 				cmd->execute_cmd =
 					spc_emulate_report_supp_op_codes;
+			if ((cdb[1] & 0x1f) ==
+			    MI_REPORT_IDENTIFYING_INFORMATION) {
+				cmd->execute_cmd =
+					spc_emulate_report_id_info;
+			}
 			*size = get_unaligned_be32(&cdb[6]);
 		} else {
 			/*
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index c4d9116904aa..c9b5edcce1eb 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -108,6 +108,9 @@
 #define SE_MODE_PAGE_BUF			512
 #define SE_SENSE_BUF				96
 
+/* Peripheral Device Text Identification Information */
+#define PD_TEXT_ID_INFO_LEN			256
+
 enum target_submit_type {
 	/* Use the fabric driver's default submission type */
 	TARGET_FABRIC_DEFAULT_SUBMIT,
@@ -347,6 +350,7 @@ struct t10_wwn {
 	struct se_device *t10_dev;
 	struct config_group t10_wwn_group;
 	struct list_head t10_vpd_list;
+	char pd_text_id_info[PD_TEXT_ID_INFO_LEN];
 };
 
 struct t10_pr_registration {
-- 
2.47.3

Re: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[John Garry]

On 26/10/2025 19:13, Gulam Mohamed wrote:

In the title, why "REPORT_IDENTIFICATION_INFORMATION" and not 
"REPORT_IDENTIFYING_INFORMATION"?

> This patch

git grep "This patch" Documentation/process/

> will implement the REPORT_IDENTIFICATION_INFORMATION using the
> configfs file pd_text_id_info in target core module. The configfs file is
> created in /sys/kernel/config/target/core/<backend type>/
> <backing_store_name>/wwn/. The user can set the peripheral device text
> identification string to the file pd_text_id_info. An emulation function
> "spc_emulate_report_id_info()" is defined in target_core_spc.c which
> returns the device text id whenever the user requests the same.

It would be nice to a mention a spec reference for the command.

> 
> Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>

Generally the code below looks ok, but I'll be nitpicking a bit...

> ---
>   drivers/target/target_core_configfs.c | 55 ++++++++++++++++++
>   drivers/target/target_core_spc.c      | 82 +++++++++++++++++++++++++++
>   include/target/target_core_base.h     |  4 ++
>   3 files changed, 141 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index b19acd662726..ac78a106e0f3 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -1726,6 +1726,59 @@ static ssize_t target_wwn_vpd_protocol_identifier_show(struct config_item *item,
>   	return len;
>   }
>   
> +static ssize_t target_wwn_pd_text_id_info_show(struct config_item *item,
> +		char *page)
> +{
> +	return sprintf(page, "%s\n", &to_t10_wwn(item)->pd_text_id_info[0]);
> +}
> +
> +static ssize_t target_wwn_pd_text_id_info_store(struct config_item *item,
> +		const char *page, size_t count)
> +{
> +	struct t10_wwn *t10_wwn = to_t10_wwn(item);
> +	struct se_device *dev = t10_wwn->t10_dev;
> +
> +	/* +2 to allow for a trailing (stripped) '\n' and null-terminator */
> +	unsigned char buf[PD_TEXT_ID_INFO_LEN + 2];
> +	char *stripped = NULL;
> +	ssize_t len;
> +
> +	len = strscpy(buf, page, sizeof(buf));
> +	if (len > 0) {

Can you just return early if len is too short?

> +		/* Strip any newline added from userspace. */
> +		stripped = strstrip(buf);
> +		len = strlen(stripped);
> +	}
> +
> +	if (len < 0 || len >= PD_TEXT_ID_INFO_LEN) {
> +		pr_err("Emulated peripheral device text id info exceeds"
> +			" PD_TEXT_ID_INFO_LEN: " __stringify(PD_TEXT_ID_INFO_LEN)
> +			"\n");

I think that this can all be a single line.

> +		return -EOVERFLOW;
> +	}
> +
> +	/*
> +	 * Check to see if any active exports exist.  If they do exist, fail
> +	 * here as changing this information on the fly (underneath the
> +	 * initiator side OS dependent multipath code) could cause negative
> +	 * effects.
> +	 */
> +	if (dev->export_count) {

I would check this before any of the string correctness

> +		pr_err("Unable to set the peripheral device text id info while"
> +			" active %d exports exist\n", dev->export_count);

I think that the following is better, i.e. keep fixed strings on same 
line to make grep'ing easier:

pr_err("Unable to set the peripheral device text id info while active %d 
exports exist\n",
	dev->export_count);

> +		return -EINVAL;
> +	}
> +
> +	BUILD_BUG_ON(sizeof(dev->t10_wwn.pd_text_id_info) != PD_TEXT_ID_INFO_LEN);
> +	strscpy(dev->t10_wwn.pd_text_id_info, stripped,
> +	       sizeof(dev->t10_wwn.pd_text_id_info));
> +
> +	pr_debug("Target_Core_ConfigFS: Set emulated peripheral dev text id info:"
> +		  " %s\n", dev->t10_wwn.pd_text_id_info);
> +
> +	return count;
> +}
> +
>   /*
>    * Generic wrapper for dumping VPD identifiers by association.
>    */
> @@ -1782,6 +1835,7 @@ CONFIGFS_ATTR_RO(target_wwn_, vpd_protocol_identifier);
>   CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
>   CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
>   CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
> +CONFIGFS_ATTR(target_wwn_, pd_text_id_info);
>   
>   static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
>   	&target_wwn_attr_vendor_id,
> @@ -1793,6 +1847,7 @@ static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
>   	&target_wwn_attr_vpd_assoc_logical_unit,
>   	&target_wwn_attr_vpd_assoc_target_port,
>   	&target_wwn_attr_vpd_assoc_scsi_target_device,
> +	&target_wwn_attr_pd_text_id_info,
>   	NULL,
>   };
>   
> diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
> index aad0096afa21..9fa84202ee4b 100644
> --- a/drivers/target/target_core_spc.c
> +++ b/drivers/target/target_core_spc.c
> @@ -1963,6 +1963,18 @@ static const struct target_opcode_descriptor tcm_opcode_report_supp_opcodes = {
>   	.enabled = spc_rsoc_enabled,
>   };
>   
> +static struct target_opcode_descriptor tcm_opcode_report_identifying_information = {
> +	.support = SCSI_SUPPORT_FULL,
> +	.serv_action_valid = 1,
> +	.opcode = MAINTENANCE_IN,
> +	.service_action = MI_REPORT_IDENTIFYING_INFORMATION,
> +	.cdb_size = 12,
> +	.usage_bits = {MAINTENANCE_IN, MI_REPORT_IDENTIFYING_INFORMATION,
> +		       0x00, 0x00,
> +		       0x00, 0x00, 0xff, 0xff,
> +		       0xff, 0xff, 0xff, SCSI_CONTROL_MASK},
> +};
> +
>   static bool tcm_is_set_tpg_enabled(const struct target_opcode_descriptor *descr,
>   				   struct se_cmd *cmd)
>   {
> @@ -2049,6 +2061,7 @@ static const struct target_opcode_descriptor *tcm_supported_opcodes[] = {
>   	&tcm_opcode_report_target_pgs,
>   	&tcm_opcode_report_supp_opcodes,
>   	&tcm_opcode_set_tpg,
> +	&tcm_opcode_report_identifying_information,
>   };
>   
>   static int
> @@ -2266,6 +2279,70 @@ spc_emulate_report_supp_op_codes(struct se_cmd *cmd)
>   	return ret;
>   }
>   
> +static sense_reason_t
> +spc_fill_pd_text_id_info(struct se_cmd *cmd, u8 *cdb)
> +{
> +	struct se_device *dev = cmd->se_dev;
> +	unsigned char *buf;
> +	unsigned char *rbuf;
> +	u32 header_len = 4;
> +	u32 actual_data_len;
> +	u32 buf_len;
> +	u16 id_len;
> +
> +	buf_len = get_unaligned_be32(&cdb[6]);
> +	if (buf_len < header_len)
> +		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;

isn't there a limit of 512 bytes for code 10b?

 > +> +	id_len = strlen(dev->t10_wwn.pd_text_id_info);
> +	if (id_len > 0)
> +		/* trailing null */
> +		id_len += 1;
> +
> +	actual_data_len = id_len + header_len;
> +
> +	if (actual_data_len < buf_len)
> +		buf_len = actual_data_len;
> +
> +	buf = kzalloc(buf_len, GFP_KERNEL);
> +	if (!buf) {
> +		pr_err("Unable to allocate response buffer for IDENTITY INFO\n");
> +		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> +	}
> +
> +	scnprintf(&buf[header_len], buf_len - header_len, "%s",
> +		 dev->t10_wwn.pd_text_id_info);
> +
> +	put_unaligned_be16(id_len, &buf[2]);
> +
> +	rbuf = transport_kmap_data_sg(cmd);

is it really ok if this returns NULL?

> +	if (rbuf) {
> +		memcpy(rbuf, buf, buf_len);
> +		transport_kunmap_data_sg(cmd);
> +	}
> +	kfree(buf);
> +
> +	target_complete_cmd_with_length(cmd, SAM_STAT_GOOD, buf_len);
> +	return TCM_NO_SENSE;
> +}
> +
> +static sense_reason_t
> +spc_emulate_report_id_info(struct se_cmd *cmd)
> +{
> +	u8 *cdb = cmd->t_task_cdb;
> +	sense_reason_t rc;
> +
> +	switch ((cdb[10] >> 1)) {
> +	case 2:
> +		rc = spc_fill_pd_text_id_info(cmd, cdb);
> +		break;
> +	default:
> +		return TCM_UNSUPPORTED_SCSI_OPCODE;
> +	}
> +
> +	return rc;

I don't know why default clause is required and not return result from 
spc_fill_pd_text_id_info() directly?

> +}
> +
>   sense_reason_t
>   spc_parse_cdb(struct se_cmd *cmd, unsigned int *size)
>   {
> @@ -2405,6 +2482,11 @@ spc_parse_cdb(struct se_cmd *cmd, unsigned int *size)
>   			    MI_REPORT_SUPPORTED_OPERATION_CODES)
>   				cmd->execute_cmd =
>   					spc_emulate_report_supp_op_codes;
> +			if ((cdb[1] & 0x1f) ==
> +			    MI_REPORT_IDENTIFYING_INFORMATION) {
> +				cmd->execute_cmd =
> +					spc_emulate_report_id_info;
> +			}
>   			*size = get_unaligned_be32(&cdb[6]);
>   		} else {
>   			/*
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index c4d9116904aa..c9b5edcce1eb 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -108,6 +108,9 @@
>   #define SE_MODE_PAGE_BUF			512
>   #define SE_SENSE_BUF				96
>   
> +/* Peripheral Device Text Identification Information */
> +#define PD_TEXT_ID_INFO_LEN			256

as above, I thought that it was 512 bytes

> +
>   enum target_submit_type {
>   	/* Use the fabric driver's default submission type */
>   	TARGET_FABRIC_DEFAULT_SUBMIT,
> @@ -347,6 +350,7 @@ struct t10_wwn {
>   	struct se_device *t10_dev;
>   	struct config_group t10_wwn_group;
>   	struct list_head t10_vpd_list;
> +	char pd_text_id_info[PD_TEXT_ID_INFO_LEN];
>   };
>   
>   struct t10_pr_registration {

RE: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[Gulam Mohamed]

Hi John,


Confidential- Oracle Internal
> -----Original Message-----
> From: John Garry <john.g.garry@oracle.com>
> Sent: Monday, November 17, 2025 9:49 PM
> To: Gulam Mohamed <gulam.mohamed@oracle.com>; Martin Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; target-
> devel@vger.kernel.org
> Subject: Re: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for
> REPORT_IDENTIFICATION_INFORMATION
>
> On 26/10/2025 19:13, Gulam Mohamed wrote:
>
> In the title, why "REPORT_IDENTIFICATION_INFORMATION" and not
> "REPORT_IDENTIFYING_INFORMATION"?
Sorry, this is by mistake. Will change it.
>
> > This patch
>
> git grep "This patch" Documentation/process/
>
> > will implement the REPORT_IDENTIFICATION_INFORMATION using the
> > configfs file pd_text_id_info in target core module. The configfs file
> > is created in /sys/kernel/config/target/core/<backend type>/
> > <backing_store_name>/wwn/. The user can set the peripheral device text
> > identification string to the file pd_text_id_info. An emulation
> > function "spc_emulate_report_id_info()" is defined in
> > target_core_spc.c which returns the device text id whenever the user
> requests the same.
>
> It would be nice to a mention a spec reference for the command.
Sure, will include it.
>
> >
> > Signed-off-by: Gulam Mohamed <gulam.mohamed@oracle.com>
>
> Generally the code below looks ok, but I'll be nitpicking a bit...
>
> > ---
> >   drivers/target/target_core_configfs.c | 55 ++++++++++++++++++
> >   drivers/target/target_core_spc.c      | 82 +++++++++++++++++++++++++++
> >   include/target/target_core_base.h     |  4 ++
> >   3 files changed, 141 insertions(+)
> >
> > diff --git a/drivers/target/target_core_configfs.c
> > b/drivers/target/target_core_configfs.c
> > index b19acd662726..ac78a106e0f3 100644
> > --- a/drivers/target/target_core_configfs.c
> > +++ b/drivers/target/target_core_configfs.c
> > @@ -1726,6 +1726,59 @@ static ssize_t
> target_wwn_vpd_protocol_identifier_show(struct config_item *item,
> >     return len;
> >   }
> >
> > +static ssize_t target_wwn_pd_text_id_info_show(struct config_item *item,
> > +           char *page)
> > +{
> > +   return sprintf(page, "%s\n", &to_t10_wwn(item)-
> >pd_text_id_info[0]);
> > +}
> > +
> > +static ssize_t target_wwn_pd_text_id_info_store(struct config_item *item,
> > +           const char *page, size_t count)
> > +{
> > +   struct t10_wwn *t10_wwn = to_t10_wwn(item);
> > +   struct se_device *dev = t10_wwn->t10_dev;
> > +
> > +   /* +2 to allow for a trailing (stripped) '\n' and null-terminator */
> > +   unsigned char buf[PD_TEXT_ID_INFO_LEN + 2];
> > +   char *stripped = NULL;
> > +   ssize_t len;
> > +
> > +   len = strscpy(buf, page, sizeof(buf));
> > +   if (len > 0) {
>
> Can you just return early if len is too short?
You mean to return before we check "if (len > 0)"?

>
> > +           /* Strip any newline added from userspace. */
> > +           stripped = strstrip(buf);
> > +           len = strlen(stripped);
> > +   }
> > +
> > +   if (len < 0 || len >= PD_TEXT_ID_INFO_LEN) {
> > +           pr_err("Emulated peripheral device text id info exceeds"
> > +                   " PD_TEXT_ID_INFO_LEN: "
> __stringify(PD_TEXT_ID_INFO_LEN)
> > +                   "\n");
>
> I think that this can all be a single line.
>
Actually here I followed the existing code style. Mike also wanted to restrict the columns per line to be 80.
Let me know if you want me to change this.
> > +           return -EOVERFLOW;
> > +   }
> > +
> > +   /*
> > +    * Check to see if any active exports exist.  If they do exist, fail
> > +    * here as changing this information on the fly (underneath the
> > +    * initiator side OS dependent multipath code) could cause negative
> > +    * effects.
> > +    */
> > +   if (dev->export_count) {
>
> I would check this before any of the string correctness

Yes, I think its good to do a check before string correctness. Will change this.
>
> > +           pr_err("Unable to set the peripheral device text id info while"
> > +                   " active %d exports exist\n", dev->export_count);
>
> I think that the following is better, i.e. keep fixed strings on same line to make
> grep'ing easier:
>
> pr_err("Unable to set the peripheral device text id info while active %d
> exports exist\n",
>       dev->export_count);
>
Sure. Will change this instance. This also I followed the number of columns to not cross 80.

> > +           return -EINVAL;
> > +   }
> > +
> > +   BUILD_BUG_ON(sizeof(dev->t10_wwn.pd_text_id_info) !=
> PD_TEXT_ID_INFO_LEN);
> > +   strscpy(dev->t10_wwn.pd_text_id_info, stripped,
> > +          sizeof(dev->t10_wwn.pd_text_id_info));
> > +
> > +   pr_debug("Target_Core_ConfigFS: Set emulated peripheral dev text id
> info:"
> > +             " %s\n", dev->t10_wwn.pd_text_id_info);
> > +
> > +   return count;
> > +}
> > +
> >   /*
> >    * Generic wrapper for dumping VPD identifiers by association.
> >    */
> > @@ -1782,6 +1835,7 @@ CONFIGFS_ATTR_RO(target_wwn_,
> vpd_protocol_identifier);
> >   CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_logical_unit);
> >   CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_target_port);
> >   CONFIGFS_ATTR_RO(target_wwn_, vpd_assoc_scsi_target_device);
> > +CONFIGFS_ATTR(target_wwn_, pd_text_id_info);
> >
> >   static struct configfs_attribute *target_core_dev_wwn_attrs[] = {
> >     &target_wwn_attr_vendor_id,
> > @@ -1793,6 +1847,7 @@ static struct configfs_attribute
> *target_core_dev_wwn_attrs[] = {
> >     &target_wwn_attr_vpd_assoc_logical_unit,
> >     &target_wwn_attr_vpd_assoc_target_port,
> >     &target_wwn_attr_vpd_assoc_scsi_target_device,
> > +   &target_wwn_attr_pd_text_id_info,
> >     NULL,
> >   };
> >
> > diff --git a/drivers/target/target_core_spc.c
> > b/drivers/target/target_core_spc.c
> > index aad0096afa21..9fa84202ee4b 100644
> > --- a/drivers/target/target_core_spc.c
> > +++ b/drivers/target/target_core_spc.c
> > @@ -1963,6 +1963,18 @@ static const struct target_opcode_descriptor
> tcm_opcode_report_supp_opcodes = {
> >     .enabled = spc_rsoc_enabled,
> >   };
> >
> > +static struct target_opcode_descriptor
> tcm_opcode_report_identifying_information = {
> > +   .support = SCSI_SUPPORT_FULL,
> > +   .serv_action_valid = 1,
> > +   .opcode = MAINTENANCE_IN,
> > +   .service_action = MI_REPORT_IDENTIFYING_INFORMATION,
> > +   .cdb_size = 12,
> > +   .usage_bits = {MAINTENANCE_IN,
> MI_REPORT_IDENTIFYING_INFORMATION,
> > +                  0x00, 0x00,
> > +                  0x00, 0x00, 0xff, 0xff,
> > +                  0xff, 0xff, 0xff, SCSI_CONTROL_MASK}, };
> > +
> >   static bool tcm_is_set_tpg_enabled(const struct target_opcode_descriptor
> *descr,
> >                                struct se_cmd *cmd)
> >   {
> > @@ -2049,6 +2061,7 @@ static const struct target_opcode_descriptor
> *tcm_supported_opcodes[] = {
> >     &tcm_opcode_report_target_pgs,
> >     &tcm_opcode_report_supp_opcodes,
> >     &tcm_opcode_set_tpg,
> > +   &tcm_opcode_report_identifying_information,
> >   };
> >
> >   static int
> > @@ -2266,6 +2279,70 @@ spc_emulate_report_supp_op_codes(struct
> se_cmd *cmd)
> >     return ret;
> >   }
> >
> > +static sense_reason_t
> > +spc_fill_pd_text_id_info(struct se_cmd *cmd, u8 *cdb) {
> > +   struct se_device *dev = cmd->se_dev;
> > +   unsigned char *buf;
> > +   unsigned char *rbuf;
> > +   u32 header_len = 4;
> > +   u32 actual_data_len;
> > +   u32 buf_len;
> > +   u16 id_len;
> > +
> > +   buf_len = get_unaligned_be32(&cdb[6]);
> > +   if (buf_len < header_len)
> > +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>
> isn't there a limit of 512 bytes for code 10b?
Earlier I kept this as 512 bytes as the userspace is sending the size as 512, but Mike pointed out the range mentioned in the specification.
So, If we see the Table 61 (Table 61 — Identifying information types) of "spc4r37", it says 0-256 bytes for "Peripheral device text identifying information".
Let me know your comments.
>
>  > +> +       id_len = strlen(dev->t10_wwn.pd_text_id_info);
> > +   if (id_len > 0)
> > +           /* trailing null */
> > +           id_len += 1;
> > +
> > +   actual_data_len = id_len + header_len;
> > +
> > +   if (actual_data_len < buf_len)
> > +           buf_len = actual_data_len;
> > +
> > +   buf = kzalloc(buf_len, GFP_KERNEL);
> > +   if (!buf) {
> > +           pr_err("Unable to allocate response buffer for IDENTITY
> INFO\n");
> > +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> > +   }
> > +
> > +   scnprintf(&buf[header_len], buf_len - header_len, "%s",
> > +            dev->t10_wwn.pd_text_id_info);
> > +
> > +   put_unaligned_be16(id_len, &buf[2]);
> > +
> > +   rbuf = (cmd);
>
> is it really ok if this returns NULL?
I think its ok to return NULL.  We just don't send any information. I kept it like this because I don't see any information defined by protocol when the memory mapping fails.
Mike, Can you please correct me if I am missing anything?
>
> > +   if (rbuf) {
> > +           memcpy(rbuf, buf, buf_len);
> > +           transport_kunmap_data_sg(cmd);
> > +   }
> > +   kfree(buf);
> > +
> > +   target_complete_cmd_with_length(cmd, SAM_STAT_GOOD, buf_len);
> > +   return TCM_NO_SENSE;
> > +}
> > +
> > +static sense_reason_t
> > +spc_emulate_report_id_info(struct se_cmd *cmd) {
> > +   u8 *cdb = cmd->t_task_cdb;
> > +   sense_reason_t rc;
> > +
> > +   switch ((cdb[10] >> 1)) {
> > +   case 2:
> > +           rc = spc_fill_pd_text_id_info(cmd, cdb);
> > +           break;
> > +   default:
> > +           return TCM_UNSUPPORTED_SCSI_OPCODE;
> > +   }
> > +
> > +   return rc;
>
> I don't know why default clause is required and not return result from
> spc_fill_pd_text_id_info() directly?

Default case is to handle the incorrect information type. As of now we are using the 10b code but in future, we may need to handle others.
>
> > +}
> > +
> >   sense_reason_t
> >   spc_parse_cdb(struct se_cmd *cmd, unsigned int *size)
> >   {
> > @@ -2405,6 +2482,11 @@ spc_parse_cdb(struct se_cmd *cmd, unsigned
> int *size)
> >                         MI_REPORT_SUPPORTED_OPERATION_CODES)
> >                             cmd->execute_cmd =
> >                                     spc_emulate_report_supp_op_codes;
> > +                   if ((cdb[1] & 0x1f) ==
> > +                       MI_REPORT_IDENTIFYING_INFORMATION) {
> > +                           cmd->execute_cmd =
> > +                                   spc_emulate_report_id_info;
> > +                   }
> >                     *size = get_unaligned_be32(&cdb[6]);
> >             } else {
> >                     /*
> > diff --git a/include/target/target_core_base.h
> > b/include/target/target_core_base.h
> > index c4d9116904aa..c9b5edcce1eb 100644
> > --- a/include/target/target_core_base.h
> > +++ b/include/target/target_core_base.h
> > @@ -108,6 +108,9 @@
> >   #define SE_MODE_PAGE_BUF                  512
> >   #define SE_SENSE_BUF                              96
> >
> > +/* Peripheral Device Text Identification Information */
> > +#define PD_TEXT_ID_INFO_LEN                        256
>
> as above, I thought that it was 512 bytes
>
As mentioned above, protocol specifies 0-256 bytes
> > +
> >   enum target_submit_type {
> >     /* Use the fabric driver's default submission type */
> >     TARGET_FABRIC_DEFAULT_SUBMIT,
> > @@ -347,6 +350,7 @@ struct t10_wwn {
> >     struct se_device *t10_dev;
> >     struct config_group t10_wwn_group;
> >     struct list_head t10_vpd_list;
> > +   char pd_text_id_info[PD_TEXT_ID_INFO_LEN];
> >   };
> >
> >   struct t10_pr_registration {

Re: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[Mike Christie]

On 11/19/25 1:58 PM, Gulam Mohamed wrote:
>>> +   buf = kzalloc(buf_len, GFP_KERNEL);
>>> +   if (!buf) {
>>> +           pr_err("Unable to allocate response buffer for IDENTITY
>> INFO\n");
>>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>> +   }
>>> +
>>> +   scnprintf(&buf[header_len], buf_len - header_len, "%s",
>>> +            dev->t10_wwn.pd_text_id_info);
>>> +
>>> +   put_unaligned_be16(id_len, &buf[2]);
>>> +
>>> +   rbuf = (cmd);
>>
>> is it really ok if this returns NULL?
> I think its ok to return NULL.  We just don't send any information. I kept it like this because I don't see any information defined by protocol when the memory mapping fails.
> Mike, Can you please correct me if I am missing anything?

It's probably best to return a failure so the caller knows that
we might have a ID but we can't return it. It will also match the
case above when we can't allocate memory.

Re: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[John Garry]

>>> +
>>> +   len = strscpy(buf, page, sizeof(buf));
>>> +   if (len > 0) {
>>
>> Can you just return early if len is too short?
> You mean to return before we check "if (len > 0)"?

I'm not sure. It's just that the checks seem odd to me. Firstly could 
you do a E2BIG check after strscpy (by checking len < 0)?

I think that logically the following is the same, but it is slightly 
neater. Please check it.

	if (strscpy(buf, page, sizeof(buf) < 0)
		return -EOVERFLOW;
	stripped = strstrip(buf);
	if (strlen(stripped) >= PD_TEXT_ID_INFO_LEN)
		return -EOVERFLOW;

> 
>>
>>> +           /* Strip any newline added from userspace. */
>>> +           stripped = strstrip(buf);
>>> +           len = strlen(stripped);
>>> +   }
>>> +
>>> +   if (len < 0 || len >= PD_TEXT_ID_INFO_LEN) {
>>> +           pr_err("Emulated peripheral device text id info exceeds"
>>> +                   " PD_TEXT_ID_INFO_LEN: "
>> __stringify(PD_TEXT_ID_INFO_LEN)
>>> +                   "\n");
>>
>> I think that this can all be a single line.
>>
> Actually here I followed the existing code style. Mike also wanted to restrict the columns per line to be 80.
> Let me know if you want me to change this.

The 80 char limit does not apply to strings, but you are using 
__stringify (so a grey area). Please disregard my comment if you like.

>>> +           return -EOVERFLOW;
>>> +   }
>>> +
>>> +   /*
>>> +    * Check to see if any active exports exist.  If they do exist, fail
>>> +    * here as changing this information on the fly (underneath the
>>> +    * initiator side OS dependent multipath code) could cause negative
>>> +    * effects.
>>> +    */
>>> +   if (dev->export_count) {

...

>>> +static sense_reason_t
>>> +spc_fill_pd_text_id_info(struct se_cmd *cmd, u8 *cdb) {
>>> +   struct se_device *dev = cmd->se_dev;
>>> +   unsigned char *buf;
>>> +   unsigned char *rbuf;
>>> +   u32 header_len = 4;
>>> +   u32 actual_data_len;
>>> +   u32 buf_len;
>>> +   u16 id_len;
>>> +
>>> +   buf_len = get_unaligned_be32(&cdb[6]);
>>> +   if (buf_len < header_len)
>>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>
>> isn't there a limit of 512 bytes for code 10b?
> Earlier I kept this as 512 bytes as the userspace is sending the size as 512, but Mike pointed out the range mentioned in the specification.
> So, If we see the Table 61 (Table 61 — Identifying information types) of "spc4r37", it says 0-256 bytes for "Peripheral device text identifying information".
> Let me know your comments.

ok, I was checking Peripheral device identifying information.

>>
>>   > +> +       id_len = strlen(dev->t10_wwn.pd_text_id_info);
>>> +   if (id_len > 0)
>>> +           /* trailing null */
>>> +           id_len += 1;
>>> +
>>> +   actual_data_len = id_len + header_len;
>>> +
>>> +   if (actual_data_len < buf_len)
>>> +           buf_len = actual_data_len;
>>> +
>>> +   buf = kzalloc(buf_len, GFP_KERNEL);
>>> +   if (!buf) {
>>> +           pr_err("Unable to allocate response buffer for IDENTITY
>> INFO\n");
>>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>> +   }
>>> +
>>> +   scnprintf(&buf[header_len], buf_len - header_len, "%s",
>>> +            dev->t10_wwn.pd_text_id_info);
>>> +
>>> +   put_unaligned_be16(id_len, &buf[2]);
>>> +
>>> +   rbuf = transport_kmap_data_sg(cmd);
>>
>> is it really ok if this returns NULL?
> I think its ok to return NULL.  We just don't send any information. I kept it like this because I don't see any information defined by protocol when the memory mapping fails.

If this fails, then we don't supply the pd_text_id_info but we are 
setting the len in the put_unaligned_be16(id_len, &buf[2]) call, which 
seems improper.

> Mike, Can you please correct me if I am missing anything?
>>
>>> +   if (rbuf) {
>>> +           memcpy(rbuf, buf, buf_len);
>>> +           transport_kunmap_data_sg(cmd);
>>> +   }
>>> +   kfree(buf);
>>> +
>>> +   target_complete_cmd_with_length(cmd, SAM_STAT_GOOD, buf_len);
>>> +   return TCM_NO_SENSE;
>>> +}
>>> +
>>> +static sense_reason_t
>>> +spc_emulate_report_id_info(struct se_cmd *cmd) {
>>> +   u8 *cdb = cmd->t_task_cdb;
>>> +   sense_reason_t rc;
>>> +
>>> +   switch ((cdb[10] >> 1)) {
>>> +   case 2:
>>> +           rc = spc_fill_pd_text_id_info(cmd, cdb);
>>> +           break;
>>> +   default:
>>> +           return TCM_UNSUPPORTED_SCSI_OPCODE;
>>> +   }
>>> +
>>> +   return rc;
>>
>> I don't know why default clause is required and not return result from
>> spc_fill_pd_text_id_info() directly?
> 
> Default case is to handle the incorrect information type. As of now we are using the 10b code but in future, we may need to handle others.

ok...

RE: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[Gulam Mohamed]

Hi John,


Confidential- Oracle Internal
> -----Original Message-----
> From: John Garry <john.g.garry@oracle.com>
> Sent: Wednesday, November 26, 2025 5:24 PM
> To: Gulam Mohamed <gulam.mohamed@oracle.com>; Martin Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; target-
> devel@vger.kernel.org
> Subject: Re: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for
> REPORT_IDENTIFICATION_INFORMATION
>
>
> >>> +
> >>> +   len = strscpy(buf, page, sizeof(buf));
> >>> +   if (len > 0) {
> >>
> >> Can you just return early if len is too short?
> > You mean to return before we check "if (len > 0)"?
>
> I'm not sure. It's just that the checks seem odd to me. Firstly could you do a
> E2BIG check after strscpy (by checking len < 0)?
>
> I think that logically the following is the same, but it is slightly neater. Please
> check it.
>
>       if (strscpy(buf, page, sizeof(buf) < 0)
>               return -EOVERFLOW;
>       stripped = strstrip(buf);
>       if (strlen(stripped) >= PD_TEXT_ID_INFO_LEN)
>               return -EOVERFLOW;
>
> >
Thanks John. I got it now. Will change it.

> >>
> >>> +           /* Strip any newline added from userspace. */
> >>> +           stripped = strstrip(buf);
> >>> +           len = strlen(stripped);
> >>> +   }
> >>> +
> >>> +   if (len < 0 || len >= PD_TEXT_ID_INFO_LEN) {
> >>> +           pr_err("Emulated peripheral device text id info exceeds"
> >>> +                   " PD_TEXT_ID_INFO_LEN: "
> >> __stringify(PD_TEXT_ID_INFO_LEN)
> >>> +                   "\n");
> >>
> >> I think that this can all be a single line.
> >>
> > Actually here I followed the existing code style. Mike also wanted to restrict
> the columns per line to be 80.
> > Let me know if you want me to change this.
>
> The 80 char limit does not apply to strings, but you are using __stringify (so a
> grey area). Please disregard my comment if you like.
Ok. __stringify() converts to a string. Will include this in a single line
>
> >>> +           return -EOVERFLOW;
> >>> +   }
> >>> +
> >>> +   /*
> >>> +    * Check to see if any active exports exist.  If they do exist, fail
> >>> +    * here as changing this information on the fly (underneath the
> >>> +    * initiator side OS dependent multipath code) could cause negative
> >>> +    * effects.
> >>> +    */
> >>> +   if (dev->export_count) {
>
> ...
>
> >>> +static sense_reason_t
> >>> +spc_fill_pd_text_id_info(struct se_cmd *cmd, u8 *cdb) {
> >>> +   struct se_device *dev = cmd->se_dev;
> >>> +   unsigned char *buf;
> >>> +   unsigned char *rbuf;
> >>> +   u32 header_len = 4;
> >>> +   u32 actual_data_len;
> >>> +   u32 buf_len;
> >>> +   u16 id_len;
> >>> +
> >>> +   buf_len = get_unaligned_be32(&cdb[6]);
> >>> +   if (buf_len < header_len)
> >>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> >>
> >> isn't there a limit of 512 bytes for code 10b?
> > Earlier I kept this as 512 bytes as the userspace is sending the size as 512,
> but Mike pointed out the range mentioned in the specification.
> > So, If we see the Table 61 (Table 61 — Identifying information types) of
> "spc4r37", it says 0-256 bytes for "Peripheral device text identifying
> information".
> > Let me know your comments.
>
> ok, I was checking Peripheral device identifying information.
>
> >>
> >>   > +> +       id_len = strlen(dev->t10_wwn.pd_text_id_info);
> >>> +   if (id_len > 0)
> >>> +           /* trailing null */
> >>> +           id_len += 1;
> >>> +
> >>> +   actual_data_len = id_len + header_len;
> >>> +
> >>> +   if (actual_data_len < buf_len)
> >>> +           buf_len = actual_data_len;
> >>> +
> >>> +   buf = kzalloc(buf_len, GFP_KERNEL);
> >>> +   if (!buf) {
> >>> +           pr_err("Unable to allocate response buffer for IDENTITY
> >> INFO\n");
> >>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> >>> +   }
> >>> +
> >>> +   scnprintf(&buf[header_len], buf_len - header_len, "%s",
> >>> +            dev->t10_wwn.pd_text_id_info);
> >>> +
> >>> +   put_unaligned_be16(id_len, &buf[2]);
> >>> +
> >>> +   rbuf = transport_kmap_data_sg(cmd);
> >>
> >> is it really ok if this returns NULL?
> > I think its ok to return NULL.  We just don't send any information. I kept it
> like this because I don't see any information defined by protocol when the
> memory mapping fails.
>
> If this fails, then we don't supply the pd_text_id_info but we are setting the
> len in the put_unaligned_be16(id_len, &buf[2]) call, which seems improper.
>
Ok. Returning an error seems to be the correct way.
> > Mike, Can you please correct me if I am missing anything?
> >>
> >>> +   if (rbuf) {
> >>> +           memcpy(rbuf, buf, buf_len);
> >>> +           transport_kunmap_data_sg(cmd);
> >>> +   }
> >>> +   kfree(buf);
> >>> +
> >>> +   target_complete_cmd_with_length(cmd, SAM_STAT_GOOD, buf_len);
> >>> +   return TCM_NO_SENSE;
> >>> +}
> >>> +
> >>> +static sense_reason_t
> >>> +spc_emulate_report_id_info(struct se_cmd *cmd) {
> >>> +   u8 *cdb = cmd->t_task_cdb;
> >>> +   sense_reason_t rc;
> >>> +
> >>> +   switch ((cdb[10] >> 1)) {
> >>> +   case 2:
> >>> +           rc = spc_fill_pd_text_id_info(cmd, cdb);
> >>> +           break;
> >>> +   default:
> >>> +           return TCM_UNSUPPORTED_SCSI_OPCODE;
> >>> +   }
> >>> +
> >>> +   return rc;
> >>
> >> I don't know why default clause is required and not return result
> >> from
> >> spc_fill_pd_text_id_info() directly?
> >
> > Default case is to handle the incorrect information type. As of now we are
> using the 10b code but in future, we may need to handle others.
>
> ok...

Will send the new version V2 after making all these changes and testing it.

Regards,
Gulam Mohamed.