[PATCH v3 0/5] Improve ata_scsi_report_supported_opcodes()

[PATCH v3 0/5] 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     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)
   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

Changes from v2:
 - Patch 2: reformatted the array of supported commands and removed
   WRITE_SAME_16 command from it.
 - Applied review tags.
Changes from v1:
 - Patch 2: Added an sa_valid field to the array of supported command to
   correctly handle sa == 0 being a valid service action (report zones) in
   patch 3
 - Dropped v1 patch 4
 - Patch 5: correctly set the servact field based on the sa_valid field of
   the array of supported commands.

Damien Le Moal (5):
  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: 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 | 283 ++++++++++++++++++++++++++++++--------
 drivers/ata/libata.h      |   2 +-
 3 files changed, 228 insertions(+), 63 deletions(-)

-- 
2.54.0

[PATCH v3 1/5] 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. Use this helper in
ata_scsiop_inq_std(), ata_scsiop_read_cap() and
ata_scsi_report_supported_opcodes() instead of having the same tests open
coded.

While at it, to stay consistent with this renaming, also rename
ata_dev_config_zac() to ata_dev_config_zoned().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/ata/libata-core.c |  6 +++---
 drivers/ata/libata-scsi.c | 11 +++++------
 drivers/ata/libata.h      |  2 +-
 3 files changed, 9 insertions(+), 10 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..a86721b5df7f 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2020,7 +2020,7 @@ static unsigned int ata_scsiop_inq_std(struct ata_device *dev,
 	if (rbuf[32] == 0 || rbuf[32] == ' ')
 		memcpy(&rbuf[32], "n/a ", 4);
 
-	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
+	if (ata_dev_is_zoned(dev))
 		memcpy(rbuf + 58, versions_zbc, sizeof(versions_zbc));
 	else
 		memcpy(rbuf + 58, versions, sizeof(versions));
@@ -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;
 	}
