Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1467343 > unrolled thread

[PATCH v6 2/4] Add support for SCT Write Same

Started byShaun Tancheff <shaun@tancheff.com>
First post2016-08-22 06:30 +0200
Last post2016-08-25 09:50 +0200
Articles 18 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun@tancheff.com> - 2016-08-22 06:30 +0200
    Re: [PATCH v6 2/4] Add support for SCT Write Same Hannes Reinecke <hare@suse.de> - 2016-08-22 08:30 +0200
    Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-22 21:30 +0200
      Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun.tancheff@seagate.com> - 2016-08-22 21:50 +0200
        Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-22 22:20 +0200
          Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun.tancheff@seagate.com> - 2016-08-23 00:10 +0200
            Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-23 01:10 +0200
              Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun.tancheff@seagate.com> - 2016-08-23 02:40 +0200
                Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-23 08:00 +0200
                  Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun.tancheff@seagate.com> - 2016-08-23 08:20 +0200
                    Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-23 10:10 +0200
    Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-23 12:50 +0200
      Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun.tancheff@seagate.com> - 2016-08-23 13:00 +0200
        Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-24 08:00 +0200
          Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-24 08:20 +0200
            Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun@tancheff.com> - 2016-08-25 00:10 +0200
              Re: [PATCH v6 2/4] Add support for SCT Write Same Tom Yan <tom.ty89@gmail.com> - 2016-08-25 08:50 +0200
                Re: [PATCH v6 2/4] Add support for SCT Write Same Shaun Tancheff <shaun.tancheff@seagate.com> - 2016-08-25 09:50 +0200

#1467343 — [PATCH v6 2/4] Add support for SCT Write Same

FromShaun Tancheff <shaun@tancheff.com>
Date2016-08-22 06:30 +0200
Subject[PATCH v6 2/4] Add support for SCT Write Same
Message-ID<s8P21-1r7-1@gated-at.bofh.it>
SATA drives may support write same via SCT. This is useful
for setting the drive contents to a specific pattern (0's).

Translate a SCSI WRITE SAME 16 command to be either a DSM TRIM
command or an SCT Write Same command.

Based on the UNMAP flag:
  - When set translate to DSM TRIM
  - When not set translate to SCT Write Same

Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com>
---
v6:
 - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de>
v5:
 - Addressed review comments
 - Report support for ZBC only for zoned devices.
 - kmap page during rewrite
 - Fix unmap set to require trim or error, if not unmap then sct write
   same or error.
v4:
 - Added partial MAINTENANCE_IN opcode simulation
 - Dropped all changes in drivers/scsi/*
 - Changed to honor the UNMAP flag -> TRIM, no UNMAP -> SCT.
v3:
 - Demux UNMAP/TRIM from WRITE SAME
v2:
 - Remove fugly ata hacking from sd.c

 drivers/ata/libata-scsi.c | 199 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/ata.h       |  43 ++++++++++
 2 files changed, 213 insertions(+), 29 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7990cb2..ebf1a04 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1159,8 +1159,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
 {
 	sdev->use_10_for_rw = 1;
 	sdev->use_10_for_ms = 1;
-	sdev->no_report_opcodes = 1;
-	sdev->no_write_same = 1;
 
 	/* Schedule policy is determined by ->qc_defer() callback and
 	 * it needs to see every deferred qc.  Set dev_blocked to 1 to
@@ -3287,7 +3285,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
  * @cmd: SCSI command being translated
  * @num: Maximum number of entries (nominally 64).
  * @sector: Starting sector
- * @count: Total Range of request
+ * @count: Total Range of request in logical sectors
  *
  * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted
  * descriptor.
@@ -3330,6 +3328,45 @@ static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num,
 	return used_bytes;
 }
 
+/**
+ * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
+ * @cmd: SCSI command being translated
+ * @lba: Starting sector
+ * @num: Number of logical sectors to be zero'd.
+ *
+ * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
+ * descriptor.
+ * NOTE: Writes a pattern (0's) in the foreground.
+ *       Large write-same requents can timeout.
+ */
+static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
+{
+	u16 *sctpg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
+	sctpg = ((void *)ata_scsi_rbuf);
+
+	put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
+	put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
+	put_unaligned_le64(lba,     &sctpg[2]);
+	put_unaligned_le64(num,     &sctpg[6]);
+	put_unaligned_le32(0u,      &sctpg[10]);
+
+	sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);
+	spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
+}
+
+/**
+ * ata_scsi_write_same_xlat() - SATL Write Same to ATA SCT Write Same
+ * @qc: Command to be translated
+ *
+ * Translate a SCSI WRITE SAME command to be either a DSM TRIM command or
+ * an SCT Write Same command.
+ * Based on WRITE SAME has the UNMAP flag
+ *   When set translate to DSM TRIM
+ *   When clear translate to SCT Write Same
+ */
 static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 {
 	struct ata_taskfile *tf = &qc->tf;
@@ -3342,6 +3379,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	u32 size;
 	u16 fp;
 	u8 bp = 0xff;
+	u8 unmap = cdb[1] & 0x8;
 
 	/* we may not issue DMA commands if no DMA mode is set */
 	if (unlikely(!dev->dma_mode))
@@ -3353,11 +3391,26 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	}
 	scsi_16_lba_len(cdb, &block, &n_block);
 
-	/* for now we only support WRITE SAME with the unmap bit set */
-	if (unlikely(!(cdb[1] & 0x8))) {
-		fp = 1;
-		bp = 3;
-		goto invalid_fld;
+	if (unmap) {
+		/* If trim is not enabled the cmd is invalid. */
+		if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
+		    !ata_id_has_trim(dev->id)) {
+			fp = 1;
+			bp = 3;
+			goto invalid_fld;
+		}
+		/* If the request is too large the cmd is invalid */
+		if (n_block > 0xffff * trmax) {
+			fp = 2;
+			goto invalid_fld;
+		}
+	} else {
+		/* If write same is not available the cmd is invalid */
+		if (!ata_id_sct_write_same(dev->id)) {
+			fp = 1;
+			bp = 3;
+			goto invalid_fld;
+		}
 	}
 
 	/*
@@ -3367,30 +3420,42 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	if (!scsi_sg_count(scmd))
 		goto invalid_param_len;
 
-	if (n_block <= 0xffff * trmax) {
+	if (unmap) {
 		size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
+		if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
+			/* Newer devices support queued TRIM commands */
+			tf->protocol = ATA_PROT_NCQ;
+			tf->command = ATA_CMD_FPDMA_SEND;
+			tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
+			tf->nsect = qc->tag << 3;
+			tf->hob_feature = (size / 512) >> 8;
+			tf->feature = size / 512;
+
+			tf->auxiliary = 1;
+		} else {
+			tf->protocol = ATA_PROT_DMA;
+			tf->hob_feature = 0;
+			tf->feature = ATA_DSM_TRIM;
+			tf->hob_nsect = (size / 512) >> 8;
+			tf->nsect = size / 512;
+			tf->command = ATA_CMD_DSM;
+		}
 	} else {
-		fp = 2;
-		goto invalid_fld;
-	}
-
-	if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
-		/* Newer devices support queued TRIM commands */
-		tf->protocol = ATA_PROT_NCQ;
-		tf->command = ATA_CMD_FPDMA_SEND;
-		tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
-		tf->nsect = qc->tag << 3;
-		tf->hob_feature = (size / 512) >> 8;
-		tf->feature = size / 512;
+		ata_format_sct_write_same(scmd, block, n_block);
 
-		tf->auxiliary = 1;
-	} else {
-		tf->protocol = ATA_PROT_DMA;
 		tf->hob_feature = 0;
-		tf->feature = ATA_DSM_TRIM;
-		tf->hob_nsect = (size / 512) >> 8;
-		tf->nsect = size / 512;
-		tf->command = ATA_CMD_DSM;
+		tf->feature = 0;
+		tf->hob_nsect = 0;
+		tf->nsect = 1;
+		tf->lbah = 0;
+		tf->lbam = 0;
+		tf->lbal = ATA_CMD_STANDBYNOW1;
+		tf->hob_lbah = 0;
+		tf->hob_lbam = 0;
+		tf->hob_lbal = 0;
+		tf->device = ATA_CMD_STANDBYNOW1;
+		tf->protocol = ATA_PROT_DMA;
+		tf->command = ATA_CMD_WRITE_LOG_DMA_EXT;
 	}
 
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
@@ -3414,6 +3479,76 @@ invalid_opcode:
 }
 
 /**
+ *	ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
+ *	@args: device MAINTENANCE_IN data / 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_scsi_args *args, u8 *rbuf)
+{
+	struct ata_device *dev = args->dev;
+	u8 *cdb = args->cmd->cmnd;
+	u8 supported = 0;
+	unsigned int err = 0;
+
+	if (cdb[2] != 1) {
+		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
+		err = 2;
+		goto out;
+	}
+	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 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 READ_16:
+	case WRITE_6:
+	case WRITE_10:
+	case WRITE_16:
+	case ATA_12:
+	case ATA_16:
+	case VERIFY:
+	case VERIFY_16:
+	case MODE_SELECT:
+	case MODE_SELECT_10:
+	case START_STOP:
+		supported = 3;
+		break;
+	case WRITE_SAME_16:
+		if (ata_id_sct_write_same(dev->id))
+			supported = 3;
+		break;
+	case ZBC_IN:
+	case ZBC_OUT:
+		if (ata_id_zoned_cap(dev->id) ||
+		    dev->class == ATA_DEV_ZAC)
+			supported = 3;
+		break;
+	default:
+		break;
+	}
+out:
+	rbuf[1] = supported; /* supported */
+	return err;
+}
+
+/**
  *	ata_scsi_report_zones_complete - convert ATA output
  *	@qc: command structure returning the data
  *
@@ -4193,6 +4328,13 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 			ata_scsi_invalid_field(dev, cmd, 1);
 		break;
 
+	case MAINTENANCE_IN:
+		if (scsicmd[1] == MI_REPORT_SUPPORTED_OPERATION_CODES)
+			ata_scsi_rbuf_fill(&args, ata_scsiop_maint_in);
+		else
+			ata_scsi_invalid_field(dev, cmd, 1);
+		break;
+
 	/* all other commands */
 	default:
 		ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x20, 0x0);
