Improve ata_scsi_report_supported_opcodes()

The helper function ata_dev_is_zac() checks if a device is a ZAC class device (host managed zoned disk) or if it is a host aware zoned disk, that is, a regular ATA disk that supports the zoned capabilities. So the name of this helper function is confusing as it hints at the first case only.

Rename this helper function to ata_dev_is_zoned() to avoid confusions and better reflect the two cases tested.

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-core.c | 6 +++---
  drivers/ata/libata-scsi.c | 7 +++----
  drivers/ata/libata.h      | 2 +-
  3 files changed, 7 insertions(+), 8 deletions(-)

ata_scsi_report_supported_opcodes() is very limited in functionality as it lacks support for the all command format and also does not handle correctly commands that have a service action.

In preparation for adding these missing features, refactor how ata_scsi_report_supported_opcodes() operates to make modifications and extensions easier. To do so, introduce the array of supported commands ata_supported_cmds. This array entries are of type struct ata_scsi_cmd. This structure stores the operation code, CDB length, and the service action of a supported SCSI command that libata SAT can translate or emulate. Since some service actions (e.g. ZI_REPORT_ZONES) can have a value of 0, the field sa_valid of struct ata_scsi_cmd is used to indicate if the sa field is valid, or if it should be ignored.

The helper function ata_scsi_get_supported_cmd() is implemented to search for a particular command by opcode in this array. This function is used in ata_scsi_cmd_is_supported() together with a struct ata_scsi_cmd_support to check based on the target device features if the specified command is supported.

ata_scsi_cmd_is_supported() is used as the main function in ata_scsi_report_supported_opcodes() to determine if a particular command is supported and fill the command reply rbuf as needed. In the case of a command that is not supported, the support field is set to 1 as specified in SPC, indicating that the command is not supported.

Of note is that the old ata_scsi_report_supported_opcodes() code did not handle the WRITE_SAME_16 and VARIABLE_LENGTH_CMD commands which are both supported and translated by libata-scsi. The ata_supported_cmds array includes these commands.

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-scsi.c | 164 +++++++++++++++++++++++++-------------
  1 file changed, 110 insertions(+), 54 deletions(-)

drivers/ata/libata-scsi.c +41 -3

index 8811bfc37369..4f0ae44f59dd 100644
@@ -3590,87 +3590,143 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
  	return 1;
  }
 