@@ -2848,7 +2848,7 @@ static unsigned int ata_scsiop_read_cap(struct ata_device *dev,
 	rbuf[10] = sector_size >> (8 * 1);
 	rbuf[11] = sector_size;
 
-	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
+	if (ata_dev_is_zoned(dev))
 		rbuf[12] = (1 << 4); /* RC_BASIS */
 	rbuf[13] = log2_per_phys;
 	rbuf[14] = (lowest_aligned >> 8) & 0x3f;
@@ -3656,8 +3656,7 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
 		break;
 	case ZBC_IN:
 	case ZBC_OUT:
-		if (ata_id_zoned_cap(dev->id) ||
-		    dev->class == ATA_DEV_ZAC)
+		if (ata_dev_is_zoned(dev))
 			supported = 3;
 		break;
 	case SECURITY_PROTOCOL_IN:
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

Re: [PATCH v3 1/5] ata: libata: rename ata_dev_is_zac()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces an undocumented functional change (implicit bug fix) for Device Managed zoned drives, while the commit message falsely claims the refactor merely replaces identical open-coded tests.
--

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

ata: libata: rename ata_dev_is_zac()

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. Use this helper in
ata_scsiop_inq_std(), ata_scsiop_read_cap() and
ata_scsi_report_supported_opcodes() instead of having the same tests open
coded.

While at it, to stay consistent with this renaming, also rename
ata_dev_config_zac() to ata_dev_config_zoned().

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d54ec1631e9a6..a86721b5df7f6 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2020,7 +2020,7 @@ static unsigned int ata_scsiop_inq_std(struct ata_device *dev,
>  	if (rbuf[32] == 0 || rbuf[32] == ' ')
>  		memcpy(&rbuf[32], "n/a ", 4);
>  
> -	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
> +	if (ata_dev_is_zoned(dev))

[Severity: Medium]
Does this introduce an undocumented functional change for Device Managed
zoned drives?

The commit message states this is a replacement of "the same tests open coded",
but the old condition would evaluate to true for Device Managed drives,
since ata_id_zoned_cap() returns 0x02 for them.

The new ata_dev_is_zoned() helper checks ata_id_zoned_cap(dev->id) == 0x01,
which evaluates to false for Device Managed drives.

This appears to alter SCSI emulation for these drives, as they will no longer
report ZBC version compatibility here.

Could the commit message be updated to mention this functional bug fix?

>  		memcpy(rbuf + 58, versions_zbc, sizeof(versions_zbc));
>  	else
>  		memcpy(rbuf + 58, versions, sizeof(versions));

[ ... ]

> @@ -2848,7 +2848,7 @@ static unsigned int ata_scsiop_read_cap(struct ata_device *dev,
>  	rbuf[10] = sector_size >> (8 * 1);
>  	rbuf[11] = sector_size;
>  
> -	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
> +	if (ata_dev_is_zoned(dev))
>  		rbuf[12] = (1 << 4); /* RC_BASIS */

[Severity: Medium]
Similarly, does this silently fix Device Managed drives incorrectly
reporting the RC_BASIS bit in READ CAPACITY 16?

>  	rbuf[13] = log2_per_phys;
>  	rbuf[14] = (lowest_aligned >> 8) & 0x3f;

[ ... ]

> @@ -3656,8 +3656,7 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
>  		break;
>  	case ZBC_IN:
>  	case ZBC_OUT:
> -		if (ata_id_zoned_cap(dev->id) ||
> -		    dev->class == ATA_DEV_ZAC)
> +		if (ata_dev_is_zoned(dev))
>  			supported = 3;

[Severity: Medium]
Does this also correctly prevent ZBC_IN and ZBC_OUT from being reported as
supported for Device Managed drives?

If these are intentional bug fixes rather than just refactoring, should the
commit message clarify that the previous open-coded tests incorrectly matched
Device Managed drives?

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

Re: [PATCH v3 1/5] ata: libata: rename ata_dev_is_zac()

[Damien Le Moal]

On 7/3/26 13:59, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The patch introduces an undocumented functional change (implicit bug fix) for Device Managed zoned drives, while the commit message falsely claims the refactor merely replaces identical open-coded tests.
> --
> 
> commit c24879d1eda5cc422a0b0da776fb6aef5fe25e22
> Author: Damien Le Moal <dlemoal@kernel.org>
> 
> ata: libata: rename ata_dev_is_zac()
> 
> 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. Use this helper in
> ata_scsiop_inq_std(), ata_scsiop_read_cap() and
> ata_scsi_report_supported_opcodes() instead of having the same tests open
> coded.
> 
> While at it, to stay consistent with this renaming, also rename
> ata_dev_config_zac() to ata_dev_config_zoned().
> 
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index d54ec1631e9a6..a86721b5df7f6 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -2020,7 +2020,7 @@ static unsigned int ata_scsiop_inq_std(struct ata_device *dev,
>>  	if (rbuf[32] == 0 || rbuf[32] == ' ')
>>  		memcpy(&rbuf[32], "n/a ", 4);
>>  
>> -	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
>> +	if (ata_dev_is_zoned(dev))
> 
> [Severity: Medium]
> Does this introduce an undocumented functional change for Device Managed
> zoned drives?
> 
> The commit message states this is a replacement of "the same tests open coded",
> but the old condition would evaluate to true for Device Managed drives,
> since ata_id_zoned_cap() returns 0x02 for them.
> 
> The new ata_dev_is_zoned() helper checks ata_id_zoned_cap(dev->id) == 0x01,
> which evaluates to false for Device Managed drives.
> 
> This appears to alter SCSI emulation for these drives, as they will no longer
> report ZBC version compatibility here.
> 
> Could the commit message be updated to mention this functional bug fix?
> 
>>  		memcpy(rbuf + 58, versions_zbc, sizeof(versions_zbc));
>>  	else
>>  		memcpy(rbuf + 58, versions, sizeof(versions));

Correct. But I think that we should never had reported that. In any case, with
host aware also now deprecated, we now definitely do *not* want to report that a
drive managed disk is compatible zbc, as we treat such drives no different than
regular disks.

> 
> [ ... ]
> 
>> @@ -2848,7 +2848,7 @@ static unsigned int ata_scsiop_read_cap(struct ata_device *dev,
>>  	rbuf[10] = sector_size >> (8 * 1);
>>  	rbuf[11] = sector_size;
>>  
>> -	if (ata_id_zoned_cap(dev->id) || dev->class == ATA_DEV_ZAC)
>> +	if (ata_dev_is_zoned(dev))
>>  		rbuf[12] = (1 << 4); /* RC_BASIS */
> 
> [Severity: Medium]
> Similarly, does this silently fix Device Managed drives incorrectly
> reporting the RC_BASIS bit in READ CAPACITY 16?

The old code was buggy ! we do not want to set rc basis for drive managed devices.

> 
>>  	rbuf[13] = log2_per_phys;
>>  	rbuf[14] = (lowest_aligned >> 8) & 0x3f;
> 
> [ ... ]
> 
>> @@ -3656,8 +3656,7 @@ static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
>>  		break;
>>  	case ZBC_IN:
>>  	case ZBC_OUT:
>> -		if (ata_id_zoned_cap(dev->id) ||
>> -		    dev->class == ATA_DEV_ZAC)
>> +		if (ata_dev_is_zoned(dev))
>>  			supported = 3;
> 
> [Severity: Medium]
> Does this also correctly prevent ZBC_IN and ZBC_OUT from being reported as
> supported for Device Managed drives?

Yes it does and that is fine.

-- 
Damien Le Moal
Western Digital Research

[PATCH v3 2/5] 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, 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 VARIABLE_LENGTH_CMD/ATA_32 command which is supported and
translated by libata-scsi. The ata_supported_cmds array includes this
command. As commented in the code, WRITE_SAME_16 is left out of the array
of supported command ata_scsi_write_same_xlat() prevents the use of this
command as a passthrough command.

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

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a86721b5df7f..e502b8702f98 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3590,87 +3590,180 @@ 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();
+ *
+ * Note: commands that are not fully supported may be left out of this array
+ * so that they are not reported as supported for passthrough but still
+ * available through the block layer. For now, this includes the following
+ * commands:
+ *  - WRITE_SAME_16: ata_scsi_write_same_xlat() forbids passthrough commands
+ */
+static const struct ata_scsi_cmd ata_supported_cmds[] = {
+	{	.op = TEST_UNIT_READY,		.cdb_len = 6	},
+	{	.op = REZERO_UNIT,		.cdb_len = 6	},
+	{	.op = REQUEST_SENSE,		.cdb_len = 6	},
+	{	.op = READ_6,			.cdb_len = 6	},
+	{	.op = WRITE_6,			.cdb_len = 6	},
+	{	.op = SEEK_6,			.cdb_len = 6	},
+	{	.op = INQUIRY,			.cdb_len = 6	},
+	{	.op = MODE_SELECT,		.cdb_len = 6	},
+	{	.op = MODE_SENSE,		.cdb_len = 6	},
+	{	.op = START_STOP,		.cdb_len = 6	},
+	{	.op = SEND_DIAGNOSTIC,		.cdb_len = 6	},
+	{	.op = READ_CAPACITY,		.cdb_len = 10	},
+	{	.op = READ_10,			.cdb_len = 10	},
+	{	.op = WRITE_10,			.cdb_len = 10	},
+	{	.op = SEEK_10,			.cdb_len = 10	},
+	{	.op = VERIFY,			.cdb_len = 10	},
+	{	.op = SYNCHRONIZE_CACHE,	.cdb_len = 10	},
+	{	.op = MODE_SELECT_10,		.cdb_len = 10	},
+	{	.op = MODE_SENSE_10,		.cdb_len = 10	},
+	{
+		.op = VARIABLE_LENGTH_CMD,	.cdb_len = 32,
+		.sa_valid = true,
+		.sa = ATA_32
+	},
+	{	.op = ATA_16,			.cdb_len = 16	},
+	{	.op = READ_16,			.cdb_len = 16	},
+	{	.op = WRITE_16,			.cdb_len = 16	},
+	{	.op = VERIFY_16,		.cdb_len = 16	},
+	{	.op = SYNCHRONIZE_CACHE_16,	.cdb_len = 16	},
+	{
+		.op = ZBC_OUT,			.cdb_len = 16,
+		.sa_valid = true,
+		.sa = ZO_CLOSE_ZONE
+	},
+	{
+		.op = ZBC_OUT,			.cdb_len = 16,
+		.sa_valid = true,
+		.sa = ZO_FINISH_ZONE
+	},
+	{
+		.op = ZBC_OUT,			.cdb_len = 16,
+		.sa_valid = true,
+		.sa = ZO_OPEN_ZONE
+	},
+	{
+		.op = ZBC_OUT,			.cdb_len = 16,
+		.sa_valid = true,
+		.sa = ZO_RESET_WRITE_POINTER
+	},
+	{
+		.op = ZBC_IN,			.cdb_len = 16,
+		.sa_valid = true,
+		.sa = ZI_REPORT_ZONES
+	},
+	{
+		.op = SERVICE_ACTION_IN_16,	.cdb_len = 16,
+		.sa_valid = true,
+		.sa = SAI_READ_CAPACITY_16
+	},
+	{	.op = REPORT_LUNS,		.cdb_len = 12	},
+	{	.op = ATA_12,			.cdb_len = 12	},
+	{	.op = SECURITY_PROTOCOL_IN,	.cdb_len = 12	},
+	{
+		.op = MAINTENANCE_IN,		.cdb_len = 12,
+		.sa_valid = true,
+		.sa = MI_REPORT_SUPPORTED_OPERATION_CODES
+	},
+	{	.op = SECURITY_PROTOCOL_OUT,	.cdb_len = 12	},
+};
+
+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;
-	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

Re: [PATCH v3 2/5] ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

[Hannes Reinecke]

On 7/3/26 6:05 AM, Damien Le Moal wrote:
> 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 VARIABLE_LENGTH_CMD/ATA_32 command which is supported and
> translated by libata-scsi. The ata_supported_cmds array includes this
> command. As commented in the code, WRITE_SAME_16 is left out of the array
> of supported command ata_scsi_write_same_xlat() prevents the use of this
> command as a passthrough command.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 201 ++++++++++++++++++++++++++++----------
>   1 file changed, 147 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a86721b5df7f..e502b8702f98 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3590,87 +3590,180 @@ 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();
> + *
> + * Note: commands that are not fully supported may be left out of this array
> + * so that they are not reported as supported for passthrough but still
> + * available through the block layer. For now, this includes the following
> + * commands:
> + *  - WRITE_SAME_16: ata_scsi_write_same_xlat() forbids passthrough commands
> + */
> +static const struct ata_scsi_cmd ata_supported_cmds[] = {
> +	{	.op = TEST_UNIT_READY,		.cdb_len = 6	},
> +	{	.op = REZERO_UNIT,		.cdb_len = 6	},
> +	{	.op = REQUEST_SENSE,		.cdb_len = 6	},
> +	{	.op = READ_6,			.cdb_len = 6	},
> +	{	.op = WRITE_6,			.cdb_len = 6	},
> +	{	.op = SEEK_6,			.cdb_len = 6	},
> +	{	.op = INQUIRY,			.cdb_len = 6	},
> +	{	.op = MODE_SELECT,		.cdb_len = 6	},
> +	{	.op = MODE_SENSE,		.cdb_len = 6	},
> +	{	.op = START_STOP,		.cdb_len = 6	},
> +	{	.op = SEND_DIAGNOSTIC,		.cdb_len = 6	},
> +	{	.op = READ_CAPACITY,		.cdb_len = 10	},
> +	{	.op = READ_10,			.cdb_len = 10	},
> +	{	.op = WRITE_10,			.cdb_len = 10	},
> +	{	.op = SEEK_10,			.cdb_len = 10	},
> +	{	.op = VERIFY,			.cdb_len = 10	},
> +	{	.op = SYNCHRONIZE_CACHE,	.cdb_len = 10	},
> +	{	.op = MODE_SELECT_10,		.cdb_len = 10	},
> +	{	.op = MODE_SENSE_10,		.cdb_len = 10	},
> +	{
> +		.op = VARIABLE_LENGTH_CMD,	.cdb_len = 32,
> +		.sa_valid = true,
> +		.sa = ATA_32
> +	},
> +	{	.op = ATA_16,			.cdb_len = 16	},
> +	{	.op = READ_16,			.cdb_len = 16	},
> +	{	.op = WRITE_16,			.cdb_len = 16	},
> +	{	.op = VERIFY_16,		.cdb_len = 16	},
> +	{	.op = SYNCHRONIZE_CACHE_16,	.cdb_len = 16	},
> +	{
> +		.op = ZBC_OUT,			.cdb_len = 16,
> +		.sa_valid = true,
> +		.sa = ZO_CLOSE_ZONE
> +	},
> +	{
> +		.op = ZBC_OUT,			.cdb_len = 16,
> +		.sa_valid = true,
> +		.sa = ZO_FINISH_ZONE
> +	},
> +	{
> +		.op = ZBC_OUT,			.cdb_len = 16,
> +		.sa_valid = true,
> +		.sa = ZO_OPEN_ZONE
> +	},
> +	{
> +		.op = ZBC_OUT,			.cdb_len = 16,
> +		.sa_valid = true,
> +		.sa = ZO_RESET_WRITE_POINTER
> +	},
> +	{
> +		.op = ZBC_IN,			.cdb_len = 16,
> +		.sa_valid = true,
> +		.sa = ZI_REPORT_ZONES
> +	},
> +	{
> +		.op = SERVICE_ACTION_IN_16,	.cdb_len = 16,
> +		.sa_valid = true,
> +		.sa = SAI_READ_CAPACITY_16
> +	},
> +	{	.op = REPORT_LUNS,		.cdb_len = 12	},
> +	{	.op = ATA_12,			.cdb_len = 12	},
> +	{	.op = SECURITY_PROTOCOL_IN,	.cdb_len = 12	},
> +	{
> +		.op = MAINTENANCE_IN,		.cdb_len = 12,
> +		.sa_valid = true,
> +		.sa = MI_REPORT_SUPPORTED_OPERATION_CODES
> +	},
> +	{	.op = SECURITY_PROTOCOL_OUT,	.cdb_len = 12	},
> +};
> +

What an unusual formatting. I would have gone for multi-line
entries even for the simple ones to keep formatting the same
for all entries.
But hey.

> +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;

That is confusing. You return 'true' here, yet once you break out
of the switch the function will return 'true', too.
Please stay with the 'break' here.

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

Same here.

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v3 2/5] ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

[Damien Le Moal]

On 7/3/26 15:44, Hannes Reinecke wrote:
>> +	{
>> +		.op = MAINTENANCE_IN,		.cdb_len = 12,
>> +		.sa_valid = true,
>> +		.sa = MI_REPORT_SUPPORTED_OPERATION_CODES
>> +	},
>> +	{	.op = SECURITY_PROTOCOL_OUT,	.cdb_len = 12	},
>> +};
>> +
> 
> What an unusual formatting. I would have gone for multi-line
> entries even for the simple ones to keep formatting the same
> for all entries.

The multi-line for everything made the array really big, and hard to read. After
trying different things, I settled on this.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 2/5] ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

[Hannes Reinecke]

On 7/3/26 9:05 AM, Damien Le Moal wrote:
> On 7/3/26 15:44, Hannes Reinecke wrote:
>>> +	{
>>> +		.op = MAINTENANCE_IN,		.cdb_len = 12,
>>> +		.sa_valid = true,
>>> +		.sa = MI_REPORT_SUPPORTED_OPERATION_CODES
>>> +	},
>>> +	{	.op = SECURITY_PROTOCOL_OUT,	.cdb_len = 12	},
>>> +};
>>> +
>>
>> What an unusual formatting. I would have gone for multi-line
>> entries even for the simple ones to keep formatting the same
>> for all entries.
> 
> The multi-line for everything made the array really big, and hard to read. After
> trying different things, I settled on this.
> 
> 
Fair enough.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCH v3 3/5] 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_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 <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/ata/libata-scsi.c | 41 ++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e502b8702f98..4d4cdf304740 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3679,32 +3679,46 @@ static const struct ata_scsi_cmd ata_supported_cmds[] = {
 	{	.op = SECURITY_PROTOCOL_OUT,	.cdb_len = 12	},
 };
 
