[PATCH v1 0/6] Improve ata_scsi_report_supported_opcodes()

[PATCH v1 0/6] Improve ata_scsi_report_supported_opcodes()

[Damien Le Moal]

The implementation of ata_scsi_report_supported_opcodes() mostly satisfies
the internal use when an ATA device is probed from the SCSI layer, but it
is very incomplete. The code is also not very flexible for extending
support for new commands.

This patch series address these issues, cleaning up the implementation and
making REPORT SUPPORTED OPERATION CODES fully functional. E.g., with these
changes, sg_opcodes can be used for listing all SCSI commands that are
supported by libata-scsi and a device.

Example:

     # 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                 32    0,0    Variable length
      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

Damien Le Moal (6):
  ata: libata: rename ata_dev_is_zac()
  ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()
  ata: libata-scsi: improve service action support in
    ata_scsi_report_supported_opcodes()
  ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands
    support
  ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED
    OPERATION CODES
  ata: libata-scsi: support the all command format for reporting
    supported commands

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

-- 
2.54.0

[PATCH v1 1/6] ata: libata: rename ata_dev_is_zac()

[Damien Le Moal]

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 <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 6 +++---
 drivers/ata/libata-scsi.c | 4 ++--
 drivers/ata/libata.h      | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3b6243f0f91e..6daa99508b72 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2488,7 +2488,7 @@ static void ata_dev_config_sense_reporting(struct ata_device *dev)
 	}
 }
 
