[PATCH v2 0/7] misc libata improvements

[PATCH v2 0/7] misc libata improvements

[Niklas Cassel]

Hello there,

This series contains misc libata improvements.

These improvements were identified while developing support for Command
Duration Limits (CDL). All patches in this series (i.e. V1 of these
patches) were orignally sent out as part of the CDL series, found here:
https://lore.kernel.org/linux-scsi/510732e0-7962-cf54-c22c-f1d7066895f5@opensource.wdc.com/T/

However, as these improvements are completely unrelated to CDL, they can
be merged independently, and should not need to wait for other patches.


Kind regards,
Niklas


Changes since V1:
-Added missing chain sign-off (in addition to author sign-off).
-Picked up tags from John.
-Rephrased commit message for patch 1/7 as suggested by John.
-Rephrased commit subject for patch 3/7 to more clearly hightlight
 that this is simply an improvement, and not strictly a bug fix.

Damien Le Moal (2):
  ata: libata: simplify qc_fill_rtf port operation interface
  ata: libata-scsi: improve ata_scsiop_maint_in()

Niklas Cassel (5):
  ata: scsi: rename flag ATA_QCFLAG_FAILED to ATA_QCFLAG_EH
  ata: libata: read the shared status for successful NCQ commands once
  ata: libata: respect successfully completed commands during errors
  ata: libata: move NCQ related ATA_DFLAGs
  ata: libata-scsi: do not overwrite SCSI ML and status bytes

 drivers/ata/acard-ahci.c      |   8 +-
 drivers/ata/libahci.c         | 171 ++++++++++++++++++++++++++--------
 drivers/ata/libata-core.c     |  12 +--
 drivers/ata/libata-eh.c       |  22 ++---
 drivers/ata/libata-sata.c     |   7 +-
 drivers/ata/libata-scsi.c     |  11 ++-
 drivers/ata/libata-sff.c      |  10 +-
 drivers/ata/libata-trace.c    |   2 +-
 drivers/ata/sata_fsl.c        |   5 +-
 drivers/ata/sata_inic162x.c   |  14 ++-
 drivers/ata/sata_promise.c    |   2 +-
 drivers/ata/sata_sil24.c      |   7 +-
 drivers/ata/sata_sx4.c        |   2 +-
 drivers/scsi/ipr.c            |  11 +--
 drivers/scsi/libsas/sas_ata.c |  11 +--
 include/linux/libata.h        |  25 ++---
 16 files changed, 201 insertions(+), 119 deletions(-)

-- 
2.38.1

[PATCH v2 1/7] ata: scsi: rename flag ATA_QCFLAG_FAILED to ATA_QCFLAG_EH

[Niklas Cassel]

The name ATA_QCFLAG_FAILED is misleading since it does not mean that a
QC completed in error, or that it didn't complete at all. It means that
libata decided to schedule EH for the QC, so the QC is now owned by the
libata error handler (EH).

The normal execution path is responsible for not accessing a QC owned
by EH. libata core enforces the rule by returning NULL from
ata_qc_from_tag() for QCs owned by EH.

It is quite easy to mistake that a QC marked with ATA_QCFLAG_FAILED was
an error. However, a QC that was actually an error is instead indicated
by having qc->err_mask set. E.g. when we have a NCQ error, we abort all
QCs, which currently will mark all QCs as ATA_QCFLAG_FAILED. However, it
will only be a single QC that is an error (i.e. has qc->err_mask set).

Rename ATA_QCFLAG_FAILED to ATA_QCFLAG_EH to more clearly highlight that
this flag simply means that a QC is now owned by EH. This new name will
not mislead to think that the QC was an error (which is instead
indicated by having qc->err_mask set).

This also makes it more obvious that the EH code skips all QCs that do
not have ATA_QCFLAG_EH set (rather than ATA_QCFLAG_FAILED), since the EH
code should simply only care about QCs that are owned by EH itself.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
 drivers/ata/acard-ahci.c      |  2 +-
 drivers/ata/libahci.c         |  4 ++--
 drivers/ata/libata-core.c     | 12 ++++++------
 drivers/ata/libata-eh.c       | 22 +++++++++++-----------
 drivers/ata/libata-sata.c     |  4 ++--
 drivers/ata/libata-sff.c      |  4 ++--
 drivers/ata/libata-trace.c    |  2 +-
 drivers/ata/sata_fsl.c        |  2 +-
 drivers/ata/sata_inic162x.c   |  2 +-
 drivers/ata/sata_promise.c    |  2 +-
 drivers/ata/sata_sil24.c      |  2 +-
 drivers/ata/sata_sx4.c        |  2 +-
 drivers/scsi/ipr.c            |  4 ++--
 drivers/scsi/libsas/sas_ata.c |  8 ++++----
 include/linux/libata.h        |  4 ++--
 15 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index 7654a40c12b4..da74a86b70ba 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -263,7 +263,7 @@ static bool acard_ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	 * Setup FIS.
 	 */
 	if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
-	    !(qc->flags & ATA_QCFLAG_FAILED)) {
+	    !(qc->flags & ATA_QCFLAG_EH)) {
 		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
 		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
 	} else
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 29acc35bf4a6..03aa9eb415d3 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2068,7 +2068,7 @@ static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	 * Setup FIS.
 	 */
 	if (qc->tf.protocol == ATA_PROT_PIO && qc->dma_dir == DMA_FROM_DEVICE &&
-	    !(qc->flags & ATA_QCFLAG_FAILED)) {
+	    !(qc->flags & ATA_QCFLAG_EH)) {
 		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
 		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
 
@@ -2138,7 +2138,7 @@ static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 
 	/* make DMA engine forget about the failed command */
-	if (qc->flags & ATA_QCFLAG_FAILED)
+	if (qc->flags & ATA_QCFLAG_EH)
 		ahci_kick_engine(ap);
 }
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 884ae73b11ea..6b03bebcde50 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1590,7 +1590,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
 		ap->ops->post_internal_cmd(qc);
 
 	/* perform minimal error analysis */
