Introduce new SATA queued commands

From: Marc Carino <[email protected]>

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."

Quoted 6 lines

From: Marc Carino <[email protected]>

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.

Quoted 1 line

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

   That's very unfortunate for me. :-(

Quoted 2 lines

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

Quoted 3 lines

Quoted 1 line

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

    That's very unfortunate for me. :-(

Hehe, sorry. :)

Quoted 7 lines

Quoted 2 lines

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.

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).

Quoted 10 lines

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.

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?

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.

Quoted 1 line

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

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

Quoted 1 line

If you have technical arguments, dish them out.

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

Quoted 3 lines

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).

Quoted 6 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 5 lines

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.

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?

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

Quoted 6 lines

From: Marc Carino <[email protected]>

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.

From: Marc Carino <[email protected]>

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 <[email protected]>

drivers/ata/libata-core.c +28 -0

index c24354d..9d02c47 100644
@@ -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

Quoted 8 lines

From: Marc Carino <[email protected]>

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 <[email protected]>

[...]

Quoted 41 lines

drivers/ata/libata-core.c +28 -0

index c24354d..9d02c47 100644
@@ -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

drivers/ata/libata-core.c +0 -0

@@ -6877,6 +6905,7 @@ EXPORT_SYMBOL_GPL(ata_sg_init);

drivers/ata/libata-core.c EXPORT_SYMBOL_GPL(ata_sg_init); · line 6909

Quoted 5 lines

drivers/ata/libata-core.c +1 -0

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

From: Marc Carino <[email protected]>

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 <[email protected]>

---
  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(-)

drivers/ata/libata-core.c static int ata_dev_config_ncq(struct ata_device *dev, · line 2134

Quoted 5 lines · Marc C

drivers/ata/libata-core.c +1 -0

@@ -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?

From: Marc Carino <[email protected]>

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 <[email protected]>

---
  drivers/ata/libata-scsi.c | 27 +++++++++++++++++++++------
  1 file changed, 21 insertions(+), 6 deletions(-)

drivers/ata/libata-scsi.c +11 -6

index 83c0890..1605ffc 100644
@@ -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;
+

drivers/ata/libata-scsi.c +6 -0

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

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.