@@ -4225,7 +4367,6 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
 		shost->max_lun = 1;
 		shost->max_channel = 1;
 		shost->max_cmd_len = 16;
-		shost->no_write_same = 1;
 
 		/* Schedule policy is determined by ->qc_defer()
 		 * callback and it needs to see every deferred qc.
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 45a1d71..fdb1803 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -105,6 +105,7 @@ enum {
 	ATA_ID_CFA_KEY_MGMT	= 162,
 	ATA_ID_CFA_MODES	= 163,
 	ATA_ID_DATA_SET_MGMT	= 169,
+	ATA_ID_SCT_CMD_XPORT	= 206,
 	ATA_ID_ROT_SPEED	= 217,
 	ATA_ID_PIO4		= (1 << 1),
 
@@ -789,6 +790,48 @@ static inline bool ata_id_sense_reporting_enabled(const u16 *id)
 }
 
 /**
+ *
+ * Word: 206 - SCT Command Transport
+ *    15:12 - Vendor Specific
+ *     11:6 - Reserved
+ *        5 - SCT Command Transport Data Tables supported
+ *        4 - SCT Command Transport Features Control supported
+ *        3 - SCT Command Transport Error Recovery Control supported
+ *        2 - SCT Command Transport Write Same supported
+ *        1 - SCT Command Transport Long Sector Access supported
+ *        0 - SCT Command Transport supported
+ */
+static inline bool ata_id_sct_data_tables(const u16 *id)
+{
+	return id[ATA_ID_SCT_CMD_XPORT] & (1 << 5) ? true : false;
+}
+
+static inline bool ata_id_sct_features_ctrl(const u16 *id)
+{
+	return id[ATA_ID_SCT_CMD_XPORT] & (1 << 4) ? true : false;
+}
+
+static inline bool ata_id_sct_error_recovery_ctrl(const u16 *id)
+{
+	return id[ATA_ID_SCT_CMD_XPORT] & (1 << 3) ? true : false;
+}
+
+static inline bool ata_id_sct_write_same(const u16 *id)
+{
+	return id[ATA_ID_SCT_CMD_XPORT] & (1 << 2) ? true : false;
+}
+
+static inline bool ata_id_sct_long_sector_access(const u16 *id)
+{
+	return id[ATA_ID_SCT_CMD_XPORT] & (1 << 1) ? true : false;
+}
+
+static inline bool ata_id_sct_supported(const u16 *id)
+{
+	return id[ATA_ID_SCT_CMD_XPORT] & (1 << 0) ? true : false;
+}
+
+/**
  *	ata_id_major_version	-	get ATA level of drive
  *	@id: Identify data
  *
-- 
2.9.3

[toc] | [next] | [standalone]

#1467386

FromHannes Reinecke <hare@suse.de>
Date2016-08-22 08:30 +0200
Message-ID<s8QU9-2AJ-13@gated-at.bofh.it>
In reply to#1467343
On 08/22/2016 06:23 AM, Shaun Tancheff wrote:
> SATA drives may support write same via SCT. This is useful
> for setting the drive contents to a specific pattern (0's).
> 
> Translate a SCSI WRITE SAME 16 command to be either a DSM TRIM
> command or an SCT Write Same command.
> 
> Based on the UNMAP flag:
>   - When set translate to DSM TRIM
>   - When not set translate to SCT Write Same
> 
> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com>
> ---
> v6:
>  - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de>
> v5:
>  - Addressed review comments
>  - Report support for ZBC only for zoned devices.
>  - kmap page during rewrite
>  - Fix unmap set to require trim or error, if not unmap then sct write
>    same or error.
> v4:
>  - Added partial MAINTENANCE_IN opcode simulation
>  - Dropped all changes in drivers/scsi/*
>  - Changed to honor the UNMAP flag -> TRIM, no UNMAP -> SCT.
> v3:
>  - Demux UNMAP/TRIM from WRITE SAME
> v2:
>  - Remove fugly ata hacking from sd.c
> 
>  drivers/ata/libata-scsi.c | 199 +++++++++++++++++++++++++++++++++++++++-------
>  include/linux/ata.h       |  43 ++++++++++
>  2 files changed, 213 insertions(+), 29 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1467992

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-22 21:30 +0200
Message-ID<s9350-22o-35@gated-at.bofh.it>
In reply to#1467343
On 22 August 2016 at 12:23, Shaun Tancheff <shaun@tancheff.com> wrote:
> SATA drives may support write same via SCT. This is useful
> for setting the drive contents to a specific pattern (0's).
>
> Translate a SCSI WRITE SAME 16 command to be either a DSM TRIM
> command or an SCT Write Same command.
>
> Based on the UNMAP flag:
>   - When set translate to DSM TRIM
>   - When not set translate to SCT Write Same
>
> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com>
> ---
> v6:
>  - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de>
> v5:
>  - Addressed review comments
>  - Report support for ZBC only for zoned devices.
>  - kmap page during rewrite
>  - Fix unmap set to require trim or error, if not unmap then sct write
>    same or error.
> v4:
>  - Added partial MAINTENANCE_IN opcode simulation
>  - Dropped all changes in drivers/scsi/*
>  - Changed to honor the UNMAP flag -> TRIM, no UNMAP -> SCT.
> v3:
>  - Demux UNMAP/TRIM from WRITE SAME
> v2:
>  - Remove fugly ata hacking from sd.c
>
>  drivers/ata/libata-scsi.c | 199 +++++++++++++++++++++++++++++++++++++++-------
>  include/linux/ata.h       |  43 ++++++++++
>  2 files changed, 213 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7990cb2..ebf1a04 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1159,8 +1159,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
>  {
>         sdev->use_10_for_rw = 1;
>         sdev->use_10_for_ms = 1;
> -       sdev->no_report_opcodes = 1;
> -       sdev->no_write_same = 1;
>
>         /* Schedule policy is determined by ->qc_defer() callback and
>          * it needs to see every deferred qc.  Set dev_blocked to 1 to
> @@ -3287,7 +3285,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>   * @cmd: SCSI command being translated
>   * @num: Maximum number of entries (nominally 64).
>   * @sector: Starting sector
> - * @count: Total Range of request
> + * @count: Total Range of request in logical sectors
>   *
>   * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted
>   * descriptor.
> @@ -3330,6 +3328,45 @@ static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num,
>         return used_bytes;
>  }
>
> +/**
> + * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
> + * @cmd: SCSI command being translated
> + * @lba: Starting sector
> + * @num: Number of logical sectors to be zero'd.
> + *
> + * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
> + * descriptor.
> + * NOTE: Writes a pattern (0's) in the foreground.
> + *       Large write-same requents can timeout.
> + */
> +static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
> +{
> +       u16 *sctpg;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
> +       sctpg = ((void *)ata_scsi_rbuf);
> +
> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
> +       put_unaligned_le64(lba,     &sctpg[2]);
> +       put_unaligned_le64(num,     &sctpg[6]);
> +       put_unaligned_le32(0u,      &sctpg[10]);
> +
> +       sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);
> +       spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
> +}
> +
> +/**
> + * ata_scsi_write_same_xlat() - SATL Write Same to ATA SCT Write Same
> + * @qc: Command to be translated
> + *
> + * Translate a SCSI WRITE SAME command to be either a DSM TRIM command or
> + * an SCT Write Same command.
> + * Based on WRITE SAME has the UNMAP flag
> + *   When set translate to DSM TRIM
> + *   When clear translate to SCT Write Same
> + */
>  static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>  {
>         struct ata_taskfile *tf = &qc->tf;
> @@ -3342,6 +3379,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>         u32 size;
>         u16 fp;
>         u8 bp = 0xff;
> +       u8 unmap = cdb[1] & 0x8;
>
>         /* we may not issue DMA commands if no DMA mode is set */
>         if (unlikely(!dev->dma_mode))
> @@ -3353,11 +3391,26 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>         }
>         scsi_16_lba_len(cdb, &block, &n_block);
>
> -       /* for now we only support WRITE SAME with the unmap bit set */
> -       if (unlikely(!(cdb[1] & 0x8))) {
> -               fp = 1;
> -               bp = 3;
> -               goto invalid_fld;
> +       if (unmap) {
> +               /* If trim is not enabled the cmd is invalid. */
> +               if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
> +                   !ata_id_has_trim(dev->id)) {
> +                       fp = 1;
> +                       bp = 3;
> +                       goto invalid_fld;
> +               }
> +               /* If the request is too large the cmd is invalid */
> +               if (n_block > 0xffff * trmax) {
> +                       fp = 2;
> +                       goto invalid_fld;
> +               }

This response should be generally applied to the Write Same (16)
translation, since it is required by SBC,

> +       } else {
> +               /* If write same is not available the cmd is invalid */
> +               if (!ata_id_sct_write_same(dev->id)) {
> +                       fp = 1;
> +                       bp = 3;
> +                       goto invalid_fld;
> +               }

therefore, you should add an n_block check here as well, if you are
going to advertise an Maximum Write Same Length even when the device
supports only SCT Write Same but not TRIM. Most likely you would want
to simply move the existing check one-level up (if the same limit is
advertised no matter TRIM is supported not or not).

>         }
>
>         /*
> @@ -3367,30 +3420,42 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>         if (!scsi_sg_count(scmd))
>                 goto invalid_param_len;
>
> -       if (n_block <= 0xffff * trmax) {
> +       if (unmap) {
>                 size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
> +               if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
> +                       /* Newer devices support queued TRIM commands */
> +                       tf->protocol = ATA_PROT_NCQ;
> +                       tf->command = ATA_CMD_FPDMA_SEND;
> +                       tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
> +                       tf->nsect = qc->tag << 3;
> +                       tf->hob_feature = (size / 512) >> 8;
> +                       tf->feature = size / 512;
> +
> +                       tf->auxiliary = 1;
> +               } else {
> +                       tf->protocol = ATA_PROT_DMA;
> +                       tf->hob_feature = 0;
> +                       tf->feature = ATA_DSM_TRIM;
> +                       tf->hob_nsect = (size / 512) >> 8;
> +                       tf->nsect = size / 512;
> +                       tf->command = ATA_CMD_DSM;
> +               }
>         } else {
> -               fp = 2;
> -               goto invalid_fld;
> -       }
> -
> -       if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
> -               /* Newer devices support queued TRIM commands */
> -               tf->protocol = ATA_PROT_NCQ;
> -               tf->command = ATA_CMD_FPDMA_SEND;
> -               tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
> -               tf->nsect = qc->tag << 3;
> -               tf->hob_feature = (size / 512) >> 8;
> -               tf->feature = size / 512;
> +               ata_format_sct_write_same(scmd, block, n_block);
>
> -               tf->auxiliary = 1;
> -       } else {
> -               tf->protocol = ATA_PROT_DMA;
>                 tf->hob_feature = 0;
> -               tf->feature = ATA_DSM_TRIM;
> -               tf->hob_nsect = (size / 512) >> 8;
> -               tf->nsect = size / 512;
> -               tf->command = ATA_CMD_DSM;
> +               tf->feature = 0;
> +               tf->hob_nsect = 0;
> +               tf->nsect = 1;
> +               tf->lbah = 0;
> +               tf->lbam = 0;
> +               tf->lbal = ATA_CMD_STANDBYNOW1;
> +               tf->hob_lbah = 0;
> +               tf->hob_lbam = 0;
> +               tf->hob_lbal = 0;
> +               tf->device = ATA_CMD_STANDBYNOW1;
> +               tf->protocol = ATA_PROT_DMA;
> +               tf->command = ATA_CMD_WRITE_LOG_DMA_EXT;
>         }
>
>         tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
> @@ -3414,6 +3479,76 @@ invalid_opcode:
>  }
>
>  /**
> + *     ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
> + *     @args: device MAINTENANCE_IN data / 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_scsi_args *args, u8 *rbuf)
> +{
> +       struct ata_device *dev = args->dev;
> +       u8 *cdb = args->cmd->cmnd;
> +       u8 supported = 0;
> +       unsigned int err = 0;
> +
> +       if (cdb[2] != 1) {
> +               ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
> +               err = 2;
> +               goto out;
> +       }
> +       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 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 READ_16:
> +       case WRITE_6:
> +       case WRITE_10:
> +       case WRITE_16:
> +       case ATA_12:
> +       case ATA_16:
> +       case VERIFY:
> +       case VERIFY_16:
> +       case MODE_SELECT:
> +       case MODE_SELECT_10:
> +       case START_STOP:
> +               supported = 3;
> +               break;
> +       case WRITE_SAME_16:
> +               if (ata_id_sct_write_same(dev->id))
> +                       supported = 3;
> +               break;
> +       case ZBC_IN:
> +       case ZBC_OUT:
> +               if (ata_id_zoned_cap(dev->id) ||
> +                   dev->class == ATA_DEV_ZAC)
> +                       supported = 3;
> +               break;
> +       default:
> +               break;
> +       }
> +out:
> +       rbuf[1] = supported; /* supported */
> +       return err;
> +}
> +
> +/**
>   *     ata_scsi_report_zones_complete - convert ATA output
>   *     @qc: command structure returning the data
>   *
> @@ -4193,6 +4328,13 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
>                         ata_scsi_invalid_field(dev, cmd, 1);
>                 break;
>
> +       case MAINTENANCE_IN:
> +               if (scsicmd[1] == MI_REPORT_SUPPORTED_OPERATION_CODES)
> +                       ata_scsi_rbuf_fill(&args, ata_scsiop_maint_in);
> +               else
> +                       ata_scsi_invalid_field(dev, cmd, 1);
> +               break;
> +
>         /* all other commands */
>         default:
>                 ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x20, 0x0);
> @@ -4225,7 +4367,6 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
>                 shost->max_lun = 1;
>                 shost->max_channel = 1;
>                 shost->max_cmd_len = 16;
> -               shost->no_write_same = 1;
>
>                 /* Schedule policy is determined by ->qc_defer()
>                  * callback and it needs to see every deferred qc.
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 45a1d71..fdb1803 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -105,6 +105,7 @@ enum {
>         ATA_ID_CFA_KEY_MGMT     = 162,
>         ATA_ID_CFA_MODES        = 163,
>         ATA_ID_DATA_SET_MGMT    = 169,
> +       ATA_ID_SCT_CMD_XPORT    = 206,
>         ATA_ID_ROT_SPEED        = 217,
>         ATA_ID_PIO4             = (1 << 1),
>
> @@ -789,6 +790,48 @@ static inline bool ata_id_sense_reporting_enabled(const u16 *id)
>  }
>
>  /**
> + *
> + * Word: 206 - SCT Command Transport
> + *    15:12 - Vendor Specific
> + *     11:6 - Reserved
> + *        5 - SCT Command Transport Data Tables supported
> + *        4 - SCT Command Transport Features Control supported
> + *        3 - SCT Command Transport Error Recovery Control supported
> + *        2 - SCT Command Transport Write Same supported
> + *        1 - SCT Command Transport Long Sector Access supported
> + *        0 - SCT Command Transport supported
> + */
> +static inline bool ata_id_sct_data_tables(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 5) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_features_ctrl(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 4) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_error_recovery_ctrl(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 3) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_write_same(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 2) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_long_sector_access(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 1) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_supported(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 0) ? true : false;
> +}
> +
> +/**
>   *     ata_id_major_version    -       get ATA level of drive
>   *     @id: Identify data
>   *
> --
> 2.9.3
>

[toc] | [prev] | [next] | [standalone]

#1468006

FromShaun Tancheff <shaun.tancheff@seagate.com>
Date2016-08-22 21:50 +0200
Message-ID<s93om-2au-21@gated-at.bofh.it>
In reply to#1467992
On Mon, Aug 22, 2016 at 2:20 PM, Tom Yan <tom.ty89@gmail.com> wrote:
> On 22 August 2016 at 12:23, Shaun Tancheff <shaun@tancheff.com> wrote:
>> SATA drives may support write same via SCT. This is useful
>> for setting the drive contents to a specific pattern (0's).
>>
>> Translate a SCSI WRITE SAME 16 command to be either a DSM TRIM
>> command or an SCT Write Same command.
>>
>> Based on the UNMAP flag:
>>   - When set translate to DSM TRIM
>>   - When not set translate to SCT Write Same
>>
>> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com>
>> ---
>> v6:
>>  - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de>
>> v5:
>>  - Addressed review comments
>>  - Report support for ZBC only for zoned devices.
>>  - kmap page during rewrite
>>  - Fix unmap set to require trim or error, if not unmap then sct write
>>    same or error.
>> v4:
>>  - Added partial MAINTENANCE_IN opcode simulation
>>  - Dropped all changes in drivers/scsi/*
>>  - Changed to honor the UNMAP flag -> TRIM, no UNMAP -> SCT.
>> v3:
>>  - Demux UNMAP/TRIM from WRITE SAME
>> v2:
>>  - Remove fugly ata hacking from sd.c
>>
>>  drivers/ata/libata-scsi.c | 199 +++++++++++++++++++++++++++++++++++++++-------
>>  include/linux/ata.h       |  43 ++++++++++
>>  2 files changed, 213 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 7990cb2..ebf1a04 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -1159,8 +1159,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
>>  {
>>         sdev->use_10_for_rw = 1;
>>         sdev->use_10_for_ms = 1;
>> -       sdev->no_report_opcodes = 1;
>> -       sdev->no_write_same = 1;
>>
>>         /* Schedule policy is determined by ->qc_defer() callback and
>>          * it needs to see every deferred qc.  Set dev_blocked to 1 to
>> @@ -3287,7 +3285,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>>   * @cmd: SCSI command being translated
>>   * @num: Maximum number of entries (nominally 64).
>>   * @sector: Starting sector
>> - * @count: Total Range of request
>> + * @count: Total Range of request in logical sectors
>>   *
>>   * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted
>>   * descriptor.
>> @@ -3330,6 +3328,45 @@ static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num,
>>         return used_bytes;
>>  }
>>
>> +/**
>> + * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
>> + * @cmd: SCSI command being translated
>> + * @lba: Starting sector
>> + * @num: Number of logical sectors to be zero'd.
>> + *
>> + * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
>> + * descriptor.
>> + * NOTE: Writes a pattern (0's) in the foreground.
>> + *       Large write-same requents can timeout.
>> + */
>> +static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
>> +{
>> +       u16 *sctpg;
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
>> +       sctpg = ((void *)ata_scsi_rbuf);
>> +
>> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
>> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
>> +       put_unaligned_le64(lba,     &sctpg[2]);
>> +       put_unaligned_le64(num,     &sctpg[6]);
>> +       put_unaligned_le32(0u,      &sctpg[10]);
>> +
>> +       sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);
>> +       spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
>> +}
>> +
>> +/**
>> + * ata_scsi_write_same_xlat() - SATL Write Same to ATA SCT Write Same
>> + * @qc: Command to be translated
>> + *
>> + * Translate a SCSI WRITE SAME command to be either a DSM TRIM command or
>> + * an SCT Write Same command.
>> + * Based on WRITE SAME has the UNMAP flag
>> + *   When set translate to DSM TRIM
>> + *   When clear translate to SCT Write Same
>> + */
>>  static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>>  {
>>         struct ata_taskfile *tf = &qc->tf;
>> @@ -3342,6 +3379,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>>         u32 size;
>>         u16 fp;
>>         u8 bp = 0xff;
>> +       u8 unmap = cdb[1] & 0x8;
>>
>>         /* we may not issue DMA commands if no DMA mode is set */
>>         if (unlikely(!dev->dma_mode))
>> @@ -3353,11 +3391,26 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>>         }
>>         scsi_16_lba_len(cdb, &block, &n_block);
>>
>> -       /* for now we only support WRITE SAME with the unmap bit set */
>> -       if (unlikely(!(cdb[1] & 0x8))) {
>> -               fp = 1;
>> -               bp = 3;
>> -               goto invalid_fld;
>> +       if (unmap) {
>> +               /* If trim is not enabled the cmd is invalid. */
>> +               if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
>> +                   !ata_id_has_trim(dev->id)) {
>> +                       fp = 1;
>> +                       bp = 3;
>> +                       goto invalid_fld;
>> +               }
>> +               /* If the request is too large the cmd is invalid */
>> +               if (n_block > 0xffff * trmax) {
>> +                       fp = 2;
>> +                       goto invalid_fld;
>> +               }
>
> This response should be generally applied to the Write Same (16)
> translation, since it is required by SBC,
>
>> +       } else {
>> +               /* If write same is not available the cmd is invalid */
>> +               if (!ata_id_sct_write_same(dev->id)) {
>> +                       fp = 1;
>> +                       bp = 3;
>> +                       goto invalid_fld;
>> +               }
>
> therefore, you should add an n_block check here as well, if you are
> going to advertise an Maximum Write Same Length even when the device
> supports only SCT Write Same but not TRIM. Most likely you would want
> to simply move the existing check one-level up (if the same limit is
> advertised no matter TRIM is supported not or not).

Why would we enforce upper level limits on something that doesn't
have any?

If the upper level, or SG_IO, chooses to set a timeout of 10 hours and
wipe a whole disk it should be free to do so.

>>         }
>>
>>         /*
>> @@ -3367,30 +3420,42 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>>         if (!scsi_sg_count(scmd))
>>                 goto invalid_param_len;
>>
>> -       if (n_block <= 0xffff * trmax) {
>> +       if (unmap) {
>>                 size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
>> +               if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
>> +                       /* Newer devices support queued TRIM commands */
>> +                       tf->protocol = ATA_PROT_NCQ;
>> +                       tf->command = ATA_CMD_FPDMA_SEND;
>> +                       tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
>> +                       tf->nsect = qc->tag << 3;
>> +                       tf->hob_feature = (size / 512) >> 8;
>> +                       tf->feature = size / 512;
>> +
>> +                       tf->auxiliary = 1;
>> +               } else {
>> +                       tf->protocol = ATA_PROT_DMA;
>> +                       tf->hob_feature = 0;
>> +                       tf->feature = ATA_DSM_TRIM;
>> +                       tf->hob_nsect = (size / 512) >> 8;
>> +                       tf->nsect = size / 512;
>> +                       tf->command = ATA_CMD_DSM;
>> +               }
>>         } else {
>> -               fp = 2;
>> -               goto invalid_fld;
>> -       }
>> -
>> -       if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
>> -               /* Newer devices support queued TRIM commands */
>> -               tf->protocol = ATA_PROT_NCQ;
>> -               tf->command = ATA_CMD_FPDMA_SEND;
>> -               tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
>> -               tf->nsect = qc->tag << 3;
>> -               tf->hob_feature = (size / 512) >> 8;
>> -               tf->feature = size / 512;
>> +               ata_format_sct_write_same(scmd, block, n_block);
>>
>> -               tf->auxiliary = 1;
>> -       } else {
>> -               tf->protocol = ATA_PROT_DMA;
>>                 tf->hob_feature = 0;
>> -               tf->feature = ATA_DSM_TRIM;
>> -               tf->hob_nsect = (size / 512) >> 8;
>> -               tf->nsect = size / 512;
>> -               tf->command = ATA_CMD_DSM;
>> +               tf->feature = 0;
>> +               tf->hob_nsect = 0;
>> +               tf->nsect = 1;
>> +               tf->lbah = 0;
>> +               tf->lbam = 0;
>> +               tf->lbal = ATA_CMD_STANDBYNOW1;
>> +               tf->hob_lbah = 0;
>> +               tf->hob_lbam = 0;
>> +               tf->hob_lbal = 0;
>> +               tf->device = ATA_CMD_STANDBYNOW1;
>> +               tf->protocol = ATA_PROT_DMA;
>> +               tf->command = ATA_CMD_WRITE_LOG_DMA_EXT;
>>         }
>>
>>         tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
>> @@ -3414,6 +3479,76 @@ invalid_opcode:
>>  }
>>
>>  /**
>> + *     ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
>> + *     @args: device MAINTENANCE_IN data / 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_scsi_args *args, u8 *rbuf)
>> +{
>> +       struct ata_device *dev = args->dev;
>> +       u8 *cdb = args->cmd->cmnd;
>> +       u8 supported = 0;
>> +       unsigned int err = 0;
>> +
>> +       if (cdb[2] != 1) {
>> +               ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
>> +               err = 2;
>> +               goto out;
>> +       }
>> +       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 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 READ_16:
>> +       case WRITE_6:
>> +       case WRITE_10:
>> +       case WRITE_16:
>> +       case ATA_12:
>> +       case ATA_16:
>> +       case VERIFY:
>> +       case VERIFY_16:
>> +       case MODE_SELECT:
>> +       case MODE_SELECT_10:
>> +       case START_STOP:
>> +               supported = 3;
>> +               break;
>> +       case WRITE_SAME_16:
>> +               if (ata_id_sct_write_same(dev->id))
>> +                       supported = 3;
>> +               break;
>> +       case ZBC_IN:
>> +       case ZBC_OUT:
>> +               if (ata_id_zoned_cap(dev->id) ||
>> +                   dev->class == ATA_DEV_ZAC)
>> +                       supported = 3;
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +out:
>> +       rbuf[1] = supported; /* supported */
>> +       return err;
>> +}
>> +
>> +/**
>>   *     ata_scsi_report_zones_complete - convert ATA output
>>   *     @qc: command structure returning the data
>>   *
>> @@ -4193,6 +4328,13 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
>>                         ata_scsi_invalid_field(dev, cmd, 1);
>>                 break;
>>
>> +       case MAINTENANCE_IN:
>> +               if (scsicmd[1] == MI_REPORT_SUPPORTED_OPERATION_CODES)
>> +                       ata_scsi_rbuf_fill(&args, ata_scsiop_maint_in);
>> +               else
>> +                       ata_scsi_invalid_field(dev, cmd, 1);
>> +               break;
>> +
>>         /* all other commands */
>>         default:
>>                 ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x20, 0x0);
>> @@ -4225,7 +4367,6 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
>>                 shost->max_lun = 1;
>>                 shost->max_channel = 1;
>>                 shost->max_cmd_len = 16;
>> -               shost->no_write_same = 1;
>>
>>                 /* Schedule policy is determined by ->qc_defer()
>>                  * callback and it needs to see every deferred qc.
>> diff --git a/include/linux/ata.h b/include/linux/ata.h
>> index 45a1d71..fdb1803 100644
>> --- a/include/linux/ata.h
>> +++ b/include/linux/ata.h
>> @@ -105,6 +105,7 @@ enum {
>>         ATA_ID_CFA_KEY_MGMT     = 162,
>>         ATA_ID_CFA_MODES        = 163,
>>         ATA_ID_DATA_SET_MGMT    = 169,
>> +       ATA_ID_SCT_CMD_XPORT    = 206,
>>         ATA_ID_ROT_SPEED        = 217,
>>         ATA_ID_PIO4             = (1 << 1),
>>
>> @@ -789,6 +790,48 @@ static inline bool ata_id_sense_reporting_enabled(const u16 *id)
>>  }
>>
>>  /**
>> + *
>> + * Word: 206 - SCT Command Transport
>> + *    15:12 - Vendor Specific
>> + *     11:6 - Reserved
>> + *        5 - SCT Command Transport Data Tables supported
>> + *        4 - SCT Command Transport Features Control supported
>> + *        3 - SCT Command Transport Error Recovery Control supported
>> + *        2 - SCT Command Transport Write Same supported
>> + *        1 - SCT Command Transport Long Sector Access supported
>> + *        0 - SCT Command Transport supported
>> + */
>> +static inline bool ata_id_sct_data_tables(const u16 *id)
>> +{
>> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 5) ? true : false;
>> +}
>> +
>> +static inline bool ata_id_sct_features_ctrl(const u16 *id)
>> +{
>> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 4) ? true : false;
>> +}
>> +
>> +static inline bool ata_id_sct_error_recovery_ctrl(const u16 *id)
>> +{
>> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 3) ? true : false;
>> +}
>> +
>> +static inline bool ata_id_sct_write_same(const u16 *id)
>> +{
>> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 2) ? true : false;
>> +}
>> +
>> +static inline bool ata_id_sct_long_sector_access(const u16 *id)
>> +{
>> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 1) ? true : false;
>> +}
>> +
>> +static inline bool ata_id_sct_supported(const u16 *id)
>> +{
>> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 0) ? true : false;
>> +}
>> +
>> +/**
>>   *     ata_id_major_version    -       get ATA level of drive
>>   *     @id: Identify data
>>   *
>> --
>> 2.9.3
>>



-- 
Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468019

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-22 22:20 +0200
Message-ID<s93Rn-2Ah-7@gated-at.bofh.it>
In reply to#1468006
On 23 August 2016 at 03:43, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
>>> +       if (unmap) {
>>> +               /* If trim is not enabled the cmd is invalid. */
>>> +               if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
>>> +                   !ata_id_has_trim(dev->id)) {
>>> +                       fp = 1;
>>> +                       bp = 3;
>>> +                       goto invalid_fld;
>>> +               }
>>> +               /* If the request is too large the cmd is invalid */
>>> +               if (n_block > 0xffff * trmax) {
>>> +                       fp = 2;
>>> +                       goto invalid_fld;
>>> +               }
>>
>> This response should be generally applied to the Write Same (16)
>> translation, since it is required by SBC,
>>
>>> +       } else {
>>> +               /* If write same is not available the cmd is invalid */
>>> +               if (!ata_id_sct_write_same(dev->id)) {
>>> +                       fp = 1;
>>> +                       bp = 3;
>>> +                       goto invalid_fld;
>>> +               }
>>
>> therefore, you should add an n_block check here as well, if you are
>> going to advertise an Maximum Write Same Length even when the device
>> supports only SCT Write Same but not TRIM. Most likely you would want
>> to simply move the existing check one-level up (if the same limit is
>> advertised no matter TRIM is supported not or not).
>
> Why would we enforce upper level limits on something that doesn't
> have any?

If we advertise a limit in our SATL, it makes sense that we should
make sure the behaviour is consistent when we issue a write same
through the block layer / ioctl and when we issue a SCSI Write Same
command directly (e.g. with sg_write_same). IMHO that's pretty much
why SBC would mandate such behaviour as well.

>
> If the upper level, or SG_IO, chooses to set a timeout of 10 hours and
> wipe a whole disk it should be free to do so.
>

That's why I said, "if you are going to advertise an Maximum Write Same Length".

[toc] | [prev] | [next] | [standalone]

#1468098

FromShaun Tancheff <shaun.tancheff@seagate.com>
Date2016-08-23 00:10 +0200
Message-ID<s95zR-3HK-73@gated-at.bofh.it>
In reply to#1468019
On Mon, Aug 22, 2016 at 3:14 PM, Tom Yan <tom.ty89@gmail.com> wrote:
> On 23 August 2016 at 03:43, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
>>
>> Why would we enforce upper level limits on something that doesn't
>> have any?
>
> If we advertise a limit in our SATL, it makes sense that we should
> make sure the behaviour is consistent when we issue a write same
> through the block layer / ioctl and when we issue a SCSI Write Same
> command directly (e.g. with sg_write_same). IMHO that's pretty much
> why SBC would mandate such behaviour as well.

Breaking would be advertising a limit that is too high and failing.
Advertising a lower limit and succeeding may not be ideal for all
possible use cases, but it's not breaking behaviour.

>>
>> If the upper level, or SG_IO, chooses to set a timeout of 10 hours and
>> wipe a whole disk it should be free to do so.
>>
>
> That's why I said, "if you are going to advertise an Maximum Write Same Length".

-- 
Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468138

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-23 01:10 +0200
Message-ID<s96vT-4jp-11@gated-at.bofh.it>
In reply to#1468098
I am not sure about what you mean here. Rejecting SCSI Write Same
commands that has its "number of blocks" field set to a value higher
than the device's reported Maximum Write Same Length is only natural
and mandated by SBC. We have no reason (even if it is practically not
a must) not to do it while we are implementing a SCSI-ATA Translation
Layer here as long as we advertise Maximum Write Same Length. It does
not matter here whether the command ends up being translated to SCT
Write Same or TRIM.

How high or how lower the limit should be advertised has nothing to do
with the checking.

FWIW, letting the SCSI/block layer fall back with SD_MAX_WS10_BLOCKS
does NOT count as advertising Maximum Write Same Length, that's why we
may or may not (in terms of SBC) check n_block against it if we are
really gonna leave ata_scsiop_inq_b0 in libata-scsi untouched.

On 22 August 2016 at 22:07, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
> On Mon, Aug 22, 2016 at 3:14 PM, Tom Yan <tom.ty89@gmail.com> wrote:
>> On 23 August 2016 at 03:43, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
>>>
>>> Why would we enforce upper level limits on something that doesn't
>>> have any?
>>
>> If we advertise a limit in our SATL, it makes sense that we should
>> make sure the behaviour is consistent when we issue a write same
>> through the block layer / ioctl and when we issue a SCSI Write Same
>> command directly (e.g. with sg_write_same). IMHO that's pretty much
>> why SBC would mandate such behaviour as well.
>
> Breaking would be advertising a limit that is too high and failing.
> Advertising a lower limit and succeeding may not be ideal for all
> possible use cases, but it's not breaking behaviour.
>
>>>
>>> If the upper level, or SG_IO, chooses to set a timeout of 10 hours and
>>> wipe a whole disk it should be free to do so.
>>>
>>
>> That's why I said, "if you are going to advertise an Maximum Write Same Length".
>
> --
> Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468204

FromShaun Tancheff <shaun.tancheff@seagate.com>
Date2016-08-23 02:40 +0200
Message-ID<s97UZ-53o-1@gated-at.bofh.it>
In reply to#1468138
On Mon, Aug 22, 2016 at 6:09 PM, Tom Yan <tom.ty89@gmail.com> wrote:
> I am not sure about what you mean here. Rejecting SCSI Write Same
> commands that has its "number of blocks" field set to a value higher
> than the device's reported Maximum Write Same Length is only natural
> and mandated by SBC. We have no reason (even if it is practically not
> a must) not to do it while we are implementing a SCSI-ATA Translation
> Layer here as long as we advertise Maximum Write Same Length. It does
> not matter here whether the command ends up being translated to SCT
> Write Same or TRIM.
>
> How high or how lower the limit should be advertised has nothing to do
> with the checking.
>
> FWIW, letting the SCSI/block layer fall back with SD_MAX_WS10_BLOCKS
> does NOT count as advertising Maximum Write Same Length, that's why we
> may or may not (in terms of SBC) check n_block against it if we are
> really gonna leave ata_scsiop_inq_b0 in libata-scsi untouched.

Sorry I'm still a bit confused.

SCT Write Same does not have a limit ... it's a u64 of logical sectors.
Any limit specified is smaller based on other parts of the stack.
The SATL code being used to emulating SCSI Write Same which does
have a limited number of sectors .. so falling back to the SCSI limit
seems reasonable.

So the limit that is being applied is either the current TRIM limit,
or the SCSI Write Same limit.
-- 
Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468300

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-23 08:00 +0200
Message-ID<s9cUF-8g8-11@gated-at.bofh.it>
In reply to#1468204
On 23 August 2016 at 00:36, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
> On Mon, Aug 22, 2016 at 6:09 PM, Tom Yan <tom.ty89@gmail.com> wrote:
>> I am not sure about what you mean here. Rejecting SCSI Write Same
>> commands that has its "number of blocks" field set to a value higher
>> than the device's reported Maximum Write Same Length is only natural
>> and mandated by SBC. We have no reason (even if it is practically not
>> a must) not to do it while we are implementing a SCSI-ATA Translation
>> Layer here as long as we advertise Maximum Write Same Length. It does
>> not matter here whether the command ends up being translated to SCT
>> Write Same or TRIM.
>>
>> How high or how lower the limit should be advertised has nothing to do
>> with the checking.
>>
>> FWIW, letting the SCSI/block layer fall back with SD_MAX_WS10_BLOCKS
>> does NOT count as advertising Maximum Write Same Length, that's why we
>> may or may not (in terms of SBC) check n_block against it if we are
>> really gonna leave ata_scsiop_inq_b0 in libata-scsi untouched.
>
> Sorry I'm still a bit confused.

It's alright. I guess I am not good at expressing my ideas. (And poor English)

>
> SCT Write Same does not have a limit ... it's a u64 of logical sectors.

As I've said, it is not really about SCT Write Same or TRIM. The check
here is added just to make our SATL (which is in some sense a SCSI
device) behave as how we advertised it (Maximum Write Same Length /
rbuf[36] set in ata_scsiop_inq_b0, that's why the value of the check
should always be the same as that).

So for both cases (TRIM and SCT Write Same), _if_ we advertise Maximum
Write Same Length, we should make our SATL (or in other word, our
"SCSI device") reject SCSI Write Same commands as per what we "told"
the users (and the SCSI/block layer).

The SCSI disk driver or so would NOT reject Write Same commands for us
as per the Maximum Write Same Length we reported, because it is the
responsibility of the SCSI device itself (as per SBC). The limit will
only be used to tell the block layer how large at max should each
split of the discard / write same request be.

> Any limit specified is smaller based on other parts of the stack.
> The SATL code being used to emulating SCSI Write Same which does
> have a limited number of sectors .. so falling back to the SCSI limit
> seems reasonable.

I am really not going to tell the whole story here again. We have a
really long discussion on whether we should advertise Maximum Write
Same Length for SCT Write Same, and the value we should advertise.
(Didn't we come to an conclusion on that as well?)

The check should be added here is just a knee-jerk reaction for the
Maximum Write Same Length we advertise. If you end up not advertising
one for SCT Write Same, you don't need to add the check here.

>
> So the limit that is being applied is either the current TRIM limit,
> or the SCSI Write Same limit.
> --
> Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468311

FromShaun Tancheff <shaun.tancheff@seagate.com>
Date2016-08-23 08:20 +0200
Message-ID<s9de1-ab-1@gated-at.bofh.it>
In reply to#1468300
On Tue, Aug 23, 2016 at 12:55 AM, Tom Yan <tom.ty89@gmail.com> wrote:
> I am really not going to tell the whole story here again. We have a
> really long discussion on whether we should advertise Maximum Write
> Same Length for SCT Write Same, and the value we should advertise.
> (Didn't we come to an conclusion on that as well?)

I had thought the conclusion was leave b0 as is and let the WS10 limit
take effect per Martin.
-- 
Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468362

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-23 10:10 +0200
Message-ID<s9eWt-1gG-1@gated-at.bofh.it>
In reply to#1468311
Hmm,

On 22 August 2016 at 18:00, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
>
> Timeout for WS is 120 seconds so we should be fine there.
>
> The number to look for is the:
>    Max. Sustained Transfer Rate OD (MB/s): 190 8TB (180 5TB)
>
> Which means the above drives should complete a 2G write in
> about 10 to 11 seconds.
>
> If these were 4Kn drives and we allowed a 16G max then it
> would be 80-90 seconds, assuming the write speed didn't
> get any better.
>
> So holding the maximum to around 2G is probably the best
> overall, in my opinion.
>
> --
> Shaun Tancheff

On 23 August 2016 at 06:11, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
>
> I had thought the conclusion was leave b0 as is and let the WS10 limit
> take effect per Martin.

If that's your final call then you do not need to add the check for
SCT Write Same. (But please make sure that the check for TRIM does not
go away)

> --
> Shaun Tancheff

[toc] | [prev] | [next] | [standalone]

#1468476

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-23 12:50 +0200
Message-ID<s9hrj-2PE-3@gated-at.bofh.it>
In reply to#1467343
On 22 August 2016 at 04:23, Shaun Tancheff <shaun@tancheff.com> wrote:
> SATA drives may support write same via SCT. This is useful
> for setting the drive contents to a specific pattern (0's).
>
> Translate a SCSI WRITE SAME 16 command to be either a DSM TRIM
> command or an SCT Write Same command.
>
> Based on the UNMAP flag:
>   - When set translate to DSM TRIM
>   - When not set translate to SCT Write Same
>
> Signed-off-by: Shaun Tancheff <shaun.tancheff@seagate.com>
> ---
> v6:
>  - Change to use sg_copy_from_buffer as per Christoph Hellwig <hch@lst.de>
> v5:
>  - Addressed review comments
>  - Report support for ZBC only for zoned devices.
>  - kmap page during rewrite
>  - Fix unmap set to require trim or error, if not unmap then sct write
>    same or error.
> v4:
>  - Added partial MAINTENANCE_IN opcode simulation
>  - Dropped all changes in drivers/scsi/*
>  - Changed to honor the UNMAP flag -> TRIM, no UNMAP -> SCT.
> v3:
>  - Demux UNMAP/TRIM from WRITE SAME
> v2:
>  - Remove fugly ata hacking from sd.c
>
>  drivers/ata/libata-scsi.c | 199 +++++++++++++++++++++++++++++++++++++++-------
>  include/linux/ata.h       |  43 ++++++++++
>  2 files changed, 213 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7990cb2..ebf1a04 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1159,8 +1159,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
>  {
>         sdev->use_10_for_rw = 1;
>         sdev->use_10_for_ms = 1;
> -       sdev->no_report_opcodes = 1;
> -       sdev->no_write_same = 1;
>
>         /* Schedule policy is determined by ->qc_defer() callback and
>          * it needs to see every deferred qc.  Set dev_blocked to 1 to
> @@ -3287,7 +3285,7 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>   * @cmd: SCSI command being translated
>   * @num: Maximum number of entries (nominally 64).
>   * @sector: Starting sector
> - * @count: Total Range of request
> + * @count: Total Range of request in logical sectors
>   *
>   * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted
>   * descriptor.
> @@ -3330,6 +3328,45 @@ static unsigned int ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 num,
>         return used_bytes;
>  }
>
> +/**
> + * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
> + * @cmd: SCSI command being translated
> + * @lba: Starting sector
> + * @num: Number of logical sectors to be zero'd.
> + *
> + * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
> + * descriptor.
> + * NOTE: Writes a pattern (0's) in the foreground.
> + *       Large write-same requents can timeout.
> + */
> +static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
> +{
> +       u16 *sctpg;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
> +       sctpg = ((void *)ata_scsi_rbuf);

Because ata_scsi_rbuf is of a fixed size of ATA_SCSI_RBUF_SIZE.

#define ATA_SCSI_RBUF_SIZE      4096
...
static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];

> +
> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
> +       put_unaligned_le64(lba,     &sctpg[2]);
> +       put_unaligned_le64(num,     &sctpg[6]);
> +       put_unaligned_le32(0u,      &sctpg[10]);
> +
> +       sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);

You have no reason to use 512 here instead of ATA_SCSI_RBUF_SIZE this time.

> +       spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
> +}
> +
> +/**
> + * ata_scsi_write_same_xlat() - SATL Write Same to ATA SCT Write Same
> + * @qc: Command to be translated
> + *
> + * Translate a SCSI WRITE SAME command to be either a DSM TRIM command or
> + * an SCT Write Same command.
> + * Based on WRITE SAME has the UNMAP flag
> + *   When set translate to DSM TRIM
> + *   When clear translate to SCT Write Same
> + */
>  static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>  {
>         struct ata_taskfile *tf = &qc->tf;
> @@ -3342,6 +3379,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>         u32 size;
>         u16 fp;
>         u8 bp = 0xff;
> +       u8 unmap = cdb[1] & 0x8;
>
>         /* we may not issue DMA commands if no DMA mode is set */
>         if (unlikely(!dev->dma_mode))
> @@ -3353,11 +3391,26 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>         }
>         scsi_16_lba_len(cdb, &block, &n_block);
>
> -       /* for now we only support WRITE SAME with the unmap bit set */
> -       if (unlikely(!(cdb[1] & 0x8))) {
> -               fp = 1;
> -               bp = 3;
> -               goto invalid_fld;
> +       if (unmap) {
> +               /* If trim is not enabled the cmd is invalid. */
> +               if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
> +                   !ata_id_has_trim(dev->id)) {
> +                       fp = 1;
> +                       bp = 3;
> +                       goto invalid_fld;
> +               }
> +               /* If the request is too large the cmd is invalid */
> +               if (n_block > 0xffff * trmax) {
> +                       fp = 2;
> +                       goto invalid_fld;
> +               }
> +       } else {
> +               /* If write same is not available the cmd is invalid */
> +               if (!ata_id_sct_write_same(dev->id)) {
> +                       fp = 1;
> +                       bp = 3;
> +                       goto invalid_fld;
> +               }
>         }
>
>         /*
> @@ -3367,30 +3420,42 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>         if (!scsi_sg_count(scmd))
>                 goto invalid_param_len;
>
> -       if (n_block <= 0xffff * trmax) {
> +       if (unmap) {
>                 size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
> +               if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
> +                       /* Newer devices support queued TRIM commands */
> +                       tf->protocol = ATA_PROT_NCQ;
> +                       tf->command = ATA_CMD_FPDMA_SEND;
> +                       tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
> +                       tf->nsect = qc->tag << 3;
> +                       tf->hob_feature = (size / 512) >> 8;
> +                       tf->feature = size / 512;
> +
> +                       tf->auxiliary = 1;
> +               } else {
> +                       tf->protocol = ATA_PROT_DMA;
> +                       tf->hob_feature = 0;
> +                       tf->feature = ATA_DSM_TRIM;
> +                       tf->hob_nsect = (size / 512) >> 8;
> +                       tf->nsect = size / 512;
> +                       tf->command = ATA_CMD_DSM;
> +               }
>         } else {
> -               fp = 2;
> -               goto invalid_fld;
> -       }
> -
> -       if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
> -               /* Newer devices support queued TRIM commands */
> -               tf->protocol = ATA_PROT_NCQ;
> -               tf->command = ATA_CMD_FPDMA_SEND;
> -               tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
> -               tf->nsect = qc->tag << 3;
> -               tf->hob_feature = (size / 512) >> 8;
> -               tf->feature = size / 512;
> +               ata_format_sct_write_same(scmd, block, n_block);
>
> -               tf->auxiliary = 1;
> -       } else {
> -               tf->protocol = ATA_PROT_DMA;
>                 tf->hob_feature = 0;
> -               tf->feature = ATA_DSM_TRIM;
> -               tf->hob_nsect = (size / 512) >> 8;
> -               tf->nsect = size / 512;
> -               tf->command = ATA_CMD_DSM;
> +               tf->feature = 0;
> +               tf->hob_nsect = 0;
> +               tf->nsect = 1;
> +               tf->lbah = 0;
> +               tf->lbam = 0;
> +               tf->lbal = ATA_CMD_STANDBYNOW1;
> +               tf->hob_lbah = 0;
> +               tf->hob_lbam = 0;
> +               tf->hob_lbal = 0;
> +               tf->device = ATA_CMD_STANDBYNOW1;
> +               tf->protocol = ATA_PROT_DMA;
> +               tf->command = ATA_CMD_WRITE_LOG_DMA_EXT;
>         }
>
>         tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
> @@ -3414,6 +3479,76 @@ invalid_opcode:
>  }
>
>  /**
> + *     ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
> + *     @args: device MAINTENANCE_IN data / 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_scsi_args *args, u8 *rbuf)
> +{
> +       struct ata_device *dev = args->dev;
> +       u8 *cdb = args->cmd->cmnd;
> +       u8 supported = 0;
> +       unsigned int err = 0;
> +
> +       if (cdb[2] != 1) {
> +               ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
> +               err = 2;
> +               goto out;
> +       }
> +       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 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 READ_16:
> +       case WRITE_6:
> +       case WRITE_10:
> +       case WRITE_16:
> +       case ATA_12:
> +       case ATA_16:
> +       case VERIFY:
> +       case VERIFY_16:
> +       case MODE_SELECT:
> +       case MODE_SELECT_10:
> +       case START_STOP:
> +               supported = 3;
> +               break;
> +       case WRITE_SAME_16:
> +               if (ata_id_sct_write_same(dev->id))
> +                       supported = 3;
> +               break;
> +       case ZBC_IN:
> +       case ZBC_OUT:
> +               if (ata_id_zoned_cap(dev->id) ||
> +                   dev->class == ATA_DEV_ZAC)
> +                       supported = 3;
> +               break;
> +       default:
> +               break;
> +       }
> +out:
> +       rbuf[1] = supported; /* supported */
> +       return err;
> +}
> +
> +/**
>   *     ata_scsi_report_zones_complete - convert ATA output
>   *     @qc: command structure returning the data
>   *
> @@ -4193,6 +4328,13 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
>                         ata_scsi_invalid_field(dev, cmd, 1);
>                 break;
>
> +       case MAINTENANCE_IN:
> +               if (scsicmd[1] == MI_REPORT_SUPPORTED_OPERATION_CODES)
> +                       ata_scsi_rbuf_fill(&args, ata_scsiop_maint_in);
> +               else
> +                       ata_scsi_invalid_field(dev, cmd, 1);
> +               break;
> +
>         /* all other commands */
>         default:
>                 ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x20, 0x0);
> @@ -4225,7 +4367,6 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
>                 shost->max_lun = 1;
>                 shost->max_channel = 1;
>                 shost->max_cmd_len = 16;
> -               shost->no_write_same = 1;
>
>                 /* Schedule policy is determined by ->qc_defer()
>                  * callback and it needs to see every deferred qc.
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 45a1d71..fdb1803 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -105,6 +105,7 @@ enum {
>         ATA_ID_CFA_KEY_MGMT     = 162,
>         ATA_ID_CFA_MODES        = 163,
>         ATA_ID_DATA_SET_MGMT    = 169,
> +       ATA_ID_SCT_CMD_XPORT    = 206,
>         ATA_ID_ROT_SPEED        = 217,
>         ATA_ID_PIO4             = (1 << 1),
>
> @@ -789,6 +790,48 @@ static inline bool ata_id_sense_reporting_enabled(const u16 *id)
>  }
>
>  /**
> + *
> + * Word: 206 - SCT Command Transport
> + *    15:12 - Vendor Specific
> + *     11:6 - Reserved
> + *        5 - SCT Command Transport Data Tables supported
> + *        4 - SCT Command Transport Features Control supported
> + *        3 - SCT Command Transport Error Recovery Control supported
> + *        2 - SCT Command Transport Write Same supported
> + *        1 - SCT Command Transport Long Sector Access supported
> + *        0 - SCT Command Transport supported
> + */
> +static inline bool ata_id_sct_data_tables(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 5) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_features_ctrl(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 4) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_error_recovery_ctrl(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 3) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_write_same(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 2) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_long_sector_access(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 1) ? true : false;
> +}
> +
> +static inline bool ata_id_sct_supported(const u16 *id)
> +{
> +       return id[ATA_ID_SCT_CMD_XPORT] & (1 << 0) ? true : false;
> +}
> +
> +/**
>   *     ata_id_major_version    -       get ATA level of drive
>   *     @id: Identify data
>   *
> --
> 2.9.3
>

[toc] | [prev] | [next] | [standalone]

#1468480

FromShaun Tancheff <shaun.tancheff@seagate.com>
Date2016-08-23 13:00 +0200
Message-ID<s9hAZ-2T7-7@gated-at.bofh.it>
In reply to#1468476
On Tue, Aug 23, 2016 at 5:37 AM, Tom Yan <tom.ty89@gmail.com> wrote:
> On 22 August 2016 at 04:23, Shaun Tancheff <shaun@tancheff.com> wrote:
>> +/**
>> + * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
>> + * @cmd: SCSI command being translated
>> + * @lba: Starting sector
>> + * @num: Number of logical sectors to be zero'd.
>> + *
>> + * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
>> + * descriptor.
>> + * NOTE: Writes a pattern (0's) in the foreground.
>> + *       Large write-same requents can timeout.
>> + */
>> +static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
>> +{
>> +       u16 *sctpg;
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
>> +       sctpg = ((void *)ata_scsi_rbuf);
>
> Because ata_scsi_rbuf is of a fixed size of ATA_SCSI_RBUF_SIZE.
>
> #define ATA_SCSI_RBUF_SIZE      4096
> ...
> static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];
>
>> +
>> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
>> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
>> +       put_unaligned_le64(lba,     &sctpg[2]);
>> +       put_unaligned_le64(num,     &sctpg[6]);
>> +       put_unaligned_le32(0u,      &sctpg[10]);
>> +
>> +       sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);
>
> You have no reason to use 512 here instead of ATA_SCSI_RBUF_SIZE this time.