-	if (qc->flags & ATA_QCFLAG_FAILED) {
+	if (qc->flags & ATA_QCFLAG_EH) {
 		if (qc->result_tf.status & (ATA_ERR | ATA_DF))
 			qc->err_mask |= AC_ERR_DEV;
 
@@ -4683,10 +4683,10 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 	/* XXX: New EH and old EH use different mechanisms to
 	 * synchronize EH with regular execution path.
 	 *
-	 * In new EH, a failed qc is marked with ATA_QCFLAG_FAILED.
+	 * In new EH, a qc owned by EH is marked with ATA_QCFLAG_EH.
 	 * Normal execution path is responsible for not accessing a
-	 * failed qc.  libata core enforces the rule by returning NULL
-	 * from ata_qc_from_tag() for failed qcs.
+	 * qc owned by EH.  libata core enforces the rule by returning NULL
+	 * from ata_qc_from_tag() for qcs owned by EH.
 	 *
 	 * Old EH depends on ata_qc_complete() nullifying completion
 	 * requests if ATA_QCFLAG_EH_SCHEDULED is set.  Old EH does
@@ -4698,7 +4698,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 		struct ata_eh_info *ehi = &dev->link->eh_info;
 
 		if (unlikely(qc->err_mask))
-			qc->flags |= ATA_QCFLAG_FAILED;
+			qc->flags |= ATA_QCFLAG_EH;
 
 		/*
 		 * Finish internal commands without any further processing
@@ -4715,7 +4715,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
 		 * Non-internal qc has failed.  Fill the result TF and
 		 * summon EH.
 		 */
-		if (unlikely(qc->flags & ATA_QCFLAG_FAILED)) {
+		if (unlikely(qc->flags & ATA_QCFLAG_EH)) {
 			fill_result_tf(qc);
 			trace_ata_qc_complete_failed(qc);
 			ata_qc_schedule_eh(qc);
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 34303ce67c14..8cb250930c48 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -575,7 +575,7 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 	 * normal completion, error completion, and SCSI timeout.
 	 * Both completions can race against SCSI timeout.  When normal
 	 * completion wins, the qc never reaches EH.  When error
-	 * completion wins, the qc has ATA_QCFLAG_FAILED set.
+	 * completion wins, the qc has ATA_QCFLAG_EH set.
 	 *
 	 * When SCSI timeout wins, things are a bit more complex.
 	 * Normal or error completion can occur after the timeout but
@@ -611,10 +611,10 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 
 			if (i < ATA_MAX_QUEUE) {
 				/* the scmd has an associated qc */
-				if (!(qc->flags & ATA_QCFLAG_FAILED)) {
+				if (!(qc->flags & ATA_QCFLAG_EH)) {
 					/* which hasn't failed yet, timeout */
 					qc->err_mask |= AC_ERR_TIMEOUT;
-					qc->flags |= ATA_QCFLAG_FAILED;
+					qc->flags |= ATA_QCFLAG_EH;
 					nr_timedout++;
 				}
 			} else {
@@ -631,7 +631,7 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		 * this point but the state of the controller is
 		 * unknown.  Freeze the port to make sure the IRQ
 		 * handler doesn't diddle with those qcs.  This must
-		 * be done atomically w.r.t. setting QCFLAG_FAILED.
+		 * be done atomically w.r.t. setting ATA_QCFLAG_EH.
 		 */
 		if (nr_timedout)
 			__ata_port_freeze(ap);
@@ -911,12 +911,12 @@ void ata_qc_schedule_eh(struct ata_queued_cmd *qc)
 
 	WARN_ON(!ap->ops->error_handler);
 
-	qc->flags |= ATA_QCFLAG_FAILED;
+	qc->flags |= ATA_QCFLAG_EH;
 	ata_eh_set_pending(ap, 1);
 
 	/* The following will fail if timeout has already expired.
 	 * ata_scsi_error() takes care of such scmds on EH entry.
-	 * Note that ATA_QCFLAG_FAILED is unconditionally set after
+	 * Note that ATA_QCFLAG_EH is unconditionally set after
 	 * this function completes.
 	 */
 	blk_abort_request(scsi_cmd_to_rq(qc->scsicmd));
@@ -994,7 +994,7 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
 	/* include internal tag in iteration */
 	ata_qc_for_each_with_internal(ap, qc, tag) {
 		if (qc && (!link || qc->dev->link == link)) {
-			qc->flags |= ATA_QCFLAG_FAILED;
+			qc->flags |= ATA_QCFLAG_EH;
 			ata_qc_complete(qc);
 			nr_aborted++;
 		}
@@ -1954,7 +1954,7 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 	all_err_mask |= ehc->i.err_mask;
 
 	ata_qc_for_each_raw(ap, qc, tag) {
-		if (!(qc->flags & ATA_QCFLAG_FAILED) ||
+		if (!(qc->flags & ATA_QCFLAG_EH) ||
 		    qc->flags & ATA_QCFLAG_RETRY ||
 		    ata_dev_phys_link(qc->dev) != link)
 			continue;
@@ -2232,7 +2232,7 @@ static void ata_eh_link_report(struct ata_link *link)
 		desc = ehc->i.desc;
 
 	ata_qc_for_each_raw(ap, qc, tag) {
-		if (!(qc->flags & ATA_QCFLAG_FAILED) ||
+		if (!(qc->flags & ATA_QCFLAG_EH) ||
 		    ata_dev_phys_link(qc->dev) != link ||
 		    ((qc->flags & ATA_QCFLAG_QUIET) &&
 		     qc->err_mask == AC_ERR_DEV))
@@ -2298,7 +2298,7 @@ static void ata_eh_link_report(struct ata_link *link)
 		char data_buf[20] = "";
 		char cdb_buf[70] = "";
 
-		if (!(qc->flags & ATA_QCFLAG_FAILED) ||
+		if (!(qc->flags & ATA_QCFLAG_EH) ||
 		    ata_dev_phys_link(qc->dev) != link || !qc->err_mask)
 			continue;
 
@@ -3802,7 +3802,7 @@ void ata_eh_finish(struct ata_port *ap)
 
 	/* retry or finish qcs */
 	ata_qc_for_each_raw(ap, qc, tag) {
-		if (!(qc->flags & ATA_QCFLAG_FAILED))
+		if (!(qc->flags & ATA_QCFLAG_EH))
 			continue;
 
 		if (qc->err_mask) {
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 18ef14e749a0..908f35acee1e 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1429,7 +1429,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 
 	/* has LLDD analyzed already? */
 	ata_qc_for_each_raw(ap, qc, tag) {
-		if (!(qc->flags & ATA_QCFLAG_FAILED))
+		if (!(qc->flags & ATA_QCFLAG_EH))
 			continue;
 
 		if (qc->err_mask)
@@ -1477,7 +1477,7 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
 	}
 
 	ata_qc_for_each_raw(ap, qc, tag) {
-		if (!(qc->flags & ATA_QCFLAG_FAILED) ||
+		if (!(qc->flags & ATA_QCFLAG_EH) ||
 		    ata_dev_phys_link(qc->dev) != link)
 			continue;
 
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 153f49e00713..34beda28e712 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2073,7 +2073,7 @@ void ata_sff_error_handler(struct ata_port *ap)
 	unsigned long flags;
 
 	qc = __ata_qc_from_tag(ap, ap->link.active_tag);
-	if (qc && !(qc->flags & ATA_QCFLAG_FAILED))
+	if (qc && !(qc->flags & ATA_QCFLAG_EH))
 		qc = NULL;
 
 	spin_lock_irqsave(ap->lock, flags);
@@ -2796,7 +2796,7 @@ void ata_bmdma_error_handler(struct ata_port *ap)
 	bool thaw = false;
 
 	qc = __ata_qc_from_tag(ap, ap->link.active_tag);
-	if (qc && !(qc->flags & ATA_QCFLAG_FAILED))
+	if (qc && !(qc->flags & ATA_QCFLAG_EH))
 		qc = NULL;
 
 	/* reset PIO HSM and stop DMA engine */
diff --git a/drivers/ata/libata-trace.c b/drivers/ata/libata-trace.c
index e0e4d0d5a100..9b5363fd0ab0 100644
--- a/drivers/ata/libata-trace.c
+++ b/drivers/ata/libata-trace.c
@@ -142,7 +142,7 @@ libata_trace_parse_qc_flags(struct trace_seq *p, unsigned int qc_flags)
 			trace_seq_printf(p, "QUIET ");
 		if (qc_flags & ATA_QCFLAG_RETRY)
 			trace_seq_printf(p, "RETRY ");
-		if (qc_flags & ATA_QCFLAG_FAILED)
+		if (qc_flags & ATA_QCFLAG_EH)
 			trace_seq_printf(p, "FAILED ");
 		if (qc_flags & ATA_QCFLAG_SENSE_VALID)
 			trace_seq_printf(p, "SENSE_VALID ");
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index b9a4f68b371d..7eab9c4e1473 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1042,7 +1042,7 @@ static void sata_fsl_error_handler(struct ata_port *ap)
 
 static void sata_fsl_post_internal_cmd(struct ata_queued_cmd *qc)
 {
-	if (qc->flags & ATA_QCFLAG_FAILED)
+	if (qc->flags & ATA_QCFLAG_EH)
 		qc->err_mask |= AC_ERR_OTHER;
 
 	if (qc->err_mask) {
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 11e518f0111c..f480ff456190 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -672,7 +672,7 @@ static void inic_error_handler(struct ata_port *ap)
 static void inic_post_internal_cmd(struct ata_queued_cmd *qc)
 {
 	/* make DMA engine forget about the failed command */
-	if (qc->flags & ATA_QCFLAG_FAILED)
+	if (qc->flags & ATA_QCFLAG_EH)
 		inic_reset_port(inic_port_base(qc->ap));
 }
 
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 9cd7d8b71361..4e60e6c4c35a 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -828,7 +828,7 @@ static void pdc_post_internal_cmd(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 
 	/* make DMA engine forget about the failed command */
-	if (qc->flags & ATA_QCFLAG_FAILED)
+	if (qc->flags & ATA_QCFLAG_EH)
 		pdc_reset_port(ap);
 }
 
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 2fef6ce93f07..0a01518a8d97 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -1185,7 +1185,7 @@ static void sil24_post_internal_cmd(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 
 	/* make DMA engine forget about the failed command */
-	if ((qc->flags & ATA_QCFLAG_FAILED) && sil24_init_port(ap))
+	if ((qc->flags & ATA_QCFLAG_EH) && sil24_init_port(ap))
 		ata_eh_freeze_port(ap);
 }
 
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index ab70cbc78f96..a92c60455b1d 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -866,7 +866,7 @@ static void pdc_post_internal_cmd(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 
 	/* make DMA engine forget about the failed command */
-	if (qc->flags & ATA_QCFLAG_FAILED)
+	if (qc->flags & ATA_QCFLAG_EH)
 		pdc_reset_port(ap);
 }
 
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 2022ffb45041..c68ca2218a05 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -5370,9 +5370,9 @@ static int __ipr_eh_dev_reset(struct scsi_cmnd *scsi_cmd)
 					continue;
 
 				ipr_cmd->done = ipr_sata_eh_done;
-				if (!(ipr_cmd->qc->flags & ATA_QCFLAG_FAILED)) {
+				if (!(ipr_cmd->qc->flags & ATA_QCFLAG_EH)) {
 					ipr_cmd->qc->err_mask |= AC_ERR_TIMEOUT;
-					ipr_cmd->qc->flags |= ATA_QCFLAG_FAILED;
+					ipr_cmd->qc->flags |= ATA_QCFLAG_EH;
 				}
 			}
 		}
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 1ccce706167a..14da33a3b6a6 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -125,7 +125,7 @@ static void sas_ata_task_done(struct sas_task *task)
 		} else {
 			link->eh_info.err_mask |= ac_err_mask(dev->sata_dev.fis[2]);
 			if (unlikely(link->eh_info.err_mask))
-				qc->flags |= ATA_QCFLAG_FAILED;
+				qc->flags |= ATA_QCFLAG_EH;
 		}
 	} else {
 		ac = sas_to_ata_err(stat);
@@ -136,7 +136,7 @@ static void sas_ata_task_done(struct sas_task *task)
 				qc->err_mask = ac;
 			} else {
 				link->eh_info.err_mask |= AC_ERR_DEV;
-				qc->flags |= ATA_QCFLAG_FAILED;
+				qc->flags |= ATA_QCFLAG_EH;
 			}
 
 			dev->sata_dev.fis[2] = ATA_ERR | ATA_DRDY; /* tf status */
@@ -476,7 +476,7 @@ static void sas_ata_internal_abort(struct sas_task *task)
 
 static void sas_ata_post_internal(struct ata_queued_cmd *qc)
 {
-	if (qc->flags & ATA_QCFLAG_FAILED)
+	if (qc->flags & ATA_QCFLAG_EH)
 		qc->err_mask |= AC_ERR_OTHER;
 
 	if (qc->err_mask) {
@@ -631,7 +631,7 @@ void sas_ata_task_abort(struct sas_task *task)
 
 	/* Internal command, fake a timeout and complete. */
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
-	qc->flags |= ATA_QCFLAG_FAILED;
+	qc->flags |= ATA_QCFLAG_EH;
 	qc->err_mask |= AC_ERR_TIMEOUT;
 	waiting = qc->private_data;
 	complete(waiting);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c9149ebe7423..7985e6e2ae0e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -206,7 +206,7 @@ enum {
 	ATA_QCFLAG_QUIET	= (1 << 6), /* don't report device error */
 	ATA_QCFLAG_RETRY	= (1 << 7), /* retry after failure */
 
-	ATA_QCFLAG_FAILED	= (1 << 16), /* cmd failed and is owned by EH */
+	ATA_QCFLAG_EH		= (1 << 16), /* cmd aborted and owned by EH */
 	ATA_QCFLAG_SENSE_VALID	= (1 << 17), /* sense data valid */
 	ATA_QCFLAG_EH_SCHEDULED = (1 << 18), /* EH scheduled (obsolete) */
 
@@ -1756,7 +1756,7 @@ static inline struct ata_queued_cmd *ata_qc_from_tag(struct ata_port *ap,
 		return qc;
 
 	if ((qc->flags & (ATA_QCFLAG_ACTIVE |
-			  ATA_QCFLAG_FAILED)) == ATA_QCFLAG_ACTIVE)
+			  ATA_QCFLAG_EH)) == ATA_QCFLAG_ACTIVE)
 		return qc;
 
 	return NULL;
-- 
2.38.1

[PATCH v2 2/7] ata: libata: simplify qc_fill_rtf port operation interface

[Niklas Cassel]

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>

The boolean return value of the qc_fill_rtf operation is used nowhere.
Simplify this operation interface by making it a void function. All
drivers defining this operation are also updated.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
---
 drivers/ata/acard-ahci.c      |  6 ++----
 drivers/ata/libahci.c         |  6 ++----
 drivers/ata/libata-sff.c      |  6 +-----
 drivers/ata/sata_fsl.c        |  3 +--
 drivers/ata/sata_inic162x.c   | 12 +++++-------
 drivers/ata/sata_sil24.c      |  5 ++---
 drivers/scsi/ipr.c            |  7 +------
 drivers/scsi/libsas/sas_ata.c |  3 +--
 include/linux/libata.h        |  4 ++--
 9 files changed, 17 insertions(+), 35 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index da74a86b70ba..993eadd173da 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -57,7 +57,7 @@ struct acard_sg {
 };
 
 static enum ata_completion_errors acard_ahci_qc_prep(struct ata_queued_cmd *qc);
-static bool acard_ahci_qc_fill_rtf(struct ata_queued_cmd *qc);
+static void acard_ahci_qc_fill_rtf(struct ata_queued_cmd *qc);
 static int acard_ahci_port_start(struct ata_port *ap);
 static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
 
@@ -248,7 +248,7 @@ static enum ata_completion_errors acard_ahci_qc_prep(struct ata_queued_cmd *qc)
 	return AC_ERR_OK;
 }
 
-static bool acard_ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void acard_ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;
 	u8 *rx_fis = pp->rx_fis;
@@ -268,8 +268,6 @@ static bool acard_ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
 	} else
 		ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
-
-	return true;
 }
 
 static int acard_ahci_port_start(struct ata_port *ap)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 03aa9eb415d3..0167aac25c34 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -55,7 +55,7 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 
 static int ahci_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val);
 static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val);
-static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc);
+static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc);
 static int ahci_port_start(struct ata_port *ap);
 static void ahci_port_stop(struct ata_port *ap);
 static enum ata_completion_errors ahci_qc_prep(struct ata_queued_cmd *qc);
@@ -2053,7 +2053,7 @@ unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 }
 EXPORT_SYMBOL_GPL(ahci_qc_issue);
 
-static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ahci_port_priv *pp = qc->ap->private_data;
 	u8 *rx_fis = pp->rx_fis;
@@ -2087,8 +2087,6 @@ static bool ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 		qc->result_tf.error = fis[3];
 	} else
 		ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
-
-	return true;
 }
 
 static void ahci_freeze(struct ata_port *ap)
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 34beda28e712..cd82d3b5ed14 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1377,14 +1377,10 @@ EXPORT_SYMBOL_GPL(ata_sff_qc_issue);
  *
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
- *
- *	RETURNS:
- *	true indicating that result TF is successfully filled.
  */
-bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc)
+void ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	qc->ap->ops->sff_tf_read(qc->ap, &qc->result_tf);
-	return true;
 }
 EXPORT_SYMBOL_GPL(ata_sff_qc_fill_rtf);
 
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 7eab9c4e1473..b052c5a65c17 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -566,7 +566,7 @@ static unsigned int sata_fsl_qc_issue(struct ata_queued_cmd *qc)
 	return 0;
 }
 
-static bool sata_fsl_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void sata_fsl_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct sata_fsl_port_priv *pp = qc->ap->private_data;
 	struct sata_fsl_host_priv *host_priv = qc->ap->host->private_data;
@@ -577,7 +577,6 @@ static bool sata_fsl_qc_fill_rtf(struct ata_queued_cmd *qc)
 	cd = pp->cmdentry + tag;
 
 	ata_tf_from_fis(cd->sfis, &qc->result_tf);
-	return true;
 }
 
 static int sata_fsl_scr_write(struct ata_link *link,
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index f480ff456190..2833c722118d 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -566,7 +566,7 @@ static void inic_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
 	tf->status	= readb(port_base + PORT_TF_COMMAND);
 }
 
-static bool inic_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void inic_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ata_taskfile *rtf = &qc->result_tf;
 	struct ata_taskfile tf;
@@ -580,12 +580,10 @@ static bool inic_qc_fill_rtf(struct ata_queued_cmd *qc)
 	 */
 	inic_tf_read(qc->ap, &tf);
 
-	if (!(tf.status & ATA_ERR))
-		return false;
-
-	rtf->status = tf.status;
-	rtf->error = tf.error;
-	return true;
+	if (tf.status & ATA_ERR) {
+		rtf->status = tf.status;
+		rtf->error = tf.error;
+	}
 }
 
 static void inic_freeze(struct ata_port *ap)
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 0a01518a8d97..22cc9e9789dd 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -328,7 +328,7 @@ static int sil24_scr_write(struct ata_link *link, unsigned sc_reg, u32 val);
 static int sil24_qc_defer(struct ata_queued_cmd *qc);
 static enum ata_completion_errors sil24_qc_prep(struct ata_queued_cmd *qc);
 static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc);