-static void ata_dev_config_zac(struct ata_device *dev)
+static void ata_dev_config_zoned(struct ata_device *dev)
 {
 	unsigned int err_mask;
 	u8 *identify_buf = dev->sector_buf;
@@ -2497,7 +2497,7 @@ static void ata_dev_config_zac(struct ata_device *dev)
 	dev->zac_zones_optimal_nonseq = U32_MAX;
 	dev->zac_zones_max_open = U32_MAX;
 
-	if (!ata_dev_is_zac(dev))
+	if (!ata_dev_is_zoned(dev))
 		return;
 
 	if (!ata_identify_page_supported(dev, ATA_LOG_ZONED_INFORMATION)) {
@@ -3093,7 +3093,7 @@ int ata_dev_configure(struct ata_device *dev)
 		ata_dev_config_fua(dev);
 		ata_dev_config_devslp(dev);
 		ata_dev_config_sense_reporting(dev);
-		ata_dev_config_zac(dev);
+		ata_dev_config_zoned(dev);
 		ata_dev_config_trusted(dev);
 		ata_dev_config_cpr(dev);
 		ata_dev_config_cdl(dev);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d54ec1631e9a..b8aabd08f786 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2060,7 +2060,7 @@ static unsigned int ata_scsiop_inq_00(struct ata_device *dev,
 	};
 
 	for (i = 0; i < sizeof(pages); i++) {
-		if (pages[i] == 0xb6 && !ata_dev_is_zac(dev))
+		if (pages[i] == 0xb6 && !ata_dev_is_zoned(dev))
 			continue;
 		rbuf[num_pages + 4] = pages[i];
 		num_pages++;
@@ -2317,7 +2317,7 @@ static unsigned int ata_scsiop_inq_b2(struct ata_device *dev,
 static unsigned int ata_scsiop_inq_b6(struct ata_device *dev,
 				      struct scsi_cmnd *cmd, u8 *rbuf)
 {
-	if (!ata_dev_is_zac(dev)) {
+	if (!ata_dev_is_zoned(dev)) {
 		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
 		return 0;
 	}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 0dd735c2e5b5..f813b1c32461 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,7 +44,7 @@ static inline bool ata_sstatus_online(u32 sstatus)
 	return (sstatus & 0xf) == 0x3;
 }
 
-static inline bool ata_dev_is_zac(struct ata_device *dev)
+static inline bool ata_dev_is_zoned(struct ata_device *dev)
 {
 	/* Host managed device or host aware device */
 	return dev->class == ATA_DEV_ZAC ||
-- 
2.54.0

[PATCH v1 2/6] ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

[Damien Le Moal]

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, service action (0 if there is
not any) and CDB length of a supported SCSI command that libata SAT can
translate or emulate.

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 <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 164 +++++++++++++++++++++++++-------------
 1 file changed, 109 insertions(+), 55 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b8aabd08f786..380ca3886a77 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3590,88 +3590,142 @@ 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;
+	u16 sa;
+};
+
+/*
+ * List of supported commands for translation or emulation, sorted in
+ * alphabetical 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[] = {
+	{ ATA_12,		12,	0 },
+	{ ATA_16,		16,	0 },
+	{ INQUIRY,		 6,	0 },
+	{ MAINTENANCE_IN,	12,	MI_REPORT_SUPPORTED_OPERATION_CODES },
+	{ MODE_SENSE,		 6,	0 },
+	{ MODE_SENSE_10,	10,	0 },
+	{ MODE_SELECT,		 6,	0 },
+	{ MODE_SELECT_10,	10,	0 },
+	{ READ_6,		 6,	0 },
+	{ READ_10,		10,	0 },
+	{ READ_16,		16,	0 },
+	{ READ_CAPACITY,	10,	0 },
+	{ REPORT_LUNS,		12,	0 },
+	{ REQUEST_SENSE,	 6,	0 },
+	{ REZERO_UNIT,		 6,	0 },
+	{ SECURITY_PROTOCOL_IN,	12,	0 },
+	{ SECURITY_PROTOCOL_OUT, 12,	0 },
+	{ SEEK_6,		 6,	0 },
+	{ SEEK_10,		10,	0 },
+	{ SEND_DIAGNOSTIC,	 6,	0 },
+	{ SERVICE_ACTION_IN_16,	16,	SAI_READ_CAPACITY_16 },
+	{ START_STOP,		 6,	0 },
+	{ SYNCHRONIZE_CACHE,	10,	0 },
+	{ SYNCHRONIZE_CACHE_16,	16,	0 },
+	{ TEST_UNIT_READY,	 6,	0 },
+	{ WRITE_6,		 6,	0 },
+	{ WRITE_10,		10,	0 },
+	{ WRITE_16,		16,	0 },
+	{ WRITE_SAME_16,	16,	0 },
+	{ VERIFY,		10,	0 },
+	{ VERIFY_16,		16,	0 },
+	{ VARIABLE_LENGTH_CMD,	32,	0 },
+	{ ZBC_IN,		16,	ZI_REPORT_ZONES },
+	{ ZBC_OUT,		16,	ZO_CLOSE_ZONE },
+	{ ZBC_OUT,		16,	ZO_FINISH_ZONE },
+	{ ZBC_OUT,		16,	ZO_OPEN_ZONE },
+	{ ZBC_OUT,		16,	ZO_RESET_WRITE_POINTER },
+};
+
+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_id_zoned_cap(dev->id) ||
-		    dev->class == ATA_DEV_ZAC)
-			supported = 3;
-		break;
+		return ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC;
 	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;
-	rbuf[1] = cdlp | supported;
+	if (ata_scsi_cmd_is_supported(dev, cdb[3], &sup)) {
+		rbuf[0] = sup.rwcdlp;
+		rbuf[1] = (sup.cdlp << 3) | 0x03;
+	} else {
+		rbuf[1] = 0x01;
+	}
 
 	return 4;
 }
-- 
2.54.0

[PATCH v1 3/6] ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

[Damien Le Moal]

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_has_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 <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 43 ++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 380ca3886a77..e432e521ad31 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3641,32 +3641,46 @@ static const struct ata_scsi_cmd ata_supported_cmds[] = {
 	{ ZBC_OUT,		16,	ZO_RESET_WRITE_POINTER },
 };
 
-static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op)
+static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op, u16 sa)
 {
 	const struct ata_scsi_cmd *cmd;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
 		cmd = &ata_supported_cmds[i];
-		if (cmd->op == op)
+		if (cmd->op == op && cmd->sa == sa)
 			return cmd;
 	}
 
 	return NULL;
 }
 
+static bool ata_scsi_supported_cmd_has_sa(u8 op)
+{
+	const struct ata_scsi_cmd *cmd;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
+		cmd = &ata_supported_cmds[i];
+		if (cmd->op == op)
+			return cmd->sa != 0;
+	}
+
+	return false;
+}
+
 struct ata_scsi_cmd_support {
 	u8 cdlp;
 	u8 rwcdlp;
 };
 
-static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op,
+static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
 				      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);
+	cmd = ata_scsi_get_supported_cmd(op, sa);
 	if (!cmd)
 		return false;
 
@@ -3696,6 +3710,8 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op,
 	case ZBC_IN:
 	case ZBC_OUT:
 		return ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC;
+	case SERVICE_ACTION_IN_16:
+		return sa == SAI_READ_CAPACITY_16;
 	case SECURITY_PROTOCOL_IN:
 	case SECURITY_PROTOCOL_OUT:
 		return dev->flags & ATA_DFLAG_TRUSTED;
@@ -3712,15 +3728,28 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
 {
 	struct ata_scsi_cmd_support sup;
 	u8 *cdb = cmd->cmnd;
-
-	if (cdb[2] != 1 && cdb[2] != 3) {
+	u16 sa = 0;
+
+	switch (cdb[2]) {
+	case 1:
+		/* One command format with command support data, ignore sa. */
+		if (ata_scsi_supported_cmd_has_sa(cdb[3])) {
+			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
+			return 0;
+		}
+		break;
+	case 3:
+		/* One command format */
+		sa = get_unaligned_be16(&cdb[4]);
+		break;
+	default:
 		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 */
-	if (ata_scsi_cmd_is_supported(dev, cdb[3], &sup)) {
+	if (ata_scsi_cmd_is_supported(dev, cdb[3], sa, &sup)) {
 		rbuf[0] = sup.rwcdlp;
 		rbuf[1] = (sup.cdlp << 3) | 0x03;
 	} else {
-- 
2.54.0

Re: [PATCH v1 3/6] ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ata_scsi_supported_cmd_has_sa() and ata_supported_cmds incorrectly handle service actions of value 0, breaking REPORT SUPPORTED OPERATION CODES.
--

commit 976ae28e21b5ac80b95af84b459ff3561417568d
Author: Damien Le Moal <dlemoal@kernel.org>

ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

This commit updates ata_scsi_report_supported_opcodes() to take service actions
into consideration. It adds ata_scsi_supported_cmd_has_sa() to check if an
opcode has service actions and uses both opcode and service action to look up
supported commands.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 380ca3886a777..e432e521ad318 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -3641,32 +3641,46 @@ static const struct ata_scsi_cmd ata_supported_cmds[] = {
>  	{ ZBC_OUT,		16,	ZO_RESET_WRITE_POINTER },
>  };
>  
> -static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op)
> +static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op, u16 sa)
>  {
>  	const struct ata_scsi_cmd *cmd;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
>  		cmd = &ata_supported_cmds[i];
> -		if (cmd->op == op)
> +		if (cmd->op == op && cmd->sa == sa)

[Severity: Medium]
Does this strict service action matching cause issues for VARIABLE_LENGTH_CMD?

Looking at the ata_supported_cmds array, VARIABLE_LENGTH_CMD uses service
actions (e.g., ATA_32 = 0x1FF0) but is mapped with sa = 0. Will this cause the
matching here to fail for valid service actions under reporting option 3?

>  			return cmd;
>  	}
>  
>  	return NULL;
>  }
>  
> +static bool ata_scsi_supported_cmd_has_sa(u8 op)
> +{
> +	const struct ata_scsi_cmd *cmd;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
> +		cmd = &ata_supported_cmds[i];
> +		if (cmd->op == op)
> +			return cmd->sa != 0;

[Severity: Medium]
Will this incorrectly handle service actions with a value of 0?

Because 0 is a valid service action (e.g., ZBC_IN uses ZI_REPORT_ZONES = 0),
this condition will incorrectly cause ata_scsi_supported_cmd_has_sa() to
return false for ZBC_IN.

> +	}
> +
> +	return false;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702015646.1702539-1-dlemoal@kernel.org?part=3

[PATCH v1 4/6] ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support

[Damien Le Moal]

Improve ata_scsi_cmd_is_supported() to check the service action for the
ZBC_IN and ZBC_OUT opcodes, to correctly report supported zone management
commands for zoned disks.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e432e521ad31..f9e6bda6833e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3708,8 +3708,17 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
 		}
 		return true;
 	case ZBC_IN:
+		return sa == ZI_REPORT_ZONES && ata_dev_is_zoned(dev);
 	case ZBC_OUT:
-		return ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC;
+		switch (sa) {
+		case ZO_CLOSE_ZONE:
+		case ZO_FINISH_ZONE:
+		case ZO_OPEN_ZONE:
+		case ZO_RESET_WRITE_POINTER:
+			return ata_dev_is_zoned(dev);
+		default:
+			return false;
+		}
 	case SERVICE_ACTION_IN_16:
 		return sa == SAI_READ_CAPACITY_16;
 	case SECURITY_PROTOCOL_IN:
-- 
2.54.0

Re: [PATCH v1 4/6] ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ZBC_IN is incorrectly evaluated as not having service actions because ZI_REPORT_ZONES equals 0, breaking all REPORT SUPPORTED OPERATION CODES (RSOC) formats.
--

commit 6af9854276b3609a524ea350c87e6e2924aee340
Author: Damien Le Moal <dlemoal@kernel.org>

ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support

Improve ata_scsi_cmd_is_supported() to check the service action for the
ZBC_IN and ZBC_OUT opcodes, to correctly report supported zone management
commands for zoned disks.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index e432e521ad318..f9e6bda6833e4 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3708,8 +3708,17 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
>  		}
>  		return true;
>  	case ZBC_IN:
> +		return sa == ZI_REPORT_ZONES && ata_dev_is_zoned(dev);

[Severity: Medium]
Does this inadvertently break REPORT SUPPORTED OPERATION CODES (RSOC) formats
for ZBC_IN?

Because ZI_REPORT_ZONES evaluates to 0, the helper
ata_scsi_supported_cmd_has_sa() will evaluate this as not having service
actions:

drivers/ata/libata-scsi.c:ata_scsi_supported_cmd_has_sa() {
    return cmd->sa != 0;
}

This appears to cause incorrect behavior during RSOC handling for ZBC_IN:

- Format 0 (ata_scsi_report_all_supported_opcodes) fails to set the SERVACTV
  (Service Action Valid) bit because cmd->sa evaluates to 0.

- Format 1 incorrectly accepts ZBC_IN.

- Format 2 (ata_scsi_report_supported_opcodes) incorrectly rejects valid
  queries for ZBC_IN with CHECK CONDITION (ILLEGAL REQUEST), preventing
  userspace from probing ZBC_IN service actions.

>  	case ZBC_OUT:

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702015646.1702539-1-dlemoal@kernel.org?part=4

Re: [PATCH v1 4/6] ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support

[Damien Le Moal]

On 7/2/26 11:07, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] ZBC_IN is incorrectly evaluated as not having service actions because ZI_REPORT_ZONES equals 0, breaking all REPORT SUPPORTED OPERATION CODES (RSOC) formats.
> --
> 
> commit 6af9854276b3609a524ea350c87e6e2924aee340
> Author: Damien Le Moal <dlemoal@kernel.org>
> 
> ata: libata-scsi: improves checks for ZBC_IN and ZBC_OUT commands support
> 
> Improve ata_scsi_cmd_is_supported() to check the service action for the
> ZBC_IN and ZBC_OUT opcodes, to correctly report supported zone management
> commands for zoned disks.
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index e432e521ad318..f9e6bda6833e4 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3708,8 +3708,17 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
>>  		}
>>  		return true;
>>  	case ZBC_IN:
>> +		return sa == ZI_REPORT_ZONES && ata_dev_is_zoned(dev);
> 
> [Severity: Medium]
> Does this inadvertently break REPORT SUPPORTED OPERATION CODES (RSOC) formats
> for ZBC_IN?
> 
> Because ZI_REPORT_ZONES evaluates to 0, the helper
> ata_scsi_supported_cmd_has_sa() will evaluate this as not having service
> actions:
> 
> drivers/ata/libata-scsi.c:ata_scsi_supported_cmd_has_sa() {
>     return cmd->sa != 0;
> }
> 
> This appears to cause incorrect behavior during RSOC handling for ZBC_IN:
> 
> - Format 0 (ata_scsi_report_all_supported_opcodes) fails to set the SERVACTV
>   (Service Action Valid) bit because cmd->sa evaluates to 0.
> 
> - Format 1 incorrectly accepts ZBC_IN.
> 
> - Format 2 (ata_scsi_report_supported_opcodes) incorrectly rejects valid
>   queries for ZBC_IN with CHECK CONDITION (ILLEGAL REQUEST), preventing
>   userspace from probing ZBC_IN service actions.

