[PATCH 0/4] mmc: core: Clean-up and improve polling code in __mmc_switch()

[PATCH 0/4] mmc: core: Clean-up and improve polling code in __mmc_switch()

[Ulf Hansson]

The code in __mmc_switch() is rather messy and hard to follow/understand. This
series starts out by cleaning-up and re-factoring that code. In the final patch
I intend to improve the behaviour regarding the polling method, which is being
used to know when the card stops signal busy.

Ulf Hansson (4):
  mmc: core: Make mmc_switch_status() available for mmc core
  mmc: core: Clarify code which deals with polling in __mmc_switch()
  mmc: core: Factor out code related to polling in __mmc_switch()
  mmc: core: Don't use ->card_busy() and CMD13 in combination when
    polling

 drivers/mmc/core/mmc.c     |  13 -----
 drivers/mmc/core/mmc_ops.c | 138 +++++++++++++++++++++++++--------------------
 drivers/mmc/core/mmc_ops.h |   2 +-
 3 files changed, 79 insertions(+), 74 deletions(-)

-- 
1.9.1

[PATCH 1/4] mmc: core: Make mmc_switch_status() available for mmc core

[Ulf Hansson]

Following changes needs mmc_switch_status() to be available both from mmc.c
and mmc_ops.c. Allow that by moving its implementation to mmc_ops.c and
make it available via mmc_ops.h.

Moving mmc_switch_status() to mmc_ops.c, also enables us to turn
mmc_switch_status_error() into static function. So let's take the
opportunity to change this as well.

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

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 39fc5b2..e811bd9 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1000,19 +1000,6 @@ static int mmc_select_bus_width(struct mmc_card *card)
 	return err;
 }
 
-/* Caller must hold re-tuning */
-static int mmc_switch_status(struct mmc_card *card)
-{
-	u32 status;
-	int err;
-
-	err = mmc_send_status(card, &status);
-	if (err)
-		return err;
-
-	return mmc_switch_status_error(card->host, status);
-}
-
 /*
  * Switch to the high-speed mode
  */
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index ad6e979..f9af1c0 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -440,7 +440,7 @@ int mmc_spi_set_crc(struct mmc_host *host, int use_crc)
 	return err;
 }
 