-static bool sil24_qc_fill_rtf(struct ata_queued_cmd *qc);
+static void sil24_qc_fill_rtf(struct ata_queued_cmd *qc);
 static void sil24_pmp_attach(struct ata_port *ap);
 static void sil24_pmp_detach(struct ata_port *ap);
 static void sil24_freeze(struct ata_port *ap);
@@ -901,10 +901,9 @@ static unsigned int sil24_qc_issue(struct ata_queued_cmd *qc)
 	return 0;
 }
 
-static bool sil24_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void sil24_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	sil24_read_tf(qc->ap, qc->hw_tag, &qc->result_tf);
-	return true;
 }
 
 static void sil24_pmp_attach(struct ata_port *ap)
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index c68ca2218a05..1c8040d250ea 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -7142,11 +7142,8 @@ static unsigned int ipr_qc_issue(struct ata_queued_cmd *qc)
 /**
  * ipr_qc_fill_rtf - Read result TF
  * @qc: ATA queued command
- *
- * Return value:
- * 	true
  **/
-static bool ipr_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void ipr_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct ipr_sata_port *sata_port = qc->ap->private_data;
 	struct ipr_ioasa_gata *g = &sata_port->ioasa;
@@ -7163,8 +7160,6 @@ static bool ipr_qc_fill_rtf(struct ata_queued_cmd *qc)
 	tf->hob_lbal = g->hob_lbal;
 	tf->hob_lbam = g->hob_lbam;
 	tf->hob_lbah = g->hob_lbah;