-static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
-						      struct scsi_cmnd *cmd,
-						      u8 *rbuf)
+struct ata_scsi_cmd {
+	u8 op;
+	u8 cdb_len;
+	bool sa_valid;
+	u16 sa;
+};
+
+/*
+ * Array of commands supported with translation or emulation, sorted in
+ * ascending opcode and service action order. All of these commands are
+ * processed either in ata_xlat_func() or in ata_scsi_simulate();
+ */
+static const struct ata_scsi_cmd ata_supported_cmds[] = {
+	{ TEST_UNIT_READY,	 6,	false,	0 },
+	{ REZERO_UNIT,		 6,	false,	0 },
+	{ REQUEST_SENSE,	 6,	false,	0 },
+	{ READ_6,		 6,	false,	0 },
+	{ WRITE_6,		 6,	false,	0 },
+	{ SEEK_6,		 6,	false,	0 },
+	{ INQUIRY,		 6,	false,	0 },
+	{ MODE_SELECT,		 6,	false,	0 },
+	{ MODE_SENSE,		 6,	false,	0 },
+	{ START_STOP,		 6,	false,	0 },
+	{ SEND_DIAGNOSTIC,	 6,	false,	0 },
+	{ READ_CAPACITY,	10,	false,	0 },
+	{ READ_10,		10,	false,	0 },
+	{ WRITE_10,		10,	false,	0 },
+	{ SEEK_10,		10,	false,	0 },
+	{ VERIFY,		10,	false,	0 },
+	{ SYNCHRONIZE_CACHE,	10,	false,	0 },
+	{ MODE_SELECT_10,	10,	false,	0 },
+	{ MODE_SENSE_10,	10,	false,	0 },
+	{ VARIABLE_LENGTH_CMD,	32,	true,	ATA_32 },
+	{ ATA_16,		16,	false,	0 },
+	{ READ_16,		16,	false,	0 },
+	{ WRITE_16,		16,	false,	0 },
+	{ VERIFY_16,		16,	false,	0 },
+	{ SYNCHRONIZE_CACHE_16,	16,	false,	0 },
+	{ WRITE_SAME_16,	16,	false,	0 },
+	{ ZBC_OUT,		16,	true,	ZO_CLOSE_ZONE },
+	{ ZBC_OUT,		16,	true,	ZO_FINISH_ZONE },

drivers/ata/libata-scsi.c +53 -50

+
+static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op)
  {
-	u8 *cdb = cmd->cmnd;
-	u8 supported = 0, cdlp = 0, rwcdlp = 0;
+	const struct ata_scsi_cmd *cmd;
+	int i;
 
-	if (cdb[2] != 1 && cdb[2] != 3) {
-		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
-		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
-		return 0;
+	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
+		cmd = &ata_supported_cmds[i];
+		if (cmd->op == op)
+			return cmd;
  	}
 
-	switch (cdb[3]) {
-	case INQUIRY:
-	case MODE_SENSE:
-	case MODE_SENSE_10:
-	case READ_CAPACITY:
-	case SERVICE_ACTION_IN_16:
-	case REPORT_LUNS:
-	case REQUEST_SENSE:
-	case SYNCHRONIZE_CACHE:
-	case SYNCHRONIZE_CACHE_16:
-	case REZERO_UNIT:
-	case SEEK_6:
-	case SEEK_10:
-	case TEST_UNIT_READY:
-	case SEND_DIAGNOSTIC:
-	case MAINTENANCE_IN:
-	case READ_6:
-	case READ_10:
-	case WRITE_6:
-	case WRITE_10:
-	case ATA_12:
-	case ATA_16:
-	case VERIFY:
-	case VERIFY_16:
-	case MODE_SELECT:
-	case MODE_SELECT_10:
-	case START_STOP:
-		supported = 3;
-		break;
+	return NULL;
+}
+
+struct ata_scsi_cmd_support {
+	u8 cdlp;
+	u8 rwcdlp;
+};
+
+static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op,
+				      struct ata_scsi_cmd_support *sup)
+{
+	const struct ata_scsi_cmd *cmd;
+
+	/* First, see if we support the command. */
+	cmd = ata_scsi_get_supported_cmd(op);
+	if (!cmd)
+		return false;
+
+	/* Now refine the support report depending on the device features. */
+	memset(sup, 0, sizeof(*sup));
+	switch (op) {
  	case READ_16:
-		supported = 3;
  		if (dev->flags & ATA_DFLAG_CDL) {
  			/*
  			 * CDL read descriptors map to the T2A page, that is,
  			 * rwcdlp = 0x01 and cdlp = 0x01
  			 */
-			rwcdlp = 0x01;
-			cdlp = 0x01 << 3;
+			sup->rwcdlp = 0x01;
+			sup->cdlp = 0x01;
  		}
-		break;
+		return true;
  	case WRITE_16:
-		supported = 3;
  		if (dev->flags & ATA_DFLAG_CDL) {
  			/*
  			 * CDL write descriptors map to the T2B page, that is,
  			 * rwcdlp = 0x01 and cdlp = 0x02
  			 */
-			rwcdlp = 0x01;
-			cdlp = 0x02 << 3;
+			sup->rwcdlp = 0x01;
+			sup->cdlp = 0x02;
  		}
-		break;
+		return true;
  	case ZBC_IN:
  	case ZBC_OUT:
-		if (ata_dev_is_zoned(dev))
-			supported = 3;
-		break;
+		return ata_dev_is_zoned(dev);
  	case SECURITY_PROTOCOL_IN:
  	case SECURITY_PROTOCOL_OUT:
-		if (dev->flags & ATA_DFLAG_TRUSTED)
-			supported = 3;
-		break;
+		return dev->flags & ATA_DFLAG_TRUSTED;
  	default:
  		break;
  	}
 
+	return true;
+}
+
+static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
+						      struct scsi_cmnd *cmd,
+						      u8 *rbuf)
+{
+	struct ata_scsi_cmd_support sup;
+	u8 *cdb = cmd->cmnd;
+
+	if (cdb[2] != 1 && cdb[2] != 3) {
+		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
+		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
+		return 0;
+	}
+
  	/* One command format */
-	rbuf[0] = rwcdlp;

drivers/ata/libata-scsi.c static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc) · line 3643

Quoted 6 lines

drivers/ata/libata-scsi.c +6 -0

+	{ REPORT_LUNS,		12,	false,	0 },
+	{ ATA_12,		12,	false,	0 },
+	{ SECURITY_PROTOCOL_IN,	12,	false,	0 },
+	{ MAINTENANCE_IN,	12,	true,	MI_REPORT_SUPPORTED_OPERATION_CODES },
+	{ SECURITY_PROTOCOL_OUT, 12,	false,	0 },
+};

This probably is a tough ask, but wouldn't it be better to move to named initializer?
IE things like

{ .op = TEST_UNIT_READY, .cdb_len = 0, .sa_valid = false, .sa = 0 },

