linux-mmc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
@ 2020-02-04  8:54 Ulf Hansson
  2020-02-04  8:54 ` [PATCH 01/12] mmc: core: Throttle polling rate for CMD6 Ulf Hansson
                   ` (13 more replies)
  0 siblings, 14 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

There exists several separate variants of polling loops, used to detect when
the card stop signals busy for various operations, in the mmc core. All of them
have different issues that needs to be fixed.

The intent with this series, is to address some of these problems, via first
improving the mmc_poll_for_busy() function, then consolidate code by moving
more users to it.

While I was working on this, I stumbled over some code here and there, that
deserved some cleanup, hence I also folded in a couple of patches for this.

So far, I have only managed to extensively test the updated mmc_poll_for_busy()
function for CMD6 commands. Some tests for erase/trim/discard and for
HPI+sanitize are needed.

Note that, there are still separate polling loops in the mmc block layer, but
moving that to mmc_poll_for_busy() involves some additional work. I am looking
into that as a next step.

Please help review and test!

Kind regards
Ulf Hansson


Ulf Hansson (12):
  mmc: core: Throttle polling rate for CMD6
  mmc: core: Drop unused define
  mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
  mmc: core: Drop redundant in-parameter to __mmc_switch()
  mmc: core: Split up mmc_poll_for_busy()
  mmc: core: Enable re-use of mmc_blk_in_tran_state()
  mmc: core: Update CMD13 busy check for CMD6 commands
  mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
  mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
  mmc: core: Convert to mmc_poll_for_busy() for HPI commands
  mmc: core: Fixup support for HW busy detection for HPI commands
  mmc: core: Re-work the error path for the eMMC sanitize command

 drivers/mmc/core/block.c   |  55 +++++--------
 drivers/mmc/core/core.c    |  53 +------------
 drivers/mmc/core/mmc.c     |  38 ++++-----
 drivers/mmc/core/mmc_ops.c | 159 ++++++++++++++++++++++---------------
 drivers/mmc/core/mmc_ops.h |  13 ++-
 include/linux/mmc/core.h   |   3 -
 include/linux/mmc/mmc.h    |  10 +++
 7 files changed, 157 insertions(+), 174 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 01/12] mmc: core: Throttle polling rate for CMD6
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-12 13:51   ` Ludovic BARRE
  2020-02-04  8:54 ` [PATCH 02/12] mmc: core: Drop unused define Ulf Hansson
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

In mmc_poll_for_busy() we loop continuously, either by sending a CMD13 or
by invoking the ->card_busy() host ops, as to detect when the card stops
signaling busy. This behaviour is problematic as it may cause CPU hogging,
especially when the busy signal time reaches beyond a few ms.

Let's fix the issue by adding a throttling mechanism, that inserts a
usleep_range() in between the polling attempts. The sleep range starts at
16-32us, but increases for each loop by a factor of 2, up until the range
reaches ~32-64ms. In this way, we are able to keep the loop fine-grained
enough for short busy signaling times, while also not hogging the CPU for
longer times.

Note that, this change is inspired by the similar throttling mechanism that
we already use for mmc_do_erase().

Reported-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index da425ee2d9bf..446a37cc2a86 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -456,6 +456,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 	struct mmc_host *host = card->host;
 	int err;
 	unsigned long timeout;
+	unsigned int udelay = 32, udelay_max = 32768;
 	u32 status = 0;
 	bool expired = false;
 	bool busy = false;
@@ -500,6 +501,13 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 				mmc_hostname(host), __func__);
 			return -ETIMEDOUT;
 		}