-
-	return true;
 }
 
 static struct ata_port_operations ipr_sata_ops = {
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 14da33a3b6a6..ac8576e7f0b7 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -226,12 +226,11 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 	return ret;
 }
 
-static bool sas_ata_qc_fill_rtf(struct ata_queued_cmd *qc)
+static void sas_ata_qc_fill_rtf(struct ata_queued_cmd *qc)
 {
 	struct domain_device *dev = qc->ap->private_data;
 
 	ata_tf_from_fis(dev->sata_dev.fis, &qc->result_tf);
-	return true;
 }
 
 static struct sas_internal *dev_to_sas_internal(struct domain_device *dev)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7985e6e2ae0e..8483d8300ea3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -876,7 +876,7 @@ struct ata_port_operations {
 	int (*check_atapi_dma)(struct ata_queued_cmd *qc);
 	enum ata_completion_errors (*qc_prep)(struct ata_queued_cmd *qc);
 	unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
-	bool (*qc_fill_rtf)(struct ata_queued_cmd *qc);
+	void (*qc_fill_rtf)(struct ata_queued_cmd *qc);
 
 	/*
 	 * Configuration and exception handling
@@ -1936,7 +1936,7 @@ extern void ata_sff_queue_delayed_work(struct delayed_work *dwork,
 		unsigned long delay);
 extern void ata_sff_queue_pio_task(struct ata_link *link, unsigned long delay);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
-extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
+extern void ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_port_intr(struct ata_port *ap,
 				      struct ata_queued_cmd *qc);
 extern irqreturn_t ata_sff_interrupt(int irq, void *dev_instance);
-- 
2.38.1

[PATCH v2 3/7] ata: libata: read the shared status for successful NCQ commands once

[Niklas Cassel]

Currently, the status is being read for each QC, inside
ata_qc_complete(), which means that QCs being completed by
ata_qc_complete_multiple() (i.e. multiple QCs completed during a single
interrupt), can have different status and error bits set. This is
because the FIS Receive Area will get updated as soon as the HBA
receives a new FIS from the device in the NCQ case.

Here is an example of the problem:
ata14.00: ata_qc_complete_multiple: done_mask: 0x180000
qc tag: 19 cmd: 0x61 flags: 0x11b err_mask: 0x0 tf->status: 0x40
qc tag: 20 cmd: 0x61 flags: 0x11b err_mask: 0x0 tf->status: 0x43

A print in ata_qc_complete_multiple(), shows that done_mask is: 0x180000
which means that tag 19 and 20 were completed. Another print in
ata_qc_complete(), after the call to fill_result_tf(), shows that tag 19
and 20 have different status values, even though they were completed in
the same ata_qc_complete_multiple() call.

If PMP is not enabled, simply read the status and error once, before
calling ata_qc_complete() for each QC. Without PMP, we know that all QCs
must share the same status and error values.

If PMP is enabled, we also read the status before calling
ata_qc_complete(), however, we still read the status for each QC, since
the QCs can belong to different PMP links (which means that the QCs
does not necessarily share the same status and error values).

Do all this by introducing the new port operation .qc_ncq_fill_rtf. If
set, this operation is called in ata_qc_complete_multiple() to set the
result tf for all completed QCs signaled by the last SDB FIS received.

QCs that have their result tf filled are marked with the new flag
ATA_QCFLAG_RTF_FILLED so that any later execution of the qc_fill_rtf
port operation does nothing (e.g. when called from ata_qc_complete()).

Co-developed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libahci.c     | 90 +++++++++++++++++++++++++++++++++++++--
 drivers/ata/libata-sata.c |  3 ++
 include/linux/libata.h    |  2 +
 3 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 0167aac25c34..e5d67eb46f3c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -56,6 +56,7 @@ static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 static int ahci_scr_read(struct ata_link *link, unsigned int sc_reg, u32 *val);
 static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val);
 static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc);
+static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask);
 static int ahci_port_start(struct ata_port *ap);
 static void ahci_port_stop(struct ata_port *ap);
 static enum ata_completion_errors ahci_qc_prep(struct ata_queued_cmd *qc);
@@ -157,6 +158,7 @@ struct ata_port_operations ahci_ops = {
 	.qc_prep		= ahci_qc_prep,
 	.qc_issue		= ahci_qc_issue,
 	.qc_fill_rtf		= ahci_qc_fill_rtf,
+	.qc_ncq_fill_rtf	= ahci_qc_ncq_fill_rtf,
 
 	.freeze			= ahci_freeze,
 	.thaw			= ahci_thaw,
@@ -2058,6 +2060,13 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	struct ahci_port_priv *pp = qc->ap->private_data;
 	u8 *rx_fis = pp->rx_fis;
 
+	/*
+	 * rtf may already be filled (e.g. for successful NCQ commands).
+	 * If that is the case, we have nothing to do.
+	 */
+	if (qc->flags & ATA_QCFLAG_RTF_FILLED)
+		return;
+
 	if (pp->fbs_enabled)
 		rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
 
@@ -2071,6 +2080,9 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	    !(qc->flags & ATA_QCFLAG_EH)) {
 		ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf);
 		qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15];
