diff mbox

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

Message ID 1476951579-26125-5-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson Oct. 20, 2016, 8:19 a.m. UTC 
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(-)

-- 
1.9.1

Comments

Adrian Hunter Oct. 21, 2016, 9:19 a.m. UTC | #1 
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);

>  }

>  

>  /**

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 24, 2016, 7:49 a.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Oct. 25, 2016, 8:44 a.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Oct. 25, 2016, 8:45 a.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter Oct. 25, 2016, 10:58 a.m. UTC | #5 
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);


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
 }
 
 /**