[PATCH v3 0/3] Introduce new SATA queued commands

[PATCH v3 0/3] Introduce new SATA queued commands

[Marc C]

From: Marc Carino <marc.ceeeee@gmail.com>

This patch series updates the libata driver with some additional
commands which are specified in the more recent versions of the
SATA and ATA specifications. These commands are:

        SEND FPDMA QUEUED
        RECEIVE FPDMA QUEUED.

The new queued commands augment the existing READ and WRITE FPDMA
QUEUED commands. The commands are intended to transport non-media
data to/from a device in a non-blocking manner.

One application of these commands include a queued-style DSM TRIM
operation. The current DSM TRIM command is non-queued, and requires
the initiator to empty out the queue before issuance. The new DSM TRIM
allows the initiator to keep the queue full.

These patches have been validated on an Intel SATA AHCI controller,
on a Micron M500 SSD, using "postmark," "fstrim," and "fsck."

Version history:

v3:
- rebased to libata/for-3.12
- put H2D FIS "auxiliary" field changes in separate patch
- added "auxiliary" field population to all FIS-based SATA drivers

v2:
- rebased to 3.11-rc4
- moved auxiliary field to ata_queued_cmd struct
- updated signoff name

v1:
- initial

Marc Carino (3):
  libata: Populate host-to-device FIS "auxiliary" field
  libata: Add support for SEND/RECEIVE FPDMA QUEUED
  libata: Add support for queued DSM TRIM

 drivers/ata/acard-ahci.c  |  2 +-
 drivers/ata/libahci.c     |  2 +-
 drivers/ata/libata-core.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-scsi.c | 27 +++++++++++++++++++++------
 drivers/ata/sata_fsl.c    |  2 +-
 drivers/ata/sata_mv.c     |  2 +-
 drivers/ata/sata_qstor.c  |  2 +-
 drivers/ata/sata_sil24.c  |  2 +-
 include/linux/ata.h       | 23 +++++++++++++++++++++++
 include/linux/libata.h    |  8 ++++++++
 10 files changed, 102 insertions(+), 12 deletions(-)

-- 
1.8.1.2

[PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Marc C]

From: Marc Carino <marc.ceeeee@gmail.com>

SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

Since there is no analog between the new field and the ATA
taskfile, a new element was added to 'struct ata_queued_cmd."

Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
---
 drivers/ata/acard-ahci.c  |  2 +-
 drivers/ata/libahci.c     |  2 +-
 drivers/ata/libata-core.c | 29 +++++++++++++++++++++++++++++
 drivers/ata/sata_fsl.c    |  2 +-
 drivers/ata/sata_mv.c     |  2 +-
 drivers/ata/sata_qstor.c  |  2 +-
 drivers/ata/sata_sil24.c  |  2 +-
 include/linux/libata.h    |  4 ++++
 8 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index fd665d9..a7bf4c4 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -274,7 +274,7 @@ static void acard_ahci_qc_prep(struct ata_queued_cmd *qc)
 	 */
 	cmd_tbl = pp->cmd_tbl + qc->tag * AHCI_CMD_TBL_SZ;
 
-	ata_tf_to_fis(&qc->tf, qc->dev->link->pmp, 1, cmd_tbl);
+	ata_qc_to_fis(qc, qc->dev->link->pmp, 1, cmd_tbl);
 	if (is_atapi) {
 		memset(cmd_tbl + AHCI_CMD_TBL_CDB, 0, 32);
 		memcpy(cmd_tbl + AHCI_CMD_TBL_CDB, qc->cdb, qc->dev->cdb_len);
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index acfd0f7..2283ea4 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1498,7 +1498,7 @@ static void ahci_qc_prep(struct ata_queued_cmd *qc)
 	 */
 	cmd_tbl = pp->cmd_tbl + qc->tag * AHCI_CMD_TBL_SZ;
 
-	ata_tf_to_fis(&qc->tf, qc->dev->link->pmp, 1, cmd_tbl);
+	ata_qc_to_fis(qc, qc->dev->link->pmp, 1, cmd_tbl);
 	if (is_atapi) {
 		memset(cmd_tbl + AHCI_CMD_TBL_CDB, 0, 32);
 		memcpy(cmd_tbl + AHCI_CMD_TBL_CDB, qc->cdb, qc->dev->cdb_len);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c24354d..9d02c47 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -532,6 +532,34 @@ int atapi_cmd_type(u8 opcode)
 }
 
 /**
+ *	ata_qc_to_fis - Convert struct ata_queued_cmd to SATA FIS structure
+ *	@qc: struct ata_queued_cmd to convert
+ *	@pmp: Port multiplier port
+ *	@is_cmd: This FIS is for command
+ *	@fis: Buffer into which data will output
+ *
+ *	Converts a struct ata_queued_cmd to a Serial ATA
+ *	FIS structure (Register - Host to Device).
+ *
+ *	Beginning with SATA 3.1, the ATA taskfile does not completely describe
+ *	all of the fields in a host-to-device FIS. More specifically, the
+ *	'auxiliary' field has no ATA taskfile analog, and thus requires us
+ *	to populate the FIS via the ata_queued_cmd structure.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+void ata_qc_to_fis(const struct ata_queued_cmd *qc, u8 pmp, int is_cmd, u8 *fis)
+{
+	ata_tf_to_fis(&qc->tf, pmp, is_cmd, fis);
+
+	fis[16] = qc->auxiliary & 0xff;
+	fis[17] = (qc->auxiliary >> 8) & 0xff;
+	fis[18] = (qc->auxiliary >> 16) & 0xff;
+	fis[19] = (qc->auxiliary >> 24) & 0xff;
+}
+
+/**
  *	ata_tf_to_fis - Convert ATA taskfile to SATA FIS structure
  *	@tf: Taskfile to convert
  *	@pmp: Port multiplier port
@@ -6877,6 +6905,7 @@ EXPORT_SYMBOL_GPL(ata_sg_init);
 EXPORT_SYMBOL_GPL(ata_qc_complete);
 EXPORT_SYMBOL_GPL(ata_qc_complete_multiple);
 EXPORT_SYMBOL_GPL(atapi_cmd_type);
+EXPORT_SYMBOL_GPL(ata_qc_to_fis);
 EXPORT_SYMBOL_GPL(ata_tf_to_fis);
 EXPORT_SYMBOL_GPL(ata_tf_from_fis);
 EXPORT_SYMBOL_GPL(ata_pack_xfermask);
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 19720a0..bbdba01 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -525,7 +525,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc)
 	cd = (struct command_desc *)pp->cmdentry + tag;
 	cd_paddr = pp->cmdentry_paddr + tag * SATA_FSL_CMD_DESC_SIZE;
 
-	ata_tf_to_fis(&qc->tf, qc->dev->link->pmp, 1, (u8 *) &cd->cfis);
+	ata_qc_to_fis(qc, qc->dev->link->pmp, 1, (u8 *) &cd->cfis);
 
 	VPRINTK("Dumping cfis : 0x%x, 0x%x, 0x%x\n",
 		cd->cfis[0], cd->cfis[1], cd->cfis[2]);
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 56be318..96fc238 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2267,7 +2267,7 @@ static unsigned int mv_qc_issue_fis(struct ata_queued_cmd *qc)
 	u32 fis[5];
 	int err = 0;
 
-	ata_tf_to_fis(&qc->tf, link->pmp, 1, (void *)fis);
+	ata_qc_to_fis(qc, link->pmp, 1, (void *)fis);
 	err = mv_send_fis(ap, fis, ARRAY_SIZE(fis));
 	if (err)
 		return err;
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 3b0dd57..e0ce396 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -311,7 +311,7 @@ static void qs_qc_prep(struct ata_queued_cmd *qc)
 	buf[28] = dflags;
 
 	/* frame information structure (FIS) */
-	ata_tf_to_fis(&qc->tf, 0, 1, &buf[32]);
+	ata_qc_to_fis(qc, 0, 1, &buf[32]);
 }
 
 static inline void qs_packet_start(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index aa1051b..7e56d48 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -877,7 +877,7 @@ static void sil24_qc_prep(struct ata_queued_cmd *qc)
 	}
 
 	prb->ctrl = cpu_to_le16(ctrl);