+		qc->flags |= ATA_QCFLAG_RTF_FILLED;
+		return;
+	}
 
 	/*
 	 * For NCQ commands, we never get a D2H FIS, so reading the D2H Register
@@ -2080,13 +2092,85 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc)
 	 * instead. However, the SDB FIS does not contain the LBA, so we can't
 	 * use the ata_tf_from_fis() helper.
 	 */
-	} else if (ata_is_ncq(qc->tf.protocol)) {
+	if (ata_is_ncq(qc->tf.protocol)) {
 		const u8 *fis = rx_fis + RX_FIS_SDB;
 
+		/*
+		 * Successful NCQ commands have been filled already.
+		 * A failed NCQ command will read the status here.
+		 * (Note that a failed NCQ command will get a more specific
+		 * error when reading the NCQ Command Error log.)
+		 */
 		qc->result_tf.status = fis[2];
 		qc->result_tf.error = fis[3];
-	} else
-		ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
+		qc->flags |= ATA_QCFLAG_RTF_FILLED;
+		return;
+	}
+
+	ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf);
+	qc->flags |= ATA_QCFLAG_RTF_FILLED;
+}
+
+static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask)
+{
+	struct ahci_port_priv *pp = ap->private_data;
+	const u8 *fis;
+
+	/* No outstanding commands. */
+	if (!ap->qc_active)
+		return;
+
+	/*
+	 * FBS not enabled, so read status and error once, since they are shared
+	 * for all QCs.
+	 */
+	if (!pp->fbs_enabled) {
+		u8 status, error;
+
+		/* No outstanding NCQ commands. */
+		if (!pp->active_link->sactive)
+			return;
+
+		fis = pp->rx_fis + RX_FIS_SDB;
+		status = fis[2];
+		error = fis[3];
+
+		while (done_mask) {
+			struct ata_queued_cmd *qc;
+			unsigned int tag = __ffs64(done_mask);
+
+			qc = ata_qc_from_tag(ap, tag);
+			if (qc && ata_is_ncq(qc->tf.protocol)) {
+				qc->result_tf.status = status;
+				qc->result_tf.error = error;
+				qc->flags |= ATA_QCFLAG_RTF_FILLED;
+			}
+			done_mask &= ~(1ULL << tag);
+		}
+
+		return;
+	}
+
+	/*
+	 * FBS enabled, so read the status and error for each QC, since the QCs
+	 * can belong to different PMP links. (Each PMP link has its own FIS
+	 * Receive Area.)
+	 */
+	while (done_mask) {
+		struct ata_queued_cmd *qc;
+		unsigned int tag = __ffs64(done_mask);
+
+		qc = ata_qc_from_tag(ap, tag);
+		if (qc && ata_is_ncq(qc->tf.protocol)) {
+			fis = pp->rx_fis;
+			fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ;
+			fis += RX_FIS_SDB;
+			qc->result_tf.status = fis[2];
+			qc->result_tf.error = fis[3];
+			qc->flags |= ATA_QCFLAG_RTF_FILLED;
+		}
+		done_mask &= ~(1ULL << tag);
+	}
 }
 
 static void ahci_freeze(struct ata_port *ap)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 908f35acee1e..f3e7396e3191 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -655,6 +655,9 @@ int ata_qc_complete_multiple(struct ata_port *ap, u64 qc_active)
 		return -EINVAL;
 	}
 
