On 05/13/2016 10:32 AM, Damien Le Moal wrote:

Hannes,

quoted

Add NCQ encapsulation for ZAC MANAGEMENT OUT and evaluate
NCQ Non-Data log pages to figure out if NCQ encapsulation
is supported.

...

quoted

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6afd084..43403aa 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c

@@ -3506,11 +3506,19 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)

	reset_all = cdb[14] & 0x1;

-	tf->protocol = ATA_PROT_NODATA;
-	tf->command = ATA_CMD_ZAC_MGMT_OUT;
-	tf->feature = sa;
-	tf->hob_feature = reset_all & 0x1;
-
+	if (ata_ncq_enabled(qc->dev) &&
+	    ata_fpdma_zac_mgmt_out_supported(qc->dev)) {
+		tf->protocol = ATA_PROT_NCQ;
+		tf->command = ATA_CMD_NCQ_NON_DATA;
+		tf->hob_nsect = ATA_SUBCMD_NCQ_NON_DATA_ZAC_MGMT_OUT;
+		tf->nsect = qc->tag << 3;
+		tf->auxiliary = sa | (reset_all & 0x1) << 8;
+	} else {
+		tf->protocol = ATA_PROT_NODATA;
+		tf->command = ATA_CMD_ZAC_MGMT_OUT;
+		tf->feature = sa;
+		tf->hob_feature = reset_all & 0x1;
+	}

Calls to ata_is_data(prot) for ATA_PROT_NCQ task files will always
return "true", even if the command is ATA_CMD_NCQ_NON_DATA (because
the DMA prot flag is always set by ata_prot_flags for NCQ protocol).
This result in all the ATA_CMD_ZAC_MGMT_OUT commands to fail in
ata_qc_issue because of the check:

         if (WARN_ON_ONCE(ata_is_data(prot) &&
                          (!qc->sg || !qc->n_elem || !qc->nbytes)))
                 goto sys_err;


I am not sure the best way to fix this... Hacking ata_qc_issue using the protocol
AND command code to define a bool "is_data" prevents the error but ata_is_data is
also used in different drivers so a more solid fix seem necessary.

Adding a new ATA_PROT_NCQ_NODATA for ATA_CMD_NCQ_NON_DATA would be better, but this
does not really correspond to anything in the standards. And with this, the error
moves to the command sg dma setup complaining that the DMA direction is not known.
Any ideas ?

Hmm. Isn't there simply an 'ATA_TFLAG_WRITE' missing in tf->flags later on?

Also, the "& 0x1" in "tf->auxiliary = sa | (reset_all & 0x1) << 8;" is not necessary
since reset_all is initialized already as "reset_all = cdb[14] & 0x1;"

Yeah. We can fix it up.

Cheers,

Hannes