Ah .. because SCT Write Same is a fixed 512 byte transfer?
Ah .. because I only have 512 bytes to copy?

>> +       spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
>> +}

[toc] | [prev] | [next] | [standalone]

#1469090

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-24 08:00 +0200
Message-ID<s9zoe-6nX-5@gated-at.bofh.it>
In reply to#1468480
Never mind. I was a bit lightheaded.

Anyway I don't think you should use ata_scsi_rbuf. It is a buffer
created and used for ata_scsi_simulate, which interacts with the SCSI
layer but not the ATA device (v.s. ata_scsi_translate). You should
probably create buffer inside ata_format_dsm_trim_descr() and
ata_format_sct_write_same() of size(s) you need.

On 23 August 2016 at 18:56, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
> On Tue, Aug 23, 2016 at 5:37 AM, Tom Yan <tom.ty89@gmail.com> wrote:
>> On 22 August 2016 at 04:23, Shaun Tancheff <shaun@tancheff.com> wrote:
>>> +/**
>>> + * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
>>> + * @cmd: SCSI command being translated
>>> + * @lba: Starting sector
>>> + * @num: Number of logical sectors to be zero'd.
>>> + *
>>> + * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
>>> + * descriptor.
>>> + * NOTE: Writes a pattern (0's) in the foreground.
>>> + *       Large write-same requents can timeout.
>>> + */
>>> +static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
>>> +{
>>> +       u16 *sctpg;
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
>>> +       sctpg = ((void *)ata_scsi_rbuf);
>>
>> Because ata_scsi_rbuf is of a fixed size of ATA_SCSI_RBUF_SIZE.
>>
>> #define ATA_SCSI_RBUF_SIZE      4096
>> ...
>> static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];
>>
>>> +
>>> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
>>> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
>>> +       put_unaligned_le64(lba,     &sctpg[2]);
>>> +       put_unaligned_le64(num,     &sctpg[6]);
>>> +       put_unaligned_le32(0u,      &sctpg[10]);
>>> +
>>> +       sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);
>>
>> You have no reason to use 512 here instead of ATA_SCSI_RBUF_SIZE this time.
>
> Ah .. because SCT Write Same is a fixed 512 byte transfer?
> Ah .. because I only have 512 bytes to copy?
>
>>> +       spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
>>> +}

