[PATCH v2 0/8] libata: remove references to 'old' error handler

[PATCH v2 0/8] libata: remove references to 'old' error handler

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Hi all,

now that the ipr driver has been modified to not hook into libata
all drivers now use the 'new' error handler, so we can remove any
references to it. And do a general cleanup to remove callbacks
which are no longer needed.


Changes since v1:
-Rebased against Damien's libata for-next branch. (Fixed conflicts in
 ata_qc_complete().)
-Fixed comments from Damien.
-Fixed comments from myself.
-Do not dump all QCs unconditionally (which should make the performance
 regression reported by the test robot go away.)
-Since ata_dump_status() was only called for drivers without .error_handler
 callback, this function is now unused and unreachable, so it was removed.
-Since atapi_qc_complete() can no longer reach the "old EH" failure paths
 (code will now always take the top branch on error), they are removed.
-Since atapi_eh_request_sense() can no longer be called, it is now removed.
 (The atapi_eh_request_sense() version called by EH is still there.)
-Patch 7/8 and 8/8 are new in V2.


Hannes Reinecke (6):
  ata: remove reference to non-existing error_handler()
  ata,scsi: remove ata_sas_port_{start,stop} callbacks
  ata,scsi: remove ata_sas_port_destroy()
  ata: remove ata_sas_sync_probe()
  ata: inline ata_port_probe()
  ata,scsi: cleanup ata_port_probe()

Niklas Cassel (2):
  ata: sata_sx4: drop already completed TODO
  ata: remove ata_bus_probe()

 drivers/ata/libata-core.c          | 355 +++++++----------------------
 drivers/ata/libata-eh.c            | 150 +++++-------
 drivers/ata/libata-sata.c          |  77 -------
 drivers/ata/libata-scsi.c          | 142 +-----------
 drivers/ata/libata-sff.c           |  30 +--
 drivers/ata/libata.h               |   3 -
 drivers/ata/sata_sx4.c             |   1 -
 drivers/scsi/libsas/sas_ata.c      |   6 +-
 drivers/scsi/libsas/sas_discover.c |   2 +-
 include/linux/libata.h             |   6 +-
 10 files changed, 170 insertions(+), 602 deletions(-)

-- 
2.41.0

[PATCH v2 1/8] ata: remove reference to non-existing error_handler()

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all
libata drivers now have the error_handler() callback provided,
so we can stop checking for non-existing error_handler callback.

Signed-off-by: Hannes Reinecke <hare@suse.de>
[niklas: fixed review comments, rebased, solved conflicts during rebase,
fixed bug that unconditionally dumped all QCs, removed the now unused
function ata_dump_status(), removed the now unreachable failure paths in
atapi_qc_complete(), removed the non-EH function to request ATAPI sense]
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 209 +++++++++++++++-----------------------
 drivers/ata/libata-eh.c   | 150 ++++++++++++---------------
 drivers/ata/libata-sata.c |   7 +-
 drivers/ata/libata-scsi.c | 142 ++------------------------
 drivers/ata/libata-sff.c  |  30 ++----
 5 files changed, 166 insertions(+), 372 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d37ab6087f2f..1f0306522649 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1586,13 +1586,11 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
 		}
 	}
 
-	if (ap->ops->error_handler)
-		ata_eh_release(ap);
+	ata_eh_release(ap);
 
 	rc = wait_for_completion_timeout(&wait, msecs_to_jiffies(timeout));
 
-	if (ap->ops->error_handler)
-		ata_eh_acquire(ap);
+	ata_eh_acquire(ap);
 
 	ata_sff_flush_pio_task(ap);
 
@@ -1607,10 +1605,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
 		if (qc->flags & ATA_QCFLAG_ACTIVE) {
 			qc->err_mask |= AC_ERR_TIMEOUT;
 
-			if (ap->ops->error_handler)
-				ata_port_freeze(ap);
-			else
-				ata_qc_complete(qc);
+			ata_port_freeze(ap);
 
 			ata_dev_warn(dev, "qc timeout after %u msecs (cmd 0x%x)\n",
 				     timeout, command);
@@ -4874,126 +4869,103 @@ static void ata_verify_xfer(struct ata_queued_cmd *qc)
 void ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	struct ata_device *dev = qc->dev;
+	struct ata_eh_info *ehi = &dev->link->eh_info;
 
 	/* Trigger the LED (if available) */
 	ledtrig_disk_activity(!!(qc->tf.flags & ATA_TFLAG_WRITE));
 
-	/* XXX: New EH and old EH use different mechanisms to
-	 * synchronize EH with regular execution path.
-	 *
-	 * In new EH, a qc owned by EH is marked with ATA_QCFLAG_EH.
-	 * 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.
+	/*
+	 * In order to synchronize EH with the regular execution path, a qc that
+	 * is owned by EH is marked with ATA_QCFLAG_EH.
 	 *
-	 * Old EH depends on ata_qc_complete() nullifying completion
-	 * requests if ATA_QCFLAG_EH_SCHEDULED is set.  Old EH does
-	 * not synchronize with interrupt handler.  Only PIO task is
-	 * taken care of.
+	 * 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.
 	 */
-	if (ap->ops->error_handler) {
-		struct ata_device *dev = qc->dev;
-		struct ata_eh_info *ehi = &dev->link->eh_info;
-
-		if (unlikely(qc->err_mask))
-			qc->flags |= ATA_QCFLAG_EH;
+	if (unlikely(qc->err_mask))
+		qc->flags |= ATA_QCFLAG_EH;
 
-		/*
-		 * Finish internal commands without any further processing
-		 * and always with the result TF filled.
-		 */
-		if (unlikely(ata_tag_internal(qc->tag))) {
-			fill_result_tf(qc);
-			trace_ata_qc_complete_internal(qc);
-			__ata_qc_complete(qc);
-			return;
-		}
+	/*
+	 * Finish internal commands without any further processing and always
+	 * with the result TF filled.
+	 */
+	if (unlikely(ata_tag_internal(qc->tag))) {
+		fill_result_tf(qc);
+		trace_ata_qc_complete_internal(qc);
+		__ata_qc_complete(qc);
+		return;
+	}
 
-		/*
-		 * Non-internal qc has failed.  Fill the result TF and
-		 * summon EH.
-		 */
-		if (unlikely(qc->flags & ATA_QCFLAG_EH)) {
-			fill_result_tf(qc);
-			trace_ata_qc_complete_failed(qc);
-			ata_qc_schedule_eh(qc);
-			return;
-		}
+	/* Non-internal qc has failed.  Fill the result TF and summon EH. */
+	if (unlikely(qc->flags & ATA_QCFLAG_EH)) {
+		fill_result_tf(qc);
+		trace_ata_qc_complete_failed(qc);
+		ata_qc_schedule_eh(qc);
+		return;
+	}
 
-		WARN_ON_ONCE(ata_port_is_frozen(ap));
+	WARN_ON_ONCE(ata_port_is_frozen(ap));
 
-		/* read result TF if requested */
-		if (qc->flags & ATA_QCFLAG_RESULT_TF)
-			fill_result_tf(qc);
+	/* read result TF if requested */
+	if (qc->flags & ATA_QCFLAG_RESULT_TF)
+		fill_result_tf(qc);
 
-		trace_ata_qc_complete_done(qc);
+	trace_ata_qc_complete_done(qc);
 
+	/*
+	 * For CDL commands that completed without an error, check if we have
+	 * sense data (ATA_SENSE is set). If we do, then the command may have
+	 * been aborted by the device due to a limit timeout using the policy
+	 * 0xD. For these commands, invoke EH to get the command sense data.
+	 */
+	if (qc->result_tf.status & ATA_SENSE &&
+	    ((ata_is_ncq(qc->tf.protocol) &&
+	      dev->flags & ATA_DFLAG_CDL_ENABLED) ||
+	     (!(ata_is_ncq(qc->tf.protocol) &&
+		ata_id_sense_reporting_enabled(dev->id))))) {
 		/*
-		 * For CDL commands that completed without an error, check if
-		 * we have sense data (ATA_SENSE is set). If we do, then the
-		 * command may have been aborted by the device due to a limit
-		 * timeout using the policy 0xD. For these commands, invoke EH
-		 * to get the command sense data.
+		 * Tell SCSI EH to not overwrite scmd->result even if this
+		 * command is finished with result SAM_STAT_GOOD.
 		 */
-		if (qc->result_tf.status & ATA_SENSE &&
-		    ((ata_is_ncq(qc->tf.protocol) &&
-		      dev->flags & ATA_DFLAG_CDL_ENABLED) ||
-		     (!(ata_is_ncq(qc->tf.protocol) &&
-			ata_id_sense_reporting_enabled(dev->id))))) {
-			/*
-			 * Tell SCSI EH to not overwrite scmd->result even if
-			 * this command is finished with result SAM_STAT_GOOD.
-			 */
-			qc->scsicmd->flags |= SCMD_FORCE_EH_SUCCESS;
-			qc->flags |= ATA_QCFLAG_EH_SUCCESS_CMD;
-			ehi->dev_action[dev->devno] |= ATA_EH_GET_SUCCESS_SENSE;
-
-			/*
-			 * set pending so that ata_qc_schedule_eh() does not
-			 * trigger fast drain, and freeze the port.
-			 */
-			ap->pflags |= ATA_PFLAG_EH_PENDING;
-			ata_qc_schedule_eh(qc);
-			return;
-		}
+		qc->scsicmd->flags |= SCMD_FORCE_EH_SUCCESS;
+		qc->flags |= ATA_QCFLAG_EH_SUCCESS_CMD;
+		ehi->dev_action[dev->devno] |= ATA_EH_GET_SUCCESS_SENSE;
 
-		/* Some commands need post-processing after successful
-		 * completion.
+		/*
+		 * set pending so that ata_qc_schedule_eh() does not trigger
+		 * fast drain, and freeze the port.
 		 */
-		switch (qc->tf.command) {
-		case ATA_CMD_SET_FEATURES:
-			if (qc->tf.feature != SETFEATURES_WC_ON &&
-			    qc->tf.feature != SETFEATURES_WC_OFF &&
-			    qc->tf.feature != SETFEATURES_RA_ON &&
-			    qc->tf.feature != SETFEATURES_RA_OFF)
-				break;
-			fallthrough;
-		case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
-		case ATA_CMD_SET_MULTI: /* multi_count changed */
-			/* revalidate device */
-			ehi->dev_action[dev->devno] |= ATA_EH_REVALIDATE;
-			ata_port_schedule_eh(ap);
-			break;
+		ap->pflags |= ATA_PFLAG_EH_PENDING;
+		ata_qc_schedule_eh(qc);
+		return;
+	}
 
-		case ATA_CMD_SLEEP:
-			dev->flags |= ATA_DFLAG_SLEEPING;
+	/* Some commands need post-processing after successful completion. */
+	switch (qc->tf.command) {
+	case ATA_CMD_SET_FEATURES:
+		if (qc->tf.feature != SETFEATURES_WC_ON &&
+		    qc->tf.feature != SETFEATURES_WC_OFF &&
+		    qc->tf.feature != SETFEATURES_RA_ON &&
+		    qc->tf.feature != SETFEATURES_RA_OFF)
 			break;
-		}
-
-		if (unlikely(dev->flags & ATA_DFLAG_DUBIOUS_XFER))
-			ata_verify_xfer(qc);
+		fallthrough;
+	case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
+	case ATA_CMD_SET_MULTI: /* multi_count changed */
+		/* revalidate device */
+		ehi->dev_action[dev->devno] |= ATA_EH_REVALIDATE;
+		ata_port_schedule_eh(ap);
+		break;
 
-		__ata_qc_complete(qc);
-	} else {
-		if (qc->flags & ATA_QCFLAG_EH_SCHEDULED)
-			return;
+	case ATA_CMD_SLEEP:
+		dev->flags |= ATA_DFLAG_SLEEPING;
+		break;
+	}
 
-		/* read result TF if failed or requested */
-		if (qc->err_mask || qc->flags & ATA_QCFLAG_RESULT_TF)
-			fill_result_tf(qc);
+	if (unlikely(dev->flags & ATA_DFLAG_DUBIOUS_XFER))
+		ata_verify_xfer(qc);
 
-		__ata_qc_complete(qc);
-	}
+	__ata_qc_complete(qc);
 }
 EXPORT_SYMBOL_GPL(ata_qc_complete);
 