+	if (ap->ops->qc_ncq_fill_rtf)
+		ap->ops->qc_ncq_fill_rtf(ap, done_mask);
+
 	while (done_mask) {
 		struct ata_queued_cmd *qc;
 		unsigned int tag = __ffs64(done_mask);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8483d8300ea3..f54e02dadc6f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -200,6 +200,7 @@ enum {
 	/* struct ata_queued_cmd flags */
 	ATA_QCFLAG_ACTIVE	= (1 << 0), /* cmd not yet ack'd to scsi lyer */
 	ATA_QCFLAG_DMAMAP	= (1 << 1), /* SG table is DMA mapped */
+	ATA_QCFLAG_RTF_FILLED	= (1 << 2), /* result TF has been filled */
 	ATA_QCFLAG_IO		= (1 << 3), /* standard IO command */
 	ATA_QCFLAG_RESULT_TF	= (1 << 4), /* result TF requested */
 	ATA_QCFLAG_CLEAR_EXCL	= (1 << 5), /* clear excl_link on completion */
@@ -877,6 +878,7 @@ struct ata_port_operations {
 	enum ata_completion_errors (*qc_prep)(struct ata_queued_cmd *qc);
 	unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
 	void (*qc_fill_rtf)(struct ata_queued_cmd *qc);
+	void (*qc_ncq_fill_rtf)(struct ata_port *ap, u64 done_mask);
 
 	/*
 	 * Configuration and exception handling
-- 
2.38.1

[PATCH v2 4/7] ata: libata: respect successfully completed commands during errors

[Niklas Cassel]

In AHCI specification 1.3.1:
"5.5.3 Processing Completed Commands"

"For each port that has an interrupt pending:

1. Software determines the cause of the interrupt by reading the PxIS
   register. It is possible for multiple bits to be set.
2. Software clears appropriate bits in the PxIS register corresponding
   to the cause of the interrupt.
3. Software clears the interrupt bit in IS.IPS corresponding to the port.
4. If executing non-queued commands, software reads the PxCI register,
   and compares the current value to the list of commands previously
   issued by software that are still outstanding. If executing native
   queued commands, software reads the PxSACT register and compares the
   current value to the list of commands previously issued by software.
   Software completes with success any outstanding command whose
   corresponding bit has been cleared in the respective register. PxCI
   and PxSACT are volatile registers; software should only use their
   values to determine commands that have completed, not to determine
   which commands have previously been issued.
5. If there were errors, noted in the PxIS register, software performs
   error recovery actions (see section 6.2.2)."

The documentation for the PxSACT shadow register in AHCI:
"The device clears bits in this field by sending a Set Device Bits FIS
to the host. The HBA clears bits in this field that are set to ‘1’ in
the SActive field of the Set Device Bits FIS. The HBA only clears bits
that correspond to native queued commands that have completed
successfully."

Additionally, in SATA specification 3.5a:
"11.15 FPDMA QUEUED command protocol"

"DFPDMAQ11: ERROR
Halt command processing and transmit Set Device Bits FIS to host
with the ERR bit in Status field set to one, Interrupt bit set to one,
ATA error code set to one in the ERROR field, bits in ACT field cleared
to zero for any outstanding queued commands, and bits set to one
for any successfully completed queued commands that completion
notification not yet delivered."

I.e. even when the HBA triggers an error interrupt, the HBA will still
clear successfully completed commands in PxSACT. Commands that did not
complete successfully will still have its bit set in PxSACT.
(Which means the command that caused the NCQ error and queued commands
that had not yet finished at the time when the NCQ error occurred.)

Additionally, for a HBA that does not have the libata flag
AHCI_HFLAG_MULTI_MSI set, all ap->locks will point to host->lock, which
means that IRQs will be disabled for one port while another port's IRQ
handler is running. The HBA will still receive FISes from the device,
even if IRQs on the HBA itself are disabled. What can thus e.g. receive
a FIS that completes several commands successfully, followed by a FIS
that does (or does not) complete additional commands with the error bit
set, to indicate that at least one command was aborted.

Therefore, modify ahci_handle_port_interrupt() using the new helper
ahci_qc_complete() to complete the commands that have already been
signaled as successfully through a regular completion SDB FIS, as not
doing so would simply cause successfully completed commands to be
retried for no good reason.

Co-developed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libahci.c | 73 +++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 31 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index e5d67eb46f3c..8f216de76648 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1849,18 +1849,47 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 		ata_port_abort(ap);
 }
 
-static void ahci_handle_port_interrupt(struct ata_port *ap,
-				       void __iomem *port_mmio, u32 status)
+static void ahci_qc_complete(struct ata_port *ap, void __iomem *port_mmio)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	struct ahci_port_priv *pp = ap->private_data;
-	struct ahci_host_priv *hpriv = ap->host->private_data;
-	int resetting = !!(ap->pflags & ATA_PFLAG_RESETTING);
 	u32 qc_active = 0;
 	int rc;
 
+	/*
+	 * pp->active_link is not reliable once FBS is enabled, both
+	 * PORT_SCR_ACT and PORT_CMD_ISSUE should be checked because
+	 * NCQ and non-NCQ commands may be in flight at the same time.
+	 */
+	if (pp->fbs_enabled) {
+		if (ap->qc_active) {
+			qc_active = readl(port_mmio + PORT_SCR_ACT);
+			qc_active |= readl(port_mmio + PORT_CMD_ISSUE);
+		}
+	} else {
+		/* pp->active_link is valid iff any command is in flight */
+		if (ap->qc_active && pp->active_link->sactive)
+			qc_active = readl(port_mmio + PORT_SCR_ACT);
+		else
+			qc_active = readl(port_mmio + PORT_CMD_ISSUE);
+	}
+
+	rc = ata_qc_complete_multiple(ap, qc_active);
+	if (unlikely(rc < 0 && !(ap->pflags & ATA_PFLAG_RESETTING))) {
+		ehi->err_mask |= AC_ERR_HSM;
+		ehi->action |= ATA_EH_RESET;
+		ata_port_freeze(ap);
+	}
+}
+
+static void ahci_handle_port_interrupt(struct ata_port *ap,
+				       void __iomem *port_mmio, u32 status)
+{
+	struct ahci_port_priv *pp = ap->private_data;
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+
 	/* ignore BAD_PMP while resetting */
-	if (unlikely(resetting))
+	if (unlikely(ap->pflags & ATA_PFLAG_RESETTING))
 		status &= ~PORT_IRQ_BAD_PMP;
 
 	if (sata_lpm_ignore_phy_events(&ap->link)) {
@@ -1869,6 +1898,12 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
 	}
 
 	if (unlikely(status & PORT_IRQ_ERROR)) {
+		/*
+		 * Before getting the error notification, we may have
+		 * received SDB FISes notifying successful completions.
+		 * Handle these first and then handle the error.
+		 */
+		ahci_qc_complete(ap, port_mmio);
 		ahci_error_intr(ap, status);
 		return;
 	}
@@ -1905,32 +1940,8 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
 		}
 	}
 
-	/* pp->active_link is not reliable once FBS is enabled, both
-	 * PORT_SCR_ACT and PORT_CMD_ISSUE should be checked because
-	 * NCQ and non-NCQ commands may be in flight at the same time.
-	 */
-	if (pp->fbs_enabled) {
-		if (ap->qc_active) {
-			qc_active = readl(port_mmio + PORT_SCR_ACT);
-			qc_active |= readl(port_mmio + PORT_CMD_ISSUE);
-		}
-	} else {
-		/* pp->active_link is valid iff any command is in flight */
-		if (ap->qc_active && pp->active_link->sactive)
-			qc_active = readl(port_mmio + PORT_SCR_ACT);
-		else
-			qc_active = readl(port_mmio + PORT_CMD_ISSUE);
-	}
-
-
-	rc = ata_qc_complete_multiple(ap, qc_active);
-
-	/* while resetting, invalid completions are expected */
-	if (unlikely(rc < 0 && !resetting)) {
-		ehi->err_mask |= AC_ERR_HSM;
-		ehi->action |= ATA_EH_RESET;
-		ata_port_freeze(ap);
-	}
+	/* Handle completed commands */
+	ahci_qc_complete(ap, port_mmio);
 }
 
 static void ahci_port_intr(struct ata_port *ap)
-- 
2.38.1

[PATCH v2 5/7] ata: libata: move NCQ related ATA_DFLAGs

[Niklas Cassel]

ata_dev_configure() starts off by clearing all flags in ATA_DFLAG_CFG_MASK:
dev->flags &= ~ATA_DFLAG_CFG_MASK;

ata_dev_configure() then calls ata_dev_config_lba() which calls
ata_dev_config_ncq().

ata_dev_config_ncq() will set the correct ATA_DFLAGs depending on what is
actually supported.

Since these flags are set by ata_dev_configure(), they should be in
ATA_DFLAG_CFG_MASK and not in ATA_DFLAG_INIT_MASK.