+
+		/* Throttle the polling rate to avoid hogging the CPU. */
+		if (busy) {
+			usleep_range(udelay, udelay * 2);
+			if (udelay < udelay_max)
+				udelay *= 2;
+		}
 	} while (busy);
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 02/12] mmc: core: Drop unused define
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
  2020-02-04  8:54 ` [PATCH 01/12] mmc: core: Throttle polling rate for CMD6 Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 03/12] mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status() Ulf Hansson
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

The last user of MMC_OPS_TIMEOUT_MS was recently removed, however the
define stayed around. Let's remove it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 446a37cc2a86..35df97fe9cdc 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -19,7 +19,6 @@
 #include "host.h"
 #include "mmc_ops.h"
 
-#define MMC_OPS_TIMEOUT_MS		(10 * 60 * 1000) /* 10min*/
 #define MMC_BKOPS_TIMEOUT_MS		(120 * 1000) /* 120s */
 #define MMC_CACHE_FLUSH_TIMEOUT_MS	(30 * 1000) /* 30s */
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 03/12] mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
  2020-02-04  8:54 ` [PATCH 01/12] mmc: core: Throttle polling rate for CMD6 Ulf Hansson
  2020-02-04  8:54 ` [PATCH 02/12] mmc: core: Drop unused define Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 04/12] mmc: core: Drop redundant in-parameter to __mmc_switch() Ulf Hansson
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

To simplify code, let's extend mmc_switch_status() to cope with needs
addressed in __mmc_switch_status(). Then move all users to the updated
mmc_switch_status() API and drop __mmc_switch_status() altogether.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c     | 16 ++++++++--------
 drivers/mmc/core/mmc_ops.c |  9 ++-------
 drivers/mmc/core/mmc_ops.h |  3 +--
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index f6912ded652d..8a1f64065a47 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1173,7 +1173,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	max_dtr = card->ext_csd.hs_max_dtr;
 	mmc_set_clock(host, max_dtr);
 
-	err = mmc_switch_status(card);
+	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
 
@@ -1211,7 +1211,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	if (host->ops->hs400_complete)
 		host->ops->hs400_complete(host);
 
-	err = mmc_switch_status(card);
+	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
 
@@ -1249,7 +1249,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 
 	mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
 
-	err = mmc_switch_status(card);
+	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
 
@@ -1265,7 +1265,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	if (host->ops->hs400_downgrade)
 		host->ops->hs400_downgrade(host);
 
-	err = mmc_switch_status(card);
+	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
 
@@ -1285,7 +1285,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	 * failed. If there really is a problem, we would expect tuning will
 	 * fail and the result ends up the same.
 	 */
-	err = __mmc_switch_status(card, false);
+	err = mmc_switch_status(card, false);
 	if (err)
 		goto out_err;
 
@@ -1366,7 +1366,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	}
 
 	mmc_set_timing(host, MMC_TIMING_MMC_HS);
-	err = mmc_switch_status(card);
+	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
 
@@ -1407,7 +1407,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	if (host->ops->hs400_enhanced_strobe)
 		host->ops->hs400_enhanced_strobe(host, &host->ios);
 
-	err = mmc_switch_status(card);
+	err = mmc_switch_status(card, true);
 	if (err)
 		goto out_err;
 
@@ -1468,7 +1468,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 		 * switch failed. If there really is a problem, we would expect
 		 * tuning will fail and the result ends up the same.
 		 */
-		err = __mmc_switch_status(card, false);
+		err = mmc_switch_status(card, false);
 
 		/*
 		 * mmc_select_timing() assumes timing has not changed if
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 35df97fe9cdc..d2371612d536 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -430,7 +430,7 @@ static int mmc_switch_status_error(struct mmc_host *host, u32 status)
 }
 
 /* Caller must hold re-tuning */
-int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
+int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
 {
 	u32 status;
 	int err;
@@ -444,11 +444,6 @@ int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
 	return mmc_switch_status_error(card->host, status);
 }
 
-int mmc_switch_status(struct mmc_card *card)
-{
-	return __mmc_switch_status(card, true);
-}
-
 static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 			bool send_status, bool retry_crc_err)
 {
@@ -594,7 +589,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		mmc_set_timing(host, timing);
 
 	if (send_status) {
-		err = mmc_switch_status(card);
+		err = mmc_switch_status(card, true);
 		if (err && timing)
 			mmc_set_timing(host, old_timing);
 	}
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 8f2f9475716d..09dee8a466a0 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -29,8 +29,7 @@ int mmc_bus_test(struct mmc_card *card, u8 bus_width);
 int mmc_interrupt_hpi(struct mmc_card *card);
 int mmc_can_ext_csd(struct mmc_card *card);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
-int mmc_switch_status(struct mmc_card *card);
-int __mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
+int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, unsigned char timing,
 		bool use_busy_signal, bool send_status,	bool retry_crc_err);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 04/12] mmc: core: Drop redundant in-parameter to __mmc_switch()
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (2 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 03/12] mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status() Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 05/12] mmc: core: Split up mmc_poll_for_busy() Ulf Hansson
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

The use_busy_signal in-parameter is set true by all callers of
__mmc_switch(), hence it's redundant so drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc.c     | 22 +++++++++++-----------
 drivers/mmc/core/mmc_ops.c | 11 +++--------
 drivers/mmc/core/mmc_ops.h |  2 +-
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 8a1f64065a47..648c1c79282f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1055,7 +1055,7 @@ static int mmc_select_hs(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
 			   card->ext_csd.generic_cmd6_time, MMC_TIMING_MMC_HS,
-			   true, true, true);
+			   true, true);
 	if (err)
 		pr_warn("%s: switch to high-speed failed, err:%d\n",
 			mmc_hostname(card->host), err);
@@ -1087,7 +1087,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
 			   ext_csd_bits,
 			   card->ext_csd.generic_cmd6_time,
 			   MMC_TIMING_MMC_DDR52,
-			   true, true, true);
+			   true, true);
 	if (err) {
 		pr_err("%s: switch to bus width %d ddr failed\n",
 			mmc_hostname(host), 1 << bus_width);
@@ -1155,7 +1155,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+			   false, true);
 	if (err) {
 		pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1197,7 +1197,7 @@ static int mmc_select_hs400(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+			   false, true);
 	if (err) {
 		pr_err("%s: switch to hs400 failed, err:%d\n",
 			 mmc_hostname(host), err);
@@ -1243,7 +1243,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	val = EXT_CSD_TIMING_HS;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
 			   val, card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+			   false, true);
 	if (err)
 		goto out_err;
 
@@ -1256,7 +1256,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	/* Switch HS DDR to HS */
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
 			   EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
-			   0, true, false, true);
+			   0, false, true);
 	if (err)
 		goto out_err;
 
@@ -1274,7 +1274,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
 	      card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
 			   val, card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+			   false, true);
 	if (err)
 		goto out_err;
 
@@ -1358,7 +1358,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+			   false, true);
 	if (err) {
 		pr_err("%s: switch to hs for hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1392,7 +1392,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			   EXT_CSD_HS_TIMING, val,
 			   card->ext_csd.generic_cmd6_time, 0,
-			   true, false, true);
+			   false, true);
 	if (err) {
 		pr_err("%s: switch to hs400es failed, err:%d\n",
 			mmc_hostname(host), err);
@@ -1457,7 +1457,7 @@ static int mmc_select_hs200(struct mmc_card *card)
 		err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 				   EXT_CSD_HS_TIMING, val,
 				   card->ext_csd.generic_cmd6_time, 0,
-				   true, false, true);
+				   false, true);
 		if (err)
 			goto err;
 		old_timing = host->ios.timing;
@@ -1955,7 +1955,7 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
 			EXT_CSD_POWER_OFF_NOTIFICATION,
-			notify_type, timeout, 0, true, false, false);
+			notify_type, timeout, 0, false, false);
 	if (err)
 		pr_err("%s: Power Off Notification timed out, %u\n",
 		       mmc_hostname(card->host), timeout);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index d2371612d536..1c5b23d99b77 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -516,7 +516,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
  *	@timeout_ms: timeout (ms) for operation performed by register write,
  *                   timeout of zero implies maximum possible timeout
  *	@timing: new timing to change to
- *	@use_busy_signal: use the busy signal as response type
  *	@send_status: send status cmd to poll for busy
  *	@retry_crc_err: retry when CRC errors when polling with CMD13 for busy
  *
@@ -524,12 +523,12 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
  */
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, unsigned char timing,
-		bool use_busy_signal, bool send_status,	bool retry_crc_err)
+		bool send_status, bool retry_crc_err)
 {
 	struct mmc_host *host = card->host;
 	int err;
 	struct mmc_command cmd = {};
-	bool use_r1b_resp = use_busy_signal;
+	bool use_r1b_resp = true;
 	unsigned char old_timing = host->ios.timing;
 
 	mmc_retune_hold(host);
@@ -569,10 +568,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	if (err)
 		goto out;
 
-	/* No need to check card status in case of unblocking command */
-	if (!use_busy_signal)
-		goto out;
-
 	/*If SPI or used HW busy detection above, then we don't need to poll. */
 	if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||
 		mmc_host_is_spi(host))
@@ -603,7 +598,7 @@ int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms)
 {
 	return __mmc_switch(card, set, index, value, timeout_ms, 0,
-			true, true, false);
+			    true, false);
 }
 EXPORT_SYMBOL_GPL(mmc_switch);
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 09dee8a466a0..de0c509a3a38 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -32,7 +32,7 @@ int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
 int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, unsigned char timing,
-		bool use_busy_signal, bool send_status,	bool retry_crc_err);
+		bool send_status, bool retry_crc_err);
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms);
 void mmc_run_bkops(struct mmc_card *card);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 05/12] mmc: core: Split up mmc_poll_for_busy()
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (3 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 04/12] mmc: core: Drop redundant in-parameter to __mmc_switch() Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 06/12] mmc: core: Enable re-use of mmc_blk_in_tran_state() Ulf Hansson
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

To make the code more readable, move the part that gets the busy status of
the card out into a separate function, mmc_busy_status(). Then call it from
mmc_poll_for_busy().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 47 +++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 1c5b23d99b77..c17b13aacc6e 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -444,6 +444,34 @@ int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
 	return mmc_switch_status_error(card->host, status);
 }
 
+static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
+			   bool *busy)
+{
+	struct mmc_host *host = card->host;
+	u32 status = 0;
+	int err;
+
+	if (host->ops->card_busy) {
+		*busy = host->ops->card_busy(host);
+		return 0;
+	}
+
+	err = mmc_send_status(card, &status);
+	if (retry_crc_err && err == -EILSEQ) {
+		*busy = true;
+		return 0;
+	}
+	if (err)
+		return err;
+
+	err = mmc_switch_status_error(card->host, status);
+	if (err)
+		return err;
+
+	*busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
+	return 0;
+}
+
 static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 			bool send_status, bool retry_crc_err)
 {
@@ -451,7 +479,6 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 	int err;
 	unsigned long timeout;
 	unsigned int udelay = 32, udelay_max = 32768;
-	u32 status = 0;
 	bool expired = false;
 	bool busy = false;
 
@@ -473,21 +500,9 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		 */
 		expired = time_after(jiffies, timeout);
 
-		if (host->ops->card_busy) {
-			busy = host->ops->card_busy(host);
-		} else {
-			err = mmc_send_status(card, &status);
-			if (retry_crc_err && err == -EILSEQ) {
-				busy = true;
-			} else if (err) {
-				return err;
-			} else {
-				err = mmc_switch_status_error(host, status);
-				if (err)
-					return err;
-				busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
-			}
-		}
+		err = mmc_busy_status(card, retry_crc_err, &busy);
+		if (err)
+			return err;
 
 		/* Timeout if the device still remains busy. */
 		if (expired && busy) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 06/12] mmc: core: Enable re-use of mmc_blk_in_tran_state()
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (4 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 05/12] mmc: core: Split up mmc_poll_for_busy() Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 07/12] mmc: core: Update CMD13 busy check for CMD6 commands Ulf Hansson
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

To allow subsequent changes to re-use the code from the static function
mmc_blk_in_tran_state(), let's move it to a public header. While at it,
let's also rename it to mmc_ready_for_data(), as to try to better describe
its purpose.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/block.c | 24 ++++--------------------
 include/linux/mmc/mmc.h  | 10 ++++++++++
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 663d87924e5e..8ac12e3fff27 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -436,16 +436,6 @@ static int ioctl_do_sanitize(struct mmc_card *card)
 	return err;
 }
 
-static inline bool mmc_blk_in_tran_state(u32 status)
-{
-	/*
-	 * Some cards mishandle the status bits, so make sure to check both the
-	 * busy indication and the card state.
-	 */
-	return status & R1_READY_FOR_DATA &&
-	       (R1_CURRENT_STATE(status) == R1_STATE_TRAN);
-}
-
 static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
 			    u32 *resp_errs)
 {
@@ -477,13 +467,7 @@ static int card_busy_detect(struct mmc_card *card, unsigned int timeout_ms,
 				 __func__, status);
 			return -ETIMEDOUT;
 		}
-
-		/*
-		 * Some cards mishandle the status bits,
-		 * so make sure to check both the busy
-		 * indication and the card state.
-		 */
-	} while (!mmc_blk_in_tran_state(status));
+	} while (!mmc_ready_for_data(status));
 
 	return err;
 }
@@ -1666,7 +1650,7 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req)
 			goto error_exit;
 
 		if (!mmc_host_is_spi(host) &&
-		    !mmc_blk_in_tran_state(status)) {
+		    !mmc_ready_for_data(status)) {
 			err = mmc_blk_fix_state(card, req);
 			if (err)
 				goto error_exit;
@@ -1726,7 +1710,7 @@ static bool mmc_blk_status_error(struct request *req, u32 status)
 	return brq->cmd.resp[0]  & CMD_ERRORS    ||
 	       brq->stop.resp[0] & stop_err_bits ||
 	       status            & stop_err_bits ||
-	       (rq_data_dir(req) == WRITE && !mmc_blk_in_tran_state(status));
+	       (rq_data_dir(req) == WRITE && !mmc_ready_for_data(status));
 }
 
 static inline bool mmc_blk_cmd_started(struct mmc_blk_request *brq)
@@ -1788,7 +1772,7 @@ static void mmc_blk_mq_rw_recovery(struct mmc_queue *mq, struct request *req)
 
 	/* Try to get back to "tran" state */
 	if (!mmc_host_is_spi(mq->card->host) &&
-	    (err || !mmc_blk_in_tran_state(status)))
+	    (err || !mmc_ready_for_data(status)))
 		err = mmc_blk_fix_state(mq->card, req);
 
 	/*
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index 897a87c4c827..4b85ef05a906 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -161,6 +161,16 @@ static inline bool mmc_op_multi(u32 opcode)
 #define R1_STATE_PRG	7
 #define R1_STATE_DIS	8
 
+static inline bool mmc_ready_for_data(u32 status)
+{
+	/*
+	 * Some cards mishandle the status bits, so make sure to check both the
+	 * busy indication and the card state.
+	 */
+	return status & R1_READY_FOR_DATA &&
+	       R1_CURRENT_STATE(status) == R1_STATE_TRAN;
+}
+
 /*
  * MMC/SD in SPI mode reports R1 status always, and R2 for SEND_STATUS
  * R1 is the low order byte; R2 is the next highest byte, when present.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 07/12] mmc: core: Update CMD13 busy check for CMD6 commands
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (5 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 06/12] mmc: core: Enable re-use of mmc_blk_in_tran_state() Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 08/12] mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard Ulf Hansson
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

Through mmc_poll_for_busy() a CMD13 may be sent to get the status of the
(e)MMC card. If the state of the card is R1_STATE_PRG, the card is
considered as being busy, which means we continue to poll with CMD13. This
seems to be sufficient, but it's also unnecessary fragile, as it means a
new command/request could potentially be sent to the card when it's in an
unknown state.

To try to improve the situation, but also to move towards a more consistent
CMD13 polling behaviour in the mmc core, let's deploy the same policy we
use for regular I/O write requests. In other words, let's check that card
returns to the R1_STATE_TRAN and that the R1_READY_FOR_DATA bit is set in
the CMD13 response, before exiting the polling loop.

Note that, potentially this changed behaviour could lead to unnecessary
waiting for the timeout to expire, if the card for some reason, moves to an
unexpected error state. However, as we bail out from the polling loop when
R1_SWITCH_ERROR bit is set or when the CMD13 fails, this shouldn't be an
issue.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index c17b13aacc6e..c14e24570b4e 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -468,7 +468,7 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
 	if (err)
 		return err;
 
-	*busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
+	*busy = !mmc_ready_for_data(status);
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 08/12] mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (6 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 07/12] mmc: core: Update CMD13 busy check for CMD6 commands Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 09/12] mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd() Ulf Hansson
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

Rather than open coding the polling loop in mmc_do_erase(), let's convert
to use mmc_poll_for_busy().

To allow a slightly different error parsing during polling, compared to the
__mmc_switch() case, a new in-parameter to mmc_poll_for_busy() is needed,
but other than that the conversion is straight forward.

Besides addressing the open coding issue, moving to mmc_poll_for_busy() for
erase/trim/discard improves the behaviour according to below.

- Adds support for polling via the optional ->card_busy() host ops.
- Returns zero to indicate success when the final polling attempt finds the
  card non-busy, even if the timeout expired.
- Exits the polling loop when state moves to R1_STATE_TRAN, rather than
  when leaving R1_STATE_PRG.
- Decreases the starting range for throttling to 32-64us.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/core.c    | 36 ++----------------------------------
 drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++++++++++++------
 drivers/mmc/core/mmc_ops.h |  7 +++++++
 3 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index aa54d359dab7..6b38c194d74f 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1658,8 +1658,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	struct mmc_command cmd = {};
 	unsigned int qty = 0, busy_timeout = 0;
 	bool use_r1b_resp = false;
-	unsigned long timeout;
-	int loop_udelay=64, udelay_max=32768;
 	int err;
 
 	mmc_retune_hold(card->host);
@@ -1760,38 +1758,8 @@ static int mmc_do_erase(struct mmc_card *card, unsigned int from,
 	if ((card->host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
 		goto out;
 
-	timeout = jiffies + msecs_to_jiffies(busy_timeout);
-	do {
-		memset(&cmd, 0, sizeof(struct mmc_command));
-		cmd.opcode = MMC_SEND_STATUS;
-		cmd.arg = card->rca << 16;
-		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
-		/* Do not retry else we can't see errors */
-		err = mmc_wait_for_cmd(card->host, &cmd, 0);
-		if (err || R1_STATUS(cmd.resp[0])) {
-			pr_err("error %d requesting status %#x\n",
-				err, cmd.resp[0]);
-			err = -EIO;
-			goto out;
-		}
-
-		/* Timeout if the device never becomes ready for data and
-		 * never leaves the program state.
-		 */
-		if (time_after(jiffies, timeout)) {
-			pr_err("%s: Card stuck in programming state! %s\n",
-				mmc_hostname(card->host), __func__);
-			err =  -EIO;
-			goto out;
-		}
-		if ((cmd.resp[0] & R1_READY_FOR_DATA) &&
-		    R1_CURRENT_STATE(cmd.resp[0]) != R1_STATE_PRG)
-			break;
-
-		usleep_range(loop_udelay, loop_udelay*2);
-		if (loop_udelay < udelay_max)
-			loop_udelay *= 2;
-	} while (1);
+	/* Let's poll to find out when the erase operation completes. */
+	err = mmc_poll_for_busy(card, busy_timeout, MMC_BUSY_ERASE);
 
 out:
 	mmc_retune_release(card->host);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index c14e24570b4e..5643277a4eeb 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -445,7 +445,7 @@ int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal)
 }
 
 static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
-			   bool *busy)
+			   enum mmc_busy_cmd busy_cmd, bool *busy)
 {
 	struct mmc_host *host = card->host;
 	u32 status = 0;
@@ -464,7 +464,17 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
 	if (err)
 		return err;
 
-	err = mmc_switch_status_error(card->host, status);
+	switch (busy_cmd) {
+	case MMC_BUSY_CMD6:
+		err = mmc_switch_status_error(card->host, status);
+		break;
+	case MMC_BUSY_ERASE:
+		err = R1_STATUS(status) ? -EIO : 0;
+		break;
+	default:
+		err = -EINVAL;
+	}
+
 	if (err)
 		return err;
 
@@ -472,8 +482,9 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
 	return 0;
 }
 
-static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
-			bool send_status, bool retry_crc_err)
+static int __mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+			       bool send_status, bool retry_crc_err,
+			       enum mmc_busy_cmd busy_cmd)
 {
 	struct mmc_host *host = card->host;
 	int err;
@@ -500,7 +511,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 		 */
 		expired = time_after(jiffies, timeout);
 
-		err = mmc_busy_status(card, retry_crc_err, &busy);
+		err = mmc_busy_status(card, retry_crc_err, busy_cmd, &busy);
 		if (err)
 			return err;
 
@@ -522,6 +533,12 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 	return 0;
 }
 
+int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+		      enum mmc_busy_cmd busy_cmd)
+{
+	return __mmc_poll_for_busy(card, timeout_ms, true, false, busy_cmd);
+}
+
 /**
  *	__mmc_switch - modify EXT_CSD register
  *	@card: the MMC card associated with the data transfer
@@ -589,7 +606,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		goto out_tim;
 
 	/* Let's try to poll to find out when the command is completed. */
-	err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);
+	err = __mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err,
+				  MMC_BUSY_CMD6);
 	if (err)
 		goto out;
 
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index de0c509a3a38..8cd05fb583da 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -10,6 +10,11 @@
 
 #include <linux/types.h>
 
+enum mmc_busy_cmd {
+	MMC_BUSY_CMD6,
+	MMC_BUSY_ERASE,
+};
+
 struct mmc_host;
 struct mmc_card;
 
@@ -30,6 +35,8 @@ int mmc_interrupt_hpi(struct mmc_card *card);
 int mmc_can_ext_csd(struct mmc_card *card);
 int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
 int mmc_switch_status(struct mmc_card *card, bool crc_err_fatal);
+int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+		      enum mmc_busy_cmd busy_cmd);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, unsigned char timing,
 		bool send_status, bool retry_crc_err);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 09/12] mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (7 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 08/12] mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 10/12] mmc: core: Convert to mmc_poll_for_busy() for HPI commands Ulf Hansson
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

The 'u32 *status' is unused by the caller, so let's drop it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 5643277a4eeb..4d8f90d01740 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -827,7 +827,7 @@ int mmc_bus_test(struct mmc_card *card, u8 bus_width)
 	return mmc_send_bus_test(card, card->host, MMC_BUS_TEST_R, width);
 }
 
-static int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status)
+static int mmc_send_hpi_cmd(struct mmc_card *card)
 {
 	struct mmc_command cmd = {};
 	unsigned int opcode;
@@ -849,8 +849,6 @@ static int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status)
 			err, cmd.resp[0]);
 		return err;
 	}
-	if (status)
-		*status = cmd.resp[0];
 
 	return 0;
 }
@@ -899,7 +897,7 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 		goto out;
 	}
 
-	err = mmc_send_hpi_cmd(card, &status);
+	err = mmc_send_hpi_cmd(card);
 	if (err)
 		goto out;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 10/12] mmc: core: Convert to mmc_poll_for_busy() for HPI commands
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (8 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 09/12] mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd() Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 11/12] mmc: core: Fixup support for HW busy detection " Ulf Hansson
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

Rather than open coding the polling loop in mmc_interrupt_hpi(), let's
convert to use mmc_poll_for_busy().

Note that, moving to mmc_poll_for_busy() for HPI also improves the
behaviour according to below.

- Adds support for polling via the optional ->card_busy() host ops.
- Require R1_READY_FOR_DATA to be set in the CMD13 response before exiting
  the polling loop.
- Adds a throttling mechanism to avoid CPU hogging when polling.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 20 +++++---------------
 drivers/mmc/core/mmc_ops.h |  1 +
 2 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 4d8f90d01740..87d54a559b31 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -471,6 +471,8 @@ static int mmc_busy_status(struct mmc_card *card, bool retry_crc_err,
 	case MMC_BUSY_ERASE:
 		err = R1_STATUS(status) ? -EIO : 0;
 		break;
+	case MMC_BUSY_HPI:
+		break;
 	default:
 		err = -EINVAL;
 	}
@@ -829,6 +831,7 @@ int mmc_bus_test(struct mmc_card *card, u8 bus_width)
 
 static int mmc_send_hpi_cmd(struct mmc_card *card)
 {
+	unsigned int busy_timeout_ms = card->ext_csd.out_of_int_time;
 	struct mmc_command cmd = {};
 	unsigned int opcode;
 	int err;
@@ -850,7 +853,8 @@ static int mmc_send_hpi_cmd(struct mmc_card *card)
 		return err;
 	}
 
-	return 0;
+	/* Let's poll to find out when the HPI request completes. */
+	return mmc_poll_for_busy(card, busy_timeout_ms, MMC_BUSY_HPI);
 }
 
 /**
@@ -864,7 +868,6 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 {
 	int err;
 	u32 status;
-	unsigned long prg_wait;
 
 	if (!card->ext_csd.hpi_en) {
 		pr_info("%s: HPI enable bit unset\n", mmc_hostname(card->host));
@@ -898,19 +901,6 @@ int mmc_interrupt_hpi(struct mmc_card *card)
 	}
 
 	err = mmc_send_hpi_cmd(card);
-	if (err)
-		goto out;
-
-	prg_wait = jiffies + msecs_to_jiffies(card->ext_csd.out_of_int_time);
-	do {
-		err = mmc_send_status(card, &status);
-
-		if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN)
-			break;
-		if (time_after(jiffies, prg_wait))
-			err = -ETIMEDOUT;
-	} while (!err);
-
 out:
 	return err;
 }
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 8cd05fb583da..38dcfeeaf6d5 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -13,6 +13,7 @@
 enum mmc_busy_cmd {
 	MMC_BUSY_CMD6,
 	MMC_BUSY_ERASE,
+	MMC_BUSY_HPI,
 };
 
 struct mmc_host;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 11/12] mmc: core: Fixup support for HW busy detection for HPI commands
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (9 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 10/12] mmc: core: Convert to mmc_poll_for_busy() for HPI commands Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-04  8:54 ` [PATCH 12/12] mmc: core: Re-work the error path for the eMMC sanitize command Ulf Hansson
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