[toc] | [prev] | [next] | [standalone]

#1469099

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-24 08:20 +0200
Message-ID<s9zHz-6Ku-15@gated-at.bofh.it>
In reply to#1469090
Btw, I wonder if you need to memset your buffer with 0 first, like
what is done in ata_scsi_rbuf_get.

On 24 August 2016 at 13:57, Tom Yan <tom.ty89@gmail.com> wrote:
> Never mind. I was a bit lightheaded.
>
> Anyway I don't think you should use ata_scsi_rbuf. It is a buffer
> created and used for ata_scsi_simulate, which interacts with the SCSI
> layer but not the ATA device (v.s. ata_scsi_translate). You should
> probably create buffer inside ata_format_dsm_trim_descr() and
> ata_format_sct_write_same() of size(s) you need.
>
> On 23 August 2016 at 18:56, Shaun Tancheff <shaun.tancheff@seagate.com> wrote:
>> On Tue, Aug 23, 2016 at 5:37 AM, Tom Yan <tom.ty89@gmail.com> wrote:
>>> On 22 August 2016 at 04:23, Shaun Tancheff <shaun@tancheff.com> wrote:
>>>> +/**
>>>> + * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
>>>> + * @cmd: SCSI command being translated
>>>> + * @lba: Starting sector
>>>> + * @num: Number of logical sectors to be zero'd.
>>>> + *
>>>> + * Rewrite the WRITE SAME descriptor to be an SCT Write Same formatted
>>>> + * descriptor.
>>>> + * NOTE: Writes a pattern (0's) in the foreground.
>>>> + *       Large write-same requents can timeout.
>>>> + */
>>>> +static void ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
>>>> +{
>>>> +       u16 *sctpg;
>>>> +       unsigned long flags;
>>>> +
>>>> +       spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
>>>> +       sctpg = ((void *)ata_scsi_rbuf);
>>>
>>> Because ata_scsi_rbuf is of a fixed size of ATA_SCSI_RBUF_SIZE.
>>>
>>> #define ATA_SCSI_RBUF_SIZE      4096
>>> ...
>>> static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];
>>>
>>>> +
>>>> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
>>>> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
>>>> +       put_unaligned_le64(lba,     &sctpg[2]);
>>>> +       put_unaligned_le64(num,     &sctpg[6]);
>>>> +       put_unaligned_le32(0u,      &sctpg[10]);
>>>> +
>>>> +       sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), sctpg, 512);
>>>
>>> You have no reason to use 512 here instead of ATA_SCSI_RBUF_SIZE this time.
>>
>> Ah .. because SCT Write Same is a fixed 512 byte transfer?
>> Ah .. because I only have 512 bytes to copy?
>>
>>>> +       spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
>>>> +}