ATA_DFLAG_NCQ_PRIO_ENABLED is set via sysfs, is should therefore not be in
ATA_DFLAG_CFG_MASK. It also cannot be in ATA_DFLAG_INIT_MASK, because
ata_eh_schedule_probe() calls ata_dev_init(), which will clear all flags in
ATA_DFLAG_INIT_MASK.

This means that ATA_DFLAG_NCQ_PRIO_ENABLED (the value the user sets via
sysfs) would get silently cleared if ata_eh_schedule_probe() is called.
While that should only happen in certain circumstances, it still doesn't
seem right that it can get silently cleared.

(ata_dev_config_ncq_prio() will still clear the ATA_DFLAG_NCQ_PRIO_ENABLED
flag if ATA_DFLAG_NCQ_PRIO is suddenly no longer supported after a
revalidation.)

Because of this, move ATA_DFLAG_NCQ_PRIO_ENABLED to be outside of both
ATA_DFLAG_CFG_MASK and ATA_DFLAG_INIT_MASK.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 include/linux/libata.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index f54e02dadc6f..3b7f5d9e2f87 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -91,22 +91,21 @@ enum {
 	ATA_DFLAG_AN		= (1 << 7), /* AN configured */
 	ATA_DFLAG_TRUSTED	= (1 << 8), /* device supports trusted send/recv */
 	ATA_DFLAG_DMADIR	= (1 << 10), /* device requires DMADIR */
-	ATA_DFLAG_CFG_MASK	= (1 << 12) - 1,
+	ATA_DFLAG_NCQ_SEND_RECV = (1 << 11), /* device supports NCQ SEND and RECV */
+	ATA_DFLAG_NCQ_PRIO	= (1 << 12), /* device supports NCQ priority */
+	ATA_DFLAG_CFG_MASK	= (1 << 13) - 1,
 
-	ATA_DFLAG_PIO		= (1 << 12), /* device limited to PIO mode */
-	ATA_DFLAG_NCQ_OFF	= (1 << 13), /* device limited to non-NCQ mode */
+	ATA_DFLAG_PIO		= (1 << 13), /* device limited to PIO mode */
+	ATA_DFLAG_NCQ_OFF	= (1 << 14), /* device limited to non-NCQ mode */
 	ATA_DFLAG_SLEEPING	= (1 << 15), /* device is sleeping */
 	ATA_DFLAG_DUBIOUS_XFER	= (1 << 16), /* data transfer not verified */
 	ATA_DFLAG_NO_UNLOAD	= (1 << 17), /* device doesn't support unload */
 	ATA_DFLAG_UNLOCK_HPA	= (1 << 18), /* unlock HPA */
-	ATA_DFLAG_NCQ_SEND_RECV = (1 << 19), /* device supports NCQ SEND and RECV */
-	ATA_DFLAG_NCQ_PRIO	= (1 << 20), /* device supports NCQ priority */
-	ATA_DFLAG_NCQ_PRIO_ENABLED = (1 << 21), /* Priority cmds sent to dev */
-	ATA_DFLAG_INIT_MASK	= (1 << 24) - 1,
+	ATA_DFLAG_INIT_MASK	= (1 << 19) - 1,
 
+	ATA_DFLAG_NCQ_PRIO_ENABLED = (1 << 19), /* Priority cmds sent to dev */
 	ATA_DFLAG_DETACH	= (1 << 24),
 	ATA_DFLAG_DETACHED	= (1 << 25),
-
 	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 */
-- 
2.38.1

[PATCH v2 6/7] ata: libata-scsi: do not overwrite SCSI ML and status bytes

[Niklas Cassel]

For SCSI ML byte:
In the case where a command is completed via libata EH:
irq -> ata_qc_complete() -> ata_qc_schedule_eh()
irq done
... -> ata_do_eh() -> ata_eh_link_autopsy() -> ata_eh_finish() ->
ata_eh_qc_complete() -> __ata_eh_qc_complete() -> __ata_qc_complete() ->
qc->complete_fn() (ata_scsi_qc_complete()) -> ata_qc_done() ->
qc->scsidone() (empty stub)
... -> scsi_eh_finish_cmd() -> scsi_eh_flush_done_q() ->
scsi_finish_command()

ata_eh_link_autopsy() will call ata_eh_analyze_tf(), which calls
scsi_check_sense(), which sets the SCSI ML byte.

Since ata_scsi_qc_complete() is called after scsi_check_sense() when
a command is completed via libata EH, we cannot simply overwrite the
SCSI ML byte that was set earlier in the call chain.

For SCSI status byte:
When a SCSI command is prepared using scsi_prepare_cmd(), it sets
cmd->result to 0. (SAM_STAT_GOOD is defined as 0x0).
Likewise, when a command is requeued from SCSI EH, scsi_queue_insert()
is called, which sets cmd->result to 0.

A SCSI command thus always has a GOOD status by default when being
sent to libata.

If libata fetches sense data from the device, it will call
ata_scsi_set_sense(), which will set the status byte to
SAM_STAT_CHECK_CONDITION, if the caller deems that the status should be
a check condition.

ata_scsi_qc_complete() should therefore never overwrite the existing
status byte, because if it is != GOOD, it was set by libata itself,
for a reason.

For the host byte:
When libata abort commands, because of a NCQ error, it will schedule
SCSI EH for all QCs using blk_abort_request(), which will all end up in
scsi_timeout(), which will call scsi_abort_command(). scsi_timeout()
sets DID_TIME_OUT regardless if a command was aborted or timed out.
If we don't clear the DID_TIME_OUT byte for the QC that caused the
NCQ error, that QC will be reported as a timed out command, instead
of being reported as a NCQ error.

For a command that actually timed out, DID_TIME_OUT would be fine to
keep, but libata has its own way of detecting that a command timed out
(see ata_scsi_cmd_error_handler()), and sets AC_ERR_TIMEOUT if that is
the case. libata will retry timed out commands.

We could clear DID_TIME_OUT only for the QC that caused the NCQ error,
but since libata has its own way of detecting timeouts, simply clear it
always.

Note that the existing ata_scsi_qc_complete() code does:
cmd->result = SAM_STAT_CHECK_CONDITION or cmd->result = SAM_STAT_GOOD.
This WILL clear the host byte. So us clearing the host byte
unconditionally is in line with the existing libata behavior.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index cbb3a7a50816..e1c43f9f6bb2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1654,7 +1654,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
-	int need_sense = (qc->err_mask != 0);
+	int need_sense = (qc->err_mask != 0) &&
+		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
 	 * user mandated it or if there's an error.  Note that if we
@@ -1668,12 +1669,11 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
 	    ((cdb[2] & 0x20) || need_sense))
 		ata_gen_passthru_sense(qc);
-	else if (qc->flags & ATA_QCFLAG_SENSE_VALID)
-		cmd->result = SAM_STAT_CHECK_CONDITION;
 	else if (need_sense)
 		ata_gen_ata_sense(qc);
 	else
-		cmd->result = SAM_STAT_GOOD;
+		/* Keep the SCSI ML and status byte, clear host byte. */
+		cmd->result &= 0x0000ffff;
 
 	if (need_sense && !ap->ops->error_handler)
 		ata_dump_status(ap, &qc->result_tf);
-- 
2.38.1

[PATCH v2 7/7] ata: libata-scsi: improve ata_scsiop_maint_in()

[Niklas Cassel]

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Allow translation of REPORT_SUPPORTED_OPERATION_CODES commands using
the command format 0x3, that is, checking support for commands that are
identified using an opcode and a service action.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-scsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e1c43f9f6bb2..46109bde8a04 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3266,11 +3266,12 @@ static unsigned int ata_scsiop_maint_in(struct ata_scsi_args *args, u8 *rbuf)
 	u8 supported = 0;
 	unsigned int err = 0;
 