In case the host specify a max_busy_timeout, we need to validate that the
needed timeout for the HPI command conforms to that requirement. If that's
not the case, let's convert from a R1B response to a R1 response, as to
instruct the host to avoid HW busy detection.

Additionally, when R1B is used we must also inform the host about the busy
timeout for the command, so let's do that via updating cmd.busy_timeout.

Finally, when R1B is used and in case the host supports HW busy detection,
there should be no need for doing polling, so then skip that.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/mmc_ops.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 87d54a559b31..aa0cab190cd8 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -832,27 +832,41 @@ int mmc_bus_test(struct mmc_card *card, u8 bus_width)
 static int mmc_send_hpi_cmd(struct mmc_card *card)
 {
 	unsigned int busy_timeout_ms = card->ext_csd.out_of_int_time;
+	struct mmc_host *host = card->host;
+	bool use_r1b_resp = true;
 	struct mmc_command cmd = {};
-	unsigned int opcode;
 	int err;
 
-	opcode = card->ext_csd.hpi_cmd;
-	if (opcode == MMC_STOP_TRANSMISSION)
+	cmd.opcode = card->ext_csd.hpi_cmd;
+	cmd.arg = card->rca << 16 | 1;
+
+	/*
+	 * Make sure the host's max_busy_timeout fit the needed timeout for HPI.
+	 * In case it doesn't, let's instruct the host to avoid HW busy
+	 * detection, by using a R1 response instead of R1B.
+	 */
+	if (host->max_busy_timeout && busy_timeout_ms > host->max_busy_timeout)
+		use_r1b_resp = false;
+
+	if (cmd.opcode == MMC_STOP_TRANSMISSION && use_r1b_resp) {
 		cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
-	else if (opcode == MMC_SEND_STATUS)
+		cmd.busy_timeout = busy_timeout_ms;
+	} else {
 		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+		use_r1b_resp = false;
+	}
 
-	cmd.opcode = opcode;
-	cmd.arg = card->rca << 16 | 1;
-
-	err = mmc_wait_for_cmd(card->host, &cmd, 0);
+	err = mmc_wait_for_cmd(host, &cmd, 0);
 	if (err) {
-		pr_warn("%s: error %d interrupting operation. "
-			"HPI command response %#x\n", mmc_hostname(card->host),
-			err, cmd.resp[0]);
+		pr_warn("%s: HPI error %d. Command response %#x\n",
+			mmc_hostname(host), err, cmd.resp[0]);
 		return err;
 	}
 
+	/* No need to poll when using HW busy detection. */
+	if (host->caps & MMC_CAP_WAIT_WHILE_BUSY && use_r1b_resp)
+		return 0;
+
 	/* Let's poll to find out when the HPI request completes. */
 	return mmc_poll_for_busy(card, busy_timeout_ms, MMC_BUSY_HPI);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 12/12] mmc: core: Re-work the error path for the eMMC sanitize command
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (10 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 11/12] mmc: core: Fixup support for HW busy detection " Ulf Hansson
@ 2020-02-04  8:54 ` Ulf Hansson
  2020-02-11 22:29   ` kbuild test robot
  2020-02-11 13:17 ` [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Baolin Wang
  2020-02-18 23:38 ` Ulf Hansson
  13 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2020-02-04  8:54 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Adrian Hunter, Wolfram Sang, Ludovic Barre, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