-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_use_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_valid;
+	}
+
+	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;
 
@@ -3750,15 +3764,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_use_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

[PATCH v3 4/5] 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>
Reviewed-by: Hannes Reinecke <hare@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 4d4cdf304740..5dd76579327e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3774,6 +3774,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;
+		}
+		fallthrough;
 	case 3:
 		/* One command format */
 		sa = get_unaligned_be16(&cdb[4]);
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Reporting Option 3 fails to ignore the REQUESTED SERVICE ACTION field for commands that do not have service actions, causing supported commands to be reported as unsupported.
--

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

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.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4d4cdf3047405..5dd76579327e9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3774,6 +3774,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;
> +		}
> +		fallthrough;
>  	case 3:
>  		/* One command format */
>  		sa = get_unaligned_be16(&cdb[4]);

[Severity: Medium]
Does this code incorrectly fail to ignore the REQUESTED SERVICE ACTION field
for commands without service actions under Option 3?

If a legitimate SCSI query uses Option 3 with a non-zero service action field
for a command that does not use service actions, this block extracts the
non-zero sa value from the CDB.

Later in ata_scsi_get_supported_cmd(), the code enforces a strict match:

	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
		cmd = &ata_supported_cmds[i];
		if (cmd->op == op && cmd->sa == sa)
			return cmd;
	}

Because cmd->sa is 0 for commands without service actions, this strict match
will fail. Will this cause valid commands (like READ_10) to be incorrectly
reported as unsupported?

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

[PATCH v3 5/5] 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 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)
   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>
Reviewed-by: Hannes Reinecke <hare@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 5dd76579327e..97fa65adafe0 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3758,6 +3758,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_valid)
+			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)
@@ -3767,6 +3803,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_use_sa(cdb[3])) {
-- 
2.54.0