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

[John Garry <john.g.garry@oracle.com>]

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 {