-int mmc_switch_status_error(struct mmc_host *host, u32 status)
+static int mmc_switch_status_error(struct mmc_host *host, u32 status)
 {
 	if (mmc_host_is_spi(host)) {
 		if (status & R1_SPI_ILLEGAL_COMMAND)
@@ -455,6 +455,19 @@ int mmc_switch_status_error(struct mmc_host *host, u32 status)
 	return 0;
 }
 
+/* Caller must hold re-tuning */
+int mmc_switch_status(struct mmc_card *card)
+{
+	u32 status;
+	int err;
+
+	err = mmc_send_status(card, &status);
+	if (err)
+		return err;
+
+	return mmc_switch_status_error(card->host, status);
+}
+
 /**
  *	__mmc_switch - modify EXT_CSD register
  *	@card: the MMC card associated with the data transfer
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index f1b8e81..7f6c0e9 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -27,7 +27,7 @@
 int mmc_bus_test(struct mmc_card *card, u8 bus_width);
 int mmc_send_hpi_cmd(struct mmc_card *card, u32 *status);
 int mmc_can_ext_csd(struct mmc_card *card);
-int mmc_switch_status_error(struct mmc_host *host, u32 status);
+int mmc_switch_status(struct mmc_card *card);
 int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		unsigned int timeout_ms, bool use_busy_signal, bool send_status,
 		bool ignore_crc);
-- 
1.9.1

[PATCH 2/4] mmc: core: Clarify code which deals with polling in __mmc_switch()

[Ulf Hansson]

The __mmc_switch() deserves a clean-up. In this step, let's move some code
outside of the do-while loop, which deal deals with the card busy polling.

This change simplifies the code in that sense that it becomes easier to follow
what is being executed during card busy polling, but it also gives a better
understanding for when polling isn't done.

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

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index f9af1c0..5a77af7 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -535,18 +535,29 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	if (!use_busy_signal)
 		goto out;
 
-	/*
-	 * CRC errors shall only be ignored in cases were CMD13 is used to poll
-	 * to detect busy completion.
-	 */
-	if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
-		ignore_crc = false;
+	/*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)) {
+		if (send_status)
+			err = mmc_switch_status(card);
+		goto out;
+	}
 
 	/* We have an unspecified cmd timeout, use the fallback value. */
 	if (!timeout_ms)
 		timeout_ms = MMC_OPS_TIMEOUT_MS;
 
-	/* Must check status to be sure of no errors. */
+	/*
+	 * In cases when not allowed to poll by using CMD13 or because we aren't
+	 * capable of polling by using ->card_busy(), then rely on waiting the
+	 * stated timeout to be sufficient.
+	 */
+	if (!send_status && !host->ops->card_busy) {
+		mmc_delay(timeout_ms);
+		goto out;
+	}
+
+	/* Let's poll to find out when the command is completed. */
 	timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
 	do {
 		/*
@@ -560,25 +571,11 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 			if (err)
 				goto out;
 		}
-		if ((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp)
-			break;
 		if (host->ops->card_busy) {
 			if (!host->ops->card_busy(host))
 				break;
 			busy = true;
 		}
-		if (mmc_host_is_spi(host))
-			break;
-
-		/*
-		 * We are not allowed to issue a status command and the host
-		 * does'nt support MMC_CAP_WAIT_WHILE_BUSY, then we can only
-		 * rely on waiting for the stated timeout to be sufficient.
-		 */
-		if (!send_status && !host->ops->card_busy) {
-			mmc_delay(timeout_ms);
-			goto out;
-		}
 
 		/* Timeout if the device never leaves the program state. */
 		if (expired &&
-- 
1.9.1

[PATCH 3/4] mmc: core: Factor out code related to polling in __mmc_switch()

[Ulf Hansson]

In yet another step of cleaning up __mmc_switch(), let's factor out the
code that deals with card busy polling.

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

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 5a77af7..a84a880 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -468,6 +468,63 @@ int mmc_switch_status(struct mmc_card *card)
 	return mmc_switch_status_error(card->host, status);
 }
 
+static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
+			bool send_status, bool ignore_crc)
+{
+	struct mmc_host *host = card->host;
+	int err;
+	unsigned long timeout;
+	u32 status = 0;
+	bool expired = false;
+	bool busy = false;
+
+	/* We have an unspecified cmd timeout, use the fallback value. */
+	if (!timeout_ms)
+		timeout_ms = MMC_OPS_TIMEOUT_MS;
+
+	/*
+	 * In cases when not allowed to poll by using CMD13 or because we aren't
+	 * capable of polling by using ->card_busy(), then rely on waiting the
+	 * stated timeout to be sufficient.
+	 */
+	if (!send_status && !host->ops->card_busy) {
+		mmc_delay(timeout_ms);
+		return 0;
+	}
+
+	timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
+	do {
+		/*
+		 * Due to the possibility of being preempted after
+		 * sending the status command, check the expiration
+		 * time first.
+		 */
+		expired = time_after(jiffies, timeout);
+		if (send_status) {
+			err = __mmc_send_status(card, &status, ignore_crc);
+			if (err)
+				return err;
+		}
+		if (host->ops->card_busy) {
+			if (!host->ops->card_busy(host))
+				break;
+			busy = true;
+		}
+
+		/* Timeout if the device never leaves the program state. */
+		if (expired &&
+		    (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
+			pr_err("%s: Card stuck in programming state! %s\n",
+				mmc_hostname(host), __func__);
+			return -ETIMEDOUT;
+		}
+	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
+
+	err = mmc_switch_status_error(host, status);
+
+	return err;
+}
+
 /**
  *	__mmc_switch - modify EXT_CSD register
  *	@card: the MMC card associated with the data transfer
@@ -489,11 +546,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 	struct mmc_host *host = card->host;
 	int err;
 	struct mmc_command cmd = {0};
-	unsigned long timeout;
-	u32 status = 0;
 	bool use_r1b_resp = use_busy_signal;
-	bool expired = false;
-	bool busy = false;
 
 	mmc_retune_hold(host);
 
@@ -543,51 +596,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		goto out;
 	}
 
-	/* We have an unspecified cmd timeout, use the fallback value. */
-	if (!timeout_ms)
-		timeout_ms = MMC_OPS_TIMEOUT_MS;
-
-	/*
-	 * In cases when not allowed to poll by using CMD13 or because we aren't
-	 * capable of polling by using ->card_busy(), then rely on waiting the
-	 * stated timeout to be sufficient.
-	 */
-	if (!send_status && !host->ops->card_busy) {
-		mmc_delay(timeout_ms);
-		goto out;
-	}
-
-	/* Let's poll to find out when the command is completed. */
-	timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
-	do {
-		/*
-		 * Due to the possibility of being preempted after
-		 * sending the status command, check the expiration
-		 * time first.
-		 */
-		expired = time_after(jiffies, timeout);
-		if (send_status) {
-			err = __mmc_send_status(card, &status, ignore_crc);
-			if (err)
-				goto out;
-		}
-		if (host->ops->card_busy) {
-			if (!host->ops->card_busy(host))
-				break;
-			busy = true;
-		}
-
-		/* Timeout if the device never leaves the program state. */
-		if (expired &&
-		    (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
-			pr_err("%s: Card stuck in programming state! %s\n",
-				mmc_hostname(host), __func__);
-			err = -ETIMEDOUT;
-			goto out;
-		}
-	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
-
-	err = mmc_switch_status_error(host, status);
+	/* Let's try to poll to find out when the command is completed. */
+	err = mmc_poll_for_busy(card, timeout_ms, send_status, ignore_crc);
 out:
 	mmc_retune_release(host);
 
-- 
1.9.1

[PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling

[Ulf Hansson]

When polling for busy after sending a MMC_SWITCH command, both the optional
->card_busy() callback and CMD13 are being used in conjunction.

This doesn't make sense. Instead it's more reasonable to rely solely on the
->card_busy() callback when it exists. Let's change that and instead use
the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
really needed.

Within this context, let's also take the opportunity to make some
additional clean-ups and clarifications to the related code.

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

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index a84a880..481bbdb 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -495,34 +495,32 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
 	timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
 	do {
 		/*
-		 * Due to the possibility of being preempted after
-		 * sending the status command, check the expiration
-		 * time first.
+		 * Due to the possibility of being preempted while polling,
+		 * check the expiration time first.
 		 */
 		expired = time_after(jiffies, timeout);
-		if (send_status) {
+
+		if (host->ops->card_busy) {
+			busy = host->ops->card_busy(host);
+		} else {
 			err = __mmc_send_status(card, &status, ignore_crc);
 			if (err)
 				return err;
-		}
-		if (host->ops->card_busy) {
-			if (!host->ops->card_busy(host))
-				break;
-			busy = true;
+			busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
 		}
 
-		/* Timeout if the device never leaves the program state. */
-		if (expired &&
-		    (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
-			pr_err("%s: Card stuck in programming state! %s\n",
+		/* Timeout if the device still remains busy. */
+		if (expired && busy) {
+			pr_err("%s: Card stuck being busy! %s\n",
 				mmc_hostname(host), __func__);
 			return -ETIMEDOUT;
 		}
-	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
+	} while (busy);
 
-	err = mmc_switch_status_error(host, status);
+	if (host->ops->card_busy && send_status)
+		return mmc_switch_status(card);
 
-	return err;
+	return mmc_switch_status_error(host, status);
 }
 
 /**
-- 
1.9.1

Re: [PATCH 0/4] mmc: core: Clean-up and improve polling code in __mmc_switch()

[Jaehoon Chung]

On 10/20/2016 05:19 PM, Ulf Hansson wrote:
> The code in __mmc_switch() is rather messy and hard to follow/understand. This
> series starts out by cleaning-up and re-factoring that code. In the final patch
> I intend to improve the behaviour regarding the polling method, which is being
> used to know when the card stops signal busy.

Looks good to me.

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
Tested-by: Jaehoon Chung <jh80.chung@samsung.com>

(Tested on Exynos Series)

Best Regards,
Jaehoon Chung

> 
> Ulf Hansson (4):
>   mmc: core: Make mmc_switch_status() available for mmc core
>   mmc: core: Clarify code which deals with polling in __mmc_switch()
>   mmc: core: Factor out code related to polling in __mmc_switch()
>   mmc: core: Don't use ->card_busy() and CMD13 in combination when
>     polling
> 
>  drivers/mmc/core/mmc.c     |  13 -----
>  drivers/mmc/core/mmc_ops.c | 138 +++++++++++++++++++++++++--------------------
>  drivers/mmc/core/mmc_ops.h |   2 +-
>  3 files changed, 79 insertions(+), 74 deletions(-)
> 

Re: [PATCH 0/4] mmc: core: Clean-up and improve polling code in __mmc_switch()

[Ulf Hansson]

On 21 October 2016 at 09:49, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 10/20/2016 05:19 PM, Ulf Hansson wrote:
>> The code in __mmc_switch() is rather messy and hard to follow/understand. This
>> series starts out by cleaning-up and re-factoring that code. In the final patch
>> I intend to improve the behaviour regarding the polling method, which is being
>> used to know when the card stops signal busy.
>
> Looks good to me.
>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> Tested-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> (Tested on Exynos Series)
>
> Best Regards,
> Jaehoon Chung

Thanks Jaehoon, I have queued them up for my next branch!

Kind regards
Uffe

>
>>
>> Ulf Hansson (4):
>>   mmc: core: Make mmc_switch_status() available for mmc core
>>   mmc: core: Clarify code which deals with polling in __mmc_switch()
>>   mmc: core: Factor out code related to polling in __mmc_switch()
>>   mmc: core: Don't use ->card_busy() and CMD13 in combination when
>>     polling
>>
>>  drivers/mmc/core/mmc.c     |  13 -----
>>  drivers/mmc/core/mmc_ops.c | 138 +++++++++++++++++++++++++--------------------
>>  drivers/mmc/core/mmc_ops.h |   2 +-
>>  3 files changed, 79 insertions(+), 74 deletions(-)
>>
>

Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling

[Adrian Hunter]

On 20/10/16 11:19, Ulf Hansson wrote:
> When polling for busy after sending a MMC_SWITCH command, both the optional
> ->card_busy() callback and CMD13 are being used in conjunction.
> 
> This doesn't make sense. Instead it's more reasonable to rely solely on the
> ->card_busy() callback when it exists. Let's change that and instead use
> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
> really needed.
> 
> Within this context, let's also take the opportunity to make some
> additional clean-ups and clarifications to the related code.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index a84a880..481bbdb 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -495,34 +495,32 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>  	timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
>  	do {
>  		/*
> -		 * Due to the possibility of being preempted after
> -		 * sending the status command, check the expiration
> -		 * time first.
> +		 * Due to the possibility of being preempted while polling,
> +		 * check the expiration time first.
>  		 */
>  		expired = time_after(jiffies, timeout);
> -		if (send_status) {
> +
> +		if (host->ops->card_busy) {
> +			busy = host->ops->card_busy(host);

I didn't really have time to look at these patches, sorry :-(.  But this
loop looks like it could use a cond_resched()

> +		} else {
>  			err = __mmc_send_status(card, &status, ignore_crc);
>  			if (err)
>  				return err;
> -		}
> -		if (host->ops->card_busy) {
> -			if (!host->ops->card_busy(host))
> -				break;
> -			busy = true;
> +			busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
>  		}
>  
> -		/* Timeout if the device never leaves the program state. */
> -		if (expired &&
> -		    (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy)) {
> -			pr_err("%s: Card stuck in programming state! %s\n",
> +		/* Timeout if the device still remains busy. */
> +		if (expired && busy) {
> +			pr_err("%s: Card stuck being busy! %s\n",
>  				mmc_hostname(host), __func__);
>  			return -ETIMEDOUT;
>  		}
> -	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG || busy);
> +	} while (busy);
>  
> -	err = mmc_switch_status_error(host, status);
> +	if (host->ops->card_busy && send_status)
> +		return mmc_switch_status(card);
>  
> -	return err;
> +	return mmc_switch_status_error(host, status);
>  }
>  
>  /**
> 

Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling

[Linus Walleij]

On Thu, Oct 20, 2016 at 10:19 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> When polling for busy after sending a MMC_SWITCH command, both the optional
> ->card_busy() callback and CMD13 are being used in conjunction.
>
> This doesn't make sense. Instead it's more reasonable to rely solely on the
> ->card_busy() callback when it exists. Let's change that and instead use
> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
> really needed.
>
> Within this context, let's also take the opportunity to make some
> additional clean-ups and clarifications to the related code.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Tried these four patches but it doesn't make my root mount before
20 minutes on the Dragonboard, but I don't know if that was the
intention even? Just cleanup? Sorry if I got it wrong...

Yours,
Linus Walleij

Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling

[Ulf Hansson]

On 24 October 2016 at 09:49, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Oct 20, 2016 at 10:19 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
>> When polling for busy after sending a MMC_SWITCH command, both the optional
>> ->card_busy() callback and CMD13 are being used in conjunction.
>>
>> This doesn't make sense. Instead it's more reasonable to rely solely on the
>> ->card_busy() callback when it exists. Let's change that and instead use
>> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
>> really needed.
>>
>> Within this context, let's also take the opportunity to make some
>> additional clean-ups and clarifications to the related code.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Tried these four patches but it doesn't make my root mount before
> 20 minutes on the Dragonboard, but I don't know if that was the
> intention even? Just cleanup? Sorry if I got it wrong...

Apologize if this was confusing, these changes was not intended to fix
your reported problem. They are clean-ups and the last change improves
the polling loop for switch commands a bit.

I will post a patch for the reported regression asap and will keep you
on cc. Appreciate if you could help in testing!

Kind regards
Uffe

Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling

[Ulf Hansson]

On 21 October 2016 at 11:19, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 20/10/16 11:19, Ulf Hansson wrote:
>> When polling for busy after sending a MMC_SWITCH command, both the optional
>> ->card_busy() callback and CMD13 are being used in conjunction.
>>
>> This doesn't make sense. Instead it's more reasonable to rely solely on the
>> ->card_busy() callback when it exists. Let's change that and instead use
>> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
>> really needed.
>>
>> Within this context, let's also take the opportunity to make some
>> additional clean-ups and clarifications to the related code.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++----------------
>>  1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index a84a880..481bbdb 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -495,34 +495,32 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>>       timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
>>       do {
>>               /*
>> -              * Due to the possibility of being preempted after
>> -              * sending the status command, check the expiration
>> -              * time first.
>> +              * Due to the possibility of being preempted while polling,
>> +              * check the expiration time first.
>>                */
>>               expired = time_after(jiffies, timeout);
>> -             if (send_status) {
>> +
>> +             if (host->ops->card_busy) {
>> +                     busy = host->ops->card_busy(host);
>
> I didn't really have time to look at these patches, sorry :-(.  But this
> loop looks like it could use a cond_resched()

Yes, something like that is definitely needed! Although, I suggest we
do that improvement on top of this change, if you are fine with that
approach?

The reason is that I am also pondering over, whether it could make
sense to poll with a dynamically increased interval. At least when
using ->card_busy().
Let's say by starting at 1 us interval, then at each poll attempt we
double the interval time. We would then rather use a combination of
udelay(), usleep_range() and msleep() to accomplish what you propose.
Does that make sense to you?

Kind regards
Uffe

Re: [PATCH 4/4] mmc: core: Don't use ->card_busy() and CMD13 in combination when polling

[Adrian Hunter]

On 25/10/16 11:45, Ulf Hansson wrote:
> On 21 October 2016 at 11:19, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 20/10/16 11:19, Ulf Hansson wrote:
>>> When polling for busy after sending a MMC_SWITCH command, both the optional
>>> ->card_busy() callback and CMD13 are being used in conjunction.
>>>
>>> This doesn't make sense. Instead it's more reasonable to rely solely on the
>>> ->card_busy() callback when it exists. Let's change that and instead use
>>> the CMD13 as a fall-back. In this way we avoid sending CMD13, unless it's
>>> really needed.
>>>
>>> Within this context, let's also take the opportunity to make some
>>> additional clean-ups and clarifications to the related code.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/mmc/core/mmc_ops.c | 30 ++++++++++++++----------------
>>>  1 file changed, 14 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>> index a84a880..481bbdb 100644
>>> --- a/drivers/mmc/core/mmc_ops.c
>>> +++ b/drivers/mmc/core/mmc_ops.c
>>> @@ -495,34 +495,32 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
>>>       timeout = jiffies + msecs_to_jiffies(timeout_ms) + 1;
>>>       do {
>>>               /*
>>> -              * Due to the possibility of being preempted after
>>> -              * sending the status command, check the expiration
>>> -              * time first.
>>> +              * Due to the possibility of being preempted while polling,
>>> +              * check the expiration time first.
>>>                */
>>>               expired = time_after(jiffies, timeout);
>>> -             if (send_status) {
>>> +
>>> +             if (host->ops->card_busy) {
>>> +                     busy = host->ops->card_busy(host);
>>
>> I didn't really have time to look at these patches, sorry :-(.  But this
>> loop looks like it could use a cond_resched()
> 
> Yes, something like that is definitely needed! Although, I suggest we
> do that improvement on top of this change, if you are fine with that
> approach?

Sure

> 
> The reason is that I am also pondering over, whether it could make
> sense to poll with a dynamically increased interval. At least when
> using ->card_busy().
> Let's say by starting at 1 us interval, then at each poll attempt we
> double the interval time. We would then rather use a combination of
> udelay(), usleep_range() and msleep() to accomplish what you propose.
> Does that make sense to you?

That potentially doubles the operation time.  It might be nicer to limit the
worst case e.g. sleep 1/8th of the total time spent looping

s64 sleep_us;
ktime_t start_time = ktime_get();
do {
	...
	sleep_us = ktime_us_delta(ktime_get(), start_time) >> 3;
	sleep_us = sleep_us ? : 1;
	...
} while(busy);