[toc] | [prev] | [next] | [standalone]

#1469759

FromShaun Tancheff <shaun@tancheff.com>
Date2016-08-25 00:10 +0200
Message-ID<s9OwV-dA-11@gated-at.bofh.it>
In reply to#1469099
On Wed, Aug 24, 2016 at 1:10 AM, Tom Yan <tom.ty89@gmail.com> wrote:
> Btw, I wonder if you need to memset your buffer with 0 first, like
> what is done in ata_scsi_rbuf_get.

It is not necessary as the defined buffer is completely filled out here.

Are you thinking as a sort of future proofing?
Ex: In the unlikely event that the SCT Write Same command
descriptor is expanded in a future ACS?

It is more likely to see the command deprecated and replaced with a
new SCT feature.

Regardless of how unlikely I would consider a memset here to clear
the remainder of the payload.

[toc] | [prev] | [next] | [standalone]

#1469867

FromTom Yan <tom.ty89@gmail.com>
Date2016-08-25 08:50 +0200
Message-ID<s9WE9-5OW-1@gated-at.bofh.it>
In reply to#1469759
You only fill the bytes that you want to to set explicitly:

+       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
+       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
+       put_unaligned_le64(lba,     &sctpg[2]);
+       put_unaligned_le64(num,     &sctpg[6]);
+       put_unaligned_le32(0u,      &sctpg[10]);