The error path for sanitize operations that returns with a -ETIMEDOUT error
code, is for some reason very tightly coupled with the internal request
handling code of the mmc core. For example, mmc_wait_for_req_done() runs
code at completion of requests, to check specific sanitize errors. This is
inefficient, as at it affects all types of requests.

To improve the behaviour, let's move the error management for sanitize
requests into ioctl_do_sanitize(), as it's really there it belongs. Moving
the error handling requires retuning to be held, so let's do that.

While updating this code, let's also take the opportunity to clean it up a
bit.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/block.c   | 33 +++++++++++++++++++--------------
 drivers/mmc/core/core.c    | 17 -----------------
 drivers/mmc/core/mmc_ops.c |  3 ---
 include/linux/mmc/core.h   |  3 ---
 4 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 8ac12e3fff27..db59c51052df 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -410,29 +410,34 @@ static int mmc_blk_ioctl_copy_to_user(struct mmc_ioc_cmd __user *ic_ptr,
 
 static int ioctl_do_sanitize(struct mmc_card *card)
 {
+	struct mmc_host *host = card->host;
 	int err;
 
 	if (!mmc_can_sanitize(card)) {
-			pr_warn("%s: %s - SANITIZE is not supported\n",
-				mmc_hostname(card->host), __func__);
-			err = -EOPNOTSUPP;
-			goto out;
+		pr_warn("%s: SANITIZE is not supported\n", mmc_hostname(host));
+		return -EOPNOTSUPP;
 	}
 
-	pr_debug("%s: %s - SANITIZE IN PROGRESS...\n",
-		mmc_hostname(card->host), __func__);
+	pr_debug("%s: SANITIZE IN PROGRESS...\n", mmc_hostname(host));
 
-	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-					EXT_CSD_SANITIZE_START, 1,
-					MMC_SANITIZE_REQ_TIMEOUT);
+	mmc_retune_hold(host);
 
+	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_SANITIZE_START,
+			 1, MMC_SANITIZE_REQ_TIMEOUT);
 	if (err)
