[PATCH 0/4] Remove link debounce delays by default

[PATCH 0/4] Remove link debounce delays by default

[Damien Le Moal]

This series removes the long link debounce delays by default for all
adapters, except for those known to require these delays
(e.g. ata_piix).

The first 2 patches are code cleanup improving the interface of several
functions handling delays.

Patch 3 removes the long delay in sata_link_resume() and reverses the
link flag ATA_LFLAG_NO_DEBOUNCE_DELAY to ATA_LFLAG_DEBOUNCE_DELAY for
adapters to request the delay if needed.

Patch 4 improves sata_link_debounce() by shortening the link stability
test, unless the link has the ATA_LFLAG_DEBOUNCE_DELAY flag set.

This series was tested on a machine with 2 AHCI adapters (Intel and
Marvell) with a port-multiplier box attached to cover that case too.

Comments and lots of testing are welcome !

Damien Le Moal (4):
  ata: libata-sata: Simplify sata_link_resume() interface
  ata: libata-sata: Introduce struct sata_deb_timing
  ata: libata-sata: Remove debounce delay by default
  ata: libata-sata: Improve sata_link_debounce()

 drivers/ata/ahci.c          |  19 ++----
 drivers/ata/ahci_brcm.c     |   1 -
 drivers/ata/ahci_qoriq.c    |   5 +-
 drivers/ata/ahci_xgene.c    |   3 +-
 drivers/ata/ata_generic.c   |   1 +
 drivers/ata/ata_piix.c      |  17 +++++
 drivers/ata/libahci.c       |   5 +-
 drivers/ata/libata-core.c   |   8 +--
 drivers/ata/libata-pmp.c    |   2 +-
 drivers/ata/libata-sata.c   | 129 +++++++++++++++++++++++++-----------
 drivers/ata/libata-sff.c    |   6 +-
 drivers/ata/sata_highbank.c |   4 +-
 drivers/ata/sata_inic162x.c |   3 +-
 drivers/ata/sata_mv.c       |   8 +--
 drivers/ata/sata_nv.c       |   5 +-
 drivers/ata/sata_sil24.c    |   2 +-
 include/linux/libata.h      |  44 +++++++-----
 17 files changed, 161 insertions(+), 101 deletions(-)

-- 
2.35.1

[PATCH 1/4] ata: libata-sata: Simplify sata_link_resume() interface

[Damien Le Moal]

When called directly outside of sata_link_hardreset(), the debounce
timing array passed to sata_link_resume() is always the array returned
by sata_ehc_deb_timing() for the target link. Based on this, the
interface of sata_link_resume() can be simplified by removing the
params argument. The timing array is infered locally using
sata_ehc_deb_timing().