that will not only make it easier to read, but will also be more user friendly if someone needs to expand the structure.

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] WRITE_SAME_16 is unconditionally reported as supported via REPORT SUPPORTED OPERATION CODES, even for devices that do not support TRIM. --

commit b8e6e9f4e6e3177fe0e766704af6b504468c2de3
Author: Damien Le Moal <[email protected]>

ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

This commit refactors ata_scsi_report_supported_opcodes() to use an array of supported commands for easier modifications and extensions. It also adds handling for WRITE_SAME_16 and VARIABLE_LENGTH_CMD to the list of commands which libata-scsi translates.

Quoted 4 lines

drivers/ata/libata-scsi.c +0 -0

index 8811bfc373697..4f0ae44f59dd2 100644

[ ... ]

Quoted 14 lines

drivers/ata/libata-scsi.c +14 -0

+static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op,
+				      struct ata_scsi_cmd_support *sup)
+{
+	const struct ata_scsi_cmd *cmd;
+
+	/* First, see if we support the command. */
+	cmd = ata_scsi_get_supported_cmd(op);
+	if (!cmd)
+		return false;
+
+	/* Now refine the support report depending on the device features. */
+	memset(sup, 0, sizeof(*sup));
+	switch (op) {
+	case READ_16:

[ ... ]

drivers/ata/libata-scsi.c

Quoted 12 lines

drivers/ata/libata-scsi.c +12 -0

+	case ZBC_IN:
+	case ZBC_OUT:
+		return ata_dev_is_zoned(dev);
+	case SECURITY_PROTOCOL_IN:
+	case SECURITY_PROTOCOL_OUT:
+		return dev->flags & ATA_DFLAG_TRUSTED;
+	default:
+		break;
+	}
+
+	return true;
+}

[Severity: Medium]
Should WRITE_SAME_16 be refined here to check for TRIM support?

Who still cares about TRIM on ATA these days to bother with any of this?

ata_scsi_report_supported_opcodes() is ignoring the service action specified in the SCSI command CDB, but the one command format must take this field into consideration. With the reporting options field set to 1, the REPORT SUPPORTED OPERATION CODES command must be failed if the specified opcode to check has service actions, while reporting option 3 must match supported opcodes together with the specified service action.

Stop ignoring the service action by passing it to ata_scsi_cmd_is_supported() and searching for commands in the array of supported commands (ata_supported_cmds) using both the command opcode and service action.

Introduce the helper function ata_scsi_supported_cmd_use_sa() to determine if a particular command has service actions and use this function to fail a REPORT SUPPORTED OPERATION CODES command if such command is specified with reporting options 1.

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-scsi.c | 41 ++++++++++++++++++++++++++++++++-------
  1 file changed, 34 insertions(+), 7 deletions(-)

Add support for the reporting options value 2 of the in REPORT SUPPORTED OPERATION CODES command in ata_scsi_report_supported_opcodes(). With this reporting option, the specified opcode to check must have a service action. Use ata_scsi_supported_cmd_has_sa() to check for this and fill the command reply buffer using the one command format based on the command support.

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-scsi.c | 7 +++++++
  1 file changed, 7 insertions(+)

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Reporting options 1 and 2 incorrectly terminate with CHECK CONDITION (INVALID FIELD IN CDB) for valid queries, violating SCSI protocol.
- [Medium] Reporting option 3 incorrectly evaluates the REQUESTED SERVICE ACTION field for commands that do not use service actions. --

commit 8b0fcbdec2fbb5690930a99f6f8a7faf9e8bd511
Author: Damien Le Moal <[email protected]>

ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES

Add support for the reporting options value 2 of the in REPORT SUPPORTED OPERATION CODES command in ata_scsi_report_supported_opcodes(). With this reporting option, the specified opcode to check must have a service action. Use ata_scsi_supported_cmd_has_sa() to check for this and fill the command reply buffer using the one command format based on the command support.

Quoted 5 lines

drivers/ata/libata-scsi.c +0 -0

index 86ac41a7d81ab..e75ed573e234b 100644

drivers/ata/libata-scsi.c static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev, · line 3745

Quoted 9 lines

drivers/ata/libata-scsi.c +6 -0

@@ -3737,6 +3737,13 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
 			return 0;
 		}
 		break;
+	case 2:
+		/* One command format, must have sa. */
+		if (!ata_scsi_supported_cmd_use_sa(cdb[3])) {
+			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
+			return 0;
+		}

[Severity: Medium]
Does this validation check incorrectly abort execution with a SCSI error for option 2 commands that are unknown?