-	if (cdb[2] != 1) {
+	if (cdb[2] != 1 && cdb[2] != 3) {
 		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
 		err = 2;
 		goto out;
 	}
+
 	switch (cdb[3]) {
 	case INQUIRY:
 	case MODE_SENSE:
-- 
2.38.1

Re: [PATCH v2 0/7] misc libata improvements

[Damien Le Moal]

On 12/30/22 01:59, Niklas Cassel wrote:
> Hello there,
> 
> This series contains misc libata improvements.
> 
> These improvements were identified while developing support for Command
> Duration Limits (CDL). All patches in this series (i.e. V1 of these
> patches) were orignally sent out as part of the CDL series, found here:
> https://lore.kernel.org/linux-scsi/510732e0-7962-cf54-c22c-f1d7066895f5@opensource.wdc.com/T/
> 
> However, as these improvements are completely unrelated to CDL, they can
> be merged independently, and should not need to wait for other patches.

Applied the series to for-6.3. Patch 1 had a small conflict that I fixed
up. Thanks !

> 
> 
> Kind regards,
> Niklas
> 
> 
> Changes since V1:
> -Added missing chain sign-off (in addition to author sign-off).
> -Picked up tags from John.
> -Rephrased commit message for patch 1/7 as suggested by John.
> -Rephrased commit subject for patch 3/7 to more clearly hightlight
>  that this is simply an improvement, and not strictly a bug fix.
> 
> Damien Le Moal (2):
>   ata: libata: simplify qc_fill_rtf port operation interface
>   ata: libata-scsi: improve ata_scsiop_maint_in()
> 
> Niklas Cassel (5):
>   ata: scsi: rename flag ATA_QCFLAG_FAILED to ATA_QCFLAG_EH
>   ata: libata: read the shared status for successful NCQ commands once
>   ata: libata: respect successfully completed commands during errors
>   ata: libata: move NCQ related ATA_DFLAGs
>   ata: libata-scsi: do not overwrite SCSI ML and status bytes
> 
>  drivers/ata/acard-ahci.c      |   8 +-
>  drivers/ata/libahci.c         | 171 ++++++++++++++++++++++++++--------
>  drivers/ata/libata-core.c     |  12 +--
>  drivers/ata/libata-eh.c       |  22 ++---
>  drivers/ata/libata-sata.c     |   7 +-
>  drivers/ata/libata-scsi.c     |  11 ++-
>  drivers/ata/libata-sff.c      |  10 +-
>  drivers/ata/libata-trace.c    |   2 +-
>  drivers/ata/sata_fsl.c        |   5 +-
>  drivers/ata/sata_inic162x.c   |  14 ++-
>  drivers/ata/sata_promise.c    |   2 +-
>  drivers/ata/sata_sil24.c      |   7 +-
>  drivers/ata/sata_sx4.c        |   2 +-
>  drivers/scsi/ipr.c            |  11 +--
>  drivers/scsi/libsas/sas_ata.c |  11 +--
>  include/linux/libata.h        |  25 ++---
>  16 files changed, 201 insertions(+), 119 deletions(-)
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/7] misc libata improvements

[Niklas Cassel]

On Wed, Jan 04, 2023 at 01:43:54PM +0900, Damien Le Moal wrote:
> On 12/30/22 01:59, Niklas Cassel wrote:
> > Hello there,
> > 
> > This series contains misc libata improvements.
> > 
> > These improvements were identified while developing support for Command
> > Duration Limits (CDL). All patches in this series (i.e. V1 of these
> > patches) were orignally sent out as part of the CDL series, found here:
> > https://lore.kernel.org/linux-scsi/510732e0-7962-cf54-c22c-f1d7066895f5@opensource.wdc.com/T/
> > 
> > However, as these improvements are completely unrelated to CDL, they can
> > be merged independently, and should not need to wait for other patches.
> 
> Applied the series to for-6.3. Patch 1 had a small conflict that I fixed
> up. Thanks !

I had a look at the SHA1 for this patch in your tree, and it looks good.

However, patches 2/7, 3/7, 4/7, 7/7 seem to miss your chain sign-off.


Kind regards,
Niklas

Re: [PATCH v2 0/7] misc libata improvements

[Niklas Cassel]

On Wed, Jan 04, 2023 at 11:12:29AM +0100, Niklas Cassel wrote:
> On Wed, Jan 04, 2023 at 01:43:54PM +0900, Damien Le Moal wrote:
> > On 12/30/22 01:59, Niklas Cassel wrote:
> > > Hello there,
> > > 
> > > This series contains misc libata improvements.
> > > 
> > > These improvements were identified while developing support for Command
> > > Duration Limits (CDL). All patches in this series (i.e. V1 of these
> > > patches) were orignally sent out as part of the CDL series, found here:
> > > https://lore.kernel.org/linux-scsi/510732e0-7962-cf54-c22c-f1d7066895f5@opensource.wdc.com/T/
> > > 
> > > However, as these improvements are completely unrelated to CDL, they can
> > > be merged independently, and should not need to wait for other patches.
> > 
> > Applied the series to for-6.3. Patch 1 had a small conflict that I fixed
> > up. Thanks !
> 
> I had a look at the SHA1 for this patch in your tree, and it looks good.
> 
> However, patches 2/7, 3/7, 4/7, 7/7 seem to miss your chain sign-off.

Is this perhaps because checkpatch complains if the same sign-off exists
twice on the same patch?

Not sure if this should be ignored or not...
To me, it seems more important to keep a record of the chain,
than to keep checkpatch happy, but I could be wrong here..


Kind regards,
Niklas

Re: [PATCH v2 0/7] misc libata improvements

[Damien Le Moal]

On 1/4/23 19:16, Niklas Cassel wrote:
> On Wed, Jan 04, 2023 at 11:12:29AM +0100, Niklas Cassel wrote:
>> On Wed, Jan 04, 2023 at 01:43:54PM +0900, Damien Le Moal wrote:
>>> On 12/30/22 01:59, Niklas Cassel wrote:
>>>> Hello there,
>>>>
>>>> This series contains misc libata improvements.
>>>>
>>>> These improvements were identified while developing support for Command
>>>> Duration Limits (CDL). All patches in this series (i.e. V1 of these
>>>> patches) were orignally sent out as part of the CDL series, found here:
>>>> https://lore.kernel.org/linux-scsi/510732e0-7962-cf54-c22c-f1d7066895f5@opensource.wdc.com/T/
>>>>
>>>> However, as these improvements are completely unrelated to CDL, they can
>>>> be merged independently, and should not need to wait for other patches.
>>>
>>> Applied the series to for-6.3. Patch 1 had a small conflict that I fixed
>>> up. Thanks !
>>
>> I had a look at the SHA1 for this patch in your tree, and it looks good.
>>
>> However, patches 2/7, 3/7, 4/7, 7/7 seem to miss your chain sign-off.
> 
> Is this perhaps because checkpatch complains if the same sign-off exists
> twice on the same patch?
> 
> Not sure if this should be ignored or not...
> To me, it seems more important to keep a record of the chain,
> than to keep checkpatch happy, but I could be wrong here..

I do not sign again patches that already have my SoB. linux-next build bot
will complain if I forget signing patches I apply, but it does not seem
that the order matters... Not entirely certain about the correct practice
with that though.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research