Skip to contentSkip to conversation

ata: libata-scsi: refactor ata_scsiop_maint_in()

PATCH Applied Niklas Cassel 2 participants 3 messages Mar 20 – Mar 23 libata

lore
Share & tools
lore

Niklas Cassel

author maintainer
#0 lore raw

PATCH 1/1 ata: libata-scsi: refactor ata_scsiop_maint_in()

ata_scsiop_maint_in() is currently quite confusing to read, because it currently only implements support for the service action REPORT SUPPORTED OPERATION CODES.

Thus, when this function is checking for "invalid command format", it is not very clear if it is an invalid command format for the MAINTENANCE IN command itself, or an invalid command format for the (currently one and only) service action/subcommand implemented for this command.

Move the service action to a separate function, so it is more clear that the "invalid command format" check is actually specific for the REPORT SUPPORTED OPERATION CODES service action.

This also makes it easier and less confusing to add support for additional service actions in the future.

Signed-off-by Niklas Cassel <[email protected]>
---
 drivers/ata/libata-scsi.c | 47 ++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 18 deletions(-)
drivers/ata/libata-scsi.c +29 -18
index 00b3ffbfe169..06f3a243f037 100644
@@ -3573,28 +3573,13 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	return 1;
 }
 
-/**
- *	ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
- *	@dev: Target device.
- *	@cmd: SCSI command of interest.
- *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
- *
- *	Yields a subset to satisfy scsi_report_opcode()
- *
- *	LOCKING:
- *	spin_lock_irqsave(host lock)
- */
-static unsigned int ata_scsiop_maint_in(struct ata_device *dev,
-					struct scsi_cmnd *cmd, u8 *rbuf)
+static unsigned int ata_scsi_report_supported_opcodes(struct ata_device *dev,
+						      struct scsi_cmnd *cmd,
+						      u8 *rbuf)
 {
 	u8 *cdb = cmd->cmnd;
 	u8 supported = 0, cdlp = 0, rwcdlp = 0;
 
-	if ((cdb[1] & 0x1f) != MI_REPORT_SUPPORTED_OPERATION_CODES) {
-		ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
-		return 0;
-	}
-
 	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);

@@ -3674,6 +3659,32 @@ static unsigned int ata_scsiop_maint_in(struct ata_device *dev,
 	return 4;
 }
 
+/**
+ *	ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
+ *	@dev: Target device.
+ *	@cmd: SCSI command of interest.
+ *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
+ *
+ *	Yields a subset to satisfy scsi_report_opcode()
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ */
+static unsigned int ata_scsiop_maint_in(struct ata_device *dev,
+					struct scsi_cmnd *cmd, u8 *rbuf)
+{
+	u8 *cdb = cmd->cmnd;
+	u8 service_action = cdb[1] & 0x1f;
+
+	switch (service_action) {
+	case MI_REPORT_SUPPORTED_OPERATION_CODES:
+		return ata_scsi_report_supported_opcodes(dev, cmd, rbuf);
+	default:
+		ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
+		return 0;
+	}
+}
+
 /**
  *	ata_scsi_report_zones_complete - convert ATA output
  *	@qc: command structure returning the data
Signature · 1 line
2.53.0
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 9 lines · Niklas Cassel

ata_scsiop_maint_in() is currently quite confusing to read, because it currently only implements support for the service action REPORT SUPPORTED OPERATION CODES.

Thus, when this function is checking for "invalid command format", it is not very clear if it is an invalid command format for the MAINTENANCE IN command itself, or an invalid command format for the (currently one and only) service action/subcommand implemented for this command.

Quoted 8 lines

Move the service action to a separate function, so it is more clear that the "invalid command format" check is actually specific for the REPORT SUPPORTED OPERATION CODES service action.

This also makes it easier and less confusing to add support for additional service actions in the future.

Signed-off-by Niklas Cassel <[email protected]>
Reviewed-by Damien Le Moal <[email protected]>
Signature · 2 lines
Damien Le Moal
Western Digital Research
author maintainer Applied
#2 lore raw
In reply to Niklas Cassel
Quoted 10 lines · Niklas Cassel

ata_scsiop_maint_in() is currently quite confusing to read, because it currently only implements support for the service action REPORT SUPPORTED OPERATION CODES.

Thus, when this function is checking for "invalid command format", it is not very clear if it is an invalid command format for the MAINTENANCE IN command itself, or an invalid command format for the (currently one and only) service action/subcommand implemented for this command.

[...]

Applied to libata/linux.git (for-7.1), thanks!

[1/1] ata: libata-scsi: refactor ata_scsiop_maint_in()

      https://git.kernel.org/libata/linux/c/a6ac0af4

Kind regards,
Niklas