-		pr_err("%s: %s - EXT_CSD_SANITIZE_START failed. err=%d\n",
-		       mmc_hostname(card->host), __func__, err);
+		pr_err("%s: SANITIZE failed err=%d\n", mmc_hostname(host), err);
 
-	pr_debug("%s: %s - SANITIZE COMPLETED\n", mmc_hostname(card->host),
-					     __func__);
-out:
+	/*
+	 * If the santize operation timed out, the card is probably still busy
+	 * in the R1_STATE_PRG. Rather than continue to wait, let's try to abort
+	 * it with a HPI command to get back into R1_STATE_TRAN.
+	 */
+	if (err == -ETIMEDOUT && !mmc_interrupt_hpi(card))
+		pr_warn("%s: Sanitize aborted\n", mmc_hostname(host));
+
+	mmc_retune_release(host);
+
+	pr_debug("%s: SANITIZE COMPLETED\n", mmc_hostname(host));
 	return err;
 }
 
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6b38c194d74f..95db8ffbdd35 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -403,23 +403,6 @@ void mmc_wait_for_req_done(struct mmc_host *host, struct mmc_request *mrq)
 
 		cmd = mrq->cmd;
 
-		/*
-		 * If host has timed out waiting for the sanitize
-		 * to complete, card might be still in programming state
-		 * so let's try to bring the card out of programming
-		 * state.
-		 */
-		if (cmd->sanitize_busy && cmd->error == -ETIMEDOUT) {
-			if (!mmc_interrupt_hpi(host->card)) {
-				pr_warn("%s: %s: Interrupted sanitize\n",
-					mmc_hostname(host), __func__);
-				cmd->error = 0;
-				break;
-			} else {
-				pr_err("%s: %s: Failed to interrupt sanitize\n",
-				       mmc_hostname(host), __func__);
-			}
-		}
 		if (!cmd->error || !cmd->retries ||
 		    mmc_card_removed(host->card))
 			break;
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index aa0cab190cd8..c08f8b723a3b 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -595,9 +595,6 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		cmd.flags |= MMC_RSP_SPI_R1 | MMC_RSP_R1;
 	}
 
-	if (index == EXT_CSD_SANITIZE_START)
-		cmd.sanitize_busy = true;
-
 	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
 	if (err)
 		goto out;
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index b7ba8810a3b5..29aa50711626 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -107,9 +107,6 @@ struct mmc_command {
  */
 
 	unsigned int		busy_timeout;	/* busy detect timeout in ms */
-	/* Set this flag only for blocking sanitize request */
-	bool			sanitize_busy;
-
 	struct mmc_data		*data;		/* data segment associated with cmd */
 	struct mmc_request	*mrq;		/* associated request */
 };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (11 preceding siblings ...)
  2020-02-04  8:54 ` [PATCH 12/12] mmc: core: Re-work the error path for the eMMC sanitize command Ulf Hansson
@ 2020-02-11 13:17 ` Baolin Wang
  2020-02-13  6:23   ` Baolin Wang
  2020-02-18 23:38 ` Ulf Hansson
  13 siblings, 1 reply; 23+ messages in thread
From: Baolin Wang @ 2020-02-11 13:17 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Adrian Hunter, Wolfram Sang, Ludovic Barre,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

Hi Ulf,

On Tue, Feb 4, 2020 at 4:55 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> There exists several separate variants of polling loops, used to detect when
> the card stop signals busy for various operations, in the mmc core. All of them
> have different issues that needs to be fixed.
>
> The intent with this series, is to address some of these problems, via first
> improving the mmc_poll_for_busy() function, then consolidate code by moving
> more users to it.
>
> While I was working on this, I stumbled over some code here and there, that
> deserved some cleanup, hence I also folded in a couple of patches for this.
>
> So far, I have only managed to extensively test the updated mmc_poll_for_busy()
> function for CMD6 commands. Some tests for erase/trim/discard and for
> HPI+sanitize are needed.
>
> Note that, there are still separate polling loops in the mmc block layer, but
> moving that to mmc_poll_for_busy() involves some additional work. I am looking
> into that as a next step.
>
> Please help review and test!

That will be help if you can supply one git branch to fetch these
patches :), and I will help to do some testing on my platform.