Yes, these observations are correct if one assumes that "0" is a valid service
action. But it is not: the SPC specifications actually define service action "0"
as "reserved". So this is a mess, but the specifications are responsible here.

With these changes, report zones is still reported as supported and everything
works.

-- 
Damien Le Moal
Western Digital Research

[PATCH v1 5/6] ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES

[Damien Le Moal]

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 <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f9e6bda6833e..3f313050c63f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3747,6 +3747,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_has_sa(cdb[3])) {
+			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
+			return 0;
+		}
+		fallthrough;
 	case 3:
 		/* One command format */
 		sa = get_unaligned_be16(&cdb[4]);
-- 
2.54.0

Re: [PATCH v1 5/6] ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The helper `ata_scsi_supported_cmd_has_sa()` incorrectly assumes that a command with `sa == 0` does not take service actions, causing Reporting Option 2 to reject valid queries for commands like `ZBC_IN`.
- [High] Reporting Option 3 incorrectly fails to ignore the REQUESTED SERVICE ACTION field for commands without service actions.
--

commit ae74931c866547ba834a0558a06a2d04d82842f2
Author: Damien Le Moal <dlemoal@kernel.org>
ata: libata-scsi: support reporting options 2 in REPORT SUPPORTED OPERATION CODES

This commit adds support for the reporting options value 2 of the REPORT
SUPPORTED OPERATION CODES command in ata_scsi_report_supported_opcodes().
It verifies that the opcode to check has a service action using the new
ata_scsi_supported_cmd_has_sa() helper. If supported, it replies using the
one command format based on the command support.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d54ec1631e9a6..3f313050c63f3 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> +static bool ata_scsi_supported_cmd_has_sa(u8 op)
> +{
> +	const struct ata_scsi_cmd *cmd;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
> +		cmd = &ata_supported_cmds[i];
> +		if (cmd->op == op)
> +			return cmd->sa != 0;

[Severity: High]
Does this incorrectly assume that commands with a service action of 0 don't
have a service action at all? 

For example, ZBC_IN has op == 0x53 and its service action ZI_REPORT_ZONES
is 0x00. This helper will return false for ZBC_IN.

> +	}
> +
> +	return false;
> +}