-	ata_tf_to_fis(&qc->tf, qc->dev->link->pmp, 1, prb->fis);
+	ata_qc_to_fis(qc, qc->dev->link->pmp, 1, prb->fis);
 
 	if (qc->flags & ATA_QCFLAG_DMAMAP)
 		sil24_fill_sg(qc, sge);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 283d66b..a4601cc 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -604,6 +604,7 @@ struct ata_queued_cmd {
 
 	struct ata_taskfile	tf;
 	u8			cdb[ATAPI_CDB_LEN];
+	u32			auxiliary;
 
 	unsigned long		flags;		/* ATA_QCFLAG_xxx */
 	unsigned int		tag;
@@ -1145,6 +1146,8 @@ extern void ata_msleep(struct ata_port *ap, unsigned int msecs);
 extern u32 ata_wait_register(struct ata_port *ap, void __iomem *reg, u32 mask,
 			u32 val, unsigned long interval, unsigned long timeout);
 extern int atapi_cmd_type(u8 opcode);
+extern void ata_qc_to_fis(const struct ata_queued_cmd *qc,
+			  u8 pmp, int is_cmd, u8 *fis);
 extern void ata_tf_to_fis(const struct ata_taskfile *tf,
 			  u8 pmp, int is_cmd, u8 *fis);
 extern void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf);
@@ -1658,6 +1661,7 @@ static inline void ata_qc_reinit(struct ata_queued_cmd *qc)
 	qc->n_elem = 0;
 	qc->err_mask = 0;
 	qc->sect_size = ATA_SECT_SIZE;
+	qc->auxiliary = 0;
 
 	ata_tf_init(qc->dev, &qc->tf);
 
-- 
1.8.1.2

[PATCH v3 2/3] libata: Add support for SEND/RECEIVE FPDMA QUEUED

[Marc C]

From: Marc Carino <marc.ceeeee@gmail.com>