@@ -5039,11 +5011,8 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_link *link = qc->dev->link;
 	u8 prot = qc->tf.protocol;
 
-	/* Make sure only one non-NCQ command is outstanding.  The
-	 * check is skipped for old EH because it reuses active qc to
-	 * request ATAPI sense.
-	 */
-	WARN_ON_ONCE(ap->ops->error_handler && ata_tag_valid(link->active_tag));
+	/* Make sure only one non-NCQ command is outstanding. */
+	WARN_ON_ONCE(ata_tag_valid(link->active_tag));
 
 	if (ata_is_ncq(prot)) {
 		WARN_ON_ONCE(link->sactive & (1 << qc->hw_tag));
@@ -5917,15 +5886,9 @@ void __ata_port_probe(struct ata_port *ap)
 
 int ata_port_probe(struct ata_port *ap)
 {
-	int rc = 0;
-
-	if (ap->ops->error_handler) {
-		__ata_port_probe(ap);
-		ata_port_wait_eh(ap);
-	} else {
-		rc = ata_bus_probe(ap);
-	}
-	return rc;
+	__ata_port_probe(ap);
+	ata_port_wait_eh(ap);
+	return 0;
 }
 
 
@@ -6130,9 +6093,6 @@ static void ata_port_detach(struct ata_port *ap)
 	struct ata_link *link;
 	struct ata_device *dev;
 
-	if (!ap->ops->error_handler)
-		goto skip_eh;
-
 	/* tell EH we're leaving & flush EH */
 	spin_lock_irqsave(ap->lock, flags);
 	ap->pflags |= ATA_PFLAG_UNLOADING;
@@ -6148,7 +6108,6 @@ static void ata_port_detach(struct ata_port *ap)
 	cancel_delayed_work_sync(&ap->hotplug_task);
 	cancel_delayed_work_sync(&ap->scsi_rescan_task);
 
- skip_eh:
 	/* clean up zpodd on port removal */
 	ata_for_each_link(link, ap, HOST_FIRST) {
 		ata_for_each_dev(dev, link, ALL) {
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 35e03679b0bf..dc7857f9aa94 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -571,13 +571,10 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 	/* make sure sff pio task is not running */
 	ata_sff_flush_pio_task(ap);
 
-	if (!ap->ops->error_handler)
-		return;
-
 	/* synchronize with host lock and sort out timeouts */
 
 	/*
-	 * For new EH, all qcs are finished in one of three ways -
+	 * For EH, all qcs are finished in one of three ways -
 	 * 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
@@ -659,94 +656,89 @@ EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
 void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
 {
 	unsigned long flags;
+	struct ata_link *link;
 
 	/* invoke error handler */
-	if (ap->ops->error_handler) {
-		struct ata_link *link;
 
-		/* acquire EH ownership */
-		ata_eh_acquire(ap);
+	/* acquire EH ownership */
+	ata_eh_acquire(ap);
  repeat:
-		/* kill fast drain timer */
-		del_timer_sync(&ap->fastdrain_timer);
+	/* kill fast drain timer */
+	del_timer_sync(&ap->fastdrain_timer);
 
-		/* process port resume request */
-		ata_eh_handle_port_resume(ap);
+	/* process port resume request */
+	ata_eh_handle_port_resume(ap);
 
-		/* fetch & clear EH info */
-		spin_lock_irqsave(ap->lock, flags);
+	/* fetch & clear EH info */
+	spin_lock_irqsave(ap->lock, flags);
 
-		ata_for_each_link(link, ap, HOST_FIRST) {
-			struct ata_eh_context *ehc = &link->eh_context;
-			struct ata_device *dev;
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		struct ata_eh_context *ehc = &link->eh_context;
+		struct ata_device *dev;
 
-			memset(&link->eh_context, 0, sizeof(link->eh_context));
-			link->eh_context.i = link->eh_info;
-			memset(&link->eh_info, 0, sizeof(link->eh_info));
+		memset(&link->eh_context, 0, sizeof(link->eh_context));
+		link->eh_context.i = link->eh_info;
+		memset(&link->eh_info, 0, sizeof(link->eh_info));
 
-			ata_for_each_dev(dev, link, ENABLED) {
-				int devno = dev->devno;
+		ata_for_each_dev(dev, link, ENABLED) {
+			int devno = dev->devno;
 
-				ehc->saved_xfer_mode[devno] = dev->xfer_mode;
-				if (ata_ncq_enabled(dev))
-					ehc->saved_ncq_enabled |= 1 << devno;
-			}
+			ehc->saved_xfer_mode[devno] = dev->xfer_mode;
+			if (ata_ncq_enabled(dev))
+				ehc->saved_ncq_enabled |= 1 << devno;
 		}
+	}
 
-		ap->pflags |= ATA_PFLAG_EH_IN_PROGRESS;
-		ap->pflags &= ~ATA_PFLAG_EH_PENDING;
-		ap->excl_link = NULL;	/* don't maintain exclusion over EH */
+	ap->pflags |= ATA_PFLAG_EH_IN_PROGRESS;
+	ap->pflags &= ~ATA_PFLAG_EH_PENDING;
+	ap->excl_link = NULL;	/* don't maintain exclusion over EH */
 
-		spin_unlock_irqrestore(ap->lock, flags);
+	spin_unlock_irqrestore(ap->lock, flags);
 
-		/* invoke EH, skip if unloading or suspended */
-		if (!(ap->pflags & (ATA_PFLAG_UNLOADING | ATA_PFLAG_SUSPENDED)))
-			ap->ops->error_handler(ap);
-		else {
-			/* if unloading, commence suicide */
-			if ((ap->pflags & ATA_PFLAG_UNLOADING) &&
-			    !(ap->pflags & ATA_PFLAG_UNLOADED))
-				ata_eh_unload(ap);
-			ata_eh_finish(ap);
-		}
+	/* invoke EH, skip if unloading or suspended */
+	if (!(ap->pflags & (ATA_PFLAG_UNLOADING | ATA_PFLAG_SUSPENDED)))
+		ap->ops->error_handler(ap);
+	else {
+		/* if unloading, commence suicide */
+		if ((ap->pflags & ATA_PFLAG_UNLOADING) &&
+		    !(ap->pflags & ATA_PFLAG_UNLOADED))
+			ata_eh_unload(ap);
+		ata_eh_finish(ap);
+	}
 
-		/* process port suspend request */
-		ata_eh_handle_port_suspend(ap);
+	/* process port suspend request */
+	ata_eh_handle_port_suspend(ap);
 
-		/* Exception might have happened after ->error_handler
-		 * recovered the port but before this point.  Repeat
-		 * EH in such case.
-		 */
-		spin_lock_irqsave(ap->lock, flags);
+	/*
+	 * Exception might have happened after ->error_handler recovered the
+	 * port but before this point.  Repeat EH in such case.
+	 */
+	spin_lock_irqsave(ap->lock, flags);
 
-		if (ap->pflags & ATA_PFLAG_EH_PENDING) {
-			if (--ap->eh_tries) {
-				spin_unlock_irqrestore(ap->lock, flags);
-				goto repeat;
-			}
-			ata_port_err(ap,
-				     "EH pending after %d tries, giving up\n",
-				     ATA_EH_MAX_TRIES);
-			ap->pflags &= ~ATA_PFLAG_EH_PENDING;
+	if (ap->pflags & ATA_PFLAG_EH_PENDING) {
+		if (--ap->eh_tries) {
+			spin_unlock_irqrestore(ap->lock, flags);
+			goto repeat;
 		}
+		ata_port_err(ap,
+			     "EH pending after %d tries, giving up\n",
+			     ATA_EH_MAX_TRIES);
+		ap->pflags &= ~ATA_PFLAG_EH_PENDING;
+	}
 
-		/* this run is complete, make sure EH info is clear */
-		ata_for_each_link(link, ap, HOST_FIRST)
-			memset(&link->eh_info, 0, sizeof(link->eh_info));
+	/* this run is complete, make sure EH info is clear */
+	ata_for_each_link(link, ap, HOST_FIRST)
+		memset(&link->eh_info, 0, sizeof(link->eh_info));
 
-		/* end eh (clear host_eh_scheduled) while holding
-		 * ap->lock such that if exception occurs after this
-		 * point but before EH completion, SCSI midlayer will
-		 * re-initiate EH.
-		 */
-		ap->ops->end_eh(ap);
+	/*
+	 * end eh (clear host_eh_scheduled) while holding ap->lock such that if
+	 * exception occurs after this point but before EH completion, SCSI
+	 * midlayer will re-initiate EH.
+	 */
+	ap->ops->end_eh(ap);
 
-		spin_unlock_irqrestore(ap->lock, flags);
-		ata_eh_release(ap);
-	} else {
-		WARN_ON(ata_qc_from_tag(ap, ap->link.active_tag) == NULL);
-		ap->ops->eng_timeout(ap);
-	}
+	spin_unlock_irqrestore(ap->lock, flags);
+	ata_eh_release(ap);
 
 	scsi_eh_flush_done_q(&ap->eh_done_q);
 
@@ -912,8 +904,6 @@ void ata_qc_schedule_eh(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 
-	WARN_ON(!ap->ops->error_handler);
-
 	qc->flags |= ATA_QCFLAG_EH;
 	ata_eh_set_pending(ap, 1);
 
@@ -934,8 +924,6 @@ void ata_qc_schedule_eh(struct ata_queued_cmd *qc)
  */
 void ata_std_sched_eh(struct ata_port *ap)
 {
-	WARN_ON(!ap->ops->error_handler);
-
 	if (ap->pflags & ATA_PFLAG_INITIALIZING)
 		return;
 
@@ -989,8 +977,6 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
 	struct ata_queued_cmd *qc;
 	int tag, nr_aborted = 0;
 
-	WARN_ON(!ap->ops->error_handler);
-
 	/* we're gonna abort all commands, no need for fast drain */
 	ata_eh_set_pending(ap, 0);
 
@@ -1065,8 +1051,6 @@ EXPORT_SYMBOL_GPL(ata_port_abort);
  */
 static void __ata_port_freeze(struct ata_port *ap)
 {
-	WARN_ON(!ap->ops->error_handler);
-
 	if (ap->ops->freeze)
 		ap->ops->freeze(ap);
 
@@ -1091,8 +1075,6 @@ static void __ata_port_freeze(struct ata_port *ap)
  */
 int ata_port_freeze(struct ata_port *ap)
 {
-	WARN_ON(!ap->ops->error_handler);
-
 	__ata_port_freeze(ap);
 
 	return ata_port_abort(ap);
@@ -1112,9 +1094,6 @@ void ata_eh_freeze_port(struct ata_port *ap)
 {
 	unsigned long flags;
 
-	if (!ap->ops->error_handler)
-		return;
-
 	spin_lock_irqsave(ap->lock, flags);
 	__ata_port_freeze(ap);
 	spin_unlock_irqrestore(ap->lock, flags);
@@ -1134,9 +1113,6 @@ void ata_eh_thaw_port(struct ata_port *ap)
 {
 	unsigned long flags;
 
-	if (!ap->ops->error_handler)
-		return;
-
 	spin_lock_irqsave(ap->lock, flags);
 
 	ap->pflags &= ~ATA_PFLAG_FROZEN;
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 85e279a12f62..99d4ab04bcce 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1158,12 +1158,7 @@ EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
  */
 int ata_sas_port_start(struct ata_port *ap)
 {
-	/*
-	 * the port is marked as frozen at allocation time, but if we don't
-	 * have new eh, we won't thaw it
-	 */
-	if (!ap->ops->error_handler)
-		ap->pflags &= ~ATA_PFLAG_FROZEN;
+	/* the port is marked as frozen at allocation time */
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_start);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 370d18aca71e..dd427a6a3276 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -709,47 +709,6 @@ static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
 	qc->nbytes = scsi_bufflen(scmd) + qc->extrabytes;
 }
 
-/**
- *	ata_dump_status - user friendly display of error info
- *	@ap: the port in question
- *	@tf: ptr to filled out taskfile
- *
- *	Decode and dump the ATA error/status registers for the user so
- *	that they have some idea what really happened at the non
- *	make-believe layer.
- *
- *	LOCKING:
- *	inherited from caller
- */
-static void ata_dump_status(struct ata_port *ap, struct ata_taskfile *tf)
-{
-	u8 stat = tf->status, err = tf->error;
-
-	if (stat & ATA_BUSY) {
-		ata_port_warn(ap, "status=0x%02x {Busy} ", stat);
-	} else {
-		ata_port_warn(ap, "status=0x%02x { %s%s%s%s%s%s%s} ", stat,
-			      stat & ATA_DRDY ? "DriveReady " : "",
-			      stat & ATA_DF ? "DeviceFault " : "",
-			      stat & ATA_DSC ? "SeekComplete " : "",
-			      stat & ATA_DRQ ? "DataRequest " : "",
-			      stat & ATA_CORR ? "CorrectedError " : "",
-			      stat & ATA_SENSE ? "Sense " : "",
-			      stat & ATA_ERR ? "Error " : "");
-		if (err)
-			ata_port_warn(ap, "error=0x%02x {%s%s%s%s%s%s", err,
-				      err & ATA_ABORTED ?
-				      "DriveStatusError " : "",
-				      err & ATA_ICRC ?
-				      (err & ATA_ABORTED ?
-				       "BadCRC " : "Sector ") : "",
-				      err & ATA_UNC ? "UncorrectableError " : "",
-				      err & ATA_IDNF ? "SectorIdNotFound " : "",
-				      err & ATA_TRK0NF ? "TrackZeroNotFound " : "",
-				      err & ATA_AMNF ? "AddrMarkNotFound " : "");
-	}
-}
-
 /**
  *	ata_to_sense_error - convert ATA error to SCSI error
  *	@id: ATA device number
@@ -904,7 +863,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
 	unsigned char *desc = sb + 8;
-	int verbose = qc->ap->ops->error_handler == NULL;
 	u8 sense_key, asc, ascq;
 
 	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
@@ -916,7 +874,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	if (qc->err_mask ||
 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
-				   &sense_key, &asc, &ascq, verbose);
+				   &sense_key, &asc, &ascq, false);
 		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
 	} else {
 		/*
@@ -999,7 +957,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
-	int verbose = qc->ap->ops->error_handler == NULL;
 	u64 block;
 	u8 sense_key, asc, ascq;
 
@@ -1017,7 +974,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 	if (qc->err_mask ||
 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
-				   &sense_key, &asc, &ascq, verbose);
+				   &sense_key, &asc, &ascq, false);
 		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
 	} else {
 		/* Could not decode error */
@@ -1179,9 +1136,6 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
 	unsigned long flags;
 	struct ata_device *dev;
 
-	if (!ap->ops->error_handler)
-		return;
-
 	spin_lock_irqsave(ap->lock, flags);
 	dev = __ata_scsi_find_dev(ap, sdev);
 	if (dev && dev->sdev) {
@@ -1668,7 +1622,6 @@ static void ata_qc_done(struct ata_queued_cmd *qc)
 
 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) &&
@@ -1692,9 +1645,6 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 		/* 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);
-
 	ata_qc_done(qc);
 }
 
@@ -2601,71 +2551,12 @@ static unsigned int ata_scsiop_report_luns(struct ata_scsi_args *args, u8 *rbuf)
 	return 0;
 }
 
-static void atapi_sense_complete(struct ata_queued_cmd *qc)
-{
-	if (qc->err_mask && ((qc->err_mask & AC_ERR_DEV) == 0)) {
-		/* FIXME: not quite right; we don't want the
-		 * translation of taskfile registers into
-		 * a sense descriptors, since that's only
-		 * correct for ATA, not ATAPI
-		 */
-		ata_gen_passthru_sense(qc);
-	}
-
-	ata_qc_done(qc);
-}
-
 /* is it pointless to prefer PIO for "safety reasons"? */
 static inline int ata_pio_use_silly(struct ata_port *ap)
 {
 	return (ap->flags & ATA_FLAG_PIO_DMA);
 }
 
-static void atapi_request_sense(struct ata_queued_cmd *qc)
-{
-	struct ata_port *ap = qc->ap;
-	struct scsi_cmnd *cmd = qc->scsicmd;
-
-	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
-
-#ifdef CONFIG_ATA_SFF
-	if (ap->ops->sff_tf_read)
-		ap->ops->sff_tf_read(ap, &qc->tf);
-#endif
-
-	/* fill these in, for the case where they are -not- overwritten */
-	cmd->sense_buffer[0] = 0x70;
-	cmd->sense_buffer[2] = qc->tf.error >> 4;
-
-	ata_qc_reinit(qc);
-
-	/* setup sg table and init transfer direction */
-	sg_init_one(&qc->sgent, cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
-	ata_sg_init(qc, &qc->sgent, 1);
-	qc->dma_dir = DMA_FROM_DEVICE;
-
-	memset(&qc->cdb, 0, qc->dev->cdb_len);
-	qc->cdb[0] = REQUEST_SENSE;
-	qc->cdb[4] = SCSI_SENSE_BUFFERSIZE;
-
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.command = ATA_CMD_PACKET;
-
-	if (ata_pio_use_silly(ap)) {
-		qc->tf.protocol = ATAPI_PROT_DMA;
-		qc->tf.feature |= ATAPI_PKT_DMA;
-	} else {
-		qc->tf.protocol = ATAPI_PROT_PIO;
-		qc->tf.lbam = SCSI_SENSE_BUFFERSIZE;
-		qc->tf.lbah = 0;
-	}
-	qc->nbytes = SCSI_SENSE_BUFFERSIZE;
-
-	qc->complete_fn = atapi_sense_complete;
-
-	ata_qc_issue(qc);
-}
-
 /*
  * ATAPI devices typically report zero for their SCSI version, and sometimes
  * deviate from the spec WRT response data format.  If SCSI version is
@@ -2691,9 +2582,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	unsigned int err_mask = qc->err_mask;
 
-	/* handle completion from new EH */
-	if (unlikely(qc->ap->ops->error_handler &&
-		     (err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID))) {
+	/* handle completion from EH */
+	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
 
 		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID)) {
 			/* FIXME: not quite right; we don't want the
@@ -2725,23 +2615,10 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 		return;
 	}
 
-	/* successful completion or old EH failure path */
-	if (unlikely(err_mask & AC_ERR_DEV)) {
-		cmd->result = SAM_STAT_CHECK_CONDITION;
-		atapi_request_sense(qc);
-		return;
-	} else if (unlikely(err_mask)) {
-		/* FIXME: not quite right; we don't want the
-		 * translation of taskfile registers into
-		 * a sense descriptors, since that's only
-		 * correct for ATA, not ATAPI
-		 */
-		ata_gen_passthru_sense(qc);
-	} else {
-		if (cmd->cmnd[0] == INQUIRY && (cmd->cmnd[1] & 0x03) == 0)
-			atapi_fixup_inquiry(cmd);
-		cmd->result = SAM_STAT_GOOD;
-	}
+	/* successful completion path */
+	if (cmd->cmnd[0] == INQUIRY && (cmd->cmnd[1] & 0x03) == 0)
+		atapi_fixup_inquiry(cmd);
+	cmd->result = SAM_STAT_GOOD;
 
 	ata_qc_done(qc);
 }
@@ -4790,9 +4667,6 @@ int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
 	unsigned long flags;
 	int devno, rc = 0;
 
-	if (!ap->ops->error_handler)
-		return -EOPNOTSUPP;
-
 	if (lun != SCAN_WILD_CARD && lun)
 		return -EINVAL;
 
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 9d28badfe41d..84471d92cd1b 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -883,31 +883,21 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 {
 	struct ata_port *ap = qc->ap;
 
-	if (ap->ops->error_handler) {
-		if (in_wq) {
-			/* EH might have kicked in while host lock is
-			 * released.
-			 */
-			qc = ata_qc_from_tag(ap, qc->tag);
-			if (qc) {
-				if (likely(!(qc->err_mask & AC_ERR_HSM))) {
-					ata_sff_irq_on(ap);
-					ata_qc_complete(qc);
-				} else
-					ata_port_freeze(ap);
-			}
-		} else {
-			if (likely(!(qc->err_mask & AC_ERR_HSM)))
+	if (in_wq) {
+		/* EH might have kicked in while host lock is released. */
+		qc = ata_qc_from_tag(ap, qc->tag);
+		if (qc) {
+			if (likely(!(qc->err_mask & AC_ERR_HSM))) {
+				ata_sff_irq_on(ap);
 				ata_qc_complete(qc);
-			else
+			} else
 				ata_port_freeze(ap);
 		}
 	} else {
-		if (in_wq) {
-			ata_sff_irq_on(ap);
-			ata_qc_complete(qc);
-		} else
+		if (likely(!(qc->err_mask & AC_ERR_HSM)))
 			ata_qc_complete(qc);
+		else
+			ata_port_freeze(ap);
 	}
 }
 
-- 
2.41.0

[PATCH v2 2/8] ata,scsi: remove ata_sas_port_{start,stop} callbacks

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

Callbacks are empty now, so remove them.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-sata.c     | 34 ----------------------------------
 drivers/scsi/libsas/sas_ata.c |  2 --
 include/linux/libata.h        |  2 --
 3 files changed, 38 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 99d4ab04bcce..d3b595294eee 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1144,40 +1144,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
 
-/**
- *	ata_sas_port_start - Set port up for dma.
- *	@ap: Port to initialize
- *
- *	Called just after data structures for each port are
- *	initialized.
- *
- *	May be used as the port_start() entry in ata_port_operations.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-int ata_sas_port_start(struct ata_port *ap)
-{
-	/* the port is marked as frozen at allocation time */
-	return 0;
-}
-EXPORT_SYMBOL_GPL(ata_sas_port_start);
-
-/**
- *	ata_sas_port_stop - Undo ata_sas_port_start()
- *	@ap: Port to shut down
- *
- *	May be used as the port_stop() entry in ata_port_operations.
- *
- *	LOCKING:
- *	Inherited from caller.
- */
-
-void ata_sas_port_stop(struct ata_port *ap)
-{
-}
-EXPORT_SYMBOL_GPL(ata_sas_port_stop);
-
 /**
  * ata_sas_async_probe - simply schedule probing and return
  * @ap: Port to probe
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 77714a495cbb..7ead1f1be97f 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -565,8 +565,6 @@ static struct ata_port_operations sas_sata_ops = {
 	.qc_prep		= ata_noop_qc_prep,
 	.qc_issue		= sas_ata_qc_issue,
 	.qc_fill_rtf		= sas_ata_qc_fill_rtf,
-	.port_start		= ata_sas_port_start,
-	.port_stop		= ata_sas_port_stop,
 	.set_dmamode		= sas_ata_set_dmamode,
 	.sched_eh		= sas_ata_sched_eh,
 	.end_eh			= sas_ata_end_eh,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 820f7a3a2749..9424c490ef0b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1244,10 +1244,8 @@ extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 extern void ata_sas_async_probe(struct ata_port *ap);
 extern int ata_sas_sync_probe(struct ata_port *ap);
 extern int ata_sas_port_init(struct ata_port *);
-extern int ata_sas_port_start(struct ata_port *ap);
 extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
 extern void ata_sas_tport_delete(struct ata_port *ap);
-extern void ata_sas_port_stop(struct ata_port *ap);
 extern int ata_sas_slave_configure(struct scsi_device *, struct ata_port *);
 extern int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap);
 extern void ata_tf_to_fis(const struct ata_taskfile *tf,
-- 
2.41.0

[PATCH v2 3/8] ata,scsi: remove ata_sas_port_destroy()

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

Is now a wrapper around kfree(), so call it directly.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-sata.c          | 18 ------------------
 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_discover.c |  2 +-
 include/linux/libata.h             |  1 -
 4 files changed, 2 insertions(+), 21 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index d3b595294eee..b5de0f40ea25 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1177,10 +1177,6 @@ EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
 
 int ata_sas_port_init(struct ata_port *ap)
 {
-	int rc = ap->ops->port_start(ap);
-
-	if (rc)
-		return rc;
 	ap->print_id = atomic_inc_return(&ata_print_id);
 	return 0;
 }
@@ -1198,20 +1194,6 @@ void ata_sas_tport_delete(struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ata_sas_tport_delete);
 
-/**
- *	ata_sas_port_destroy - Destroy a SATA port allocated by ata_sas_port_alloc
- *	@ap: SATA port to destroy
- *
- */
-
-void ata_sas_port_destroy(struct ata_port *ap)
-{
-	if (ap->ops->port_stop)
-		ap->ops->port_stop(ap);
-	kfree(ap);
-}
-EXPORT_SYMBOL_GPL(ata_sas_port_destroy);
-
 /**
  *	ata_sas_slave_configure - Default slave_config routine for libata devices
  *	@sdev: SCSI device to configure
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 7ead1f1be97f..a2eb9a2191c0 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -619,7 +619,7 @@ int sas_ata_init(struct domain_device *found_dev)
 	return 0;
 
 destroy_port:
-	ata_sas_port_destroy(ap);
+	kfree(ap);
 free_host:
 	ata_host_put(ata_host);
 	return rc;
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 8c6afe724944..07e18cdb85c7 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
 
 	if (dev_is_sata(dev) && dev->sata_dev.ap) {
 		ata_sas_tport_delete(dev->sata_dev.ap);
-		ata_sas_port_destroy(dev->sata_dev.ap);
+		kfree(dev->sata_dev.ap);
 		ata_host_put(dev->sata_dev.ata_host);
 		dev->sata_dev.ata_host = NULL;
 		dev->sata_dev.ap = NULL;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9424c490ef0b..53cfb1a4b97a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1238,7 +1238,6 @@ extern int sata_link_debounce(struct ata_link *link,
 extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			     bool spm_wakeup);
 extern int ata_slave_link_init(struct ata_port *ap);
-extern void ata_sas_port_destroy(struct ata_port *);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
 extern void ata_sas_async_probe(struct ata_port *ap);
-- 
2.41.0

[PATCH v2 4/8] ata: remove ata_sas_sync_probe()

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

Unused.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-sata.c | 7 -------
 include/linux/libata.h    | 1 -
 2 files changed, 8 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index b5de0f40ea25..23252ebe312d 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1157,13 +1157,6 @@ void ata_sas_async_probe(struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ata_sas_async_probe);
 
-int ata_sas_sync_probe(struct ata_port *ap)
-{
-	return ata_port_probe(ap);
-}
-EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
-
-
 /**
  *	ata_sas_port_init - Initialize a SATA device
  *	@ap: SATA port to initialize
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 53cfb1a4b97a..86490718cd0d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1241,7 +1241,6 @@ extern int ata_slave_link_init(struct ata_port *ap);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
 extern void ata_sas_async_probe(struct ata_port *ap);
-extern int ata_sas_sync_probe(struct ata_port *ap);
 extern int ata_sas_port_init(struct ata_port *);
 extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
 extern void ata_sas_tport_delete(struct ata_port *ap);
-- 
2.41.0

[PATCH v2 5/8] ata: inline ata_port_probe()

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

Just used in one place.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1f0306522649..c5e93e1a560d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5884,14 +5884,6 @@ void __ata_port_probe(struct ata_port *ap)
 	spin_unlock_irqrestore(ap->lock, flags);
 }
 
-int ata_port_probe(struct ata_port *ap)
-{
-	__ata_port_probe(ap);
-	ata_port_wait_eh(ap);
-	return 0;
-}
-
-
 static void async_port_probe(void *data, async_cookie_t cookie)
 {
 	struct ata_port *ap = data;
@@ -5906,7 +5898,8 @@ static void async_port_probe(void *data, async_cookie_t cookie)
 	if (!(ap->host->flags & ATA_HOST_PARALLEL_SCAN) && ap->port_no != 0)
 		async_synchronize_cookie(cookie);
 
-	(void)ata_port_probe(ap);
+	__ata_port_probe(ap);
+	ata_port_wait_eh(ap);
 
 	/* in order to keep device order, we need to synchronize at this point */
 	async_synchronize_cookie(cookie);
-- 
2.41.0

[PATCH v2 6/8] ata,scsi: cleanup ata_port_probe()

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

Rename __ata_port_probe() to ata_port_probe() and drop the wrapper
ata_sas_async_probe().

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c     |  5 +++--
 drivers/ata/libata-sata.c     | 13 -------------
 drivers/ata/libata.h          |  2 --
 drivers/scsi/libsas/sas_ata.c |  2 +-
 include/linux/libata.h        |  2 +-
 5 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c5e93e1a560d..dedae669c9da 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5865,7 +5865,7 @@ void ata_host_init(struct ata_host *host, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(ata_host_init);
 
-void __ata_port_probe(struct ata_port *ap)
+void ata_port_probe(struct ata_port *ap)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned long flags;
@@ -5883,6 +5883,7 @@ void __ata_port_probe(struct ata_port *ap)
 
 	spin_unlock_irqrestore(ap->lock, flags);
 }
+EXPORT_SYMBOL_GPL(ata_port_probe);
 
 static void async_port_probe(void *data, async_cookie_t cookie)
 {
@@ -5898,7 +5899,7 @@ static void async_port_probe(void *data, async_cookie_t cookie)
 	if (!(ap->host->flags & ATA_HOST_PARALLEL_SCAN) && ap->port_no != 0)
 		async_synchronize_cookie(cookie);
 
-	__ata_port_probe(ap);
+	ata_port_probe(ap);
 	ata_port_wait_eh(ap);
 
 	/* in order to keep device order, we need to synchronize at this point */
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 23252ebe312d..c0253adbf47c 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1144,19 +1144,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
 
-/**
- * ata_sas_async_probe - simply schedule probing and return
- * @ap: Port to probe
- *
- * For batch scheduling of probe for sas attached ata devices, assumes
- * the port has already been through ata_sas_port_init()
- */
-void ata_sas_async_probe(struct ata_port *ap)
-{
-	__ata_port_probe(ap);
-}
-EXPORT_SYMBOL_GPL(ata_sas_async_probe);
-
 /**
  *	ata_sas_port_init - Initialize a SATA device
  *	@ap: SATA port to initialize
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index cf993885d2b2..1ec9b4427b84 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -78,8 +78,6 @@ extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern const char *sata_spd_string(unsigned int spd);
-extern int ata_port_probe(struct ata_port *ap);
-extern void __ata_port_probe(struct ata_port *ap);
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 				      u8 page, void *buf, unsigned int sectors);
 
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index a2eb9a2191c0..d6bb37b3974a 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -653,7 +653,7 @@ void sas_probe_sata(struct asd_sas_port *port)
 		if (!dev_is_sata(dev))
 			continue;
 
-		ata_sas_async_probe(dev->sata_dev.ap);
+		ata_port_probe(dev->sata_dev.ap);
 	}
 	mutex_unlock(&port->ha->disco_mutex);
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 86490718cd0d..458cb24958ca 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1240,7 +1240,7 @@ extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 extern int ata_slave_link_init(struct ata_port *ap);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
-extern void ata_sas_async_probe(struct ata_port *ap);
+extern void ata_port_probe(struct ata_port *ap);
 extern int ata_sas_port_init(struct ata_port *);
 extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
 extern void ata_sas_tport_delete(struct ata_port *ap);
-- 
2.41.0

[PATCH v2 7/8] ata: sata_sx4: drop already completed TODO

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The TODO claims that the pdc_20621_ops should set the .inherits
function pointer to &ata_base_port_ops after it has been converted
to use the new EH.

However, the driver was converted to use the new EH a long time ago,
in commit 67651ee5710c ("[libata] sata_sx4: convert to new exception
handling methods"), which also did set .inherits function pointer to
&ata_sff_port_ops (and ata_sff_port_ops itself has .inherits set to
&ata_base_port_ops).

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/sata_sx4.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
index ccc016072637..b51d7a9d0d90 100644
--- a/drivers/ata/sata_sx4.c
+++ b/drivers/ata/sata_sx4.c
@@ -232,7 +232,6 @@ static const struct scsi_host_template pdc_sata_sht = {
 	.dma_boundary		= ATA_DMA_BOUNDARY,
 };
 
-/* TODO: inherit from base port_ops after converting to new EH */
 static struct ata_port_operations pdc_20621_ops = {
 	.inherits		= &ata_sff_port_ops,
 
-- 
2.41.0

[PATCH v2 8/8] ata: remove ata_bus_probe()

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Remove ata_bus_probe() as it is unused.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 138 --------------------------------------
 drivers/ata/libata.h      |   1 -
 2 files changed, 139 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dedae669c9da..0af88ef231d1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3057,144 +3057,6 @@ int ata_cable_sata(struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ata_cable_sata);
 
-/**
- *	ata_bus_probe - Reset and probe ATA bus
- *	@ap: Bus to probe
- *
- *	Master ATA bus probing function.  Initiates a hardware-dependent
- *	bus reset, then attempts to identify any devices found on
- *	the bus.
- *
- *	LOCKING:
- *	PCI/etc. bus probe sem.
- *
- *	RETURNS:
- *	Zero on success, negative errno otherwise.
- */
-
-int ata_bus_probe(struct ata_port *ap)
-{
-	unsigned int classes[ATA_MAX_DEVICES];
-	int tries[ATA_MAX_DEVICES];
-	int rc;
-	struct ata_device *dev;
-
-	ata_for_each_dev(dev, &ap->link, ALL)
-		tries[dev->devno] = ATA_PROBE_MAX_TRIES;
-
- retry:
-	ata_for_each_dev(dev, &ap->link, ALL) {
-		/* If we issue an SRST then an ATA drive (not ATAPI)
-		 * may change configuration and be in PIO0 timing. If
-		 * we do a hard reset (or are coming from power on)
-		 * this is true for ATA or ATAPI. Until we've set a
-		 * suitable controller mode we should not touch the
-		 * bus as we may be talking too fast.
-		 */
-		dev->pio_mode = XFER_PIO_0;
-		dev->dma_mode = 0xff;
-
-		/* If the controller has a pio mode setup function
-		 * then use it to set the chipset to rights. Don't
-		 * touch the DMA setup as that will be dealt with when
-		 * configuring devices.
-		 */
-		if (ap->ops->set_piomode)
-			ap->ops->set_piomode(ap, dev);
-	}
-
-	/* reset and determine device classes */
-	ap->ops->phy_reset(ap);
-
-	ata_for_each_dev(dev, &ap->link, ALL) {
-		if (dev->class != ATA_DEV_UNKNOWN)
-			classes[dev->devno] = dev->class;
-		else
-			classes[dev->devno] = ATA_DEV_NONE;
-
-		dev->class = ATA_DEV_UNKNOWN;
-	}
-
-	/* read IDENTIFY page and configure devices. We have to do the identify
-	   specific sequence bass-ackwards so that PDIAG- is released by
-	   the slave device */
-
-	ata_for_each_dev(dev, &ap->link, ALL_REVERSE) {
-		if (tries[dev->devno])
-			dev->class = classes[dev->devno];
-
-		if (!ata_dev_enabled(dev))
-			continue;
-
-		rc = ata_dev_read_id(dev, &dev->class, ATA_READID_POSTRESET,
-				     dev->id);
-		if (rc)
-			goto fail;
-	}
-
-	/* Now ask for the cable type as PDIAG- should have been released */
-	if (ap->ops->cable_detect)
-		ap->cbl = ap->ops->cable_detect(ap);
-
-	/* We may have SATA bridge glue hiding here irrespective of
-	 * the reported cable types and sensed types.  When SATA
-	 * drives indicate we have a bridge, we don't know which end
-	 * of the link the bridge is which is a problem.
-	 */
-	ata_for_each_dev(dev, &ap->link, ENABLED)
-		if (ata_id_is_sata(dev->id))
-			ap->cbl = ATA_CBL_SATA;
-
-	/* After the identify sequence we can now set up the devices. We do
-	   this in the normal order so that the user doesn't get confused */
-
-	ata_for_each_dev(dev, &ap->link, ENABLED) {
-		ap->link.eh_context.i.flags |= ATA_EHI_PRINTINFO;
-		rc = ata_dev_configure(dev);
-		ap->link.eh_context.i.flags &= ~ATA_EHI_PRINTINFO;
-		if (rc)
-			goto fail;
-	}
-
-	/* configure transfer mode */
-	rc = ata_set_mode(&ap->link, &dev);
-	if (rc)
-		goto fail;
-
-	ata_for_each_dev(dev, &ap->link, ENABLED)
-		return 0;
-
-	return -ENODEV;
-
- fail:
-	tries[dev->devno]--;
-
-	switch (rc) {
-	case -EINVAL:
-		/* eeek, something went very wrong, give up */
-		tries[dev->devno] = 0;
-		break;
-
-	case -ENODEV:
-		/* give it just one more chance */
-		tries[dev->devno] = min(tries[dev->devno], 1);
-		fallthrough;
-	case -EIO:
-		if (tries[dev->devno] == 1) {
-			/* This is the last chance, better to slow
-			 * down than lose it.
-			 */
-			sata_down_spd_limit(&ap->link, 0);
-			ata_down_xfermask_limit(dev, ATA_DNXFER_PIO);
-		}
-	}
-
-	if (!tries[dev->devno])
-		ata_dev_disable(dev);
-
-	goto retry;
-}
-
 /**
  *	sata_print_link_status - Print SATA link status
  *	@link: SATA link to printk link status about
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 1ec9b4427b84..6e7d352803bd 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -122,7 +122,6 @@ extern void ata_scsi_media_change_notify(struct ata_device *dev);
 extern void ata_scsi_hotplug(struct work_struct *work);
 extern void ata_schedule_scsi_eh(struct Scsi_Host *shost);
 extern void ata_scsi_dev_rescan(struct work_struct *work);
-extern int ata_bus_probe(struct ata_port *ap);
 extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
 			      unsigned int id, u64 lun);
 void ata_scsi_sdev_config(struct scsi_device *sdev);
-- 
2.41.0

Re: [PATCH v2 2/8] ata,scsi: remove ata_sas_port_{start,stop} callbacks

[Jason Yan]

On 2023/7/20 8:42, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Callbacks are empty now, so remove them.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-sata.c     | 34 ----------------------------------
>   drivers/scsi/libsas/sas_ata.c |  2 --
>   include/linux/libata.h        |  2 --
>   3 files changed, 38 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 99d4ab04bcce..d3b595294eee 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1144,40 +1144,6 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
>   }
>   EXPORT_SYMBOL_GPL(ata_sas_port_alloc);
>   
> -/**
> - *	ata_sas_port_start - Set port up for dma.
> - *	@ap: Port to initialize
> - *
> - *	Called just after data structures for each port are
> - *	initialized.
> - *
> - *	May be used as the port_start() entry in ata_port_operations.
> - *
> - *	LOCKING:
> - *	Inherited from caller.
> - */
> -int ata_sas_port_start(struct ata_port *ap)
> -{
> -	/* the port is marked as frozen at allocation time */
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(ata_sas_port_start);
> -
> -/**
> - *	ata_sas_port_stop - Undo ata_sas_port_start()
> - *	@ap: Port to shut down
> - *
> - *	May be used as the port_stop() entry in ata_port_operations.
> - *
> - *	LOCKING:
> - *	Inherited from caller.
> - */
> -
> -void ata_sas_port_stop(struct ata_port *ap)
> -{
> -}
> -EXPORT_SYMBOL_GPL(ata_sas_port_stop);
> -
>   /**
>    * ata_sas_async_probe - simply schedule probing and return
>    * @ap: Port to probe
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 77714a495cbb..7ead1f1be97f 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -565,8 +565,6 @@ static struct ata_port_operations sas_sata_ops = {
>   	.qc_prep		= ata_noop_qc_prep,
>   	.qc_issue		= sas_ata_qc_issue,
>   	.qc_fill_rtf		= sas_ata_qc_fill_rtf,
> -	.port_start		= ata_sas_port_start,
> -	.port_stop		= ata_sas_port_stop,


Hi Niklas,

->port_start is NULL now but ata_sas_port_init() is still using it 
without checking whether it is NULL. I know the patch #3 will remove it 
finally. But this will bring a great inconvenience because the kernel 
will crash when bisectting to this commit.

Either put patch #3 before this patch or fold patch #3 directly into 
path #2 ?

Thanks,
Jason

Re: [PATCH v2 4/8] ata: remove ata_sas_sync_probe()

[Jason Yan]

On 2023/7/20 8:42, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Unused.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-sata.c | 7 -------
>   include/linux/libata.h    | 1 -
>   2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b5de0f40ea25..23252ebe312d 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1157,13 +1157,6 @@ void ata_sas_async_probe(struct ata_port *ap)
>   }
>   EXPORT_SYMBOL_GPL(ata_sas_async_probe);
>   
> -int ata_sas_sync_probe(struct ata_port *ap)
> -{
> -	return ata_port_probe(ap);
> -}
> -EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
> -
> -
>   /**
>    *	ata_sas_port_init - Initialize a SATA device
>    *	@ap: SATA port to initialize
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 53cfb1a4b97a..86490718cd0d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1241,7 +1241,6 @@ extern int ata_slave_link_init(struct ata_port *ap);
>   extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
>   					   struct ata_port_info *, struct Scsi_Host *);
>   extern void ata_sas_async_probe(struct ata_port *ap);
> -extern int ata_sas_sync_probe(struct ata_port *ap);
>   extern int ata_sas_port_init(struct ata_port *);
>   extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
>   extern void ata_sas_tport_delete(struct ata_port *ap);
> 

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Thanks,
Jason

Re: [PATCH v2 5/8] ata: inline ata_port_probe()

[Jason Yan]

On 2023/7/20 8:42, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Just used in one place.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-core.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1f0306522649..c5e93e1a560d 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5884,14 +5884,6 @@ void __ata_port_probe(struct ata_port *ap)
>   	spin_unlock_irqrestore(ap->lock, flags);
>   }
>   
> -int ata_port_probe(struct ata_port *ap)
> -{
> -	__ata_port_probe(ap);
> -	ata_port_wait_eh(ap);
> -	return 0;
> -}
> -
> -
>   static void async_port_probe(void *data, async_cookie_t cookie)
>   {
>   	struct ata_port *ap = data;
> @@ -5906,7 +5898,8 @@ static void async_port_probe(void *data, async_cookie_t cookie)
>   	if (!(ap->host->flags & ATA_HOST_PARALLEL_SCAN) && ap->port_no != 0)
>   		async_synchronize_cookie(cookie);
>   
> -	(void)ata_port_probe(ap);
> +	__ata_port_probe(ap);
> +	ata_port_wait_eh(ap);
>   
>   	/* in order to keep device order, we need to synchronize at this point */
>   	async_synchronize_cookie(cookie);
> 

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Thanks,
Jason

Re: [PATCH v2 6/8] ata,scsi: cleanup ata_port_probe()

[Jason Yan]

On 2023/7/20 8:42, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Rename __ata_port_probe() to ata_port_probe() and drop the wrapper
> ata_sas_async_probe().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-core.c     |  5 +++--
>   drivers/ata/libata-sata.c     | 13 -------------
>   drivers/ata/libata.h          |  2 --
>   drivers/scsi/libsas/sas_ata.c |  2 +-
>   include/linux/libata.h        |  2 +-
>   5 files changed, 5 insertions(+), 19 deletions(-)

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Thanks,
Jason

Re: [PATCH v2 7/8] ata: sata_sx4: drop already completed TODO

[Jason Yan]

On 2023/7/20 8:42, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The TODO claims that the pdc_20621_ops should set the .inherits
> function pointer to &ata_base_port_ops after it has been converted
> to use the new EH.
> 
> However, the driver was converted to use the new EH a long time ago,
> in commit 67651ee5710c ("[libata] sata_sx4: convert to new exception
> handling methods"), which also did set .inherits function pointer to
> &ata_sff_port_ops (and ata_sff_port_ops itself has .inherits set to
> &ata_base_port_ops).
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/sata_sx4.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Thanks,
Jason

Re: [PATCH v2 8/8] ata: remove ata_bus_probe()

[Jason Yan]

On 2023/7/20 8:42, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Remove ata_bus_probe() as it is unused.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-core.c | 138 --------------------------------------
>   drivers/ata/libata.h      |   1 -
>   2 files changed, 139 deletions(-)

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Thanks,
Jason

Re: [PATCH v2 7/8] ata: sata_sx4: drop already completed TODO

[Hannes Reinecke]

On 7/20/23 02:42, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> The TODO claims that the pdc_20621_ops should set the .inherits
> function pointer to &ata_base_port_ops after it has been converted
> to use the new EH.
> 
> However, the driver was converted to use the new EH a long time ago,
> in commit 67651ee5710c ("[libata] sata_sx4: convert to new exception
> handling methods"), which also did set .inherits function pointer to
> &ata_sff_port_ops (and ata_sff_port_ops itself has .inherits set to
> &ata_base_port_ops).
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/sata_sx4.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c
> index ccc016072637..b51d7a9d0d90 100644
> --- a/drivers/ata/sata_sx4.c
> +++ b/drivers/ata/sata_sx4.c
> @@ -232,7 +232,6 @@ static const struct scsi_host_template pdc_sata_sht = {
>   	.dma_boundary		= ATA_DMA_BOUNDARY,
>   };
>   
> -/* TODO: inherit from base port_ops after converting to new EH */
>   static struct ata_port_operations pdc_20621_ops = {
>   	.inherits		= &ata_sff_port_ops,
>   
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v2 8/8] ata: remove ata_bus_probe()

[Hannes Reinecke]

On 7/20/23 02:42, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Remove ata_bus_probe() as it is unused.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-core.c | 138 --------------------------------------
>   drivers/ata/libata.h      |   1 -
>   2 files changed, 139 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v2 8/8] ata: remove ata_bus_probe()

[John Garry]

On 20/07/2023 01:42, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Remove ata_bus_probe() as it is unused.

Is there still a reference to this in Documentation? There seems to be.

> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-core.c | 138 --------------------------------------
>   drivers/ata/libata.h      |   1 -
>   2 files changed, 139 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index dedae669c9da..0af88ef231d1 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3057,144 +3057,6 @@ int ata_cable_sata(struct ata_port *ap)
>   }
>   EXPORT_SYMBOL_GPL(ata_cable_sata);
>   
> -/**
> - *	ata_bus_probe - Reset and probe ATA bus
> - *	@ap: Bus to probe
> - *
> - *	Master ATA bus probing function.  Initiates a hardware-dependent
> - *	bus reset, then attempts to identify any devices found on
> - *	the bus.
> - *
> - *	LOCKING:
> - *	PCI/etc. bus probe sem.
> - *
> - *	RETURNS:
> - *	Zero on success, negative errno otherwise.
> - */
> -
> -int ata_bus_probe(struct ata_port *ap)
> -{
> -	unsigned int classes[ATA_MAX_DEVICES];
> -	int tries[ATA_MAX_DEVICES];
> -	int rc;
> -	struct ata_device *dev;
> -
> -	ata_for_each_dev(dev, &ap->link, ALL)
> -		tries[dev->devno] = ATA_PROBE_MAX_TRIES;

ATA_PROBE_MAX_TRIES is no longer referenced, AFAICS

> -
> - retry:
> -	ata_for_each_dev(dev, &ap->link, ALL) {
> -		/* If we issue an SRST then an ATA drive (not ATAPI)
> -		 * may change configuration and be in PIO0 timing. If
> -		 * we do a hard reset (or are coming from power on)
> -		 * this is true for ATA or ATAPI. Until we've set a
> -		 * suitable controller mode we should not touch the
> -		 * bus as we may be talking too fast.
> -		 */
> -		dev->pio_mode = XFER_PIO_0;
> -		dev->dma_mode = 0xff;
> -
> -		/* If the controller has a pio mode setup function
> -		 * then use it to set the chipset to rights. Don't
> -		 * touch the DMA setup as that will be dealt with when
> -		 * configuring devices.
> -		 */
> -		if (ap->ops->set_piomode)
> -			ap->ops->set_piomode(ap, dev);
> -	}
> -
> -	/* reset and determine device classes */
> -	ap->ops->phy_reset(ap);

Does .phy_reset still have a user? If not, maybe all referenced symbols 
removed here can be checked for remaining references.

> -
> -	ata_for_each_dev(dev, &ap->link, ALL) {
> -		if (dev->class != ATA_DEV_UNKNOWN)
> -			classes[dev->devno] = dev->class;
> -		else
> -			classes[dev->devno] = ATA_DEV_NONE;
> -
> -		dev->class = ATA_DEV_UNKNOWN;
> -	}
> -
> -	/* read IDENTIFY page and configure devices. We have to do the identify
> -	   specific sequence bass-ackwards so that PDIAG- is released by
> -	   the slave device */
> -
> -	ata_for_each_dev(dev, &ap->link, ALL_REVERSE) {
> -		if (tries[dev->devno])
> -			dev->class = classes[dev->devno];
> -
> -		if (!ata_dev_enabled(dev))
> -			continue;
> -
> -		rc = ata_dev_read_id(dev, &dev->class, ATA_READID_POSTRESET,
> -				     dev->id);
> -		if (rc)
> -			goto fail;
> -	}
> -
> -	/* Now ask for the cable type as PDIAG- should have been released */
> -	if (ap->ops->cable_detect)
> -		ap->cbl = ap->ops->cable_detect(ap);
> -
> -	/* We may have SATA bridge glue hiding here irrespective of
> -	 * the reported cable types and sensed types.  When SATA
> -	 * drives indicate we have a bridge, we don't know which end
> -	 * of the link the bridge is which is a problem.
> -	 */
> -	ata_for_each_dev(dev, &ap->link, ENABLED)
> -		if (ata_id_is_sata(dev->id))
> -			ap->cbl = ATA_CBL_SATA;
> -
> -	/* After the identify sequence we can now set up the devices. We do
> -	   this in the normal order so that the user doesn't get confused */
> -
> -	ata_for_each_dev(dev, &ap->link, ENABLED) {
> -		ap->link.eh_context.i.flags |= ATA_EHI_PRINTINFO;
> -		rc = ata_dev_configure(dev);
> -		ap->link.eh_context.i.flags &= ~ATA_EHI_PRINTINFO;
> -		if (rc)
> -			goto fail;
> -	}
> -
> -	/* configure transfer mode */
> -	rc = ata_set_mode(&ap->link, &dev);
> -	if (rc)
> -		goto fail;
> -
> -	ata_for_each_dev(dev, &ap->link, ENABLED)
> -		return 0;
> -
> -	return -ENODEV;
> -
> - fail:
> -	tries[dev->devno]--;
> -
> -	switch (rc) {
> -	case -EINVAL:
> -		/* eeek, something went very wrong, give up */
> -		tries[dev->devno] = 0;
> -		break;
> -
> -	case -ENODEV:
> -		/* give it just one more chance */
> -		tries[dev->devno] = min(tries[dev->devno], 1);
> -		fallthrough;
> -	case -EIO:
> -		if (tries[dev->devno] == 1) {
> -			/* This is the last chance, better to slow
> -			 * down than lose it.
> -			 */
> -			sata_down_spd_limit(&ap->link, 0);
> -			ata_down_xfermask_limit(dev, ATA_DNXFER_PIO);
> -		}
> -	}
> -
> -	if (!tries[dev->devno])
> -		ata_dev_disable(dev);
> -
> -	goto retry;
> -}
> -
>   /**
>    *	sata_print_link_status - Print SATA link status
>    *	@link: SATA link to printk link status about
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 1ec9b4427b84..6e7d352803bd 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -122,7 +122,6 @@ extern void ata_scsi_media_change_notify(struct ata_device *dev);
>   extern void ata_scsi_hotplug(struct work_struct *work);
>   extern void ata_schedule_scsi_eh(struct Scsi_Host *shost);
>   extern void ata_scsi_dev_rescan(struct work_struct *work);
> -extern int ata_bus_probe(struct ata_port *ap);
>   extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
>   			      unsigned int id, u64 lun);
>   void ata_scsi_sdev_config(struct scsi_device *sdev);

Re: [PATCH v2 6/8] ata,scsi: cleanup ata_port_probe()

[John Garry]

On 20/07/2023 01:42, Niklas Cassel wrote:
> From: Hannes Reinecke<hare@suse.de>
> 
> Rename __ata_port_probe() to ata_port_probe() and drop the wrapper
> ata_sas_async_probe().


It is confusing that the previous patch is "inline ata_port_probe" - 
from that I would expect it to be gone, but here is it back again :)

> 
> Signed-off-by: Hannes Reinecke<hare@suse.de>
> Signed-off-by: Niklas Cassel<niklas.cassel@wdc.com>

Re: [PATCH v2 1/8] ata: remove reference to non-existing error_handler()

[John Garry]

On 20/07/2023 01:42, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all
> libata drivers now have the error_handler() callback provided,
> so we can stop checking for non-existing error_handler callback.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> [niklas: fixed review comments, rebased, solved conflicts during rebase,
> fixed bug that unconditionally dumped all QCs, removed the now unused
> function ata_dump_status(), removed the now unreachable failure paths in
> atapi_qc_complete(), removed the non-EH function to request ATAPI sense]
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

ata_qc_from_tag() still has a ap->ops->error_handler check, right?

> ---
>   drivers/ata/libata-core.c | 209 +++++++++++++++-----------------------
>   drivers/ata/libata-eh.c   | 150 ++++++++++++---------------
>   drivers/ata/libata-sata.c |   7 +-
>   drivers/ata/libata-scsi.c | 142 ++------------------------
>   drivers/ata/libata-sff.c  |  30 ++----
>   5 files changed, 166 insertions(+), 372 deletions(-)
> 
...


>   
>   	/*
> -	 * For new EH, all qcs are finished in one of three ways -
> +	 * For EH, all qcs are finished in one of three ways -
>   	 * 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
> @@ -659,94 +656,89 @@ EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
>   void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
>   {
>   	unsigned long flags;
> +	struct ata_link *link;
>   
>   	/* invoke error handler */

Is this comment only really relevant when we may not previously invoked 
the error handler?

> -	if (ap->ops->error_handler) {
> -		struct ata_link *link;
>   
> -		/* acquire EH ownership */
> -		ata_eh_acquire(ap);
> +	/* acquire EH ownership */
> +	ata_eh_acquire(ap);
>    repeat:
> -		/* kill fast drain timer */
> -		del_timer_sync(&ap->fastdrain_timer);
> +	/* kill fast drain timer */
> +	del_timer_sync(&ap->fastdrain_timer);
>   
> -		/* process port resume request */
> -		ata_eh_handle_port_resume(ap);
> +	/* process port resume request */
> +	ata_eh_handle_port_resume(ap);
>   
> -		/* fetch & clear EH info */
> -		spin_lock_irqsave(ap->lock, flags);
> +	/* fetch & clear EH info */
> +	spin_lock_irqsave(ap->lock, flags);

...

>    *	ata_to_sense_error - convert ATA error to SCSI error
>    *	@id: ATA device number
> @@ -904,7 +863,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>   	struct ata_taskfile *tf = &qc->result_tf;
>   	unsigned char *sb = cmd->sense_buffer;
>   	unsigned char *desc = sb + 8;
> -	int verbose = qc->ap->ops->error_handler == NULL;
>   	u8 sense_key, asc, ascq;
>   
>   	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> @@ -916,7 +874,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
>   	if (qc->err_mask ||
>   	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>   		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> -				   &sense_key, &asc, &ascq, verbose);
> +				   &sense_key, &asc, &ascq, false);
>   		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
>   	} else {
>   		/*
> @@ -999,7 +957,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>   	struct scsi_cmnd *cmd = qc->scsicmd;
>   	struct ata_taskfile *tf = &qc->result_tf;
>   	unsigned char *sb = cmd->sense_buffer;
> -	int verbose = qc->ap->ops->error_handler == NULL;
>   	u64 block;
>   	u8 sense_key, asc, ascq;
>   
> @@ -1017,7 +974,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
>   	if (qc->err_mask ||
>   	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
>   		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> -				   &sense_key, &asc, &ascq, verbose);
> +				   &sense_key, &asc, &ascq, false);

Please check this - AFAICS, we only ever pass false for @verbose arg now 
(so it would not be needed, and ata_to_sense_error() may be simplified)

>   		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
>   	} else {
>   		/* Could not decode error */
> @@ -1179,9 +1136,6 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
>   	unsigned long flags;
>   	struct ata_device *dev;
>   
> -	if (!ap->ops->error_handler)

Re: [PATCH v2 3/8] ata,scsi: remove ata_sas_port_destroy()

[John Garry]

On 20/07/2023 01:42, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Is now a wrapper around kfree(), so call it directly.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-sata.c          | 18 ------------------
>   drivers/scsi/libsas/sas_ata.c      |  2 +-
>   drivers/scsi/libsas/sas_discover.c |  2 +-
>   include/linux/libata.h             |  1 -
>   4 files changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index d3b595294eee..b5de0f40ea25 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1177,10 +1177,6 @@ EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
>   
>   int ata_sas_port_init(struct ata_port *ap)

This is a bit of a daft function now, considering it only does 
atomic_inc_return(&ata_print_id). Do we really need to export a symbol 
for that?

>   {
> -	int rc = ap->ops->port_start(ap);

I am not sure how this change is really relevant to " Is 
(ata_sas_port_destroy()) now a wrapper around kfree(), so call it directly."

> -
> -	if (rc)
> -		return rc;
>   	ap->print_id = atomic_inc_return(&ata_print_id);
>   	return 0;

always returns 0, so pretty pointless to return a value at all

>   }
> @@ -1198,20 +1194,6 @@ void ata_sas_tport_delete(struct ata_port *ap)
>   }
>   EXPORT_SYMBOL_GPL(ata_sas_tport_delete);
>   
> -/**
> - *	ata_sas_port_destroy - Destroy a SATA port allocated by ata_sas_port_alloc
> - *	@ap: SATA port to destroy
> - *
> - */
> -
> -void ata_sas_port_destroy(struct ata_port *ap)
> -{
> -	if (ap->ops->port_stop)
> -		ap->ops->port_stop(ap);
> -	kfree(ap);
> -}
> -EXPORT_SYMBOL_GPL(ata_sas_port_destroy);
> -
>   /**
>    *	ata_sas_slave_configure - Default slave_config routine for libata devices
>    *	@sdev: SCSI device to configure
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 7ead1f1be97f..a2eb9a2191c0 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -619,7 +619,7 @@ int sas_ata_init(struct domain_device *found_dev)
>   	return 0;
>   
>   destroy_port:
> -	ata_sas_port_destroy(ap);
> +	kfree(ap);
>   free_host:
>   	ata_host_put(ata_host);
>   	return rc;
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 8c6afe724944..07e18cdb85c7 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
>   
>   	if (dev_is_sata(dev) && dev->sata_dev.ap) {
>   		ata_sas_tport_delete(dev->sata_dev.ap);
> -		ata_sas_port_destroy(dev->sata_dev.ap);
> +		kfree(dev->sata_dev.ap);
>   		ata_host_put(dev->sata_dev.ata_host);
>   		dev->sata_dev.ata_host = NULL;
>   		dev->sata_dev.ap = NULL;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 9424c490ef0b..53cfb1a4b97a 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1238,7 +1238,6 @@ extern int sata_link_debounce(struct ata_link *link,
>   extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>   			     bool spm_wakeup);
>   extern int ata_slave_link_init(struct ata_port *ap);
> -extern void ata_sas_port_destroy(struct ata_port *);
>   extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
>   					   struct ata_port_info *, struct Scsi_Host *);
>   extern void ata_sas_async_probe(struct ata_port *ap);

Re: [PATCH v2 1/8] ata: remove reference to non-existing error_handler()

[Niklas Cassel]

On Thu, Jul 20, 2023 at 09:47:08AM +0100, John Garry wrote:
> On 20/07/2023 01:42, Niklas Cassel wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all
> > libata drivers now have the error_handler() callback provided,
> > so we can stop checking for non-existing error_handler callback.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > [niklas: fixed review comments, rebased, solved conflicts during rebase,
> > fixed bug that unconditionally dumped all QCs, removed the now unused
> > function ata_dump_status(), removed the now unreachable failure paths in
> > atapi_qc_complete(), removed the non-EH function to request ATAPI sense]
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Hello John,

thank you for your review!

> 
> ata_qc_from_tag() still has a ap->ops->error_handler check, right?

Correct, I will remove the check from ata_qc_from_tag() as well.


> 
> > ---
> >   drivers/ata/libata-core.c | 209 +++++++++++++++-----------------------
> >   drivers/ata/libata-eh.c   | 150 ++++++++++++---------------
> >   drivers/ata/libata-sata.c |   7 +-
> >   drivers/ata/libata-scsi.c | 142 ++------------------------
> >   drivers/ata/libata-sff.c  |  30 ++----
> >   5 files changed, 166 insertions(+), 372 deletions(-)
> > 
> ...
> 
> 
> >   	/*
> > -	 * For new EH, all qcs are finished in one of three ways -
> > +	 * For EH, all qcs are finished in one of three ways -
> >   	 * 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
> > @@ -659,94 +656,89 @@ EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
> >   void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap)
> >   {
> >   	unsigned long flags;
> > +	struct ata_link *link;
> >   	/* invoke error handler */
> 
> Is this comment only really relevant when we may not previously invoked the
> error handler?

I'm not sure what you mean. I will simply drop this comment.

Further down in this same function, we have:

/* invoke EH, skip if unloading or suspended */
       if (!(ap->pflags & (ATA_PFLAG_UNLOADING | ATA_PFLAG_SUSPENDED)))
               ap->ops->error_handler(ap);

So a comment at the start of the function as well feels redundant.


> 
> > -	if (ap->ops->error_handler) {
> > -		struct ata_link *link;
> > -		/* acquire EH ownership */
> > -		ata_eh_acquire(ap);
> > +	/* acquire EH ownership */
> > +	ata_eh_acquire(ap);
> >    repeat:
> > -		/* kill fast drain timer */
> > -		del_timer_sync(&ap->fastdrain_timer);
> > +	/* kill fast drain timer */
> > +	del_timer_sync(&ap->fastdrain_timer);
> > -		/* process port resume request */
> > -		ata_eh_handle_port_resume(ap);
> > +	/* process port resume request */
> > +	ata_eh_handle_port_resume(ap);
> > -		/* fetch & clear EH info */
> > -		spin_lock_irqsave(ap->lock, flags);
> > +	/* fetch & clear EH info */
> > +	spin_lock_irqsave(ap->lock, flags);
> 
> ...
> 
> >    *	ata_to_sense_error - convert ATA error to SCSI error
> >    *	@id: ATA device number
> > @@ -904,7 +863,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >   	struct ata_taskfile *tf = &qc->result_tf;
> >   	unsigned char *sb = cmd->sense_buffer;
> >   	unsigned char *desc = sb + 8;
> > -	int verbose = qc->ap->ops->error_handler == NULL;
> >   	u8 sense_key, asc, ascq;
> >   	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> > @@ -916,7 +874,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >   	if (qc->err_mask ||
> >   	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> >   		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> > -				   &sense_key, &asc, &ascq, verbose);
> > +				   &sense_key, &asc, &ascq, false);
> >   		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
> >   	} else {
> >   		/*
> > @@ -999,7 +957,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >   	struct scsi_cmnd *cmd = qc->scsicmd;
> >   	struct ata_taskfile *tf = &qc->result_tf;
> >   	unsigned char *sb = cmd->sense_buffer;
> > -	int verbose = qc->ap->ops->error_handler == NULL;
> >   	u64 block;
> >   	u8 sense_key, asc, ascq;
> > @@ -1017,7 +974,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >   	if (qc->err_mask ||
> >   	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> >   		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
> > -				   &sense_key, &asc, &ascq, verbose);
> > +				   &sense_key, &asc, &ascq, false);
> 
> Please check this - AFAICS, we only ever pass false for @verbose arg now (so
> it would not be needed, and ata_to_sense_error() may be simplified)

You are correct, I will remove the parameter.


Kind regards,
Niklas

> 
> >   		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
> >   	} else {
> >   		/* Could not decode error */
> > @@ -1179,9 +1136,6 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
> >   	unsigned long flags;
> >   	struct ata_device *dev;
> > -	if (!ap->ops->error_handler)
> 

Re: [PATCH v2 3/8] ata,scsi: remove ata_sas_port_destroy()

[Niklas Cassel]

On Thu, Jul 20, 2023 at 09:57:29AM +0100, John Garry wrote:
> On 20/07/2023 01:42, Niklas Cassel wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > Is now a wrapper around kfree(), so call it directly.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >   drivers/ata/libata-sata.c          | 18 ------------------
> >   drivers/scsi/libsas/sas_ata.c      |  2 +-
> >   drivers/scsi/libsas/sas_discover.c |  2 +-
> >   include/linux/libata.h             |  1 -
> >   4 files changed, 2 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index d3b595294eee..b5de0f40ea25 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -1177,10 +1177,6 @@ EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
> >   int ata_sas_port_init(struct ata_port *ap)
> 
> This is a bit of a daft function now, considering it only does
> atomic_inc_return(&ata_print_id). Do we really need to export a symbol for
> that?

$ git grep ata_print_id
drivers/ata/libata-core.c:atomic_t ata_print_id = ATOMIC_INIT(0);
drivers/ata/libata-core.c:              host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
drivers/ata/libata-sata.c:      ap->print_id = atomic_inc_return(&ata_print_id);
drivers/ata/libata.h:extern atomic_t ata_print_id;

It seems to be defined and used only in libata, while I agree that the function
is a bit silly, with my limited knowledge of how the linker works, moving it to
libsas seems a bit dangerous...

You can build libata as a module and libsas as built-in, and vice versa...

Also, since there are no direct users in libsas, I'd rather keep it in libata.


> 
> >   {
> > -	int rc = ap->ops->port_start(ap);
> 
> I am not sure how this change is really relevant to " Is
> (ata_sas_port_destroy()) now a wrapper around kfree(), so call it directly."

Agreed, I will move it to the previous patch, so we also avoid the null
pointer dereference in the previous patch.


> 
> > -
> > -	if (rc)
> > -		return rc;
> >   	ap->print_id = atomic_inc_return(&ata_print_id);
> >   	return 0;
> 
> always returns 0, so pretty pointless to return a value at all

Yes, that would be a small optimization, but I would consider such
a change outside the scope of this series.


Kind regards,
Niklas

> 
> >   }
> > @@ -1198,20 +1194,6 @@ void ata_sas_tport_delete(struct ata_port *ap)
> >   }
> >   EXPORT_SYMBOL_GPL(ata_sas_tport_delete);
> > -/**
> > - *	ata_sas_port_destroy - Destroy a SATA port allocated by ata_sas_port_alloc
> > - *	@ap: SATA port to destroy
> > - *
> > - */
> > -
> > -void ata_sas_port_destroy(struct ata_port *ap)
> > -{
> > -	if (ap->ops->port_stop)
> > -		ap->ops->port_stop(ap);
> > -	kfree(ap);
> > -}
> > -EXPORT_SYMBOL_GPL(ata_sas_port_destroy);
> > -
> >   /**
> >    *	ata_sas_slave_configure - Default slave_config routine for libata devices
> >    *	@sdev: SCSI device to configure
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index 7ead1f1be97f..a2eb9a2191c0 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -619,7 +619,7 @@ int sas_ata_init(struct domain_device *found_dev)
> >   	return 0;
> >   destroy_port:
> > -	ata_sas_port_destroy(ap);
> > +	kfree(ap);
> >   free_host:
> >   	ata_host_put(ata_host);
> >   	return rc;
> > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> > index 8c6afe724944..07e18cdb85c7 100644
> > --- a/drivers/scsi/libsas/sas_discover.c
> > +++ b/drivers/scsi/libsas/sas_discover.c
> > @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
> >   	if (dev_is_sata(dev) && dev->sata_dev.ap) {
> >   		ata_sas_tport_delete(dev->sata_dev.ap);
> > -		ata_sas_port_destroy(dev->sata_dev.ap);
> > +		kfree(dev->sata_dev.ap);
> >   		ata_host_put(dev->sata_dev.ata_host);
> >   		dev->sata_dev.ata_host = NULL;
> >   		dev->sata_dev.ap = NULL;
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 9424c490ef0b..53cfb1a4b97a 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1238,7 +1238,6 @@ extern int sata_link_debounce(struct ata_link *link,
> >   extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
> >   			     bool spm_wakeup);
> >   extern int ata_slave_link_init(struct ata_port *ap);
> > -extern void ata_sas_port_destroy(struct ata_port *);
> >   extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> >   					   struct ata_port_info *, struct Scsi_Host *);
> >   extern void ata_sas_async_probe(struct ata_port *ap);
> 

Re: [PATCH v2 6/8] ata,scsi: cleanup ata_port_probe()

[Niklas Cassel]

On Thu, Jul 20, 2023 at 09:27:51AM +0100, John Garry wrote:
> On 20/07/2023 01:42, Niklas Cassel wrote:
> > From: Hannes Reinecke<hare@suse.de>
> > 
> > Rename __ata_port_probe() to ata_port_probe() and drop the wrapper
> > ata_sas_async_probe().
> 
> 
> It is confusing that the previous patch is "inline ata_port_probe" - from
> that I would expect it to be gone, but here is it back again :)


Yes, I agree.
Remember, I'm not the author of most of these patches :)

I will change commit message from:
ata,scsi: cleanup ata_port_probe()
to
ata,scsi: cleanup __ata_port_probe()

I hope that is enough to address your comment.


Kind regards,
Niklas

Re: [PATCH v2 3/8] ata,scsi: remove ata_sas_port_destroy()

[John Garry]

On 21/07/2023 14:33, Niklas Cassel wrote:
> On Thu, Jul 20, 2023 at 09:57:29AM +0100, John Garry wrote:
>> On 20/07/2023 01:42, Niklas Cassel wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> Is now a wrapper around kfree(), so call it directly.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>    drivers/ata/libata-sata.c          | 18 ------------------
>>>    drivers/scsi/libsas/sas_ata.c      |  2 +-
>>>    drivers/scsi/libsas/sas_discover.c |  2 +-
>>>    include/linux/libata.h             |  1 -
>>>    4 files changed, 2 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>> index d3b595294eee..b5de0f40ea25 100644
>>> --- a/drivers/ata/libata-sata.c
>>> +++ b/drivers/ata/libata-sata.c
>>> @@ -1177,10 +1177,6 @@ EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
>>>    int ata_sas_port_init(struct ata_port *ap)
>>

Hi Niklas,

>> This is a bit of a daft function now, considering it only does
>> atomic_inc_return(&ata_print_id). Do we really need to export a symbol for
>> that?
> 
> $ git grep ata_print_id
> drivers/ata/libata-core.c:atomic_t ata_print_id = ATOMIC_INIT(0);
> drivers/ata/libata-core.c:              host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
> drivers/ata/libata-sata.c:      ap->print_id = atomic_inc_return(&ata_print_id);
> drivers/ata/libata.h:extern atomic_t ata_print_id;
> 
> It seems to be defined and used only in libata, while I agree that the function
> is a bit silly, with my limited knowledge of how the linker works, moving it to
> libsas seems a bit dangerous...
> 
> You can build libata as a module and libsas as built-in, and vice versa...
> 
> Also, since there are no direct users in libsas, I'd rather keep it in libata.

I wasn't really suggesting to move it to libsas - indeed, it is libata 
functionality.

Could you just put the ap->print_id = atomic_inc_return(&ata_print_id) 
call in ata_sas_port_alloc() (and remove ata_sas_port_init())? 
ata_sas_port_alloc() is only called from libsas, and ata_sas_port_init() 
is called straight after ata_sas_port_alloc() there. And 
ata_sas_port_alloc() is already doing ap init also (so setting 
ap->print_id would not be out of place there).

Thanks,
John