[ ... ]

> +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;
> +	u16 sa = 0;
> +
> +	switch (cdb[2]) {
> +	case 1:
> +		/* One command format with command support data, ignore sa. */
> +		if (ata_scsi_supported_cmd_has_sa(cdb[3])) {
> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}
> +		break;
> +	case 2:
> +		/* One command format, must have sa. */
> +		if (!ata_scsi_supported_cmd_has_sa(cdb[3])) {
> +			ata_scsi_set_invalid_field(dev, cmd, 3, 0xff);
> +			return 0;
> +		}

[Severity: High]
Because of the logic in ata_scsi_supported_cmd_has_sa(), if a host issues a
REPORT SUPPORTED OPERATION CODES command with Option 2 and CDB[3] = ZBC_IN,
won't this actively reject a valid zone capabilities query with an Invalid
Field in CDB error?

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

[Severity: High]
When processing Reporting Option 3, is it safe to unconditionally extract the
requested service action without verifying if the command actually takes
service actions?

The SPC specification requires ignoring this field for commands that do not
take service actions. If a host sends an Option 3 query for a command like
READ_16 and leaves uninitialized non-zero data in the ignored service action
field, the exact match lookup in ata_scsi_get_supported_cmd() will fail and
incorrectly report the command as unsupported.

> +		break;
> +	default:
> +		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
> +		ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
> +		return 0;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702015646.1702539-1-dlemoal@kernel.org?part=5

[PATCH v1 6/6] ata: libata-scsi: support the all command format for reporting supported commands

[Damien Le Moal]

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 a device. An example below is shown for SATA diski
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                 32    0,0    Variable length
  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 <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3f313050c63f..30e2cf3b03dc 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3731,6 +3731,42 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
 	return true;
 }
 