Add support for the following ATA opcodes, which are present
in SATA 3.1 and T13 ATA ACS-3:

        SEND FPDMA QUEUED
        RECEIVE FPDMA QUEUED

Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
---
 drivers/ata/libata-core.c | 15 +++++++++++++++
 include/linux/ata.h       | 23 +++++++++++++++++++++++
 include/linux/libata.h    |  4 ++++
 3 files changed, 42 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9d02c47..9fd8d92 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2131,6 +2131,7 @@ static int ata_dev_config_ncq(struct ata_device *dev,
 	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
 	unsigned int err_mask;
 	char *aa_desc = "";
+	u8 sata_setting[ATA_SECT_SIZE];
 
 	if (!ata_id_has_ncq(dev->id)) {
 		desc[0] = '\0';
@@ -2167,6 +2168,20 @@ static int ata_dev_config_ncq(struct ata_device *dev,
 	else
 		snprintf(desc, desc_sz, "NCQ (depth %d/%d)%s", hdepth,
 			ddepth, aa_desc);
+
+	if (ata_id_has_ncq_send_and_recv(dev->id)) {
+		err_mask = ata_read_log_page(dev,
+					     ATA_LOG_NCQ_SEND_RECV,
+					     0,
+					     sata_setting,
+					     1);
+		if (!err_mask) {
+			dev->flags |= ATA_DFLAG_NCQ_SEND_RECV;
+			memcpy(dev->ncq_send_recv_cmds, sata_setting,
+				ATA_LOG_NCQ_SEND_RECV_SIZE);
+		}
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/ata.h b/include/linux/ata.h
index f63fb1a..f9a0465 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -239,6 +239,8 @@ enum {
 	ATA_CMD_WRITE_QUEUED_FUA_EXT = 0x3E,
 	ATA_CMD_FPDMA_READ	= 0x60,
 	ATA_CMD_FPDMA_WRITE	= 0x61,
+	ATA_CMD_FPDMA_SEND	= 0x64,
+	ATA_CMD_FPDMA_RECV	= 0x65,
 	ATA_CMD_PIO_READ	= 0x20,
 	ATA_CMD_PIO_READ_EXT	= 0x24,
 	ATA_CMD_PIO_WRITE	= 0x30,
@@ -293,8 +295,13 @@ enum {
 	/* marked obsolete in the ATA/ATAPI-7 spec */
 	ATA_CMD_RESTORE		= 0x10,
 
+	/* Subcmds for ATA_CMD_FPDMA_SEND */
+	ATA_SUBCMD_FPDMA_SEND_DSM            = 0x00,
+	ATA_SUBCMD_FPDMA_SEND_WR_LOG_DMA_EXT = 0x02,
+
 	/* READ_LOG_EXT pages */
 	ATA_LOG_SATA_NCQ	= 0x10,
+	ATA_LOG_NCQ_SEND_RECV	  = 0x13,
 	ATA_LOG_SATA_ID_DEV_DATA  = 0x30,
 	ATA_LOG_SATA_SETTINGS	  = 0x08,
 	ATA_LOG_DEVSLP_OFFSET	  = 0x30,
@@ -305,6 +312,15 @@ enum {
 	ATA_LOG_DEVSLP_VALID	  = 0x07,
 	ATA_LOG_DEVSLP_VALID_MASK = 0x80,
 
+	/* NCQ send and receive log */
+	ATA_LOG_NCQ_SEND_RECV_SUBCMDS_OFFSET	= 0x00,
+	ATA_LOG_NCQ_SEND_RECV_SUBCMDS_DSM	= (1 << 0),
+	ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET	= 0x04,
+	ATA_LOG_NCQ_SEND_RECV_DSM_TRIM		= (1 << 0),
+	ATA_LOG_NCQ_SEND_RECV_RD_LOG_OFFSET	= 0x08,
+	ATA_LOG_NCQ_SEND_RECV_WR_LOG_OFFSET	= 0x0C,
+	ATA_LOG_NCQ_SEND_RECV_SIZE		= 0x10,
+
 	/* READ/WRITE LONG (obsolete) */
 	ATA_CMD_READ_LONG	= 0x22,
 	ATA_CMD_READ_LONG_ONCE	= 0x23,
@@ -772,6 +788,13 @@ static inline int ata_id_rotation_rate(const u16 *id)
 	return val;
 }
 
+static inline bool ata_id_has_ncq_send_and_recv(const u16 *id)
+{
+	if (id[ATA_ID_SATA_CAPABILITY_2] & BIT(6))
+		return true;
+	return false;
+}
+
 static inline bool ata_id_has_trim(const u16 *id)
 {
 	if (ata_id_major_version(id) >= 7 &&
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a4601cc..9eed812 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -180,6 +180,7 @@ enum {
 	ATA_DFLAG_DA		= (1 << 26), /* device supports Device Attention */
 	ATA_DFLAG_DEVSLP	= (1 << 27), /* device supports Device Sleep */
 	ATA_DFLAG_ACPI_DISABLED = (1 << 28), /* ACPI for the device is disabled */
+	ATA_DFLAG_NCQ_SEND_RECV = (1 << 29), /* device supports NCQ SEND and RECV */
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
@@ -700,6 +701,9 @@ struct ata_device {
 	/* DEVSLP Timing Variables from Identify Device Data Log */
 	u8			devslp_timing[ATA_LOG_DEVSLP_SIZE];
 
+	/* NCQ send and receive log subcommand support */
+	u8			ncq_send_recv_cmds[ATA_LOG_NCQ_SEND_RECV_SIZE];
+
 	/* error history */
 	int			spdn_cnt;
 	/* ering is CLEAR_END, read comment above CLEAR_END */
-- 
1.8.1.2

[PATCH v3 3/3] libata: Add support for queued DSM TRIM

[Marc C]

From: Marc Carino <marc.ceeeee@gmail.com>

Some new SSDs support the queued version of the DSM TRIM command.
Let the driver use the new command if supported.

Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
---
 drivers/ata/libata-scsi.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 83c0890..1605ffc 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3098,12 +3098,27 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	buf = page_address(sg_page(scsi_sglist(scmd)));
 	size = ata_set_lba_range_entries(buf, 512, block, n_block);
 
-	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;
+	if (ata_ncq_enabled(dev) &&
+	    (dev->ncq_send_recv_cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &
+	     ATA_LOG_NCQ_SEND_RECV_DSM_TRIM)) {
+		/* 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;
+
+		qc->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->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
 		     ATA_TFLAG_WRITE;
 
-- 
1.8.1.2

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Tejun Heo]

On Thu, Aug 08, 2013 at 09:49:10PM -0700, Marc C wrote:
> From: Marc Carino <marc.ceeeee@gmail.com>
> 
> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.
> 
> Since there is no analog between the new field and the ATA
> taskfile, a new element was added to 'struct ata_queued_cmd."

Ummm... I'm sorry but I'm gonna do 180 from Sergei on this one.  The
auxiliary field is part of ata taskfile for all intents and purposes.
FIS is the new command structure anyway and struct ata_taskfile proper
should be able to describe the command with ata_queuedcmd providing
the surrounding context.  The argument that ata_taskfile shouldn't
contain anything which wasn't in PATA taskfile is bogus as it already
contains ATA_TFLAG_*.

So, please put the aux field into ata_taskfile.  That's where it
belongs.

Thanks.

-- 
tejun

Re: [PATCH v3 2/3] libata: Add support for SEND/RECEIVE FPDMA QUEUED

[Tejun Heo]

On Thu, Aug 08, 2013 at 09:49:11PM -0700, Marc C wrote:
> @@ -2131,6 +2131,7 @@ static int ata_dev_config_ncq(struct ata_device *dev,
>  	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
>  	unsigned int err_mask;
>  	char *aa_desc = "";
> +	u8 sata_setting[ATA_SECT_SIZE];

Nope.  Please don't do this.  Can't you use ap->sector_buf?

-- 
tejun

Re: [PATCH v3 3/3] libata: Add support for queued DSM TRIM

[Sergei Shtylyov]

Hello.

On 08/09/2013 08:49 AM, Marc C wrote:

> From: Marc Carino <marc.ceeeee@gmail.com>

> Some new SSDs support the queued version of the DSM TRIM command.
> Let the driver use the new command if supported.

> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
> ---
>   drivers/ata/libata-scsi.c | 27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 83c0890..1605ffc 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3098,12 +3098,27 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>   	buf = page_address(sg_page(scsi_sglist(scmd)));
>   	size = ata_set_lba_range_entries(buf, 512, block, n_block);
>
> -	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;
> +	if (ata_ncq_enabled(dev) &&
> +	    (dev->ncq_send_recv_cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &
> +	     ATA_LOG_NCQ_SEND_RECV_DSM_TRIM)) {
> +		/* 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;
> +
> +		qc->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;
> +	}
> +

    The remaining issue I see with this is that we don't check whether the 
controller/driver is capable of transferring the new commands to device, i.e. 
that it's FIS-based.

WBR, Sergei

Re: [PATCH v3 3/3] libata: Add support for queued DSM TRIM

[Tejun Heo]

Hello,

On Thu, Aug 08, 2013 at 09:49:12PM -0700, Marc C wrote:
> +	if (ata_ncq_enabled(dev) &&
> +	    (dev->ncq_send_recv_cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &
> +	     ATA_LOG_NCQ_SEND_RECV_DSM_TRIM)) {

Can you add a feature test macro for this?

> +		/* 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;
> +
> +		qc->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;

So, there are controllers which actually look at the command code and
thus will choke on new commands and just looking at whether the device
supports the new commands is likely to be insufficient.  Have you
tested this?  If so, on which controllers / devices?  We're likely to
need whitelisting on controller side, I think.

Thanks.

-- 
tejun

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

Hello.

On 08/09/2013 08:49 AM, Marc C wrote:

> From: Marc Carino <marc.ceeeee@gmail.com>

> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

> Since there is no analog between the new field and the ATA
> taskfile, a new element was added to 'struct ata_queued_cmd."

> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>

[...]

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index c24354d..9d02c47 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -532,6 +532,34 @@ int atapi_cmd_type(u8 opcode)
>   }
>
>   /**
> + *	ata_qc_to_fis - Convert struct ata_queued_cmd to SATA FIS structure
> + *	@qc: struct ata_queued_cmd to convert
> + *	@pmp: Port multiplier port
> + *	@is_cmd: This FIS is for command
> + *	@fis: Buffer into which data will output
> + *
> + *	Converts a struct ata_queued_cmd to a Serial ATA
> + *	FIS structure (Register - Host to Device).
> + *
> + *	Beginning with SATA 3.1, the ATA taskfile does not completely describe
> + *	all of the fields in a host-to-device FIS. More specifically, the
> + *	'auxiliary' field has no ATA taskfile analog, and thus requires us
> + *	to populate the FIS via the ata_queued_cmd structure.
> + *
> + *	LOCKING:
> + *	Inherited from caller.
> + */
> +void ata_qc_to_fis(const struct ata_queued_cmd *qc, u8 pmp, int is_cmd, u8 *fis)
> +{
> +	ata_tf_to_fis(&qc->tf, pmp, is_cmd, fis);
> +
> +	fis[16] = qc->auxiliary & 0xff;
> +	fis[17] = (qc->auxiliary >> 8) & 0xff;
> +	fis[18] = (qc->auxiliary >> 16) & 0xff;
> +	fis[19] = (qc->auxiliary >> 24) & 0xff;
> +}
> +
> +/**
>    *	ata_tf_to_fis - Convert ATA taskfile to SATA FIS structure
>    *	@tf: Taskfile to convert
>    *	@pmp: Port multiplier port
> @@ -6877,6 +6905,7 @@ EXPORT_SYMBOL_GPL(ata_sg_init);
>   EXPORT_SYMBOL_GPL(ata_qc_complete);
>   EXPORT_SYMBOL_GPL(ata_qc_complete_multiple);
>   EXPORT_SYMBOL_GPL(atapi_cmd_type);
> +EXPORT_SYMBOL_GPL(ata_qc_to_fis);
>   EXPORT_SYMBOL_GPL(ata_tf_to_fis);

    I think we should now unexport this function and make it static since it 
would be no longer valid to call it from the drivers...

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

Hello.

On 08/09/2013 08:49 AM, Marc C wrote:

> From: Marc Carino <marc.ceeeee@gmail.com>

> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

> Since there is no analog between the new field and the ATA
> taskfile, a new element was added to 'struct ata_queued_cmd."

> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>
> ---
>   drivers/ata/acard-ahci.c  |  2 +-
>   drivers/ata/libahci.c     |  2 +-
>   drivers/ata/libata-core.c | 29 +++++++++++++++++++++++++++++
>   drivers/ata/sata_fsl.c    |  2 +-
>   drivers/ata/sata_mv.c     |  2 +-
>   drivers/ata/sata_qstor.c  |  2 +-
>   drivers/ata/sata_sil24.c  |  2 +-
>   include/linux/libata.h    |  4 ++++
>   8 files changed, 39 insertions(+), 6 deletions(-)

    Looks like you missed drivers/scsi/libsas/sas_ata.c...

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

On 08/09/2013 06:17 PM, Sergei Shtylyov wrote:

>> From: Marc Carino <marc.ceeeee@gmail.com>

>> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

>> Since there is no analog between the new field and the ATA
>> taskfile, a new element was added to 'struct ata_queued_cmd."

>> Signed-off-by: Marc Carino <marc.ceeeee@gmail.com>

> [...]

>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index c24354d..9d02c47 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -532,6 +532,34 @@ int atapi_cmd_type(u8 opcode)
>>   }
>>
>>   /**
>> + *    ata_qc_to_fis - Convert struct ata_queued_cmd to SATA FIS structure
>> + *    @qc: struct ata_queued_cmd to convert
>> + *    @pmp: Port multiplier port
>> + *    @is_cmd: This FIS is for command
>> + *    @fis: Buffer into which data will output
>> + *
>> + *    Converts a struct ata_queued_cmd to a Serial ATA
>> + *    FIS structure (Register - Host to Device).
>> + *
>> + *    Beginning with SATA 3.1, the ATA taskfile does not completely describe
>> + *    all of the fields in a host-to-device FIS. More specifically, the
>> + *    'auxiliary' field has no ATA taskfile analog, and thus requires us
>> + *    to populate the FIS via the ata_queued_cmd structure.
>> + *
>> + *    LOCKING:
>> + *    Inherited from caller.
>> + */
>> +void ata_qc_to_fis(const struct ata_queued_cmd *qc, u8 pmp, int is_cmd, u8
>> *fis)
>> +{
>> +    ata_tf_to_fis(&qc->tf, pmp, is_cmd, fis);
>> +
>> +    fis[16] = qc->auxiliary & 0xff;
>> +    fis[17] = (qc->auxiliary >> 8) & 0xff;
>> +    fis[18] = (qc->auxiliary >> 16) & 0xff;
>> +    fis[19] = (qc->auxiliary >> 24) & 0xff;
>> +}
>> +
>> +/**
>>    *    ata_tf_to_fis - Convert ATA taskfile to SATA FIS structure
>>    *    @tf: Taskfile to convert
>>    *    @pmp: Port multiplier port
>> @@ -6877,6 +6905,7 @@ EXPORT_SYMBOL_GPL(ata_sg_init);
>>   EXPORT_SYMBOL_GPL(ata_qc_complete);
>>   EXPORT_SYMBOL_GPL(ata_qc_complete_multiple);
>>   EXPORT_SYMBOL_GPL(atapi_cmd_type);
>> +EXPORT_SYMBOL_GPL(ata_qc_to_fis);
>>   EXPORT_SYMBOL_GPL(ata_tf_to_fis);

>     I think we should now unexport this function and make it static since it
> would be no longer valid to call it from the drivers...

    I was somewhat rash in this conclusion: drivers use this function not only 
to issue commands but also to fake D2H FIS and not only that...

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

On 08/09/2013 06:03 PM, Tejun Heo wrote:

>> From: Marc Carino <marc.ceeeee@gmail.com>

>> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

>> Since there is no analog between the new field and the ATA
>> taskfile, a new element was added to 'struct ata_queued_cmd."

> Ummm... I'm sorry but I'm gonna do 180 from Sergei on this one.  The

    That's very unfortunate for me. :-(

> auxiliary field is part of ata taskfile for all intents and purposes.
> FIS is the new command structure anyway and struct ata_taskfile proper
> should be able to describe the command with ata_queuedcmd providing
> the surrounding context.  The argument that ata_taskfile shouldn't
> contain anything which wasn't in PATA taskfile is bogus as it already
> contains ATA_TFLAG_*.

    That's what make 'struct ata_taskfile' bogus. I'm going to remove 
'protocol', 'flags', and 'ctl' fields from there (at least to save a space in 
'struct ata_queued_cmd' because they're not used in 'result_tf').

> So, please put the aux field into ata_taskfile.  That's where it
> belongs.

    Can't agree to that. I was going to make 'struct ata_taskfile' reflect the 
historical notion and remove from it all not belonging to that notion. Alas, 
libata has a bad history of mistreating the historical terms...

> Thanks.

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Tejun Heo]

On Fri, Aug 09, 2013 at 06:36:19PM +0400, Sergei Shtylyov wrote:
> >Ummm... I'm sorry but I'm gonna do 180 from Sergei on this one.  The
> 
>    That's very unfortunate for me. :-(

Hehe, sorry. :)

> >So, please put the aux field into ata_taskfile.  That's where it
> >belongs.
> 
>    Can't agree to that. I was going to make 'struct ata_taskfile'
> reflect the historical notion and remove from it all not belonging
> to that notion. Alas, libata has a bad history of mistreating the
> historical terms...

But what does sticking to the historical definition give us when the
whole concept of TF is no longer maintained in new specs.  The
distinction between "command proper" and "supporting / plumbing
information" is still useful so I think it's natural to assign the
former to ata_taskfile and the latter to ata_queuedcmd.  After all,
there are cases where we want to describe command without all the
plumbing stuff libata imposes and given that the aux field is part of
FIS proper it might even get used for command results too in the
future.  I mean, FIS is the new TF.  We can rename ata_taskfile to
ata_fis and map things the other way but that'd just be extra churn.

Thanks.

-- 
tejun

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

On 08/09/2013 06:03 PM, Tejun Heo wrote:

>> From: Marc Carino <marc.ceeeee@gmail.com>

>> SATA 3.1 added an "auxiliary" field to the host-to-device FIS.

>> Since there is no analog between the new field and the ATA
>> taskfile, a new element was added to 'struct ata_queued_cmd."

> Ummm... I'm sorry but I'm gonna do 180 from Sergei on this one.  The
> auxiliary field is part of ata taskfile for all intents and purposes.

    Which would be another abuse of the ATA taskfile term (reminds me of SFF 
abuse wehre everybody hushed me down saying that it's too late to change 
anything and for all intenets and purposes SFF means "taskfile based" for 
everybody). As Marc rightly said, this field won't be deliverable by the usual 
taskfile methods. Moreover, it won't be used in qc->result_tf' and so will 
waste 4 bytes for no reason.

> FIS is the new command structure anyway and struct ata_taskfile proper
> should be able to describe the command with ata_queuedcmd providing
> the surrounding context.

    It depends on your definition of surrounding context. In my definition, 
'flags' and 'protocol' fields should be a part of surrounding context. 'ctl' 
too sicen the device control register was never considered a part of the ATA 
taskfile and it's not used in 'qc->result_tf' (as well as the other two 
fields), hence just wasting memory for no reason.

> The argument that ata_taskfile shouldn't
> contain anything which wasn't in PATA taskfile is bogus as it already
> contains ATA_TFLAG_*.

    I regret that I haven't done my patches earlier to get rid of this example 
of abuse of the ATA taskfile before.

> So, please put the aux field into ata_taskfile.  That's where it
> belongs.

    Sigh.

> Thanks.

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

Hello.

On 08/09/2013 06:53 PM, Tejun Heo wrote:

>>> Ummm... I'm sorry but I'm gonna do 180 from Sergei on this one.  The

>>     That's very unfortunate for me. :-(

> Hehe, sorry. :)

    I've started to work on my taskfile patchset about a year ago (while being 
in hospital) and worked on it on my copious free time (perhaps, not actively 
enough until I realized I don't have much time anymore), so it doesn't sound 
funny for me. If you're going to reject my patches once submitted outright, 
just tell me now, and with some regret for the wasted time, I'll find a better 
use for my free time, making a note to myself that the taskfile support in 
libata is hopeless and the maintainer doesn't care a bit about that. (In case 
you want an example of better taskfile support, look at IDE).

>>> So, please put the aux field into ata_taskfile.  That's where it
>>> belongs.

>>     Can't agree to that. I was going to make 'struct ata_taskfile'
>> reflect the historical notion and remove from it all not belonging
>> to that notion. Alas, libata has a bad history of mistreating the
>> historical terms...

> But what does sticking to the historical definition give us when the
> whole concept of TF is no longer maintained in new specs.

    It predated even the old specs, so what?

> The
> distinction between "command proper" and "supporting / plumbing
> information" is still useful

    Yes, it's just the border was drawn incorrectly from the start IMO.
Jeff even placed the structure and friends into ata.h for no apparent reason 
(part of those *enum* friends eventually got shared by IDE).

> so I think it's natural to assign the
> former to ata_taskfile and the latter to ata_queuedcmd.

    That's exactly what I want to do.

> After all, there are cases where we want to describe command without all the
> plumbing stuff libata imposes

    Exactly what I want to achieve.

> and given that the aux field is part of
> FIS proper it might even get used for command results too in the
> future.

    If and when this happens, we'll see. So far, the structure of H2D and D2H 
FIS is different enough that libata doesn't have a stucture defined for FISes, 
and just treats them as array of u8.

> I mean, FIS is the new TF.

    FISes are used by what, handful of controllers? While the others, even 
SATA are still taskfile based and can't transfer FISes directly (especially 
true for the embedded world where not everybody wants to use AHCI). So that's 
kind of wishful thinking for now. And don't forget about the PATA legacy 
you'll have to drag around forever.

> We can rename ata_taskfile to
> ata_fis and map things the other way but that'd just be extra churn.

    Indeed. And totally stupid move concerning the amount of direct FIS 
support in different hardware.

> Thanks.

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Tejun Heo]

Hello, Sergei.

On Sat, Aug 10, 2013 at 01:39:08AM +0400, Sergei Shtylyov wrote:
>    I've started to work on my taskfile patchset about a year ago
> (while being in hospital) and worked on it on my copious free time
> (perhaps, not actively enough until I realized I don't have much
> time anymore), so it doesn't sound funny for me. If you're going to
> reject my patches once submitted outright, just tell me now, and
> with some regret for the wasted time, I'll find a better use for my
> free time, making a note to myself that the taskfile support in
> libata is hopeless and the maintainer doesn't care a bit about that.
> (In case you want an example of better taskfile support, look at
> IDE).

Can you explain why following the traditional TF definition matters so
much?  What practical difference does that make?  The new field is
part of command code definition, so it belongs with the rest of them
regardless of what the structure it's contained in is named.  If the
only reason for strictly separating TF regs into a separate struct is
because the spec says so, I indeed don't give a flying hoot.

Also, the only controller interface which would continue to be
relevant is ahci and that's it.  There will be no new development
whatsoever happening with TF based interface, ever.  I don't see why
you're getting all passive agressive about it.  If you have technical
arguments, dish them out.

Thanks.

-- 
tejun

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

Hello.

On 08/10/2013 01:51 AM, Tejun Heo wrote:

>>     I've started to work on my taskfile patchset about a year ago
>> (while being in hospital) and worked on it on my copious free time
>> (perhaps, not actively enough until I realized I don't have much
>> time anymore), so it doesn't sound funny for me. If you're going to
>> reject my patches once submitted outright, just tell me now, and
>> with some regret for the wasted time, I'll find a better use for my
>> free time, making a note to myself that the taskfile support in
>> libata is hopeless and the maintainer doesn't care a bit about that.
>> (In case you want an example of better taskfile support, look at
>> IDE).

> Can you explain why following the traditional TF definition matters so
> much?

    Because it's more clear, saves some memory and matches the (rather poor) 
capabilities of the libata's driver taskfile interface (which you can't throw 
out however you wanted).

> What practical difference does that make?  The new field is
> part of command code definition, so it belongs with the rest of them
> regardless of what the structure it's contained in is named.

    So why not place it to 'struct ata_queued_cmd' then? If it doesn't really 
matter where to put it if it serves to describe a command, and additionally 
will save you some memory?

> If the only reason for strictly separating TF regs into a separate struct is
> because the spec says so,

    No. Besides, as I told you, taskfile isn't in any spec, it's pre-ATA term.

> I indeed don't give a flying hoot.

    Excellent reply from a maintainer. :-D

> Also, the only controller interface which would continue to be
> relevant is ahci and that's it.  There will be no new development
> whatsoever happening with TF based interface, ever.

    In x86 world maybe but how much does it help you with the legacy stuff you 
have to drag around?
    Tell about AHCI to my embedded customer, Renesas. Remember the most recent 
sata_rcar driver I have submitted? It's taskfile based (there's also SATA 
driver we at MontaVista did write but didn't submit). And it's used in their 
top-notch R-Car SoCs.

> I don't see why you're getting all passive agressive about it.

    Don't intimidate me with psychological terms. :-)

> If you have technical arguments, dish them out.

    I have. It seems you intentionally ignore them, and reply to non-technical 
passage only.

> Thanks.

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Tejun Heo]

Hey,

On Sat, Aug 10, 2013 at 02:17:24AM +0400, Sergei Shtylyov wrote:
>    So why not place it to 'struct ata_queued_cmd' then? If it
> doesn't really matter where to put it if it serves to describe a
> command, and additionally will save you some memory?

Because it's just more churn?  Now TF basically matches what goes into
FIS.  Adding aux to qc break that for no good reason (no, 4 bytes
isn't a good reason).

>    In x86 world maybe but how much does it help you with the legacy
> stuff you have to drag around?
>    Tell about AHCI to my embedded customer, Renesas. Remember the
> most recent sata_rcar driver I have submitted? It's taskfile based
> (there's also SATA driver we at MontaVista did write but didn't
> submit). And it's used in their top-notch R-Car SoCs.

Oh sure, we'll carry the existing ones and there will be new drivers
for sure.  I'm saying that in terms of command protocol and standard,
it is and has long been a dead end.  Nothing can happen there anymore.
There's no point in putting TF based interface in the center of
attention.

> >I don't see why you're getting all passive agressive about it.
> 
>    Don't intimidate me with psychological terms. :-)

Please adjust your tone then because your original reply was very
easily escalatable.  If you want to escalate things, I'm game but if
you don't want that, please make that clear too.

> >If you have technical arguments, dish them out.
> 
>    I have. It seems you intentionally ignore them, and reply to
> non-technical passage only.

The only concrete thing I got upto this point is saving 4 bytes, which
is a bit difficult to consider seriously.  Other parts, I don't get at
all.  Sure there are TF drivers, but how does adding aux field to TF
make it worse for them?

Thanks.

-- 
tejun

Re: [PATCH v3 2/3] libata: Add support for SEND/RECEIVE FPDMA QUEUED

[Marc C]

Hello,

On 8/9/2013 7:05 AM, Tejun Heo wrote:
> On Thu, Aug 08, 2013 at 09:49:11PM -0700, Marc C wrote:
>> @@ -2131,6 +2131,7 @@ static int ata_dev_config_ncq(struct ata_device *dev,
>>  	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
>>  	unsigned int err_mask;
>>  	char *aa_desc = "";
>> +	u8 sata_setting[ATA_SECT_SIZE];
> 
> Nope.  Please don't do this.  Can't you use ap->sector_buf?
> 
Yup, I should be able to use that instead.

Thanks,
Marc

Re: [PATCH v3 3/3] libata: Add support for queued DSM TRIM

[Marc C]

Hello,

On 8/9/2013 7:08 AM, Tejun Heo wrote:

> So, there are controllers which actually look at the command code and
> thus will choke on new commands and just looking at whether the device
> supports the new commands is likely to be insufficient.  Have you
> tested this?  If so, on which controllers / devices?  We're likely to
> need whitelisting on controller side, I think.
Not entirely sure. Non-AHCI controllers scare me since they typically
don't come along with specs, or are just finicky. This is especially
true since this is a new NCQ command. Anyway, I'll use a flag (as
suggested in the other thread) to differentiate between controllers that
_should_ be able to handle this properly (AHCI) and other
vendor-specific SATA controllers.

Thanks,
Marc

Re: [PATCH v3 3/3] libata: Add support for queued DSM TRIM

[Tejun Heo]

Hello,

On Fri, Aug 09, 2013 at 07:14:07PM -0700, Marc C wrote:
> Not entirely sure. Non-AHCI controllers scare me since they typically
> don't come along with specs, or are just finicky. This is especially
> true since this is a new NCQ command. Anyway, I'll use a flag (as
> suggested in the other thread) to differentiate between controllers that
> _should_ be able to handle this properly (AHCI) and other
> vendor-specific SATA controllers.

I'm fine with making it only available on ahci's, at least for now,
but can you please make the flag distinguish the specific feature
rather than the controller is AHCI or not?

Thanks.

-- 
tejun

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Sergei Shtylyov]

Hello.

On 08/10/2013 02:26 AM, Tejun Heo wrote:

>>     So why not place it to 'struct ata_queued_cmd' then? If it
>> doesn't really matter where to put it if it serves to describe a
>> command, and additionally will save you some memory?

> Because it's just more churn?  Now TF basically matches what goes into
> FIS.  Adding aux to qc break that for no good reason (no, 4 bytes
> isn't a good reason).

    Funny to remember, when I was just starting my Linux IDE development back 
in 2006, I published a patch that added 4-byte field to 'ide_hwif_t' and some 
people started complaining about memory waste, so that I had to promise I'd 
send a next patch that would remove another 40bte field. Clearly, they didn't 
expect an explosion in IDE development that stated happening the next year. 
:-) That time however is gone, and I probably can't promise as stable stream 
of patches as it was back then, especially given my one-year development 
period for the mentioned taskfile patches (which I still can't publish due to 
completely unexpected issue guaranteeing several debugging sessions).

>>     In x86 world maybe but how much does it help you with the legacy
>> stuff you have to drag around?
>>     Tell about AHCI to my embedded customer, Renesas. Remember the
>> most recent sata_rcar driver I have submitted? It's taskfile based
>> (there's also SATA driver we at MontaVista did write but didn't
>> submit). And it's used in their top-notch R-Car SoCs.

> Oh sure, we'll carry the existing ones and there will be new drivers
> for sure.  I'm saying that in terms of command protocol and standard,
> it is and has long been a dead end.  Nothing can happen there anymore.
> There's no point in putting TF based interface in the center of
> attention.

    I still would like it improved regardless how old the issues there are, 
I'm just not sure I can do it. Being engaged in the embedded area, I feel more 
comfortable with the old stuff than the new, hence my interest.

>>> I don't see why you're getting all passive agressive about it.

>>     Don't intimidate me with psychological terms. :-)

> Please adjust your tone then because your original reply was very
> easily escalatable.  If you want to escalate things, I'm game but if
> you don't want that, please make that clear too.

    No, I don't want to escalate to Linus, if you meant it. I was just feeling 
somewhat bitter. However, I would like to hear a clear answer to my question 
about the taskfile patches currnetly in the works (although they seem to make 
less sence now that the taskfile "purity" is out of question): is there a 
chance you apply them or will you reject them as a needless churn?

>>> If you have technical arguments, dish them out.

>>     I have. It seems you intentionally ignore them, and reply to
>> non-technical passage only.

> The only concrete thing I got upto this point is saving 4 bytes, which
> is a bit difficult to consider seriously.  Other parts, I don't get at
> all.  Sure there are TF drivers, but how does adding aux field to TF
> make it worse for them?

    Well, it seems I'm just out of arguments at this point. It seems just 
aesthetically unpleasing and not right to me to have a field in taskfile which 
can't be delivered via the taskfile interface, if you want.
    To be honest, I also have a patch that makes the taskfile lose it's 'hob_' 
fields, and so the '[result_]tf' variables in the 'struct ata_queued_cmd' 
becoming arrays of 2 elements but I was somewhat afraid of publishing it 
because it's not exactly a memory saver (and a candidate of being labelled a 
pointless churn). Adding the new field to struct ata_taskfile just would make 
this patch completely impractical...

> Thanks.

Not at all.

WBR, Sergei

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Tejun Heo]

Hello, Sergei.

On Sun, Aug 11, 2013 at 01:59:05AM +0400, Sergei Shtylyov wrote:
> >Oh sure, we'll carry the existing ones and there will be new drivers
> >for sure.  I'm saying that in terms of command protocol and standard,
> >it is and has long been a dead end.  Nothing can happen there anymore.
> >There's no point in putting TF based interface in the center of
> >attention.
> 
>    I still would like it improved regardless how old the issues
> there are, I'm just not sure I can do it. Being engaged in the
> embedded area, I feel more comfortable with the old stuff than the
> new, hence my interest.

Sure, improvements are always welcome.  My point is that when making
trade-offs, it'd make more sense to prioritize design concerns for FIS
based ones rather than the other way around.  Here, the only concrete
downside for TF based ones is 4 extra bytes in ata_taskfile, which I
don't think matters at all.

>    No, I don't want to escalate to Linus, if you meant it. I was
> just feeling somewhat bitter. However, I would like to hear a clear
> answer to my question about the taskfile patches currnetly in the
> works (although they seem to make less sence now that the taskfile
> "purity" is out of question): is there a chance you apply them or
> will you reject them as a needless churn?

I can't say for sure before actually looking at the patch but I'm
likely to nack it if the only reason behind it is saving some bytes
and conformance to the traditional definition of taskfile, especially
as I'd much prefer to be able to put all fields of a FIS into
ata_taskfile.  If the moving out the prot and flags out of ata_tf
makes the code generally cleaner, sure, but I'm a bit skeptical it
would.  Why don't you post the patch as-is?  Let's see how the whole
thing looks like.

>    Well, it seems I'm just out of arguments at this point. It seems
> just aesthetically unpleasing and not right to me to have a field in
> taskfile which can't be delivered via the taskfile interface, if you
> want.
>    To be honest, I also have a patch that makes the taskfile lose
> it's 'hob_' fields, and so the '[result_]tf' variables in the
> 'struct ata_queued_cmd' becoming arrays of 2 elements but I was
> somewhat afraid of publishing it because it's not exactly a memory
> saver (and a candidate of being labelled a pointless churn). Adding
> the new field to struct ata_taskfile just would make this patch
> completely impractical...

So, two things.  Code cleanup is always a good idea; however,
"aesthetically unpleasing" is pretty subjective and I'm not too likely
to be fond of changes which are puristic in its nature without any
practical cleanup value.

Secondly, optimization is good but it has to matter.  There's no point
in saving some bytes in a struct which aren't allocated in massive
numbers.  Likewise, there's no point in optimizing some cycles in
paths which aren't hot.  Straight-forwardness and maintainability
should be the prime concerns in most cases.

Thanks.

-- 
tejun

Re: [PATCH v3 1/3] libata: Populate host-to-device FIS "auxiliary" field

[Marc C]

Hello,
> The auxiliary field is part of ata taskfile for all intents and purposes.
> FIS is the new command structure anyway and struct ata_taskfile proper
> should be able to describe the command with ata_queuedcmd providing
> the surrounding context.  The argument that ata_taskfile shouldn't
> contain anything which wasn't in PATA taskfile is bogus as it already
> contains ATA_TFLAG_*.
>     That's what make 'struct ata_taskfile' bogus. I'm going to remove 
> 'protocol', 'flags', and 'ctl' fields from there (at least to save a space in 
> 'struct ata_queued_cmd' because they're not used in 'result_tf').
>
>> So, please put the aux field into ata_taskfile.  That's where it
>> belongs.
>     Can't agree to that. I was going to make 'struct ata_taskfile' reflect the 
> historical notion and remove from it all not belonging to that notion. Alas, 
> libata has a bad history of mistreating the historical terms...

While I understand that some non-spec items don't belong in the taskfile struct (and should/will be cleaned up), I have to agree with Tejun regarding how the auxiliary field should be put in 'struct ata_taskfile'. If anything, the struct should represent ALL items that an ATA command will use as inputs. As of now, it's the stuff that's currently in the taskfile + this pesky new 'auxiliary' entry. (FYI, ATA-8 ACS refers to these items as"fields.")

>From what I can tell, there are a couple of places where a rigid definition of a taskfile truly matters, and that's:

1) SCSI to ATA passthrough
2) PATA

But in any event, sticking 'auxiliary' in doesn't matter from a compatibility standpoint, since only NCQ commands use the new field, and the above can't transport NCQ (yet in (1) case).

Tejun put it succinctly by stating:

> I mean, FIS is the new TF.  We can rename ata_taskfile to
> ata_fis and map things the other way but that'd just be extra churn.

Ideally this would be the case, since the SATA FIS is a super-set of any/all fields defined in all ATA commands.

Regards,
Marc