According to SPC-5, option 2 queries for unsupported commands (and similarly, option 1 queries for commands with service actions) must return SUPPORT=001b. Instead, this implementation erroneously aborts the command by calling ata_scsi_set_invalid_field(), preventing the code from falling through and correctly populating the response buffer.

drivers/ata/libata-scsi.c static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev, · line 3749

Quoted 5 lines

drivers/ata/libata-scsi.c +1 -0

+		fallthrough;
 	case 3:
 		/* One command format */
 		sa = get_unaligned_be16(&cdb[4]);

[Severity: Medium]
If an initiator issues REPORT SUPPORTED OPERATION CODES with option 3 for a standard command that lacks service actions, while providing non-zero data in the requested service action field, wouldn't this unconditional extraction of sa cause a regression?

SPC-5 dictates that if the requested opcode does not have service actions, the requested service action field must be ignored. By unconditionally extracting sa here, it will fail to match in ata_scsi_get_supported_cmd() since non-service-action commands expect sa == 0, improperly yielding SUPPORT=001b for a valid, supported command.

Quoted 5 lines

According to SPC-5, option 2 queries for unsupported commands (and similarly, option 1 queries for commands with service actions) must return SUPPORT=001b. Instead, this implementation erroneously aborts the command by calling ata_scsi_set_invalid_field(), preventing the code from falling through and correctly populating the response buffer.

Sashiko, please reread the SPC specifications. It says:

If the REQUESTED OPERATION CODE field specifies an operation code for which the device server does not implement service actions a, then the device server shall terminate the command with CHECK CONDITION status, with the sense key set to ILLEGAL REQUEST, and the additional sense code set to INVALID FIELD IN CDB.

Which is exactly what we are testing here.

Add support for the reporting options value 2 of the in REPORT SUPPORTED OPERATION CODES command in ata_scsi_report_supported_opcodes(). With this reporting option, the specified opcode to check must have a service action. Use ata_scsi_supported_cmd_has_sa() to check for this and fill the command reply buffer using the one command format based on the command support.

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-scsi.c | 7 +++++++
  1 file changed, 7 insertions(+)

drivers/ata/libata-scsi.c +0 -0

index 86ac41a7d81a..e75ed573e234 100644
@@ -3737,6 +3737,13 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
  			return 0;
  		}
  		break;

Add support to ata_scsi_report_supported_opcodes() for the all command format indicated with a reporting option of 0. The function ata_scsi_report_all_supported_opcodes() is introduced to implement this support. This function operates by testing all commands of the ata_supported_cmds array and testing them using ata_scsi_cmd_is_supported(), filling rbuf as it loops through all the commands that libata can emulate or translate.

With this change, sg_opcodes /dev/sdX is able to list all commands supported by libata and a device. An example below is shown for a SATA disk also supporting CDL:

   # sg_opcodes /dev/sda
     ATA       WDC  WUH722626AL  WZ41
     Peripheral device type: disk

   Opcode  Service    CDB  RWCDLP,  Name
   (hex)   action(h)  size   CDLP
   -----------------------------------------------
    00                  6    0,0    Test Unit Ready
    01                  6    0,0    Rezero Unit
    03                  6    0,0    Request Sense
    08                  6    0,0    Read(6)
    0a                  6    0,0    Write(6)
    0b                  6    0,0    Seek(6)
    12                  6    0,0    Inquiry
    15                  6    0,0    Mode select(6)
    1a                  6    0,0    Mode sense(6)
    1b                  6    0,0    Start stop unit
    1d                  6    0,0    Send diagnostic
    25                 10    0,0    Read capacity(10)
    28                 10    0,0    Read(10)
    2a                 10    0,0    Write(10)
    2b                 10    0,0    Seek(10)
    2f                 10    0,0    Verify(10)
    35                 10    0,0    Synchronize cache(10)
    55                 10    0,0    Mode select(10)
    5a                 10    0,0    Mode sense(10)
    7f     1ff0        32    0,0    ATA pass-through(32)
    85                 16    0,0    ATA pass-through(16)
    88                 16    1,1    Read(16)
    8a                 16    1,2    Write(16)
    8f                 16    0,0    Verify(16)
    91                 16    0,0    Synchronize cache(16)
    93                 16    0,0    Write same(16)
    9e       10        16    0,0    Read capacity(16)
    a0                 12    0,0    Report luns
    a1                 12    0,0    ATA pass-through(12)
    a3        c        12    0,0    Report supported operation codes

Signed-off-by Damien Le Moal <[email protected]>

---
  drivers/ata/libata-scsi.c | 39 +++++++++++++++++++++++++++++++++++++++
  1 file changed, 39 insertions(+)