+static unsigned int
+ata_scsi_report_all_supported_opcodes(struct ata_device *dev, u8 *rbuf)
+{
+	struct ata_scsi_cmd_support sup;
+	const struct ata_scsi_cmd *cmd;
+	unsigned int len = 4;
+	u8 *buf = &rbuf[len];
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
+		if (len > ATA_SCSI_RBUF_SIZE - 8)
+			break;
+
+		cmd = &ata_supported_cmds[i];
+
+		/* All command format */
+		if (!ata_scsi_cmd_is_supported(dev, cmd->op, cmd->sa, &sup))
+			continue;
+
+		buf[0] = cmd->op;
+		put_unaligned_be16(cmd->sa, &buf[2]);
+		buf[5] |= (sup.rwcdlp << 6) | (sup.cdlp << 2);
+		if (cmd->sa)
+			buf[5] |= 0x01;
+		put_unaligned_be16(cmd->cdb_len, &buf[6]);
+
+		/* CTDP == 0 */
+		len += 8;
+		buf += 8;
+	}
+
+	put_unaligned_be32(len - 4, &rbuf[0]);
+
+	return len;
+}
+
 static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
 						      struct scsi_cmnd *cmd,
 						      u8 *rbuf)
@@ -3740,6 +3776,9 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
 	u16 sa = 0;
 
 	switch (cdb[2]) {
+	case 0:
+		/* All command format */
+		return ata_scsi_report_all_supported_opcodes(dev, rbuf);
 	case 1:
 		/* One command format with command support data, ignore sa. */
 		if (ata_scsi_supported_cmd_has_sa(cdb[3])) {
-- 
2.54.0

Re: [PATCH v1 6/6] ata: libata-scsi: support the all command format for reporting supported commands

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect bit-shifting and field conflation for the CDLP and RWCDLP fields corrupts Command Duration Limits (CDL) reporting.
- [Medium] ZBC_IN and VARIABLE_LENGTH_CMD are incorrectly reported as lacking service actions in the All Commands format.
--

commit f7b18a5d6dd37eabc2de02696bfa0461178f0b87
Author: Damien Le Moal <dlemoal@kernel.org>

ata: libata-scsi: support the all command format for reporting supported commands

This commit introduces support for the "All Commands" format (reporting option 0)
in ata_scsi_report_supported_opcodes(). It adds ata_scsi_report_all_supported_opcodes()
to iterate through ata_supported_cmds and report supported operations.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 3f313050c63f3..30e2cf3b03dc7 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3731,6 +3731,42 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
[ ... ]
> +static unsigned int
> +ata_scsi_report_all_supported_opcodes(struct ata_device *dev, u8 *rbuf)
> +{
[ ... ]
> +		/* All command format */
> +		if (!ata_scsi_cmd_is_supported(dev, cmd->op, cmd->sa, &sup))
> +			continue;
> +
> +		buf[0] = cmd->op;
> +		put_unaligned_be16(cmd->sa, &buf[2]);
> +		buf[5] |= (sup.rwcdlp << 6) | (sup.cdlp << 2);

[Severity: High]
Does this code correctly shift and map the CDLP and RWCDLP fields?

It looks like sup.rwcdlp is a 1-bit boolean flag (0 for A/B, 1 for T2A/T2B)
used in the One Command format, not the 2-bit RWCDLP enum (01b=Read, 10b=Write)
required for the All Commands format. Shifting 1 by 6 sets bit 6, resulting in
10b, which would incorrectly report READ_16 as a Write command.

Additionally, the CDLP field is defined at bits 4-3, but sup.cdlp is shifted
by 2, placing it at bits 3-2. For WRITE_16 (sup.cdlp=2), shifting by 2 results
in setting bit 3, which falls into the CDLP field as 01b instead of 10b.

> +		if (cmd->sa)
> +			buf[5] |= 0x01;

[Severity: Medium]
Will this correctly set the SERVACTV bit for all commands that require service
actions?

Commands like ZBC_IN (0x95) and VARIABLE_LENGTH_CMD (0x7F) have their service
action explicitly defined as 0x00 in ata_supported_cmds. Since cmd->sa evaluates
to 0, the SERVACTV bit won't be set. This incorrectly tells the host that these
commands do not use the SERVICE ACTION field.

> +		put_unaligned_be16(cmd->cdb_len, &buf[6]);
> +
> +		/* CTDP == 0 */
> +		len += 8;
> +		buf += 8;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702015646.1702539-1-dlemoal@kernel.org?part=6

Re: [PATCH v1 6/6] ata: libata-scsi: support the all command format for reporting supported commands

[Damien Le Moal]

On 7/2/26 11:11, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Incorrect bit-shifting and field conflation for the CDLP and RWCDLP fields corrupts Command Duration Limits (CDL) reporting.
> - [Medium] ZBC_IN and VARIABLE_LENGTH_CMD are incorrectly reported as lacking service actions in the All Commands format.
> --
> 
> commit f7b18a5d6dd37eabc2de02696bfa0461178f0b87
> Author: Damien Le Moal <dlemoal@kernel.org>
> 
> ata: libata-scsi: support the all command format for reporting supported commands
> 
> This commit introduces support for the "All Commands" format (reporting option 0)
> in ata_scsi_report_supported_opcodes(). It adds ata_scsi_report_all_supported_opcodes()
> to iterate through ata_supported_cmds and report supported operations.
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 3f313050c63f3..30e2cf3b03dc7 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3731,6 +3731,42 @@ static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op, u16 sa,
> [ ... ]
>> +static unsigned int
>> +ata_scsi_report_all_supported_opcodes(struct ata_device *dev, u8 *rbuf)
>> +{
> [ ... ]
>> +		/* All command format */
>> +		if (!ata_scsi_cmd_is_supported(dev, cmd->op, cmd->sa, &sup))
>> +			continue;
>> +
>> +		buf[0] = cmd->op;
>> +		put_unaligned_be16(cmd->sa, &buf[2]);
>> +		buf[5] |= (sup.rwcdlp << 6) | (sup.cdlp << 2);
> 
> [Severity: High]
> Does this code correctly shift and map the CDLP and RWCDLP fields?

No it does not.

> It looks like sup.rwcdlp is a 1-bit boolean flag (0 for A/B, 1 for T2A/T2B)
> used in the One Command format, not the 2-bit RWCDLP enum (01b=Read, 10b=Write)
> required for the All Commands format. Shifting 1 by 6 sets bit 6, resulting in
> 10b, which would incorrectly report READ_16 as a Write command.

rdwcdlp is always 1 bit, for both the one command format and the all command format.

> 
> Additionally, the CDLP field is defined at bits 4-3, but sup.cdlp is shifted
> by 2, placing it at bits 3-2. For WRITE_16 (sup.cdlp=2), shifting by 2 results
> in setting bit 3, which falls into the CDLP field as 01b instead of 10b.

cdlp in bits 4-3 of byte 1 is for the one command format, which is different
from the all command format which has cdlp in bits 3-2 of byte 5 of a descriptor.

> 
>> +		if (cmd->sa)
>> +			buf[5] |= 0x01;
> 
> [Severity: Medium]
> Will this correctly set the SERVACTV bit for all commands that require service
> actions?
> 
> Commands like ZBC_IN (0x95) and VARIABLE_LENGTH_CMD (0x7F) have their service
> action explicitly defined as 0x00 in ata_supported_cmds. Since cmd->sa evaluates
> to 0, the SERVACTV bit won't be set. This incorrectly tells the host that these
> commands do not use the SERVICE ACTION field.

Will fix that, despite the fact that this is originally a problem with the specs
making a mess with service action "0".

> 
>> +		put_unaligned_be16(cmd->cdb_len, &buf[6]);
>> +
>> +		/* CTDP == 0 */
>> +		len += 8;
>> +		buf += 8;
>> +	}
> 


-- 
Damien Le Moal
Western Digital Research