> Ulf Hansson (12):
>   mmc: core: Throttle polling rate for CMD6
>   mmc: core: Drop unused define
>   mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
>   mmc: core: Drop redundant in-parameter to __mmc_switch()
>   mmc: core: Split up mmc_poll_for_busy()
>   mmc: core: Enable re-use of mmc_blk_in_tran_state()
>   mmc: core: Update CMD13 busy check for CMD6 commands
>   mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
>   mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
>   mmc: core: Convert to mmc_poll_for_busy() for HPI commands
>   mmc: core: Fixup support for HW busy detection for HPI commands
>   mmc: core: Re-work the error path for the eMMC sanitize command
>
>  drivers/mmc/core/block.c   |  55 +++++--------
>  drivers/mmc/core/core.c    |  53 +------------
>  drivers/mmc/core/mmc.c     |  38 ++++-----
>  drivers/mmc/core/mmc_ops.c | 159 ++++++++++++++++++++++---------------
>  drivers/mmc/core/mmc_ops.h |  13 ++-
>  include/linux/mmc/core.h   |   3 -
>  include/linux/mmc/mmc.h    |  10 +++
>  7 files changed, 157 insertions(+), 174 deletions(-)
>
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 12/12] mmc: core: Re-work the error path for the eMMC sanitize command
  2020-02-04  8:54 ` [PATCH 12/12] mmc: core: Re-work the error path for the eMMC sanitize command Ulf Hansson
@ 2020-02-11 22:29   ` kbuild test robot
  0 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2020-02-11 22:29 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: kbuild-all, linux-mmc, Ulf Hansson, Adrian Hunter, Wolfram Sang,
	Ludovic Barre, Baolin Wang, Linus Walleij, Chaotian Jing,
	Shawn Lin, mirq-linux

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.6-rc1 next-20200211]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/mmc-core-Improve-code-for-polling-and-HW-busy-detect/20200204-165854
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 322bf2d3446aabdaf5e8887775bd9ced80dbc0f0
config: riscv-allmodconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=riscv 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ERROR: "mmc_interrupt_hpi" [drivers/mmc/core/mmc_block.ko] undefined!
>> ERROR: "mmc_retune_hold" [drivers/mmc/core/mmc_block.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61644 bytes --]

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 01/12] mmc: core: Throttle polling rate for CMD6
  2020-02-04  8:54 ` [PATCH 01/12] mmc: core: Throttle polling rate for CMD6 Ulf Hansson
@ 2020-02-12 13:51   ` Ludovic BARRE
  2020-02-12 14:18     ` Ulf Hansson
  0 siblings, 1 reply; 23+ messages in thread
From: Ludovic BARRE @ 2020-02-12 13:51 UTC (permalink / raw)
  To: Ulf Hansson, linux-mmc
  Cc: Adrian Hunter, Wolfram Sang, Baolin Wang, Linus Walleij,
	Chaotian Jing, Shawn Lin, mirq-linux

hi Ulf

Le 2/4/20 à 9:54 AM, Ulf Hansson a écrit :
> In mmc_poll_for_busy() we loop continuously, either by sending a CMD13 or
> by invoking the ->card_busy() host ops, as to detect when the card stops
> signaling busy. This behaviour is problematic as it may cause CPU hogging,
> especially when the busy signal time reaches beyond a few ms.
> 
> Let's fix the issue by adding a throttling mechanism, that inserts a
> usleep_range() in between the polling attempts. The sleep range starts at
> 16-32us, but increases for each loop by a factor of 2, up until the range

Just to align comment and code: in the code the first usleep range start 
at 32-64us.

> reaches ~32-64ms. In this way, we are able to keep the loop fine-grained
> enough for short busy signaling times, while also not hogging the CPU for
> longer times.
> 
> Note that, this change is inspired by the similar throttling mechanism that
> we already use for mmc_do_erase().
> 
> Reported-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>   drivers/mmc/core/mmc_ops.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index da425ee2d9bf..446a37cc2a86 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -456,6 +456,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>   	struct mmc_host *host = card->host;
>   	int err;
>   	unsigned long timeout;
> +	unsigned int udelay = 32, udelay_max = 32768;
>   	u32 status = 0;
>   	bool expired = false;
>   	bool busy = false;
> @@ -500,6 +501,13 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>   				mmc_hostname(host), __func__);
>   			return -ETIMEDOUT;
>   		}
> +
> +		/* Throttle the polling rate to avoid hogging the CPU. */
> +		if (busy) {
> +			usleep_range(udelay, udelay * 2);
> +			if (udelay < udelay_max)
> +				udelay *= 2;
> +		}
>   	} while (busy);
>   
>   	return 0;
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 01/12] mmc: core: Throttle polling rate for CMD6
  2020-02-12 13:51   ` Ludovic BARRE
@ 2020-02-12 14:18     ` Ulf Hansson
  2020-02-12 14:24       ` Ludovic BARRE
  0 siblings, 1 reply; 23+ messages in thread
From: Ulf Hansson @ 2020-02-12 14:18 UTC (permalink / raw)
  To: Ludovic BARRE
  Cc: linux-mmc, Adrian Hunter, Wolfram Sang, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin,
	Michał Mirosław

On Wed, 12 Feb 2020 at 14:51, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> hi Ulf
>
> Le 2/4/20 à 9:54 AM, Ulf Hansson a écrit :
> > In mmc_poll_for_busy() we loop continuously, either by sending a CMD13 or
> > by invoking the ->card_busy() host ops, as to detect when the card stops
> > signaling busy. This behaviour is problematic as it may cause CPU hogging,
> > especially when the busy signal time reaches beyond a few ms.
> >
> > Let's fix the issue by adding a throttling mechanism, that inserts a
> > usleep_range() in between the polling attempts. The sleep range starts at
> > 16-32us, but increases for each loop by a factor of 2, up until the range
>
> Just to align comment and code: in the code the first usleep range start
> at 32-64us.

Yeah, good point, thanks. I was trying different values, but forgot to
update the commit message. :-)

Other than that, does the change look good to you?

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 01/12] mmc: core: Throttle polling rate for CMD6
  2020-02-12 14:18     ` Ulf Hansson
@ 2020-02-12 14:24       ` Ludovic BARRE
  0 siblings, 0 replies; 23+ messages in thread
From: Ludovic BARRE @ 2020-02-12 14:24 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Adrian Hunter, Wolfram Sang, Baolin Wang,
	Linus Walleij, Chaotian Jing, Shawn Lin,
	Michał Mirosław



Le 2/12/20 à 3:18 PM, Ulf Hansson a écrit :
> On Wed, 12 Feb 2020 at 14:51, Ludovic BARRE <ludovic.barre@st.com> wrote:
>>
>> hi Ulf
>>
>> Le 2/4/20 à 9:54 AM, Ulf Hansson a écrit :
>>> In mmc_poll_for_busy() we loop continuously, either by sending a CMD13 or
>>> by invoking the ->card_busy() host ops, as to detect when the card stops
>>> signaling busy. This behaviour is problematic as it may cause CPU hogging,
>>> especially when the busy signal time reaches beyond a few ms.
>>>
>>> Let's fix the issue by adding a throttling mechanism, that inserts a
>>> usleep_range() in between the polling attempts. The sleep range starts at
>>> 16-32us, but increases for each loop by a factor of 2, up until the range
>>
>> Just to align comment and code: in the code the first usleep range start
>> at 32-64us.
> 
> Yeah, good point, thanks. I was trying different values, but forgot to
> update the commit message. :-)

I tested series on mmci, sdmmc variant with/out MMC_CAP_WAIT_WHILE_BUSY
and it seems OK

yes, I reviewing the other patch of series but for this patch is OK.

Reviewed-by: Ludovic Barre <ludovic.barre@st.com>

> 
> Other than that, does the change look good to you?
> 
> [...]
> 
> Kind regards
> Uffe
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
  2020-02-11 13:17 ` [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Baolin Wang
@ 2020-02-13  6:23   ` Baolin Wang
  2020-02-13 11:08     ` Ulf Hansson
  2020-02-13 14:42     ` Ludovic BARRE
  0 siblings, 2 replies; 23+ messages in thread
From: Baolin Wang @ 2020-02-13  6:23 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Adrian Hunter, Wolfram Sang, Ludovic Barre,
	Linus Walleij, Chaotian Jing, Shawn Lin, mirq-linux

On Tue, Feb 11, 2020 at 9:17 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> Hi Ulf,
>
> On Tue, Feb 4, 2020 at 4:55 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > There exists several separate variants of polling loops, used to detect when
> > the card stop signals busy for various operations, in the mmc core. All of them
> > have different issues that needs to be fixed.
> >
> > The intent with this series, is to address some of these problems, via first
> > improving the mmc_poll_for_busy() function, then consolidate code by moving
> > more users to it.
> >
> > While I was working on this, I stumbled over some code here and there, that
> > deserved some cleanup, hence I also folded in a couple of patches for this.
> >
> > So far, I have only managed to extensively test the updated mmc_poll_for_busy()
> > function for CMD6 commands. Some tests for erase/trim/discard and for
> > HPI+sanitize are needed.
> >
> > Note that, there are still separate polling loops in the mmc block layer, but
> > moving that to mmc_poll_for_busy() involves some additional work. I am looking
> > into that as a next step.
> >
> > Please help review and test!
>
> That will be help if you can supply one git branch to fetch these
> patches :), and I will help to do some testing on my platform.