What I doubted is, if you don't memset (zero-fill) the buffer first,
will other bytes have indeterministic value that causes random
unexpected behavior?

On 25 August 2016 at 06:04, Shaun Tancheff <shaun@tancheff.com> wrote:
> On Wed, Aug 24, 2016 at 1:10 AM, Tom Yan <tom.ty89@gmail.com> wrote:
>> Btw, I wonder if you need to memset your buffer with 0 first, like
>> what is done in ata_scsi_rbuf_get.
>
> It is not necessary as the defined buffer is completely filled out here.
>
> Are you thinking as a sort of future proofing?
> Ex: In the unlikely event that the SCT Write Same command
> descriptor is expanded in a future ACS?
>
> It is more likely to see the command deprecated and replaced with a
> new SCT feature.
>
> Regardless of how unlikely I would consider a memset here to clear
> the remainder of the payload.

[toc] | [prev] | [next] | [standalone]

#1469929

FromShaun Tancheff <shaun.tancheff@seagate.com>
Date2016-08-25 09:50 +0200
Message-ID<s9XAd-6uu-11@gated-at.bofh.it>
In reply to#1469867
On Thu, Aug 25, 2016 at 1:23 AM, Tom Yan <tom.ty89@gmail.com> wrote:
> You only fill the bytes that you want to to set explicitly:
>
> +       put_unaligned_le16(0x0002,  &sctpg[0]); /* SCT_ACT_WRITE_SAME */
> +       put_unaligned_le16(0x0101,  &sctpg[1]); /* WRITE PTRN FG */
> +       put_unaligned_le64(lba,     &sctpg[2]);
> +       put_unaligned_le64(num,     &sctpg[6]);
> +       put_unaligned_le32(0u,      &sctpg[10]);
>
> What I doubted is, if you don't memset (zero-fill) the buffer first,
> will other bytes have indeterministic value that causes random
> unexpected behavior?

No.
If there is random or unexpected behaviour the device is broken
and some other remedy, such as blacklisting, is required.

---
Shaun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web