To allow sata_link_hardreset() to specify a timing array, the helper
function __sata_link_resume() is introduced and sata_link_resume()
implemented using this helper.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/libata-core.c   |  3 +--
 drivers/ata/libata-sata.c   | 42 +++++++++++++++++++++----------------
 drivers/ata/sata_inic162x.c |  3 +--
 drivers/ata/sata_nv.c       |  3 +--
 include/linux/libata.h      |  4 +---
 5 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cceedde51126..1bdb6e78f0ed 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3575,7 +3575,6 @@ int ata_std_prereset(struct ata_link *link, unsigned long deadline)
 {
 	struct ata_port *ap = link->ap;
 	struct ata_eh_context *ehc = &link->eh_context;
-	const unsigned long *timing = sata_ehc_deb_timing(ehc);
 	int rc;
 
 	/* if we're about to do hardreset, nothing more to do */
@@ -3584,7 +3583,7 @@ int ata_std_prereset(struct ata_link *link, unsigned long deadline)
 
 	/* if SATA, resume link */
 	if (ap->flags & ATA_FLAG_SATA) {
-		rc = sata_link_resume(link, timing, deadline);
+		rc = sata_link_resume(link, deadline);
 		/* whine about phy resume failure but proceed */
 		if (rc && rc != -EOPNOTSUPP)
 			ata_link_warn(link,
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 044a16daa2d4..86f1475e5bca 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -279,22 +279,9 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 }
 EXPORT_SYMBOL_GPL(sata_link_debounce);
 
-/**
- *	sata_link_resume - resume SATA link
- *	@link: ATA link to resume SATA
- *	@params: timing parameters { interval, duration, timeout } in msec
- *	@deadline: deadline jiffies for the operation
- *
- *	Resume SATA phy @link and debounce it.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- *
- *	RETURNS:
- *	0 on success, -errno on failure.
- */
-int sata_link_resume(struct ata_link *link, const unsigned long *params,
-		     unsigned long deadline)
+static int __sata_link_resume(struct ata_link *link,
+			      const unsigned long *timing,
+			      unsigned long deadline)
 {
 	int tries = ATA_LINK_RESUME_TRIES;
 	u32 scontrol, serror;
@@ -335,7 +322,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
 		ata_link_warn(link, "link resume succeeded after %d retries\n",
 			      ATA_LINK_RESUME_TRIES - tries);
 
-	if ((rc = sata_link_debounce(link, params, deadline)))
+	if ((rc = sata_link_debounce(link, timing, deadline)))
 		return rc;
 
 	/* clear SError, some PHYs require this even for SRST to work */
@@ -344,6 +331,25 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
 
 	return rc != -EINVAL ? rc : 0;
 }
+
+/**
+ *	sata_link_resume - resume SATA link
+ *	@link: ATA link to resume SATA
+ *	@deadline: deadline jiffies for the operation
+ *
+ *	Resume SATA phy @link and debounce it.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, -errno on failure.
+ */
+int sata_link_resume(struct ata_link *link, unsigned long deadline)
+{
+	return __sata_link_resume(link,
+			sata_ehc_deb_timing(&link->eh_context), deadline);
+}
 EXPORT_SYMBOL_GPL(sata_link_resume);
 
 /**
@@ -568,7 +574,7 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 	ata_msleep(link->ap, 1);
 
 	/* bring link back */
-	rc = sata_link_resume(link, timing, deadline);
+	rc = __sata_link_resume(link, timing, deadline);
 	if (rc)
 		goto out;
 	/* if link is offline nothing more to do */
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 11e518f0111c..8d4d041cc724 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -621,7 +621,6 @@ static int inic_hardreset(struct ata_link *link, unsigned int *class,
 	struct ata_port *ap = link->ap;
 	void __iomem *port_base = inic_port_base(ap);
 	void __iomem *idma_ctl = port_base + PORT_IDMA_CTL;
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	int rc;
 
 	/* hammer it into sane state */
@@ -632,7 +631,7 @@ static int inic_hardreset(struct ata_link *link, unsigned int *class,
 	ata_msleep(ap, 1);
 	writew(0, idma_ctl);
 
-	rc = sata_link_resume(link, timing, deadline);
+	rc = sata_link_resume(link, deadline);
 	if (rc) {
 		ata_link_warn(link,
 			      "failed to resume link after reset (errno=%d)\n",
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 7f14d0d31057..b5f27eac86b1 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1529,7 +1529,6 @@ static int nv_hardreset(struct ata_link *link, unsigned int *class,
 		sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
 				    NULL, NULL);
 	else {
-		const unsigned long *timing = sata_ehc_deb_timing(ehc);
 		int rc;
 
 		if (!(ehc->i.flags & ATA_EHI_QUIET))
@@ -1537,7 +1536,7 @@ static int nv_hardreset(struct ata_link *link, unsigned int *class,
 				      "nv: skipping hardreset on occupied port\n");
 
 		/* make sure the link is online */
-		rc = sata_link_resume(link, timing, deadline);
+		rc = sata_link_resume(link, deadline);
 		/* whine about phy resume failure but proceed */
 		if (rc && rc != -EOPNOTSUPP)
 			ata_link_warn(link, "failed to resume link (errno=%d)\n",
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9b1d3d8b1252..e89d612326f6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1171,8 +1171,7 @@ extern int sata_set_spd(struct ata_link *link);
 extern int sata_link_hardreset(struct ata_link *link,
 			const unsigned long *timing, unsigned long deadline,
 			bool *online, int (*check_ready)(struct ata_link *));
-extern int sata_link_resume(struct ata_link *link, const unsigned long *params,
-			    unsigned long deadline);
+extern int sata_link_resume(struct ata_link *link, unsigned long deadline);
 extern void ata_eh_analyze_ncq_error(struct ata_link *link);
 #else
 static inline const unsigned long *
@@ -1205,7 +1204,6 @@ static inline int sata_link_hardreset(struct ata_link *link,
 	return -EOPNOTSUPP;
 }
 static inline int sata_link_resume(struct ata_link *link,
-				   const unsigned long *params,
 				   unsigned long deadline)
 {
 	return -EOPNOTSUPP;
-- 
2.35.1

[PATCH 2/4] ata: libata-sata: Introduce struct sata_deb_timing

[Damien Le Moal]

The SATA interval, duration and timeout debounce timing parameters
(sata_deb_timing_normal, sata_deb_timing_hotplug and
sata_deb_timing_long) are defined as an array of 3 unsigned long
integers. The entries are referenced directly without any index macro
indicating the name of the field being accessed.

Introduce struct sata_deb_timing to more clearly define the values and
their use. The interface of the sata_ehc_deb_timing(),
sata_link_hardreset() and sata_link_debounce() functions is modified to
take this new structure as argument.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/ahci.c          |  7 +++---
 drivers/ata/ahci_qoriq.c    |  5 ++--
 drivers/ata/ahci_xgene.c    |  3 ++-
 drivers/ata/libahci.c       |  5 ++--
 drivers/ata/libata-core.c   |  5 ++--
 drivers/ata/libata-pmp.c    |  2 +-
 drivers/ata/libata-sata.c   | 47 ++++++++++++++++++++++++++-----------
 drivers/ata/libata-sff.c    |  6 ++---
 drivers/ata/sata_highbank.c |  4 ++--
 drivers/ata/sata_mv.c       |  8 +++----
 drivers/ata/sata_nv.c       |  2 +-
 drivers/ata/sata_sil24.c    |  2 +-
 include/linux/libata.h      | 38 +++++++++++++++++++-----------
 13 files changed, 80 insertions(+), 54 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 84456c05e845..ccf94e8a3056 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -787,7 +787,6 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
 static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
 			      unsigned long deadline)
 {
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
@@ -811,8 +810,10 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
 		tf.status = ATA_BUSY;
 		ata_tf_to_fis(&tf, 0, 0, d2h_fis);
 
-		rc = sata_link_hardreset(link, timing, deadline, &online,
-				ahci_check_ready);
+		rc = sata_link_hardreset(link,
+					 sata_ehc_deb_timing(&link->eh_context),
+					 deadline, &online,
+					 ahci_check_ready);
 
 		if (sata_scr_read(link, SCR_STATUS, &sstatus) != 0 ||
 				(sstatus & 0xf) != 1)
diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c
index 6cd61842ad48..a5eeedadf0c9 100644
--- a/drivers/ata/ahci_qoriq.c
+++ b/drivers/ata/ahci_qoriq.c
@@ -90,7 +90,6 @@ MODULE_DEVICE_TABLE(acpi, ahci_qoriq_acpi_match);
 static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
 			  unsigned long deadline)
 {
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	void __iomem *port_mmio = ahci_port_base(link->ap);
 	u32 px_cmd, px_is, px_val;
 	struct ata_port *ap = link->ap;
@@ -126,8 +125,8 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
 	tf.status = ATA_BUSY;
 	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
 
-	rc = sata_link_hardreset(link, timing, deadline, &online,
-				 ahci_check_ready);
+	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
+				 deadline, &online, ahci_check_ready);
 
 	/* restore the PxCMD and PxIS on ls1021 */
 	if (ls1021a_workaround) {
diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index 7bb5db17f864..8d1598232e92 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -350,7 +350,8 @@ static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel)
 static int xgene_ahci_do_hardreset(struct ata_link *link,
 				   unsigned long deadline, bool *online)
 {
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
+	const struct sata_deb_timing *timing =
+		sata_ehc_deb_timing(&link->eh_context);
 	struct ata_port *ap = link->ap;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
 	struct xgene_ahci_context *ctx = hpriv->plat_data;
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index cf8c7fd59ada..0ac3b382fa52 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1549,7 +1549,6 @@ static int ahci_pmp_retry_softreset(struct ata_link *link, unsigned int *class,
 int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
 		      unsigned long deadline, bool *online)
 {
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
@@ -1564,8 +1563,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
 	tf.status = ATA_BUSY;
 	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
 
-	rc = sata_link_hardreset(link, timing, deadline, online,
-				 ahci_check_ready);
+	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
+				 deadline, online, ahci_check_ready);
 
 	hpriv->start_engine(ap);
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1bdb6e78f0ed..ffad7c1afb64 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3616,12 +3616,13 @@ EXPORT_SYMBOL_GPL(ata_std_prereset);
 int sata_std_hardreset(struct ata_link *link, unsigned int *class,
 		       unsigned long deadline)
 {
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	bool online;
 	int rc;
 
 	/* do hardreset */
-	rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
+	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
+				 deadline, &online, NULL);
+
 	return online ? -EAGAIN : rc;
 }
 EXPORT_SYMBOL_GPL(sata_std_hardreset);
diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
index e2e9cbd405fa..1ea472ddbe3f 100644
--- a/drivers/ata/libata-pmp.c
+++ b/drivers/ata/libata-pmp.c
@@ -851,7 +851,7 @@ static int sata_pmp_eh_handle_disabled_links(struct ata_port *ap)
 		/* Some PMPs require hardreset sequence to get
 		 * SError.N working.
 		 */
-		sata_link_hardreset(link, sata_deb_timing_normal,
+		sata_link_hardreset(link, &sata_deb_timing_normal,
 				ata_deadline(jiffies, ATA_TMOUT_INTERNAL_QUICK),
 				NULL, NULL);
 
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 86f1475e5bca..be46833d77a6 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -16,12 +16,31 @@
 #include "libata.h"
 #include "libata-transport.h"
 
-/* debounce timing parameters in msecs { interval, duration, timeout } */
-const unsigned long sata_deb_timing_normal[]		= {   5,  100, 2000 };
+/*
+ * Debounce timing parameters in msecs.
+ */
+const struct sata_deb_timing sata_deb_timing_normal =
+{
+	.interval	= 5,
+	.duration	= 100,
+	.timeout	= 2000
+};
 EXPORT_SYMBOL_GPL(sata_deb_timing_normal);
-const unsigned long sata_deb_timing_hotplug[]		= {  25,  500, 2000 };
+
+const struct sata_deb_timing sata_deb_timing_hotplug =
+{
+	.interval	= 25,
+	.duration	= 500,
+	.timeout	= 2000
+};
 EXPORT_SYMBOL_GPL(sata_deb_timing_hotplug);
-const unsigned long sata_deb_timing_long[]		= { 100, 2000, 5000 };
+
+const struct sata_deb_timing sata_deb_timing_long =
+{
+	.interval	= 100,
+	.duration	= 2000,
+	.timeout	= 5000
+};
 EXPORT_SYMBOL_GPL(sata_deb_timing_long);
 
 /**
@@ -211,7 +230,7 @@ EXPORT_SYMBOL_GPL(ata_tf_from_fis);
 /**
  *	sata_link_debounce - debounce SATA phy status
  *	@link: ATA link to debounce SATA phy status for
- *	@params: timing parameters { interval, duration, timeout } in msec
+ *	@timing: debounce timing
  *	@deadline: deadline jiffies for the operation
  *
  *	Make sure SStatus of @link reaches stable state, determined by
@@ -230,16 +249,15 @@ EXPORT_SYMBOL_GPL(ata_tf_from_fis);
  *	RETURNS:
  *	0 on success, -errno on failure.
  */
-int sata_link_debounce(struct ata_link *link, const unsigned long *params,
+int sata_link_debounce(struct ata_link *link,
+		       const struct sata_deb_timing *timing,
 		       unsigned long deadline)
 {
-	unsigned long interval = params[0];
-	unsigned long duration = params[1];
 	unsigned long last_jiffies, t;
 	u32 last, cur;
 	int rc;
 
-	t = ata_deadline(jiffies, params[2]);
+	t = ata_deadline(jiffies, timing->timeout);
 	if (time_before(t, deadline))
 		deadline = t;
 
@@ -251,7 +269,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 	last_jiffies = jiffies;
 
 	while (1) {
-		ata_msleep(link->ap, interval);
+		ata_msleep(link->ap, timing->interval);
 		if ((rc = sata_scr_read(link, SCR_STATUS, &cur)))
 			return rc;
 		cur &= 0xf;
@@ -261,7 +279,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 			if (cur == 1 && time_before(jiffies, deadline))
 				continue;
 			if (time_after(jiffies,
-				       ata_deadline(last_jiffies, duration)))
+				ata_deadline(last_jiffies, timing->duration)))
 				return 0;
 			continue;
 		}
@@ -280,7 +298,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
 EXPORT_SYMBOL_GPL(sata_link_debounce);
 
 static int __sata_link_resume(struct ata_link *link,
-			      const unsigned long *timing,
+			      const struct sata_deb_timing *timing,
 			      unsigned long deadline)
 {
 	int tries = ATA_LINK_RESUME_TRIES;
@@ -511,7 +529,7 @@ EXPORT_SYMBOL_GPL(sata_set_spd);
 /**
  *	sata_link_hardreset - reset link via SATA phy reset
  *	@link: link to reset
- *	@timing: timing parameters { interval, duration, timeout } in msec
+ *	@timing: debounce timing parameters
  *	@deadline: deadline jiffies for the operation
  *	@online: optional out parameter indicating link onlineness
  *	@check_ready: optional callback to check link readiness
@@ -532,7 +550,8 @@ EXPORT_SYMBOL_GPL(sata_set_spd);
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
+int sata_link_hardreset(struct ata_link *link,
+			const struct sata_deb_timing *timing,
 			unsigned long deadline,
 			bool *online, int (*check_ready)(struct ata_link *))
 {
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index b3be7a8f5bea..ffd085b09abc 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2030,13 +2030,11 @@ EXPORT_SYMBOL_GPL(ata_sff_softreset);
 int sata_sff_hardreset(struct ata_link *link, unsigned int *class,
 		       unsigned long deadline)
 {
-	struct ata_eh_context *ehc = &link->eh_context;
-	const unsigned long *timing = sata_ehc_deb_timing(ehc);
 	bool online;
 	int rc;
 
-	rc = sata_link_hardreset(link, timing, deadline, &online,
-				 ata_sff_check_ready);
+	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
+				 deadline, &online, ata_sff_check_ready);
 	if (online)
 		*class = ata_sff_dev_classify(link->device, 1, NULL);
 
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index dfbf9493e451..16da571e8083 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -385,7 +385,7 @@ static int highbank_initialize_phys(struct device *dev, void __iomem *addr)
 static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 				unsigned long deadline)
 {
-	static const unsigned long timing[] = { 5, 100, 500};
+	const struct sata_deb_timing timing = { 5, 100, 500};
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
@@ -405,7 +405,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 
 	do {
 		highbank_cphy_disable_overrides(link->ap->port_no);
-		rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
+		rc = sata_link_hardreset(link, &timing, deadline, &online, NULL);
 		highbank_cphy_override_lane(link->ap->port_no);
 
 		/* If the status is 1, we are connected, but the link did not
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index de5bd02cad44..8ad0f3776c48 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -3633,11 +3633,9 @@ static int mv_hardreset(struct ata_link *link, unsigned int *class,
 
 	/* Workaround for errata FEr SATA#10 (part 2) */
 	do {
-		const unsigned long *timing =
-				sata_ehc_deb_timing(&link->eh_context);
-
-		rc = sata_link_hardreset(link, timing, deadline + extra,
-					 &online, NULL);
+		rc = sata_link_hardreset(link,
+					 sata_ehc_deb_timing(&link->eh_context),
+					 deadline + extra, &online, NULL);
 		rc = online ? -EAGAIN : rc;
 		if (rc)
 			return rc;
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index b5f27eac86b1..5c8db8f8c47f 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1526,7 +1526,7 @@ static int nv_hardreset(struct ata_link *link, unsigned int *class,
 	 */
 	if (!(link->ap->pflags & ATA_PFLAG_LOADING) &&
 	    !ata_dev_enabled(link->device))
-		sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
+		sata_link_hardreset(link, &sata_deb_timing_hotplug, deadline,
 				    NULL, NULL);
 	else {
 		int rc;
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 2fef6ce93f07..4c4ff67bf06a 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -735,7 +735,7 @@ static int sil24_hardreset(struct ata_link *link, unsigned int *class,
 	/* SStatus oscillates between zero and valid status after
 	 * DEV_RST, debounce it.
 	 */
-	rc = sata_link_debounce(link, sata_deb_timing_long, deadline);
+	rc = sata_link_debounce(link, &sata_deb_timing_long, deadline);
 	if (rc) {
 		reason = "PHY debouncing failed";
 		goto err;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e89d612326f6..166263d9bbc7 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1150,17 +1150,26 @@ extern void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *
  * SATA specific code - drivers/ata/libata-sata.c
  */
 #ifdef CONFIG_SATA_HOST
-extern const unsigned long sata_deb_timing_normal[];
-extern const unsigned long sata_deb_timing_hotplug[];
-extern const unsigned long sata_deb_timing_long[];
 
-static inline const unsigned long *
+/*
+ * Debounce timing parameters in msecs.
+ */
+struct sata_deb_timing {
+	unsigned long interval;
+	unsigned long duration;
+	unsigned long timeout;
+};
+
+extern const struct sata_deb_timing sata_deb_timing_normal;
+extern const struct sata_deb_timing sata_deb_timing_hotplug;
+extern const struct sata_deb_timing sata_deb_timing_long;
+
+static inline const struct sata_deb_timing *
 sata_ehc_deb_timing(struct ata_eh_context *ehc)
 {
 	if (ehc->i.flags & ATA_EHI_HOTPLUGGED)
-		return sata_deb_timing_hotplug;
-	else
-		return sata_deb_timing_normal;
+		return &sata_deb_timing_hotplug;
+	return &sata_deb_timing_normal;
 }
 
 extern int sata_scr_valid(struct ata_link *link);
@@ -1169,12 +1178,13 @@ extern int sata_scr_write(struct ata_link *link, int reg, u32 val);
 extern int sata_scr_write_flush(struct ata_link *link, int reg, u32 val);
 extern int sata_set_spd(struct ata_link *link);
 extern int sata_link_hardreset(struct ata_link *link,
-			const unsigned long *timing, unsigned long deadline,
-			bool *online, int (*check_ready)(struct ata_link *));
+			       const struct sata_deb_timing *timing,
+			       unsigned long deadline, bool *online,
+			       int (*check_ready)(struct ata_link *));
 extern int sata_link_resume(struct ata_link *link, unsigned long deadline);
 extern void ata_eh_analyze_ncq_error(struct ata_link *link);
 #else
-static inline const unsigned long *
+static inline const struct sata_deb_timing *
 sata_ehc_deb_timing(struct ata_eh_context *ehc)
 {
 	return NULL;
@@ -1194,9 +1204,8 @@ static inline int sata_scr_write_flush(struct ata_link *link, int reg, u32 val)
 }
 static inline int sata_set_spd(struct ata_link *link) { return -EOPNOTSUPP; }
 static inline int sata_link_hardreset(struct ata_link *link,
-				      const unsigned long *timing,
-				      unsigned long deadline,
-				      bool *online,
+				      const struct sata_deb_timing *timing,
+				      unsigned long deadline, bool *online,
 				      int (*check_ready)(struct ata_link *))
 {
 	if (online)
@@ -1211,7 +1220,8 @@ static inline int sata_link_resume(struct ata_link *link,
 static inline void ata_eh_analyze_ncq_error(struct ata_link *link) { }
 #endif
 extern int sata_link_debounce(struct ata_link *link,
-			const unsigned long *params, unsigned long deadline);
+			      const struct sata_deb_timing *timing,
+			      unsigned long deadline);
 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);
-- 
2.35.1

[PATCH 3/4] ata: libata-sata: Remove debounce delay by default

[Damien Le Moal]

Many adapters and SATA controllers are fine with accesses to the
SControl register right after resuming. For these adapters, the
additional 200ms delay implemented in sata_link_resume() is redundant
and unnecessarily increase boot time.

Drivers can opt out from this additional delay using the
ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, but by default, this additional
delay exists for all adapters.

Reverse the situation and do not add by default this 200ms delay. For
adapters that actually need this delay, rename the
ATA_LFLAG_NO_DEBOUNCE_DELAY link flag to ATA_LFLAG_DEBOUNCE_DELAY and
execute the 200ms delay in sata_link_resume() only if a driver request
it using the new link flag. Otherwise, arbitrarily delay by 1ms only.

Since ata_piix adapters are known to require the longer delay, specify
the ATA_LFLAG_DEBOUNCE_DELAY for all adapters supported by the ata_piix
driver.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/ahci.c        | 12 ++----------
 drivers/ata/ahci_brcm.c   |  1 -
 drivers/ata/ata_generic.c |  1 +
 drivers/ata/ata_piix.c    | 17 +++++++++++++++++
 drivers/ata/libata-sata.c | 18 +++++++++++-------
 include/linux/libata.h    |  2 +-
 6 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ccf94e8a3056..ac1e227a07cd 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -51,7 +51,6 @@ enum board_ids {
 	board_ahci,
 	board_ahci_ign_iferr,
 	board_ahci_low_power,
-	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
 	board_ahci_nosntf,
@@ -142,13 +141,6 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_no_debounce_delay] = {
-		.flags		= AHCI_FLAG_COMMON,
-		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
-		.pio_mask	= ATA_PIO4,
-		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_ops,
-	},
 	[board_ahci_nomsi] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
 		.flags		= AHCI_FLAG_COMMON,
@@ -445,7 +437,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 		board_ahci_al },
 	/* AMD */
 	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
-	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /* AMD Hudson-2 (AHCI mode) */
+	{ PCI_VDEVICE(AMD, 0x7801), board_ahci }, /* AMD Hudson-2 (AHCI mode) */
 	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
 	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_low_power }, /* AMD Green Sardine */
 	/* AMD is using RAID class only for ahci controllers */
@@ -583,7 +575,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL_EXT, 0x9230),
 	  .driver_data = board_ahci_yes_fbs },
 	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL_EXT, 0x9235),
-	  .driver_data = board_ahci_no_debounce_delay },
+	  .driver_data = board_ahci },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TTI, 0x0642), /* highpoint rocketraid 642L */
 	  .driver_data = board_ahci_yes_fbs },
 	{ PCI_DEVICE(PCI_VENDOR_ID_TTI, 0x0645), /* highpoint rocketraid 644L */
diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index ab8552b1ff2a..e4584aed0ded 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -333,7 +333,6 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
 
 static const struct ata_port_info ahci_brcm_port_info = {
 	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
-	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
 	.pio_mask	= ATA_PIO4,
 	.udma_mask	= ATA_UDMA6,
 	.port_ops	= &ahci_brcm_platform_ops,
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index 20a32e4d501d..f02c824b26e6 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -165,6 +165,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
 	u16 command;
 	static const struct ata_port_info info = {
 		.flags = ATA_FLAG_SLAVE_POSS,
+		.link_flags = ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask = ATA_PIO4,
 		.mwdma_mask = ATA_MWDMA2,
 		.udma_mask = ATA_UDMA5,
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index ade5e894563b..dcfcfb5d8a05 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -1112,6 +1112,7 @@ static struct ata_port_info piix_port_info[] = {
 	[piix_pata_mwdma] =	/* PIIX3 MWDMA only */
 	{
 		.flags		= PIIX_PATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
 		.port_ops	= &piix_pata_ops,
@@ -1120,6 +1121,7 @@ static struct ata_port_info piix_port_info[] = {
 	[piix_pata_33] =	/* PIIX4 at 33MHz */
 	{
 		.flags		= PIIX_PATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
 		.udma_mask	= ATA_UDMA2,
@@ -1129,6 +1131,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich_pata_33] =		/* ICH0 - ICH at 33Mhz*/
 	{
 		.flags		= PIIX_PATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY, /* Check: maybe MWDMA0 is ok  */
 		.udma_mask	= ATA_UDMA2,
@@ -1138,6 +1141,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich_pata_66] =		/* ICH controllers up to 66MHz */
 	{
 		.flags		= PIIX_PATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY, /* MWDMA0 is broken on chip */
 		.udma_mask	= ATA_UDMA4,
@@ -1147,6 +1151,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich_pata_100] =
 	{
 		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY,
 		.udma_mask	= ATA_UDMA5,
@@ -1156,6 +1161,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich_pata_100_nomwdma1] =
 	{
 		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2_ONLY,
 		.udma_mask	= ATA_UDMA5,
@@ -1165,6 +1171,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich5_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1174,6 +1181,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich6_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1183,6 +1191,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich6m_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1192,6 +1201,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich8_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1201,6 +1211,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich8_2port_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1210,6 +1221,7 @@ static struct ata_port_info piix_port_info[] = {
 	[tolapai_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1219,6 +1231,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich8m_apple_sata] =
 	{
 		.flags		= PIIX_SATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1228,6 +1241,7 @@ static struct ata_port_info piix_port_info[] = {
 	[piix_pata_vmw] =
 	{
 		.flags		= PIIX_PATA_FLAGS,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA12_ONLY, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
 		.udma_mask	= ATA_UDMA2,
@@ -1241,6 +1255,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich8_sata_snb] =
 	{
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR | PIIX_FLAG_PIO16,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1251,6 +1266,7 @@ static struct ata_port_info piix_port_info[] = {
 	{
 		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR
 					| PIIX_FLAG_PIO16,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA6,
@@ -1260,6 +1276,7 @@ static struct ata_port_info piix_port_info[] = {
 	[ich8_2port_sata_byt] =
 	{
 		.flags          = PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR | PIIX_FLAG_PIO16,
+		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
 		.pio_mask       = ATA_PIO4,
 		.mwdma_mask     = ATA_MWDMA2,
 		.udma_mask      = ATA_UDMA6,
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index be46833d77a6..87ad03c2e49f 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -302,9 +302,18 @@ static int __sata_link_resume(struct ata_link *link,
 			      unsigned long deadline)
 {
 	int tries = ATA_LINK_RESUME_TRIES;
+	unsigned int db_delay = 1;
 	u32 scontrol, serror;
 	int rc;
 
+	/*
+	 * Some PHYs react badly if SControl is pounded immediately after
+	 * resuming. For drivers requesting it, delay 200ms before debouncing.
+	 * Otherwise, only delay by 1ms (arbitrary delay).
+	 */
+	if (link->flags & ATA_LFLAG_DEBOUNCE_DELAY)
+		db_delay = 200;
+
 	if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
 		return rc;
 
@@ -317,13 +326,8 @@ static int __sata_link_resume(struct ata_link *link,
 		scontrol = (scontrol & 0x0f0) | 0x300;
 		if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))
 			return rc;
-		/*
-		 * Some PHYs react badly if SStatus is pounded
-		 * immediately after resuming.  Delay 200ms before
-		 * debouncing.
-		 */
-		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
-			ata_msleep(link->ap, 200);
+
+		ata_msleep(link->ap, db_delay);
 
 		/* is SControl restored correctly? */
 		if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 166263d9bbc7..cc3a8e9c78b0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -143,7 +143,7 @@ enum {
 	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
 	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
 	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
-	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
+	ATA_LFLAG_DEBOUNCE_DELAY = (1 << 11), /* Debounce delay on link resume */
 
 	/* struct ata_port flags */
 	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */
-- 
2.35.1

[PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

[Damien Le Moal]

sata_link_debounce() polls the SStatus register DET field to ensure that
a stable value is provided, to reliably detect device presence and PHY
readiness. Polling is done for at least timing->duration if there is no
device detected. For the device detected case, polling last up to
deadline to ensure that the PHY becomes ready.

However, the PHY ready state is actually never checked, leading to the
poll loop duration always reaching the maximum duration.

For adapters that do not require a debounce delay (link flag
ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates
device present *and* PHY ready and bail out of the polling loop if it
does.

While at it, add comments to clarify the various checks in
sata_link_debounce() polling loop.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 87ad03c2e49f..15423723c9dd 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -276,8 +276,27 @@ int sata_link_debounce(struct ata_link *link,
 
 		/* DET stable? */
 		if (cur == last) {
+			/*
+			 * If the device presence was detected but PHY
+			 * communication is not yet established, wait until
+			 * deadline.
+			 */
 			if (cur == 1 && time_before(jiffies, deadline))
 				continue;
+
+			/*
+			 * If PHY is ready and the device is present, and the
+			 * driver did not request debounce delay, bail out early
+			 * assuming that the link is stable.
+			 */
+			if (cur == 3 &&
+			    !(link->flags & ATA_LFLAG_DEBOUNCE_DELAY))
+				return 0;
+
+			/*
+			 * If DET value has remained stable for
+			 * timing->duration, bail out.
+			 */
 			if (time_after(jiffies,
 				ata_deadline(last_jiffies, timing->duration)))
 				return 0;
@@ -288,8 +307,9 @@ int sata_link_debounce(struct ata_link *link,
 		last = cur;
 		last_jiffies = jiffies;
 
-		/* Check deadline.  If debouncing failed, return
-		 * -EPIPE to tell upper layer to lower link speed.
+		/*
+		 * If debouncing has not succeeded before dealine, return
+		 * -EPIPE to tell the upper layer to lower the link speed.
 		 */
 		if (time_after(jiffies, deadline))
 			return -EPIPE;
-- 
2.35.1

Re: [PATCH 0/4] Remove link debounce delays by default

[Paul Menzel]

Dear Damien,


Thank you for sending these patches.

Am 23.03.22 um 09:17 schrieb Damien Le Moal:
> This series removes the long link debounce delays by default for all
> adapters, except for those known to require these delays
> (e.g. ata_piix).

Is it know, or just a theory?

> The first 2 patches are code cleanup improving the interface of several
> functions handling delays.
> 
> Patch 3 removes the long delay in sata_link_resume() and reverses the
> link flag ATA_LFLAG_NO_DEBOUNCE_DELAY to ATA_LFLAG_DEBOUNCE_DELAY for
> adapters to request the delay if needed.
> 
> Patch 4 improves sata_link_debounce() by shortening the link stability
> test, unless the link has the ATA_LFLAG_DEBOUNCE_DELAY flag set.
> 
> This series was tested on a machine with 2 AHCI adapters (Intel and
> Marvell) with a port-multiplier box attached to cover that case too.

It’d be great if you gave an example benchmark.

> Comments and lots of testing are welcome !
> 
> Damien Le Moal (4):
>    ata: libata-sata: Simplify sata_link_resume() interface
>    ata: libata-sata: Introduce struct sata_deb_timing
>    ata: libata-sata: Remove debounce delay by default
>    ata: libata-sata: Improve sata_link_debounce()

[…]

I am wondering how sure we can be, there won’t be any regressions? Not 
having the boot disk detected is quite a serious issue/regression, and 
it should be made easy for users to fix that without having to rebuild 
the Linux kernel. A Linux kernel CLI parameter to enable the delay would 
be helpful for effected users.


Kind regards,

Paul

Re: [PATCH 0/4] Remove link debounce delays by default

[Paul Menzel]

[Use Mario’s current address]

Am 23.03.22 um 09:43 schrieb Paul Menzel:
> Dear Damien,
> 
> 
> Thank you for sending these patches.
> 
> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>> This series removes the long link debounce delays by default for all
>> adapters, except for those known to require these delays
>> (e.g. ata_piix).
> 
> Is it know, or just a theory?
> 
>> The first 2 patches are code cleanup improving the interface of several
>> functions handling delays.
>>
>> Patch 3 removes the long delay in sata_link_resume() and reverses the
>> link flag ATA_LFLAG_NO_DEBOUNCE_DELAY to ATA_LFLAG_DEBOUNCE_DELAY for
>> adapters to request the delay if needed.
>>
>> Patch 4 improves sata_link_debounce() by shortening the link stability
>> test, unless the link has the ATA_LFLAG_DEBOUNCE_DELAY flag set.
>>
>> This series was tested on a machine with 2 AHCI adapters (Intel and
>> Marvell) with a port-multiplier box attached to cover that case too.
> 
> It’d be great if you gave an example benchmark.
> 
>> Comments and lots of testing are welcome !
>>
>> Damien Le Moal (4):
>>    ata: libata-sata: Simplify sata_link_resume() interface
>>    ata: libata-sata: Introduce struct sata_deb_timing
>>    ata: libata-sata: Remove debounce delay by default
>>    ata: libata-sata: Improve sata_link_debounce()
> 
> […]
> 
> I am wondering how sure we can be, there won’t be any regressions? Not 
> having the boot disk detected is quite a serious issue/regression, and 
> it should be made easy for users to fix that without having to rebuild 
> the Linux kernel. A Linux kernel CLI parameter to enable the delay would 
> be helpful for effected users.
> 
> 
> Kind regards,
> 
> Paul

Re: [PATCH 0/4] Remove link debounce delays by default

[Damien Le Moal]

On 3/23/22 17:43, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Thank you for sending these patches.
> 
> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>> This series removes the long link debounce delays by default for all
>> adapters, except for those known to require these delays
>> (e.g. ata_piix).
> 
> Is it know, or just a theory?
> 
>> The first 2 patches are code cleanup improving the interface of several
>> functions handling delays.
>>
>> Patch 3 removes the long delay in sata_link_resume() and reverses the
>> link flag ATA_LFLAG_NO_DEBOUNCE_DELAY to ATA_LFLAG_DEBOUNCE_DELAY for
>> adapters to request the delay if needed.
>>
>> Patch 4 improves sata_link_debounce() by shortening the link stability
>> test, unless the link has the ATA_LFLAG_DEBOUNCE_DELAY flag set.
>>
>> This series was tested on a machine with 2 AHCI adapters (Intel and
>> Marvell) with a port-multiplier box attached to cover that case too.
> 
> It’d be great if you gave an example benchmark.

No need for a benchmark. This is not hot path stuff. This code runs only
during device discovery on boot and on resume after suspend.
So apply the patches and reboot, check dmesg if you see errors or not and
if your disks are all there. Same after a suspend+resume.

> 
>> Comments and lots of testing are welcome !
>>
>> Damien Le Moal (4):
>>    ata: libata-sata: Simplify sata_link_resume() interface
>>    ata: libata-sata: Introduce struct sata_deb_timing
>>    ata: libata-sata: Remove debounce delay by default
>>    ata: libata-sata: Improve sata_link_debounce()
> 
> […]
> 
> I am wondering how sure we can be, there won’t be any regressions? Not 
> having the boot disk detected is quite a serious issue/regression, and 
> it should be made easy for users to fix that without having to rebuild 
> the Linux kernel. A Linux kernel CLI parameter to enable the delay would 
> be helpful for effected users.
> 
> 
> Kind regards,
> 
> Paul


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] Remove link debounce delays by default

[Damien Le Moal]

On 3/23/22 17:43, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Thank you for sending these patches.
> 
> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>> This series removes the long link debounce delays by default for all
>> adapters, except for those known to require these delays
>> (e.g. ata_piix).
> 
> Is it know, or just a theory?
> 
>> The first 2 patches are code cleanup improving the interface of several
>> functions handling delays.
>>
>> Patch 3 removes the long delay in sata_link_resume() and reverses the
>> link flag ATA_LFLAG_NO_DEBOUNCE_DELAY to ATA_LFLAG_DEBOUNCE_DELAY for
>> adapters to request the delay if needed.
>>
>> Patch 4 improves sata_link_debounce() by shortening the link stability
>> test, unless the link has the ATA_LFLAG_DEBOUNCE_DELAY flag set.
>>
>> This series was tested on a machine with 2 AHCI adapters (Intel and
>> Marvell) with a port-multiplier box attached to cover that case too.
> 
> It’d be great if you gave an example benchmark.
> 
>> Comments and lots of testing are welcome !
>>
>> Damien Le Moal (4):
>>    ata: libata-sata: Simplify sata_link_resume() interface
>>    ata: libata-sata: Introduce struct sata_deb_timing
>>    ata: libata-sata: Remove debounce delay by default
>>    ata: libata-sata: Improve sata_link_debounce()
> 
> […]
> 
> I am wondering how sure we can be, there won’t be any regressions? Not 
> having the boot disk detected is quite a serious issue/regression, and 
> it should be made easy for users to fix that without having to rebuild 
> the Linux kernel. A Linux kernel CLI parameter to enable the delay would 
> be helpful for effected users.

I am working on another series for that. The patches will allow
controlling most horkage and link flags on/off using libata.force kernel
boot parameter. That will allow figuring out problems without patching in
the field, for patches to be later added.

I do not think we need to have the flexibility of changing the debounce
delay. The 200ms delay is already arbitrary. There is no reliable way of
figuring out the ideal short delay. So havin "on" (default patch applied),
and "off" (delay added like before) is enough I think.

> 
> 
> Kind regards,
> 
> Paul


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] Remove link debounce delays by default

[Paul Menzel]

[collapsing both of your replies]


Dear Damien,


Am 23.03.22 um 10:39 schrieb Damien Le Moal:
> On 3/23/22 17:43, Paul Menzel wrote:

>> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>>> This series removes the long link debounce delays by default for all
>>> adapters, except for those known to require these delays
>>> (e.g. ata_piix).
>>
>> Is it know, or just a theory?
>>
>>> The first 2 patches are code cleanup improving the interface of several
>>> functions handling delays.
>>>
>>> Patch 3 removes the long delay in sata_link_resume() and reverses the
>>> link flag ATA_LFLAG_NO_DEBOUNCE_DELAY to ATA_LFLAG_DEBOUNCE_DELAY for
>>> adapters to request the delay if needed.
>>>
>>> Patch 4 improves sata_link_debounce() by shortening the link stability
>>> test, unless the link has the ATA_LFLAG_DEBOUNCE_DELAY flag set.
>>>
>>> This series was tested on a machine with 2 AHCI adapters (Intel and
>>> Marvell) with a port-multiplier box attached to cover that case too.
>>
>> It’d be great if you gave an example benchmark.
> 
> No need for a benchmark. This is not hot path stuff. This code runs only
> during device discovery on boot and on resume after suspend.
> So apply the patches and reboot, check dmesg if you see errors or not and
> if your disks are all there. Same after a suspend+resume.

Yes, I know what I need to check. I meant, that you write without this 
patchset my system with HBA controller X and 32 [vendor/model] disks 
attached reaches the message Y in Z1 seconds, and with the series Z2 
seconds.

>>> Comments and lots of testing are welcome !
>>>
>>> Damien Le Moal (4):
>>>     ata: libata-sata: Simplify sata_link_resume() interface
>>>     ata: libata-sata: Introduce struct sata_deb_timing
>>>     ata: libata-sata: Remove debounce delay by default
>>>     ata: libata-sata: Improve sata_link_debounce()
>>
>> […]
>>
>> I am wondering how sure we can be, there won’t be any regressions? Not
>> having the boot disk detected is quite a serious issue/regression, and
>> it should be made easy for users to fix that without having to rebuild
>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>> be helpful for effected users.
> 
> I am working on another series for that. The patches will allow
> controlling most horkage and link flags on/off using libata.force kernel
> boot parameter. That will allow figuring out problems without patching in
> the field, for patches to be later added.

Sounds good. But this needs to be available before the changes at hand, 
doesn’t it?

> I do not think we need to have the flexibility of changing the debounce
> delay. The 200ms delay is already arbitrary. There is no reliable way of
> figuring out the ideal short delay. So havin "on" (default patch applied),
> and "off" (delay added like before) is enough I think.

Yes, I was only talking about on/off. (You probably thought, I was 
referring to the mechanism I tried to implement in my RFC series.)


Kind regards,

Paul

Re: [PATCH 2/4] ata: libata-sata: Introduce struct sata_deb_timing

[Paul Menzel]

Dear Damien,


Am 23.03.22 um 09:17 schrieb Damien Le Moal:
> The SATA interval, duration and timeout debounce timing parameters
> (sata_deb_timing_normal, sata_deb_timing_hotplug and
> sata_deb_timing_long) are defined as an array of 3 unsigned long
> integers. The entries are referenced directly without any index macro
> indicating the name of the field being accessed.

> Introduce struct sata_deb_timing to more clearly define the values and
> their use. The interface of the sata_ehc_deb_timing(),
> sata_link_hardreset() and sata_link_debounce() functions is modified to
> take this new structure as argument.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/ata/ahci.c          |  7 +++---
>   drivers/ata/ahci_qoriq.c    |  5 ++--
>   drivers/ata/ahci_xgene.c    |  3 ++-
>   drivers/ata/libahci.c       |  5 ++--
>   drivers/ata/libata-core.c   |  5 ++--
>   drivers/ata/libata-pmp.c    |  2 +-
>   drivers/ata/libata-sata.c   | 47 ++++++++++++++++++++++++++-----------
>   drivers/ata/libata-sff.c    |  6 ++---
>   drivers/ata/sata_highbank.c |  4 ++--
>   drivers/ata/sata_mv.c       |  8 +++----
>   drivers/ata/sata_nv.c       |  2 +-
>   drivers/ata/sata_sil24.c    |  2 +-
>   include/linux/libata.h      | 38 +++++++++++++++++++-----------
>   13 files changed, 80 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 84456c05e845..ccf94e8a3056 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -787,7 +787,6 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
>   static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
>   			      unsigned long deadline)
>   {
> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>   	struct ata_port *ap = link->ap;
>   	struct ahci_port_priv *pp = ap->private_data;
>   	struct ahci_host_priv *hpriv = ap->host->private_data;
> @@ -811,8 +810,10 @@ static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
>   		tf.status = ATA_BUSY;
>   		ata_tf_to_fis(&tf, 0, 0, d2h_fis);
>   
> -		rc = sata_link_hardreset(link, timing, deadline, &online,
> -				ahci_check_ready);
> +		rc = sata_link_hardreset(link,
> +					 sata_ehc_deb_timing(&link->eh_context),
> +					 deadline, &online,
> +					 ahci_check_ready);
>   
>   		if (sata_scr_read(link, SCR_STATUS, &sstatus) != 0 ||
>   				(sstatus & 0xf) != 1)
> diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c
> index 6cd61842ad48..a5eeedadf0c9 100644
> --- a/drivers/ata/ahci_qoriq.c
> +++ b/drivers/ata/ahci_qoriq.c
> @@ -90,7 +90,6 @@ MODULE_DEVICE_TABLE(acpi, ahci_qoriq_acpi_match);
>   static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
>   			  unsigned long deadline)
>   {
> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>   	void __iomem *port_mmio = ahci_port_base(link->ap);
>   	u32 px_cmd, px_is, px_val;
>   	struct ata_port *ap = link->ap;
> @@ -126,8 +125,8 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
>   	tf.status = ATA_BUSY;
>   	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
>   
> -	rc = sata_link_hardreset(link, timing, deadline, &online,
> -				 ahci_check_ready);
> +	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
> +				 deadline, &online, ahci_check_ready);
>   
>   	/* restore the PxCMD and PxIS on ls1021 */
>   	if (ls1021a_workaround) {
> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index 7bb5db17f864..8d1598232e92 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -350,7 +350,8 @@ static void xgene_ahci_set_phy_cfg(struct xgene_ahci_context *ctx, int channel)
>   static int xgene_ahci_do_hardreset(struct ata_link *link,
>   				   unsigned long deadline, bool *online)
>   {
> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
> +	const struct sata_deb_timing *timing =
> +		sata_ehc_deb_timing(&link->eh_context);
>   	struct ata_port *ap = link->ap;
>   	struct ahci_host_priv *hpriv = ap->host->private_data;
>   	struct xgene_ahci_context *ctx = hpriv->plat_data;
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index cf8c7fd59ada..0ac3b382fa52 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1549,7 +1549,6 @@ static int ahci_pmp_retry_softreset(struct ata_link *link, unsigned int *class,
>   int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>   		      unsigned long deadline, bool *online)
>   {
> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>   	struct ata_port *ap = link->ap;
>   	struct ahci_port_priv *pp = ap->private_data;
>   	struct ahci_host_priv *hpriv = ap->host->private_data;
> @@ -1564,8 +1563,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>   	tf.status = ATA_BUSY;
>   	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
>   
> -	rc = sata_link_hardreset(link, timing, deadline, online,
> -				 ahci_check_ready);
> +	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
> +				 deadline, online, ahci_check_ready);
>   
>   	hpriv->start_engine(ap);
>   
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 1bdb6e78f0ed..ffad7c1afb64 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3616,12 +3616,13 @@ EXPORT_SYMBOL_GPL(ata_std_prereset);
>   int sata_std_hardreset(struct ata_link *link, unsigned int *class,
>   		       unsigned long deadline)
>   {
> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>   	bool online;
>   	int rc;
>   
>   	/* do hardreset */
> -	rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
> +	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
> +				 deadline, &online, NULL);
> +
>   	return online ? -EAGAIN : rc;
>   }
>   EXPORT_SYMBOL_GPL(sata_std_hardreset);
> diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
> index e2e9cbd405fa..1ea472ddbe3f 100644
> --- a/drivers/ata/libata-pmp.c
> +++ b/drivers/ata/libata-pmp.c
> @@ -851,7 +851,7 @@ static int sata_pmp_eh_handle_disabled_links(struct ata_port *ap)
>   		/* Some PMPs require hardreset sequence to get
>   		 * SError.N working.
>   		 */
> -		sata_link_hardreset(link, sata_deb_timing_normal,
> +		sata_link_hardreset(link, &sata_deb_timing_normal,
>   				ata_deadline(jiffies, ATA_TMOUT_INTERNAL_QUICK),
>   				NULL, NULL);
>   
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 86f1475e5bca..be46833d77a6 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -16,12 +16,31 @@
>   #include "libata.h"
>   #include "libata-transport.h"
>   
> -/* debounce timing parameters in msecs { interval, duration, timeout } */
> -const unsigned long sata_deb_timing_normal[]		= {   5,  100, 2000 };
> +/*
> + * Debounce timing parameters in msecs.
> + */
> +const struct sata_deb_timing sata_deb_timing_normal =
> +{
> +	.interval	= 5,

I find it always useful to append the unit to the variable name: 
`interval_ms`.


The rest looks good.


Kind regards,

Paul


> +	.duration	= 100,
> +	.timeout	= 2000
> +};
>   EXPORT_SYMBOL_GPL(sata_deb_timing_normal);
> -const unsigned long sata_deb_timing_hotplug[]		= {  25,  500, 2000 };
> +
> +const struct sata_deb_timing sata_deb_timing_hotplug =
> +{
> +	.interval	= 25,
> +	.duration	= 500,
> +	.timeout	= 2000
> +};
>   EXPORT_SYMBOL_GPL(sata_deb_timing_hotplug);
> -const unsigned long sata_deb_timing_long[]		= { 100, 2000, 5000 };
> +
> +const struct sata_deb_timing sata_deb_timing_long =
> +{
> +	.interval	= 100,
> +	.duration	= 2000,
> +	.timeout	= 5000
> +};
>   EXPORT_SYMBOL_GPL(sata_deb_timing_long);
>   
>   /**
> @@ -211,7 +230,7 @@ EXPORT_SYMBOL_GPL(ata_tf_from_fis);
>   /**
>    *	sata_link_debounce - debounce SATA phy status
>    *	@link: ATA link to debounce SATA phy status for
> - *	@params: timing parameters { interval, duration, timeout } in msec
> + *	@timing: debounce timing
>    *	@deadline: deadline jiffies for the operation
>    *
>    *	Make sure SStatus of @link reaches stable state, determined by
> @@ -230,16 +249,15 @@ EXPORT_SYMBOL_GPL(ata_tf_from_fis);
>    *	RETURNS:
>    *	0 on success, -errno on failure.
>    */
> -int sata_link_debounce(struct ata_link *link, const unsigned long *params,
> +int sata_link_debounce(struct ata_link *link,
> +		       const struct sata_deb_timing *timing,
>   		       unsigned long deadline)
>   {
> -	unsigned long interval = params[0];
> -	unsigned long duration = params[1];
>   	unsigned long last_jiffies, t;
>   	u32 last, cur;
>   	int rc;
>   
> -	t = ata_deadline(jiffies, params[2]);
> +	t = ata_deadline(jiffies, timing->timeout);
>   	if (time_before(t, deadline))
>   		deadline = t;
>   
> @@ -251,7 +269,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
>   	last_jiffies = jiffies;
>   
>   	while (1) {
> -		ata_msleep(link->ap, interval);
> +		ata_msleep(link->ap, timing->interval);
>   		if ((rc = sata_scr_read(link, SCR_STATUS, &cur)))
>   			return rc;
>   		cur &= 0xf;
> @@ -261,7 +279,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
>   			if (cur == 1 && time_before(jiffies, deadline))
>   				continue;
>   			if (time_after(jiffies,
> -				       ata_deadline(last_jiffies, duration)))
> +				ata_deadline(last_jiffies, timing->duration)))
>   				return 0;
>   			continue;
>   		}
> @@ -280,7 +298,7 @@ int sata_link_debounce(struct ata_link *link, const unsigned long *params,
>   EXPORT_SYMBOL_GPL(sata_link_debounce);
>   
>   static int __sata_link_resume(struct ata_link *link,
> -			      const unsigned long *timing,
> +			      const struct sata_deb_timing *timing,
>   			      unsigned long deadline)
>   {
>   	int tries = ATA_LINK_RESUME_TRIES;
> @@ -511,7 +529,7 @@ EXPORT_SYMBOL_GPL(sata_set_spd);
>   /**
>    *	sata_link_hardreset - reset link via SATA phy reset
>    *	@link: link to reset
> - *	@timing: timing parameters { interval, duration, timeout } in msec
> + *	@timing: debounce timing parameters
>    *	@deadline: deadline jiffies for the operation
>    *	@online: optional out parameter indicating link onlineness
>    *	@check_ready: optional callback to check link readiness
> @@ -532,7 +550,8 @@ EXPORT_SYMBOL_GPL(sata_set_spd);
>    *	RETURNS:
>    *	0 on success, -errno otherwise.
>    */
> -int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
> +int sata_link_hardreset(struct ata_link *link,
> +			const struct sata_deb_timing *timing,
>   			unsigned long deadline,
>   			bool *online, int (*check_ready)(struct ata_link *))
>   {
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index b3be7a8f5bea..ffd085b09abc 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2030,13 +2030,11 @@ EXPORT_SYMBOL_GPL(ata_sff_softreset);
>   int sata_sff_hardreset(struct ata_link *link, unsigned int *class,
>   		       unsigned long deadline)
>   {
> -	struct ata_eh_context *ehc = &link->eh_context;
> -	const unsigned long *timing = sata_ehc_deb_timing(ehc);
>   	bool online;
>   	int rc;
>   
> -	rc = sata_link_hardreset(link, timing, deadline, &online,
> -				 ata_sff_check_ready);
> +	rc = sata_link_hardreset(link, sata_ehc_deb_timing(&link->eh_context),
> +				 deadline, &online, ata_sff_check_ready);
>   	if (online)
>   		*class = ata_sff_dev_classify(link->device, 1, NULL);
>   
> diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
> index dfbf9493e451..16da571e8083 100644
> --- a/drivers/ata/sata_highbank.c
> +++ b/drivers/ata/sata_highbank.c
> @@ -385,7 +385,7 @@ static int highbank_initialize_phys(struct device *dev, void __iomem *addr)
>   static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
>   				unsigned long deadline)
>   {
> -	static const unsigned long timing[] = { 5, 100, 500};
> +	const struct sata_deb_timing timing = { 5, 100, 500};
>   	struct ata_port *ap = link->ap;
>   	struct ahci_port_priv *pp = ap->private_data;
>   	struct ahci_host_priv *hpriv = ap->host->private_data;
> @@ -405,7 +405,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
>   
>   	do {
>   		highbank_cphy_disable_overrides(link->ap->port_no);
> -		rc = sata_link_hardreset(link, timing, deadline, &online, NULL);
> +		rc = sata_link_hardreset(link, &timing, deadline, &online, NULL);
>   		highbank_cphy_override_lane(link->ap->port_no);
>   
>   		/* If the status is 1, we are connected, but the link did not
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index de5bd02cad44..8ad0f3776c48 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -3633,11 +3633,9 @@ static int mv_hardreset(struct ata_link *link, unsigned int *class,
>   
>   	/* Workaround for errata FEr SATA#10 (part 2) */
>   	do {
> -		const unsigned long *timing =
> -				sata_ehc_deb_timing(&link->eh_context);
> -
> -		rc = sata_link_hardreset(link, timing, deadline + extra,
> -					 &online, NULL);
> +		rc = sata_link_hardreset(link,
> +					 sata_ehc_deb_timing(&link->eh_context),
> +					 deadline + extra, &online, NULL);
>   		rc = online ? -EAGAIN : rc;
>   		if (rc)
>   			return rc;
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index b5f27eac86b1..5c8db8f8c47f 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -1526,7 +1526,7 @@ static int nv_hardreset(struct ata_link *link, unsigned int *class,
>   	 */
>   	if (!(link->ap->pflags & ATA_PFLAG_LOADING) &&
>   	    !ata_dev_enabled(link->device))
> -		sata_link_hardreset(link, sata_deb_timing_hotplug, deadline,
> +		sata_link_hardreset(link, &sata_deb_timing_hotplug, deadline,
>   				    NULL, NULL);
>   	else {
>   		int rc;
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 2fef6ce93f07..4c4ff67bf06a 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -735,7 +735,7 @@ static int sil24_hardreset(struct ata_link *link, unsigned int *class,
>   	/* SStatus oscillates between zero and valid status after
>   	 * DEV_RST, debounce it.
>   	 */
> -	rc = sata_link_debounce(link, sata_deb_timing_long, deadline);
> +	rc = sata_link_debounce(link, &sata_deb_timing_long, deadline);
>   	if (rc) {
>   		reason = "PHY debouncing failed";
>   		goto err;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index e89d612326f6..166263d9bbc7 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1150,17 +1150,26 @@ extern void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *
>    * SATA specific code - drivers/ata/libata-sata.c
>    */
>   #ifdef CONFIG_SATA_HOST
> -extern const unsigned long sata_deb_timing_normal[];
> -extern const unsigned long sata_deb_timing_hotplug[];
> -extern const unsigned long sata_deb_timing_long[];
>   
> -static inline const unsigned long *
> +/*
> + * Debounce timing parameters in msecs.
> + */
> +struct sata_deb_timing {
> +	unsigned long interval;
> +	unsigned long duration;
> +	unsigned long timeout;
> +};
> +
> +extern const struct sata_deb_timing sata_deb_timing_normal;
> +extern const struct sata_deb_timing sata_deb_timing_hotplug;
> +extern const struct sata_deb_timing sata_deb_timing_long;
> +
> +static inline const struct sata_deb_timing *
>   sata_ehc_deb_timing(struct ata_eh_context *ehc)
>   {
>   	if (ehc->i.flags & ATA_EHI_HOTPLUGGED)
> -		return sata_deb_timing_hotplug;
> -	else
> -		return sata_deb_timing_normal;
> +		return &sata_deb_timing_hotplug;
> +	return &sata_deb_timing_normal;
>   }
>   
>   extern int sata_scr_valid(struct ata_link *link);
> @@ -1169,12 +1178,13 @@ extern int sata_scr_write(struct ata_link *link, int reg, u32 val);
>   extern int sata_scr_write_flush(struct ata_link *link, int reg, u32 val);
>   extern int sata_set_spd(struct ata_link *link);
>   extern int sata_link_hardreset(struct ata_link *link,
> -			const unsigned long *timing, unsigned long deadline,
> -			bool *online, int (*check_ready)(struct ata_link *));
> +			       const struct sata_deb_timing *timing,
> +			       unsigned long deadline, bool *online,
> +			       int (*check_ready)(struct ata_link *));
>   extern int sata_link_resume(struct ata_link *link, unsigned long deadline);
>   extern void ata_eh_analyze_ncq_error(struct ata_link *link);
>   #else
> -static inline const unsigned long *
> +static inline const struct sata_deb_timing *
>   sata_ehc_deb_timing(struct ata_eh_context *ehc)
>   {
>   	return NULL;
> @@ -1194,9 +1204,8 @@ static inline int sata_scr_write_flush(struct ata_link *link, int reg, u32 val)
>   }
>   static inline int sata_set_spd(struct ata_link *link) { return -EOPNOTSUPP; }
>   static inline int sata_link_hardreset(struct ata_link *link,
> -				      const unsigned long *timing,
> -				      unsigned long deadline,
> -				      bool *online,
> +				      const struct sata_deb_timing *timing,
> +				      unsigned long deadline, bool *online,
>   				      int (*check_ready)(struct ata_link *))
>   {
>   	if (online)
> @@ -1211,7 +1220,8 @@ static inline int sata_link_resume(struct ata_link *link,
>   static inline void ata_eh_analyze_ncq_error(struct ata_link *link) { }
>   #endif
>   extern int sata_link_debounce(struct ata_link *link,
> -			const unsigned long *params, unsigned long deadline);
> +			      const struct sata_deb_timing *timing,
> +			      unsigned long deadline);
>   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);

Re: [PATCH 3/4] ata: libata-sata: Remove debounce delay by default

[Paul Menzel]

Am 23.03.22 um 09:17 schrieb Damien Le Moal:
> Many adapters and SATA controllers are fine with accesses to the
> SControl register right after resuming. For these adapters, the
> additional 200ms delay implemented in sata_link_resume() is redundant
> and unnecessarily increase boot time.

increase*s*

> Drivers can opt out from this additional delay using the
> ATA_LFLAG_NO_DEBOUNCE_DELAY link flag, but by default, this additional
> delay exists for all adapters.
> 
> Reverse the situation and do not add by default this 200ms delay. For
> adapters that actually need this delay, rename the
> ATA_LFLAG_NO_DEBOUNCE_DELAY link flag to ATA_LFLAG_DEBOUNCE_DELAY and
> execute the 200ms delay in sata_link_resume() only if a driver request
> it using the new link flag. Otherwise, arbitrarily delay by 1ms only.
> 
> Since ata_piix adapters are known to require the longer delay, specify

Was it 200 ms or some delay, they require?

> the ATA_LFLAG_DEBOUNCE_DELAY for all adapters supported by the ata_piix
> driver.

Maybe also add a reference to the archeology [1] I once did.

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/ata/ahci.c        | 12 ++----------
>   drivers/ata/ahci_brcm.c   |  1 -
>   drivers/ata/ata_generic.c |  1 +
>   drivers/ata/ata_piix.c    | 17 +++++++++++++++++
>   drivers/ata/libata-sata.c | 18 +++++++++++-------
>   include/linux/libata.h    |  2 +-
>   6 files changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index ccf94e8a3056..ac1e227a07cd 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -51,7 +51,6 @@ enum board_ids {
>   	board_ahci,
>   	board_ahci_ign_iferr,
>   	board_ahci_low_power,
> -	board_ahci_no_debounce_delay,
>   	board_ahci_nomsi,
>   	board_ahci_noncq,
>   	board_ahci_nosntf,
> @@ -142,13 +141,6 @@ static const struct ata_port_info ahci_port_info[] = {
>   		.udma_mask	= ATA_UDMA6,
>   		.port_ops	= &ahci_ops,
>   	},
> -	[board_ahci_no_debounce_delay] = {
> -		.flags		= AHCI_FLAG_COMMON,
> -		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
> -		.pio_mask	= ATA_PIO4,
> -		.udma_mask	= ATA_UDMA6,
> -		.port_ops	= &ahci_ops,
> -	},
>   	[board_ahci_nomsi] = {
>   		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>   		.flags		= AHCI_FLAG_COMMON,
> @@ -445,7 +437,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>   		board_ahci_al },
>   	/* AMD */
>   	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
> -	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /* AMD Hudson-2 (AHCI mode) */
> +	{ PCI_VDEVICE(AMD, 0x7801), board_ahci }, /* AMD Hudson-2 (AHCI mode) */
>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>   	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_low_power }, /* AMD Green Sardine */
>   	/* AMD is using RAID class only for ahci controllers */
> @@ -583,7 +575,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>   	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL_EXT, 0x9230),
>   	  .driver_data = board_ahci_yes_fbs },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL_EXT, 0x9235),
> -	  .driver_data = board_ahci_no_debounce_delay },
> +	  .driver_data = board_ahci },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_TTI, 0x0642), /* highpoint rocketraid 642L */
>   	  .driver_data = board_ahci_yes_fbs },
>   	{ PCI_DEVICE(PCI_VENDOR_ID_TTI, 0x0645), /* highpoint rocketraid 644L */
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index ab8552b1ff2a..e4584aed0ded 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -333,7 +333,6 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
>   
>   static const struct ata_port_info ahci_brcm_port_info = {
>   	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
> -	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
>   	.pio_mask	= ATA_PIO4,
>   	.udma_mask	= ATA_UDMA6,
>   	.port_ops	= &ahci_brcm_platform_ops,
> diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
> index 20a32e4d501d..f02c824b26e6 100644
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
> @@ -165,6 +165,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
>   	u16 command;
>   	static const struct ata_port_info info = {
>   		.flags = ATA_FLAG_SLAVE_POSS,
> +		.link_flags = ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask = ATA_PIO4,
>   		.mwdma_mask = ATA_MWDMA2,
>   		.udma_mask = ATA_UDMA5,
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index ade5e894563b..dcfcfb5d8a05 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -1112,6 +1112,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[piix_pata_mwdma] =	/* PIIX3 MWDMA only */
>   	{
>   		.flags		= PIIX_PATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA12_ONLY, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
>   		.port_ops	= &piix_pata_ops,
> @@ -1120,6 +1121,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[piix_pata_33] =	/* PIIX4 at 33MHz */
>   	{
>   		.flags		= PIIX_PATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA12_ONLY, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
>   		.udma_mask	= ATA_UDMA2,
> @@ -1129,6 +1131,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich_pata_33] =		/* ICH0 - ICH at 33Mhz*/
>   	{
>   		.flags		= PIIX_PATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA12_ONLY, /* Check: maybe MWDMA0 is ok  */
>   		.udma_mask	= ATA_UDMA2,
> @@ -1138,6 +1141,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich_pata_66] =		/* ICH controllers up to 66MHz */
>   	{
>   		.flags		= PIIX_PATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA12_ONLY, /* MWDMA0 is broken on chip */
>   		.udma_mask	= ATA_UDMA4,
> @@ -1147,6 +1151,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich_pata_100] =
>   	{
>   		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA12_ONLY,
>   		.udma_mask	= ATA_UDMA5,
> @@ -1156,6 +1161,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich_pata_100_nomwdma1] =
>   	{
>   		.flags		= PIIX_PATA_FLAGS | PIIX_FLAG_CHECKINTR,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2_ONLY,
>   		.udma_mask	= ATA_UDMA5,
> @@ -1165,6 +1171,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich5_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1174,6 +1181,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich6_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1183,6 +1191,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich6m_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1192,6 +1201,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich8_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1201,6 +1211,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich8_2port_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1210,6 +1221,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[tolapai_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1219,6 +1231,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich8m_apple_sata] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1228,6 +1241,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[piix_pata_vmw] =
>   	{
>   		.flags		= PIIX_PATA_FLAGS,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA12_ONLY, /* mwdma1-2 ?? CHECK 0 should be ok but slow */
>   		.udma_mask	= ATA_UDMA2,
> @@ -1241,6 +1255,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich8_sata_snb] =
>   	{
>   		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR | PIIX_FLAG_PIO16,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1251,6 +1266,7 @@ static struct ata_port_info piix_port_info[] = {
>   	{
>   		.flags		= PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR
>   					| PIIX_FLAG_PIO16,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask	= ATA_PIO4,
>   		.mwdma_mask	= ATA_MWDMA2,
>   		.udma_mask	= ATA_UDMA6,
> @@ -1260,6 +1276,7 @@ static struct ata_port_info piix_port_info[] = {
>   	[ich8_2port_sata_byt] =
>   	{
>   		.flags          = PIIX_SATA_FLAGS | PIIX_FLAG_SIDPR | PIIX_FLAG_PIO16,
> +		.link_flags	= ATA_LFLAG_DEBOUNCE_DELAY,
>   		.pio_mask       = ATA_PIO4,
>   		.mwdma_mask     = ATA_MWDMA2,
>   		.udma_mask      = ATA_UDMA6,
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index be46833d77a6..87ad03c2e49f 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -302,9 +302,18 @@ static int __sata_link_resume(struct ata_link *link,
>   			      unsigned long deadline)
>   {
>   	int tries = ATA_LINK_RESUME_TRIES;
> +	unsigned int db_delay = 1;

Also append the unit to the name: db_delay_ms?

The other delays use long, but 64 s where int is 16-bit wide, should be 
enough.

>   	u32 scontrol, serror;
>   	int rc;
>   
> +	/*
> +	 * Some PHYs react badly if SControl is pounded immediately after
> +	 * resuming. For drivers requesting it, delay 200ms before debouncing.
> +	 * Otherwise, only delay by 1ms (arbitrary delay).
> +	 */
> +	if (link->flags & ATA_LFLAG_DEBOUNCE_DELAY)
> +		db_delay = 200;
> +

Use ternary operator?

	db_delay = (link->flags & ATA_LFLAG_DEBOUNCE_DELAY) ? 200 : 1;

>   	if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
>   		return rc;
>   
> @@ -317,13 +326,8 @@ static int __sata_link_resume(struct ata_link *link,
>   		scontrol = (scontrol & 0x0f0) | 0x300;
>   		if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))
>   			return rc;
> -		/*
> -		 * Some PHYs react badly if SStatus is pounded
> -		 * immediately after resuming.  Delay 200ms before
> -		 * debouncing.
> -		 */
> -		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
> -			ata_msleep(link->ap, 200);
> +
> +		ata_msleep(link->ap, db_delay);
>   
>   		/* is SControl restored correctly? */
>   		if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 166263d9bbc7..cc3a8e9c78b0 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -143,7 +143,7 @@ enum {
>   	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>   	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
>   	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
> -	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
> +	ATA_LFLAG_DEBOUNCE_DELAY = (1 << 11), /* Debounce delay on link resume */
>   
>   	/* struct ata_port flags */
>   	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */

The rest looks good.


Kind regards,

Paul


[1]: 
https://lore.kernel.org/linux-ide/20220113154635.17581-2-pmenzel@molgen.mpg.de/

Re: [PATCH 2/4] ata: libata-sata: Introduce struct sata_deb_timing

[Damien Le Moal]

On 3/23/22 23:09, Paul Menzel wrote:
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 86f1475e5bca..be46833d77a6 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -16,12 +16,31 @@
>>   #include "libata.h"
>>   #include "libata-transport.h"
>>   
>> -/* debounce timing parameters in msecs { interval, duration, timeout } */
>> -const unsigned long sata_deb_timing_normal[]		= {   5,  100, 2000 };
>> +/*
>> + * Debounce timing parameters in msecs.
>> + */
>> +const struct sata_deb_timing sata_deb_timing_normal =
>> +{
>> +	.interval	= 5,
> 
> I find it always useful to append the unit to the variable name: 
> `interval_ms`.

Yes, we can do that.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] Remove link debounce delays by default

[Damien Le Moal]

On 3/23/22 19:59, Paul Menzel wrote:
>>> It’d be great if you gave an example benchmark.
>>
>> No need for a benchmark. This is not hot path stuff. This code runs only
>> during device discovery on boot and on resume after suspend.
>> So apply the patches and reboot, check dmesg if you see errors or not and
>> if your disks are all there. Same after a suspend+resume.
> 
> Yes, I know what I need to check. I meant, that you write without this 
> patchset my system with HBA controller X and 32 [vendor/model] disks 
> attached reaches the message Y in Z1 seconds, and with the series Z2 
> seconds.

There is going to be too much variation from machine to machine as that
depends on the adapter & devices used for testing. The only sensible thing
to do is to compare timing before patching with timing after patching and
see if there are some gains. On my test rig, I have so many drives and
various HBAs connected that the boot time gains overall are nil. But I do
see faster per-ata drive scan times.

> 
>>>> Comments and lots of testing are welcome !
>>>>
>>>> Damien Le Moal (4):
>>>>     ata: libata-sata: Simplify sata_link_resume() interface
>>>>     ata: libata-sata: Introduce struct sata_deb_timing
>>>>     ata: libata-sata: Remove debounce delay by default
>>>>     ata: libata-sata: Improve sata_link_debounce()
>>>
>>> […]
>>>
>>> I am wondering how sure we can be, there won’t be any regressions? Not
>>> having the boot disk detected is quite a serious issue/regression, and
>>> it should be made easy for users to fix that without having to rebuild
>>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>>> be helpful for effected users.
>>
>> I am working on another series for that. The patches will allow
>> controlling most horkage and link flags on/off using libata.force kernel
>> boot parameter. That will allow figuring out problems without patching in
>> the field, for patches to be later added.
> 
> Sounds good. But this needs to be available before the changes at hand, 
> doesn’t it?

Not really. For now, we need to check if these patches break anything,
regardless of the libata.force changes. I consider libata.force for field
debugging. A user should not have to use it to get a system running. The
kernel should have sensible defaults for that and things should run out of
the box without the need for additional kernel boot parameters.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] Remove link debounce delays by default

[Paul Menzel]

Dear Damien,


Am 24.03.22 um 00:58 schrieb Damien Le Moal:
> On 3/23/22 19:59, Paul Menzel wrote:
>>>> It’d be great if you gave an example benchmark.
>>>
>>> No need for a benchmark. This is not hot path stuff. This code runs only
>>> during device discovery on boot and on resume after suspend.
>>> So apply the patches and reboot, check dmesg if you see errors or not and
>>> if your disks are all there. Same after a suspend+resume.
>>
>> Yes, I know what I need to check. I meant, that you write without this
>> patchset my system with HBA controller X and 32 [vendor/model] disks
>> attached reaches the message Y in Z1 seconds, and with the series Z2
>> seconds.
> 
> There is going to be too much variation from machine to machine as that
> depends on the adapter & devices used for testing. The only sensible thing
> to do is to compare timing before patching with timing after patching and
> see if there are some gains. On my test rig, I have so many drives and
> various HBAs connected that the boot time gains overall are nil. But I do
> see faster per-ata drive scan times.

And for one of these it’d be great to have exact numbers. ;-)

>>>>> Comments and lots of testing are welcome !
>>>>>
>>>>> Damien Le Moal (4):
>>>>>      ata: libata-sata: Simplify sata_link_resume() interface
>>>>>      ata: libata-sata: Introduce struct sata_deb_timing
>>>>>      ata: libata-sata: Remove debounce delay by default
>>>>>      ata: libata-sata: Improve sata_link_debounce()
>>>>
>>>> […]
>>>>
>>>> I am wondering how sure we can be, there won’t be any regressions? Not
>>>> having the boot disk detected is quite a serious issue/regression, and
>>>> it should be made easy for users to fix that without having to rebuild
>>>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>>>> be helpful for effected users.
>>>
>>> I am working on another series for that. The patches will allow
>>> controlling most horkage and link flags on/off using libata.force kernel
>>> boot parameter. That will allow figuring out problems without patching in
>>> the field, for patches to be later added.
>>
>> Sounds good. But this needs to be available before the changes at hand,
>> doesn’t it?
> 
> Not really. For now, we need to check if these patches break anything,
> regardless of the libata.force changes. I consider libata.force for field
> debugging. A user should not have to use it to get a system running. The
> kernel should have sensible defaults for that and things should run out of
> the box without the need for additional kernel boot parameters.

Sorry, I have the feeling we misunderstand each other. Just to be clear, 
you are saying before shipping this to users, we can be 100 % certain 
that these changes won’t break any systems out there?


Kind regards,

Paul

Re: [PATCH 0/4] Remove link debounce delays by default

[Damien Le Moal]

On 3/24/22 14:30, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 24.03.22 um 00:58 schrieb Damien Le Moal:
>> On 3/23/22 19:59, Paul Menzel wrote:
>>>>> It’d be great if you gave an example benchmark.
>>>>
>>>> No need for a benchmark. This is not hot path stuff. This code runs only
>>>> during device discovery on boot and on resume after suspend.
>>>> So apply the patches and reboot, check dmesg if you see errors or not and
>>>> if your disks are all there. Same after a suspend+resume.
>>>
>>> Yes, I know what I need to check. I meant, that you write without this
>>> patchset my system with HBA controller X and 32 [vendor/model] disks
>>> attached reaches the message Y in Z1 seconds, and with the series Z2
>>> seconds.
>>
>> There is going to be too much variation from machine to machine as that
>> depends on the adapter & devices used for testing. The only sensible thing
>> to do is to compare timing before patching with timing after patching and
>> see if there are some gains. On my test rig, I have so many drives and
>> various HBAs connected that the boot time gains overall are nil. But I do
>> see faster per-ata drive scan times.
> 
> And for one of these it’d be great to have exact numbers. ;-)
> 
>>>>>> Comments and lots of testing are welcome !
>>>>>>
>>>>>> Damien Le Moal (4):
>>>>>>      ata: libata-sata: Simplify sata_link_resume() interface
>>>>>>      ata: libata-sata: Introduce struct sata_deb_timing
>>>>>>      ata: libata-sata: Remove debounce delay by default
>>>>>>      ata: libata-sata: Improve sata_link_debounce()
>>>>>
>>>>> […]
>>>>>
>>>>> I am wondering how sure we can be, there won’t be any regressions? Not
>>>>> having the boot disk detected is quite a serious issue/regression, and
>>>>> it should be made easy for users to fix that without having to rebuild
>>>>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>>>>> be helpful for effected users.
>>>>
>>>> I am working on another series for that. The patches will allow
>>>> controlling most horkage and link flags on/off using libata.force kernel
>>>> boot parameter. That will allow figuring out problems without patching in
>>>> the field, for patches to be later added.
>>>
>>> Sounds good. But this needs to be available before the changes at hand,
>>> doesn’t it?
>>
>> Not really. For now, we need to check if these patches break anything,
>> regardless of the libata.force changes. I consider libata.force for field
>> debugging. A user should not have to use it to get a system running. The
>> kernel should have sensible defaults for that and things should run out of
>> the box without the need for additional kernel boot parameters.
> 
> Sorry, I have the feeling we misunderstand each other. Just to be clear, 
> you are saying before shipping this to users, we can be 100 % certain 
> that these changes won’t break any systems out there?

The patches are only an improvement for what can be controlled using the
libata.force boot parameter. No other change. So nothing will break with
these patches.

See Documentation/admin-guide/kernel-parameters.txt and compare the list
of possible arguments that libata.force can take to the number of
ATA_HORKAGE and ATA_LFLAGS defined... There are many that cannot be
tweaked using libata.force, which is a pain when debugging a problem in
the field. With more arguments for libata.force, we can test adding by
default horkages/lflags for a device without having to patch and recompile
a kernel, tasks that many end user do not know how to do.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] Remove link debounce delays by default

[Paul Menzel]

Dear Damien,


Am 24.03.22 um 07:15 schrieb Damien Le Moal:
> On 3/24/22 14:30, Paul Menzel wrote:

>> Am 24.03.22 um 00:58 schrieb Damien Le Moal:
>>> On 3/23/22 19:59, Paul Menzel wrote:
>>>>>> It’d be great if you gave an example benchmark.
>>>>>
>>>>> No need for a benchmark. This is not hot path stuff. This code runs only
>>>>> during device discovery on boot and on resume after suspend.
>>>>> So apply the patches and reboot, check dmesg if you see errors or not and
>>>>> if your disks are all there. Same after a suspend+resume.
>>>>
>>>> Yes, I know what I need to check. I meant, that you write without this
>>>> patchset my system with HBA controller X and 32 [vendor/model] disks
>>>> attached reaches the message Y in Z1 seconds, and with the series Z2
>>>> seconds.
>>>
>>> There is going to be too much variation from machine to machine as that
>>> depends on the adapter & devices used for testing. The only sensible thing
>>> to do is to compare timing before patching with timing after patching and
>>> see if there are some gains. On my test rig, I have so many drives and
>>> various HBAs connected that the boot time gains overall are nil. But I do
>>> see faster per-ata drive scan times.
>>
>> And for one of these it’d be great to have exact numbers. ;-)
>>
>>>>>>> Comments and lots of testing are welcome !
>>>>>>>
>>>>>>> Damien Le Moal (4):
>>>>>>>       ata: libata-sata: Simplify sata_link_resume() interface
>>>>>>>       ata: libata-sata: Introduce struct sata_deb_timing
>>>>>>>       ata: libata-sata: Remove debounce delay by default
>>>>>>>       ata: libata-sata: Improve sata_link_debounce()
>>>>>>
>>>>>> […]
>>>>>>
>>>>>> I am wondering how sure we can be, there won’t be any regressions? Not
>>>>>> having the boot disk detected is quite a serious issue/regression, and
>>>>>> it should be made easy for users to fix that without having to rebuild
>>>>>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>>>>>> be helpful for effected users.
>>>>>
>>>>> I am working on another series for that. The patches will allow
>>>>> controlling most horkage and link flags on/off using libata.force kernel
>>>>> boot parameter. That will allow figuring out problems without patching in
>>>>> the field, for patches to be later added.
>>>>
>>>> Sounds good. But this needs to be available before the changes at hand,
>>>> doesn’t it?
>>>
>>> Not really. For now, we need to check if these patches break anything,
>>> regardless of the libata.force changes. I consider libata.force for field
>>> debugging. A user should not have to use it to get a system running. The
>>> kernel should have sensible defaults for that and things should run out of
>>> the box without the need for additional kernel boot parameters.
>>
>> Sorry, I have the feeling we misunderstand each other. Just to be clear,
>> you are saying before shipping this to users, we can be 100 % certain
>> that these changes won’t break any systems out there?
> 
> The patches are only an improvement for what can be controlled using the
> libata.force boot parameter. No other change. So nothing will break with
> these patches.

Still we are misunderstanding each other? I am talking about the testing 
and possible regressions of the debounce delay patches, and not the 
“libata.force/horkage patches”.

> See Documentation/admin-guide/kernel-parameters.txt and compare the list
> of possible arguments that libata.force can take to the number of
> ATA_HORKAGE and ATA_LFLAGS defined... There are many that cannot be
> tweaked using libata.force, which is a pain when debugging a problem in
> the field. With more arguments for libata.force, we can test adding by
> default horkages/lflags for a device without having to patch and recompile
> a kernel, tasks that many end user do not know how to do.

Yes, I agree, that the “libata.force/horkage patches” are going to be safe.


Kind regards,

Paul

Re: [PATCH 0/4] Remove link debounce delays by default

[Damien Le Moal]

On 3/24/22 15:35, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 24.03.22 um 07:15 schrieb Damien Le Moal:
>> On 3/24/22 14:30, Paul Menzel wrote:
> 
>>> Am 24.03.22 um 00:58 schrieb Damien Le Moal:
>>>> On 3/23/22 19:59, Paul Menzel wrote:
>>>>>>> It’d be great if you gave an example benchmark.
>>>>>>
>>>>>> No need for a benchmark. This is not hot path stuff. This code runs only
>>>>>> during device discovery on boot and on resume after suspend.
>>>>>> So apply the patches and reboot, check dmesg if you see errors or not and
>>>>>> if your disks are all there. Same after a suspend+resume.
>>>>>
>>>>> Yes, I know what I need to check. I meant, that you write without this
>>>>> patchset my system with HBA controller X and 32 [vendor/model] disks
>>>>> attached reaches the message Y in Z1 seconds, and with the series Z2
>>>>> seconds.
>>>>
>>>> There is going to be too much variation from machine to machine as that
>>>> depends on the adapter & devices used for testing. The only sensible thing
>>>> to do is to compare timing before patching with timing after patching and
>>>> see if there are some gains. On my test rig, I have so many drives and
>>>> various HBAs connected that the boot time gains overall are nil. But I do
>>>> see faster per-ata drive scan times.
>>>
>>> And for one of these it’d be great to have exact numbers. ;-)
>>>
>>>>>>>> Comments and lots of testing are welcome !
>>>>>>>>
>>>>>>>> Damien Le Moal (4):
>>>>>>>>       ata: libata-sata: Simplify sata_link_resume() interface
>>>>>>>>       ata: libata-sata: Introduce struct sata_deb_timing
>>>>>>>>       ata: libata-sata: Remove debounce delay by default
>>>>>>>>       ata: libata-sata: Improve sata_link_debounce()
>>>>>>>
>>>>>>> […]
>>>>>>>
>>>>>>> I am wondering how sure we can be, there won’t be any regressions? Not
>>>>>>> having the boot disk detected is quite a serious issue/regression, and
>>>>>>> it should be made easy for users to fix that without having to rebuild
>>>>>>> the Linux kernel. A Linux kernel CLI parameter to enable the delay would
>>>>>>> be helpful for effected users.
>>>>>>
>>>>>> I am working on another series for that. The patches will allow
>>>>>> controlling most horkage and link flags on/off using libata.force kernel
>>>>>> boot parameter. That will allow figuring out problems without patching in
>>>>>> the field, for patches to be later added.
>>>>>
>>>>> Sounds good. But this needs to be available before the changes at hand,
>>>>> doesn’t it?
>>>>
>>>> Not really. For now, we need to check if these patches break anything,
>>>> regardless of the libata.force changes. I consider libata.force for field
>>>> debugging. A user should not have to use it to get a system running. The
>>>> kernel should have sensible defaults for that and things should run out of
>>>> the box without the need for additional kernel boot parameters.
>>>
>>> Sorry, I have the feeling we misunderstand each other. Just to be clear,
>>> you are saying before shipping this to users, we can be 100 % certain
>>> that these changes won’t break any systems out there?
>>
>> The patches are only an improvement for what can be controlled using the
>> libata.force boot parameter. No other change. So nothing will break with
>> these patches.
> 
> Still we are misunderstanding each other? I am talking about the testing 
> and possible regressions of the debounce delay patches, and not the 
> “libata.force/horkage patches”.

Ah. OK. Then the answer is no, there is a possibility that the patches
break the detection of drives with some adapters. Most likely with old
systems. This is not 100% safe.

But the alternative is to keep the delays as is and keep patching as you
did for disabling the delays on systems that are identified as OK. And I
do not like this alternative either since I suspect that the majority of
recent drives+AHCI adapters will be OK.

I keep looking at the code to see how to reduce the risks. However, since
the code being patched runs before we even know what drive is connected,
we cannot for example rely on the drive specs version (old drive == old
specs).

The safe approach will be to mark most adapters with
ATA_LFLAG_DEBOUNCE_DELAY and remove that flag for tested adapters, which
will be easier to do with the libata.force changes, which will allow
specifying "nodebounce_delay".

The problem though is that there are lots of AHCI adapters that do not
have an specific entry in ahci_port_info and use the default entry.
Changing that one is the main risk.

Still looking at the code to see what can be done to minimize that.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

[Paul Menzel]

Dear Damien,


Am 23.03.22 um 09:17 schrieb Damien Le Moal:
> sata_link_debounce() polls the SStatus register DET field to ensure that
> a stable value is provided, to reliably detect device presence and PHY
> readiness. Polling is done for at least timing->duration if there is no
> device detected. For the device detected case, polling last up to

last*s*

> deadline to ensure that the PHY becomes ready.
> 
> However, the PHY ready state is actually never checked, leading to the
> poll loop duration always reaching the maximum duration.
> 
> For adapters that do not require a debounce delay (link flag
> ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates

no*t*?

> device present *and* PHY ready and bail out of the polling loop if it
> does.
> 
> While at it, add comments to clarify the various checks in
> sata_link_debounce() polling loop.

Were you able to verify that the check now terminates the loop on your 
devices earlier?

Are below the right lines to compare? On the Dell OptiPlex 5055 (AMD FCH 
SATA Controller [AHCI mode] [1022:7901]) I see:

Before (this very patch)

     [    0.428015] ata1: hard resetting link
     [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)

After:

     [    0.428907] ata1: hard resetting link
     [    0.648092] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)

So a decrease of (268 - 219 = 49) ms. Nice.

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 87ad03c2e49f..15423723c9dd 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -276,8 +276,27 @@ int sata_link_debounce(struct ata_link *link,
>   
>   		/* DET stable? */
>   		if (cur == last) {
> +			/*
> +			 * If the device presence was detected but PHY
> +			 * communication is not yet established, wait until
> +			 * deadline.
> +			 */
>   			if (cur == 1 && time_before(jiffies, deadline))
>   				continue;
> +
> +			/*
> +			 * If PHY is ready and the device is present, and the
> +			 * driver did not request debounce delay, bail out early
> +			 * assuming that the link is stable.
> +			 */
> +			if (cur == 3 &&
> +			    !(link->flags & ATA_LFLAG_DEBOUNCE_DELAY))
> +				return 0;
> +
> +			/*
> +			 * If DET value has remained stable for
> +			 * timing->duration, bail out.
> +			 */
>   			if (time_after(jiffies,
>   				ata_deadline(last_jiffies, timing->duration)))
>   				return 0;
> @@ -288,8 +307,9 @@ int sata_link_debounce(struct ata_link *link,
>   		last = cur;
>   		last_jiffies = jiffies;
>   
> -		/* Check deadline.  If debouncing failed, return
> -		 * -EPIPE to tell upper layer to lower link speed.
> +		/*
> +		 * If debouncing has not succeeded before dealine, return

dea*d*line

> +		 * -EPIPE to tell the upper layer to lower the link speed.
>   		 */
>   		if (time_after(jiffies, deadline))
>   			return -EPIPE;

One separate for the new check would have been nice, but looks good to 
me overall.


Kind regards,

Paul

Re: [PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

[Damien Le Moal]

On 3/25/22 18:10, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>> sata_link_debounce() polls the SStatus register DET field to ensure that
>> a stable value is provided, to reliably detect device presence and PHY
>> readiness. Polling is done for at least timing->duration if there is no
>> device detected. For the device detected case, polling last up to
> 
> last*s*
> 
>> deadline to ensure that the PHY becomes ready.
>>
>> However, the PHY ready state is actually never checked, leading to the
>> poll loop duration always reaching the maximum duration.
>>
>> For adapters that do not require a debounce delay (link flag
>> ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates
> 
> no*t*?
> 
>> device present *and* PHY ready and bail out of the polling loop if it
>> does.
>>
>> While at it, add comments to clarify the various checks in
>> sata_link_debounce() polling loop.
> 
> Were you able to verify that the check now terminates the loop on your 
> devices earlier?

Yes it does for me. The function goes from taking about 100ms to taking
only one iteration of the loop, which has only one 5ms sleep.

> 
> Are below the right lines to compare? On the Dell OptiPlex 5055 (AMD FCH 
> SATA Controller [AHCI mode] [1022:7901]) I see:
> 
> Before (this very patch)
> 
>      [    0.428015] ata1: hard resetting link
>      [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> 
> After:
> 
>      [    0.428907] ata1: hard resetting link
>      [    0.648092] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> 
> So a decrease of (268 - 219 = 49) ms. Nice.

For a regular boot, sata_deb_timing_normal duration is used. So the save
is 100ms almost (there is still one iteration of the loop needed, which
takes interval = 5ms).

Together with the long 200ms sleep in sata_link_resume, the gain overall
should be about 300ms.

> 
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>   drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 87ad03c2e49f..15423723c9dd 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -276,8 +276,27 @@ int sata_link_debounce(struct ata_link *link,
>>   
>>   		/* DET stable? */
>>   		if (cur == last) {
>> +			/*
>> +			 * If the device presence was detected but PHY
>> +			 * communication is not yet established, wait until
>> +			 * deadline.
>> +			 */
>>   			if (cur == 1 && time_before(jiffies, deadline))
>>   				continue;
>> +
>> +			/*
>> +			 * If PHY is ready and the device is present, and the
>> +			 * driver did not request debounce delay, bail out early
>> +			 * assuming that the link is stable.
>> +			 */
>> +			if (cur == 3 &&
>> +			    !(link->flags & ATA_LFLAG_DEBOUNCE_DELAY))
>> +				return 0;
>> +
>> +			/*
>> +			 * If DET value has remained stable for
>> +			 * timing->duration, bail out.
>> +			 */
>>   			if (time_after(jiffies,
>>   				ata_deadline(last_jiffies, timing->duration)))
>>   				return 0;
>> @@ -288,8 +307,9 @@ int sata_link_debounce(struct ata_link *link,
>>   		last = cur;
>>   		last_jiffies = jiffies;
>>   
>> -		/* Check deadline.  If debouncing failed, return
>> -		 * -EPIPE to tell upper layer to lower link speed.
>> +		/*
>> +		 * If debouncing has not succeeded before dealine, return
> 
> dea*d*line
> 
>> +		 * -EPIPE to tell the upper layer to lower the link speed.
>>   		 */
>>   		if (time_after(jiffies, deadline))
>>   			return -EPIPE;
> 
> One separate for the new check would have been nice, but looks good to 
> me overall.
> 
> 
> Kind regards,
> 
> Paul


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

[Paul Menzel]

Dear Damien,


Am 28.03.22 um 09:19 schrieb Damien Le Moal:
> On 3/25/22 18:10, Paul Menzel wrote:

>> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>>> sata_link_debounce() polls the SStatus register DET field to ensure that
>>> a stable value is provided, to reliably detect device presence and PHY
>>> readiness. Polling is done for at least timing->duration if there is no
>>> device detected. For the device detected case, polling last up to
>>
>> last*s*
>>
>>> deadline to ensure that the PHY becomes ready.
>>>
>>> However, the PHY ready state is actually never checked, leading to the
>>> poll loop duration always reaching the maximum duration.
>>>
>>> For adapters that do not require a debounce delay (link flag
>>> ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates
>>
>> no*t*?
>>
>>> device present *and* PHY ready and bail out of the polling loop if it
>>> does.
>>>
>>> While at it, add comments to clarify the various checks in
>>> sata_link_debounce() polling loop.
>>
>> Were you able to verify that the check now terminates the loop on your
>> devices earlier?
> 
> Yes it does for me. The function goes from taking about 100ms to taking
> only one iteration of the loop, which has only one 5ms sleep.
> 
>>
>> Are below the right lines to compare? On the Dell OptiPlex 5055 (AMD FCH
>> SATA Controller [AHCI mode] [1022:7901]) I see:
>>
>> Before (this very patch)
>>
>>       [    0.428015] ata1: hard resetting link
>>       [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>
>> After:
>>
>>       [    0.428907] ata1: hard resetting link
>>       [    0.648092] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>
>> So a decrease of (268 - 219 = 49) ms. Nice.
> 
> For a regular boot, sata_deb_timing_normal duration is used. So the save
> is 100ms almost (there is still one iteration of the loop needed, which
> takes interval = 5ms).
> 
> Together with the long 200ms sleep in sata_link_resume, the gain overall
> should be about 300ms.

To my knowledge this was a regular boot (`systemctl reboot`). So where 
did the 50 ms go? ;-)


Kind regards,

Paul


>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> ---
>>>    drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++--
>>>    1 file changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>> index 87ad03c2e49f..15423723c9dd 100644
>>> --- a/drivers/ata/libata-sata.c
>>> +++ b/drivers/ata/libata-sata.c
>>> @@ -276,8 +276,27 @@ int sata_link_debounce(struct ata_link *link,
>>>    
>>>    		/* DET stable? */
>>>    		if (cur == last) {
>>> +			/*
>>> +			 * If the device presence was detected but PHY
>>> +			 * communication is not yet established, wait until
>>> +			 * deadline.
>>> +			 */
>>>    			if (cur == 1 && time_before(jiffies, deadline))
>>>    				continue;
>>> +
>>> +			/*
>>> +			 * If PHY is ready and the device is present, and the
>>> +			 * driver did not request debounce delay, bail out early
>>> +			 * assuming that the link is stable.
>>> +			 */
>>> +			if (cur == 3 &&
>>> +			    !(link->flags & ATA_LFLAG_DEBOUNCE_DELAY))
>>> +				return 0;
>>> +
>>> +			/*
>>> +			 * If DET value has remained stable for
>>> +			 * timing->duration, bail out.
>>> +			 */
>>>    			if (time_after(jiffies,
>>>    				ata_deadline(last_jiffies, timing->duration)))
>>>    				return 0;
>>> @@ -288,8 +307,9 @@ int sata_link_debounce(struct ata_link *link,
>>>    		last = cur;
>>>    		last_jiffies = jiffies;
>>>    
>>> -		/* Check deadline.  If debouncing failed, return
>>> -		 * -EPIPE to tell upper layer to lower link speed.
>>> +		/*
>>> +		 * If debouncing has not succeeded before dealine, return
>>
>> dea*d*line
>>
>>> +		 * -EPIPE to tell the upper layer to lower the link speed.
>>>    		 */
>>>    		if (time_after(jiffies, deadline))
>>>    			return -EPIPE;
>>
>> One separate for the new check would have been nice, but looks good to
>> me overall.
>>
>>
>> Kind regards,
>>
>> Paul

Re: [PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

[Damien Le Moal]

On 3/28/22 16:41, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 28.03.22 um 09:19 schrieb Damien Le Moal:
>> On 3/25/22 18:10, Paul Menzel wrote:
> 
>>> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>>>> sata_link_debounce() polls the SStatus register DET field to ensure that
>>>> a stable value is provided, to reliably detect device presence and PHY
>>>> readiness. Polling is done for at least timing->duration if there is no
>>>> device detected. For the device detected case, polling last up to
>>>
>>> last*s*
>>>
>>>> deadline to ensure that the PHY becomes ready.
>>>>
>>>> However, the PHY ready state is actually never checked, leading to the
>>>> poll loop duration always reaching the maximum duration.
>>>>
>>>> For adapters that do not require a debounce delay (link flag
>>>> ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates
>>>
>>> no*t*?
>>>
>>>> device present *and* PHY ready and bail out of the polling loop if it
>>>> does.
>>>>
>>>> While at it, add comments to clarify the various checks in
>>>> sata_link_debounce() polling loop.
>>>
>>> Were you able to verify that the check now terminates the loop on your
>>> devices earlier?
>>
>> Yes it does for me. The function goes from taking about 100ms to taking
>> only one iteration of the loop, which has only one 5ms sleep.
>>
>>>
>>> Are below the right lines to compare? On the Dell OptiPlex 5055 (AMD FCH
>>> SATA Controller [AHCI mode] [1022:7901]) I see:
>>>
>>> Before (this very patch)
>>>
>>>       [    0.428015] ata1: hard resetting link
>>>       [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>>
>>> After:
>>>
>>>       [    0.428907] ata1: hard resetting link
>>>       [    0.648092] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>>
>>> So a decrease of (268 - 219 = 49) ms. Nice.
>>
>> For a regular boot, sata_deb_timing_normal duration is used. So the save
>> is 100ms almost (there is still one iteration of the loop needed, which
>> takes interval = 5ms).
>>
>> Together with the long 200ms sleep in sata_link_resume, the gain overall
>> should be about 300ms.
> 
> To my knowledge this was a regular boot (`systemctl reboot`). So where 
> did the 50 ms go? ;-)

Not sure. But I do see variations. And I did add meesages around all the
debouncing in sata_link_resume() to get a better idea of the gains.

Working on a new version now, so I will be rechecking everything.

> 
> 
> Kind regards,
> 
> Paul
> 
> 
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>> ---
>>>>    drivers/ata/libata-sata.c | 24 ++++++++++++++++++++++--
>>>>    1 file changed, 22 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>>> index 87ad03c2e49f..15423723c9dd 100644
>>>> --- a/drivers/ata/libata-sata.c
>>>> +++ b/drivers/ata/libata-sata.c
>>>> @@ -276,8 +276,27 @@ int sata_link_debounce(struct ata_link *link,
>>>>    
>>>>    		/* DET stable? */
>>>>    		if (cur == last) {
>>>> +			/*
>>>> +			 * If the device presence was detected but PHY
>>>> +			 * communication is not yet established, wait until
>>>> +			 * deadline.
>>>> +			 */
>>>>    			if (cur == 1 && time_before(jiffies, deadline))
>>>>    				continue;
>>>> +
>>>> +			/*
>>>> +			 * If PHY is ready and the device is present, and the
>>>> +			 * driver did not request debounce delay, bail out early
>>>> +			 * assuming that the link is stable.
>>>> +			 */
>>>> +			if (cur == 3 &&
>>>> +			    !(link->flags & ATA_LFLAG_DEBOUNCE_DELAY))
>>>> +				return 0;
>>>> +
>>>> +			/*
>>>> +			 * If DET value has remained stable for
>>>> +			 * timing->duration, bail out.
>>>> +			 */
>>>>    			if (time_after(jiffies,
>>>>    				ata_deadline(last_jiffies, timing->duration)))
>>>>    				return 0;
>>>> @@ -288,8 +307,9 @@ int sata_link_debounce(struct ata_link *link,
>>>>    		last = cur;
>>>>    		last_jiffies = jiffies;
>>>>    
>>>> -		/* Check deadline.  If debouncing failed, return
>>>> -		 * -EPIPE to tell upper layer to lower link speed.
>>>> +		/*
>>>> +		 * If debouncing has not succeeded before dealine, return
>>>
>>> dea*d*line
>>>
>>>> +		 * -EPIPE to tell the upper layer to lower the link speed.
>>>>    		 */
>>>>    		if (time_after(jiffies, deadline))
>>>>    			return -EPIPE;
>>>
>>> One separate for the new check would have been nice, but looks good to
>>> me overall.
>>>
>>>
>>> Kind regards,
>>>
>>> Paul


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] ata: libata-sata: Improve sata_link_debounce()

[Damien Le Moal]

On 3/28/22 16:41, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 28.03.22 um 09:19 schrieb Damien Le Moal:
>> On 3/25/22 18:10, Paul Menzel wrote:
> 
>>> Am 23.03.22 um 09:17 schrieb Damien Le Moal:
>>>> sata_link_debounce() polls the SStatus register DET field to ensure that
>>>> a stable value is provided, to reliably detect device presence and PHY
>>>> readiness. Polling is done for at least timing->duration if there is no
>>>> device detected. For the device detected case, polling last up to
>>>
>>> last*s*
>>>
>>>> deadline to ensure that the PHY becomes ready.
>>>>
>>>> However, the PHY ready state is actually never checked, leading to the
>>>> poll loop duration always reaching the maximum duration.
>>>>
>>>> For adapters that do not require a debounce delay (link flag
>>>> ATA_LFLAG_DEBOUNCE_DELAY no set), add a check to test if DET indicates
>>>
>>> no*t*?
>>>
>>>> device present *and* PHY ready and bail out of the polling loop if it
>>>> does.
>>>>
>>>> While at it, add comments to clarify the various checks in
>>>> sata_link_debounce() polling loop.
>>>
>>> Were you able to verify that the check now terminates the loop on your
>>> devices earlier?
>>
>> Yes it does for me. The function goes from taking about 100ms to taking
>> only one iteration of the loop, which has only one 5ms sleep.
>>
>>>
>>> Are below the right lines to compare? On the Dell OptiPlex 5055 (AMD FCH
>>> SATA Controller [AHCI mode] [1022:7901]) I see:
>>>
>>> Before (this very patch)
>>>
>>>       [    0.428015] ata1: hard resetting link
>>>       [    0.696316] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>>
>>> After:
>>>
>>>       [    0.428907] ata1: hard resetting link
>>>       [    0.648092] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>>
>>> So a decrease of (268 - 219 = 49) ms. Nice.
>>
>> For a regular boot, sata_deb_timing_normal duration is used. So the save
>> is 100ms almost (there is still one iteration of the loop needed, which
>> takes interval = 5ms).
>>
>> Together with the long 200ms sleep in sata_link_resume, the gain overall
>> should be about 300ms.
> 
> To my knowledge this was a regular boot (`systemctl reboot`). So where 
> did the 50 ms go? ;-)

The device also sometimes need time to reach DET == 3 state... This is an
optimization that reduces the time waited to reach DET == 3. But we still
wait for it :)

On a hot reboot, as the device are already ready, I do see the time
reduced to one interval (5ms). But for a cold boot, it takes longer sometimes.


-- 
Damien Le Moal
Western Digital Research