I've tested on my platform, incuding reading, writing, mounting and
running all cases in mmc_test.c, and I did not find any problem. So
please feel free to add my test tag. Thanks.

Tested-by: Baolin Wang <baolin.wang7@gmail.com>

> > Ulf Hansson (12):
> >   mmc: core: Throttle polling rate for CMD6
> >   mmc: core: Drop unused define
> >   mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
> >   mmc: core: Drop redundant in-parameter to __mmc_switch()
> >   mmc: core: Split up mmc_poll_for_busy()
> >   mmc: core: Enable re-use of mmc_blk_in_tran_state()
> >   mmc: core: Update CMD13 busy check for CMD6 commands
> >   mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
> >   mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
> >   mmc: core: Convert to mmc_poll_for_busy() for HPI commands
> >   mmc: core: Fixup support for HW busy detection for HPI commands
> >   mmc: core: Re-work the error path for the eMMC sanitize command
> >
> >  drivers/mmc/core/block.c   |  55 +++++--------
> >  drivers/mmc/core/core.c    |  53 +------------
> >  drivers/mmc/core/mmc.c     |  38 ++++-----
> >  drivers/mmc/core/mmc_ops.c | 159 ++++++++++++++++++++++---------------
> >  drivers/mmc/core/mmc_ops.h |  13 ++-
> >  include/linux/mmc/core.h   |   3 -
> >  include/linux/mmc/mmc.h    |  10 +++
> >  7 files changed, 157 insertions(+), 174 deletions(-)
> >
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
  2020-02-13  6:23   ` Baolin Wang
@ 2020-02-13 11:08     ` Ulf Hansson
  2020-02-13 14:42     ` Ludovic BARRE
  1 sibling, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-13 11:08 UTC (permalink / raw)
  To: Baolin Wang
  Cc: linux-mmc, Adrian Hunter, Wolfram Sang, Ludovic Barre,
	Linus Walleij, Chaotian Jing, Shawn Lin,
	Michał Mirosław

On Thu, 13 Feb 2020 at 07:24, Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> On Tue, Feb 11, 2020 at 9:17 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >
> > Hi Ulf,
> >
> > On Tue, Feb 4, 2020 at 4:55 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > There exists several separate variants of polling loops, used to detect when
> > > the card stop signals busy for various operations, in the mmc core. All of them
> > > have different issues that needs to be fixed.
> > >
> > > The intent with this series, is to address some of these problems, via first
> > > improving the mmc_poll_for_busy() function, then consolidate code by moving
> > > more users to it.
> > >
> > > While I was working on this, I stumbled over some code here and there, that
> > > deserved some cleanup, hence I also folded in a couple of patches for this.
> > >
> > > So far, I have only managed to extensively test the updated mmc_poll_for_busy()
> > > function for CMD6 commands. Some tests for erase/trim/discard and for
> > > HPI+sanitize are needed.
> > >
> > > Note that, there are still separate polling loops in the mmc block layer, but
> > > moving that to mmc_poll_for_busy() involves some additional work. I am looking
> > > into that as a next step.
> > >
> > > Please help review and test!
> >
> > That will be help if you can supply one git branch to fetch these
> > patches :), and I will help to do some testing on my platform.
>
> I've tested on my platform, incuding reading, writing, mounting and
> running all cases in mmc_test.c, and I did not find any problem. So
> please feel free to add my test tag. Thanks.
>
> Tested-by: Baolin Wang <baolin.wang7@gmail.com>

Thanks a lot for testing!

Kind regards
Uffe

>
> > > Ulf Hansson (12):
> > >   mmc: core: Throttle polling rate for CMD6
> > >   mmc: core: Drop unused define
> > >   mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
> > >   mmc: core: Drop redundant in-parameter to __mmc_switch()
> > >   mmc: core: Split up mmc_poll_for_busy()
> > >   mmc: core: Enable re-use of mmc_blk_in_tran_state()
> > >   mmc: core: Update CMD13 busy check for CMD6 commands
> > >   mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
> > >   mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
> > >   mmc: core: Convert to mmc_poll_for_busy() for HPI commands
> > >   mmc: core: Fixup support for HW busy detection for HPI commands
> > >   mmc: core: Re-work the error path for the eMMC sanitize command
> > >
> > >  drivers/mmc/core/block.c   |  55 +++++--------
> > >  drivers/mmc/core/core.c    |  53 +------------
> > >  drivers/mmc/core/mmc.c     |  38 ++++-----
> > >  drivers/mmc/core/mmc_ops.c | 159 ++++++++++++++++++++++---------------
> > >  drivers/mmc/core/mmc_ops.h |  13 ++-
> > >  include/linux/mmc/core.h   |   3 -
> > >  include/linux/mmc/mmc.h    |  10 +++
> > >  7 files changed, 157 insertions(+), 174 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
  2020-02-13  6:23   ` Baolin Wang
  2020-02-13 11:08     ` Ulf Hansson
@ 2020-02-13 14:42     ` Ludovic BARRE
  2020-02-14 14:21       ` Ulf Hansson
  1 sibling, 1 reply; 23+ messages in thread
From: Ludovic BARRE @ 2020-02-13 14:42 UTC (permalink / raw)
  To: Baolin Wang, Ulf Hansson
  Cc: linux-mmc, Adrian Hunter, Wolfram Sang, Linus Walleij,
	Chaotian Jing, Shawn Lin, mirq-linux

Hi Ulf

Le 2/13/20 à 7:23 AM, Baolin Wang a écrit :
> On Tue, Feb 11, 2020 at 9:17 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>>
>> Hi Ulf,
>>
>> On Tue, Feb 4, 2020 at 4:55 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>
>>> There exists several separate variants of polling loops, used to detect when
>>> the card stop signals busy for various operations, in the mmc core. All of them
>>> have different issues that needs to be fixed.
>>>
>>> The intent with this series, is to address some of these problems, via first
>>> improving the mmc_poll_for_busy() function, then consolidate code by moving
>>> more users to it.
>>>
>>> While I was working on this, I stumbled over some code here and there, that
>>> deserved some cleanup, hence I also folded in a couple of patches for this.
>>>
>>> So far, I have only managed to extensively test the updated mmc_poll_for_busy()
>>> function for CMD6 commands. Some tests for erase/trim/discard and for
>>> HPI+sanitize are needed.
>>>
>>> Note that, there are still separate polling loops in the mmc block layer, but
>>> moving that to mmc_poll_for_busy() involves some additional work. I am looking
>>> into that as a next step.
>>>
>>> Please help review and test!
>>
>> That will be help if you can supply one git branch to fetch these
>> patches :), and I will help to do some testing on my platform.
> 
> I've tested on my platform, incuding reading, writing, mounting and
> running all cases in mmc_test.c, and I did not find any problem. So
> please feel free to add my test tag. Thanks.
> 

Tested on mmci: sdmmc variant with/out MMC_CAP_WAIT_WHILE_BUSY
and I see no regression.
After series review, I've just a comment on patch 01/12
(code/comment alignment 32-64)

Tested-by: Ludovic Barre <ludovic.barre@st.com>
Reviewed-by: Ludovic Barre <ludovic.barre@st.com>

> Tested-by: Baolin Wang <baolin.wang7@gmail.com>
> 
>>> Ulf Hansson (12):
>>>    mmc: core: Throttle polling rate for CMD6
>>>    mmc: core: Drop unused define
>>>    mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
>>>    mmc: core: Drop redundant in-parameter to __mmc_switch()
>>>    mmc: core: Split up mmc_poll_for_busy()
>>>    mmc: core: Enable re-use of mmc_blk_in_tran_state()
>>>    mmc: core: Update CMD13 busy check for CMD6 commands
>>>    mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
>>>    mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
>>>    mmc: core: Convert to mmc_poll_for_busy() for HPI commands
>>>    mmc: core: Fixup support for HW busy detection for HPI commands
>>>    mmc: core: Re-work the error path for the eMMC sanitize command
>>>
>>>   drivers/mmc/core/block.c   |  55 +++++--------
>>>   drivers/mmc/core/core.c    |  53 +------------
>>>   drivers/mmc/core/mmc.c     |  38 ++++-----
>>>   drivers/mmc/core/mmc_ops.c | 159 ++++++++++++++++++++++---------------
>>>   drivers/mmc/core/mmc_ops.h |  13 ++-
>>>   include/linux/mmc/core.h   |   3 -
>>>   include/linux/mmc/mmc.h    |  10 +++
>>>   7 files changed, 157 insertions(+), 174 deletions(-)
>>>
>>> --
>>> 2.17.1
>>>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
  2020-02-13 14:42     ` Ludovic BARRE
@ 2020-02-14 14:21       ` Ulf Hansson
  0 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-14 14:21 UTC (permalink / raw)
  To: Ludovic BARRE
  Cc: Baolin Wang, linux-mmc, Adrian Hunter, Wolfram Sang,
	Linus Walleij, Chaotian Jing, Shawn Lin,
	Michał Mirosław

On Thu, 13 Feb 2020 at 15:42, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> Hi Ulf
>
> Le 2/13/20 à 7:23 AM, Baolin Wang a écrit :
> > On Tue, Feb 11, 2020 at 9:17 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
> >>
> >> Hi Ulf,
> >>
> >> On Tue, Feb 4, 2020 at 4:55 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>
> >>> There exists several separate variants of polling loops, used to detect when
> >>> the card stop signals busy for various operations, in the mmc core. All of them
> >>> have different issues that needs to be fixed.
> >>>
> >>> The intent with this series, is to address some of these problems, via first
> >>> improving the mmc_poll_for_busy() function, then consolidate code by moving
> >>> more users to it.
> >>>
> >>> While I was working on this, I stumbled over some code here and there, that
> >>> deserved some cleanup, hence I also folded in a couple of patches for this.
> >>>
> >>> So far, I have only managed to extensively test the updated mmc_poll_for_busy()
> >>> function for CMD6 commands. Some tests for erase/trim/discard and for
> >>> HPI+sanitize are needed.
> >>>
> >>> Note that, there are still separate polling loops in the mmc block layer, but
> >>> moving that to mmc_poll_for_busy() involves some additional work. I am looking
> >>> into that as a next step.
> >>>
> >>> Please help review and test!
> >>
> >> That will be help if you can supply one git branch to fetch these
> >> patches :), and I will help to do some testing on my platform.
> >
> > I've tested on my platform, incuding reading, writing, mounting and
> > running all cases in mmc_test.c, and I did not find any problem. So
> > please feel free to add my test tag. Thanks.
> >
>
> Tested on mmci: sdmmc variant with/out MMC_CAP_WAIT_WHILE_BUSY
> and I see no regression.
> After series review, I've just a comment on patch 01/12
> (code/comment alignment 32-64)
>
> Tested-by: Ludovic Barre <ludovic.barre@st.com>
> Reviewed-by: Ludovic Barre <ludovic.barre@st.com>

Thanks a lot, much appreciated!

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect
  2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
                   ` (12 preceding siblings ...)
  2020-02-11 13:17 ` [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Baolin Wang
@ 2020-02-18 23:38 ` Ulf Hansson
  13 siblings, 0 replies; 23+ messages in thread
From: Ulf Hansson @ 2020-02-18 23:38 UTC (permalink / raw)
  To: linux-mmc, Baolin Wang, Ludovic Barre
  Cc: Adrian Hunter, Wolfram Sang, Ulf Hansson, Linus Walleij,
	Chaotian Jing, Shawn Lin, Michał Mirosław

On Tue, 4 Feb 2020 at 09:55, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> There exists several separate variants of polling loops, used to detect when
> the card stop signals busy for various operations, in the mmc core. All of them
> have different issues that needs to be fixed.
>
> The intent with this series, is to address some of these problems, via first
> improving the mmc_poll_for_busy() function, then consolidate code by moving
> more users to it.
>
> While I was working on this, I stumbled over some code here and there, that
> deserved some cleanup, hence I also folded in a couple of patches for this.
>
> So far, I have only managed to extensively test the updated mmc_poll_for_busy()
> function for CMD6 commands. Some tests for erase/trim/discard and for
> HPI+sanitize are needed.
>
> Note that, there are still separate polling loops in the mmc block layer, but
> moving that to mmc_poll_for_busy() involves some additional work. I am looking
> into that as a next step.
>
> Please help review and test!
>
> Kind regards
> Ulf Hansson
>
>
> Ulf Hansson (12):
>   mmc: core: Throttle polling rate for CMD6
>   mmc: core: Drop unused define
>   mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status()
>   mmc: core: Drop redundant in-parameter to __mmc_switch()
>   mmc: core: Split up mmc_poll_for_busy()
>   mmc: core: Enable re-use of mmc_blk_in_tran_state()
>   mmc: core: Update CMD13 busy check for CMD6 commands
>   mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard
>   mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd()
>   mmc: core: Convert to mmc_poll_for_busy() for HPI commands
>   mmc: core: Fixup support for HW busy detection for HPI commands
>   mmc: core: Re-work the error path for the eMMC sanitize command
>
>  drivers/mmc/core/block.c   |  55 +++++--------
>  drivers/mmc/core/core.c    |  53 +------------
>  drivers/mmc/core/mmc.c     |  38 ++++-----
>  drivers/mmc/core/mmc_ops.c | 159 ++++++++++++++++++++++---------------
>  drivers/mmc/core/mmc_ops.h |  13 ++-
>  include/linux/mmc/core.h   |   3 -
>  include/linux/mmc/mmc.h    |  10 +++
>  7 files changed, 157 insertions(+), 174 deletions(-)
>
> --
> 2.17.1
>

FYI, I have queued up this series for next (except patch12 that
deserves another re-spin). I also amended the changelog for patch1,
according the comment from Ludovic.

Feel free to provide additional feedback and test-reports, while we
monitor how this cook in linux-next.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-02-18 23:39 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04  8:54 [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Ulf Hansson
2020-02-04  8:54 ` [PATCH 01/12] mmc: core: Throttle polling rate for CMD6 Ulf Hansson
2020-02-12 13:51   ` Ludovic BARRE
2020-02-12 14:18     ` Ulf Hansson
2020-02-12 14:24       ` Ludovic BARRE
2020-02-04  8:54 ` [PATCH 02/12] mmc: core: Drop unused define Ulf Hansson
2020-02-04  8:54 ` [PATCH 03/12] mmc: core: Extend mmc_switch_status() to rid of __mmc_switch_status() Ulf Hansson
2020-02-04  8:54 ` [PATCH 04/12] mmc: core: Drop redundant in-parameter to __mmc_switch() Ulf Hansson
2020-02-04  8:54 ` [PATCH 05/12] mmc: core: Split up mmc_poll_for_busy() Ulf Hansson
2020-02-04  8:54 ` [PATCH 06/12] mmc: core: Enable re-use of mmc_blk_in_tran_state() Ulf Hansson
2020-02-04  8:54 ` [PATCH 07/12] mmc: core: Update CMD13 busy check for CMD6 commands Ulf Hansson
2020-02-04  8:54 ` [PATCH 08/12] mmc: core: Convert to mmc_poll_for_busy() for erase/trim/discard Ulf Hansson
2020-02-04  8:54 ` [PATCH 09/12] mmc: core: Drop redundant out-parameter to mmc_send_hpi_cmd() Ulf Hansson
2020-02-04  8:54 ` [PATCH 10/12] mmc: core: Convert to mmc_poll_for_busy() for HPI commands Ulf Hansson
2020-02-04  8:54 ` [PATCH 11/12] mmc: core: Fixup support for HW busy detection " Ulf Hansson
2020-02-04  8:54 ` [PATCH 12/12] mmc: core: Re-work the error path for the eMMC sanitize command Ulf Hansson
2020-02-11 22:29   ` kbuild test robot
2020-02-11 13:17 ` [PATCH 00/12] mmc: core: Improve code for polling and HW busy detect Baolin Wang
2020-02-13  6:23   ` Baolin Wang
2020-02-13 11:08     ` Ulf Hansson
2020-02-13 14:42     ` Ludovic BARRE
2020-02-14 14:21       ` Ulf Hansson
2020-02-18 23:38 ` Ulf Hansson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).