[PATCH] mmc: disable tuning when checking card presence

[PATCH] mmc: disable tuning when checking card presence

[Wolfram Sang]

When we use the alive callback, we expect a command to fail if the card
is not present. We should not trigger a retune then which will confuse
users with a failed retune on a removed card:

 mmc2: tuning execution failed: -5
 mmc2: card 0001 removed

Disable retuning in this code path.

Reported-by: Ulrich Hecht <uli+renesas@fpond.eu>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/core/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 54f0814f110c..eb792dd845a3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host)
 	if (!host->card || mmc_card_removed(host->card))
 		return 1;
 
+	/* we expect a failure if the card is removed */
+	mmc_retune_disable(host);
+
 	ret = host->bus_ops->alive(host);
 
 	/*
@@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host)
 		pr_debug("%s: card remove detected\n", mmc_hostname(host));
 	}
 
+	mmc_retune_enable(host);
+
 	return ret;
 }
 
-- 
2.30.2

Re: [PATCH] mmc: disable tuning when checking card presence

[Ulrich Hecht]

> On 06/18/2021 10:23 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> 
>  
> When we use the alive callback, we expect a command to fail if the card
> is not present. We should not trigger a retune then which will confuse
> users with a failed retune on a removed card:
> 
>  mmc2: tuning execution failed: -5
>  mmc2: card 0001 removed
> 
> Disable retuning in this code path.
> 
> Reported-by: Ulrich Hecht <uli+renesas@fpond.eu>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/core/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 54f0814f110c..eb792dd845a3 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>  	if (!host->card || mmc_card_removed(host->card))
>  		return 1;
>  
> +	/* we expect a failure if the card is removed */
> +	mmc_retune_disable(host);
> +
>  	ret = host->bus_ops->alive(host);
>  
>  	/*
> @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>  		pr_debug("%s: card remove detected\n", mmc_hostname(host));
>  	}
>  
> +	mmc_retune_enable(host);
> +
>  	return ret;
>  }
>  
> -- 
> 2.30.2

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH] mmc: disable tuning when checking card presence

[Ulf Hansson]

+ Adrian

On Fri, 18 Jun 2021 at 10:23, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> When we use the alive callback, we expect a command to fail if the card
> is not present. We should not trigger a retune then which will confuse
> users with a failed retune on a removed card:
>
>  mmc2: tuning execution failed: -5
>  mmc2: card 0001 removed
>
> Disable retuning in this code path.
>
> Reported-by: Ulrich Hecht <uli+renesas@fpond.eu>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/mmc/core/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 54f0814f110c..eb792dd845a3 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>         if (!host->card || mmc_card_removed(host->card))
>                 return 1;
>
> +       /* we expect a failure if the card is removed */
> +       mmc_retune_disable(host);
> +

Some controllers require a retune after it has been runtime suspended.

In the above path, when called via the bus_ops->detect() callback, it
could be that the controller may have been runtime suspended and then
got resumed by the call to mmc_get_card().

I think we need something more clever here, to make sure we don't end
up in that situation. I have looped in Adrian, to see if has some
ideas for how this can be fixed.

>         ret = host->bus_ops->alive(host);
>
>         /*
> @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>                 pr_debug("%s: card remove detected\n", mmc_hostname(host));
>         }
>
> +       mmc_retune_enable(host);
> +
>         return ret;
>  }
>
> --
> 2.30.2
>

Kind regards
Uffe

Re: [PATCH] mmc: disable tuning when checking card presence

[Adrian Hunter]

On 18/06/21 1:42 pm, Ulf Hansson wrote:
> + Adrian
> 
> On Fri, 18 Jun 2021 at 10:23, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>>
>> When we use the alive callback, we expect a command to fail if the card
>> is not present. We should not trigger a retune then which will confuse
>> users with a failed retune on a removed card:
>>
>>  mmc2: tuning execution failed: -5
>>  mmc2: card 0001 removed
>>
>> Disable retuning in this code path.
>>
>> Reported-by: Ulrich Hecht <uli+renesas@fpond.eu>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>  drivers/mmc/core/core.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 54f0814f110c..eb792dd845a3 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>>         if (!host->card || mmc_card_removed(host->card))
>>                 return 1;
>>
>> +       /* we expect a failure if the card is removed */
>> +       mmc_retune_disable(host);
>> +
> 
> Some controllers require a retune after it has been runtime suspended.
> 
> In the above path, when called via the bus_ops->detect() callback, it
> could be that the controller may have been runtime suspended and then
> got resumed by the call to mmc_get_card().
> 
> I think we need something more clever here, to make sure we don't end
> up in that situation. I have looped in Adrian, to see if has some
> ideas for how this can be fixed.

Can we clarify, is the only problem that the error message is confusing?

> 
>>         ret = host->bus_ops->alive(host);
>>
>>         /*
>> @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>>                 pr_debug("%s: card remove detected\n", mmc_hostname(host));
>>         }
>>
>> +       mmc_retune_enable(host);
>> +
>>         return ret;
>>  }
>>
>> --
>> 2.30.2
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH] mmc: disable tuning when checking card presence

[Ulrich Hecht]

> On 06/21/2021 9:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> Can we clarify, is the only problem that the error message is confusing?

AFAICT there are no ill effects of the retune failing apart from the error message.

CU
Uli

Re: [PATCH] mmc: disable tuning when checking card presence

[Adrian Hunter]

On 21/06/21 10:32 am, Ulrich Hecht wrote:
> 
>> On 06/21/2021 9:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Can we clarify, is the only problem that the error message is confusing?
> 
> AFAICT there are no ill effects of the retune failing apart from the error message.
> 

So maybe the simplest thing to do is just amend the message:
e.g.

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 4e52eb14198a..5cbf05e331c4 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -936,13 +936,22 @@ int mmc_execute_tuning(struct mmc_card *card)
 		opcode = MMC_SEND_TUNING_BLOCK;
 
 	err = host->ops->execute_tuning(host, opcode);
-
 	if (err)
-		pr_err("%s: tuning execution failed: %d\n",
-			mmc_hostname(host), err);
-	else
-		mmc_retune_enable(host);
+		goto out_err;
+
+	mmc_retune_enable(host);
 
+	return 0;
+
+out_err:
+	if (mmc_card_is_removable(host)) {
+		if (err != -ENOMEDIUM)
+			pr_err("%s: tuning execution failed: %d (this is normal if card removed)\n",
+			       mmc_hostname(host), err);
+	} else {
+		pr_err("%s: tuning execution failed: %d\n",
+		       mmc_hostname(host), err);
+	}
 	return err;
 }
 

Re: [PATCH] mmc: disable tuning when checking card presence

[Wolfram Sang]

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


> +			pr_err("%s: tuning execution failed: %d (this is normal if card removed)\n",
> +			       mmc_hostname(host), err);

Hmm, an error message saying "this is normal" doesn't look like a good
option to me. Can't we surpress the message somehow or even avoid tuning
somehow if the card is removed? Sorry, I can't look this up myself right
now, working on another task today.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] mmc: disable tuning when checking card presence

[Adrian Hunter]

On 21/06/21 11:11 am, Wolfram Sang wrote:
> On 21/06/21 10:54 am, Adrian Hunter wrote:
>> On 21/06/21 10:32 am, Ulrich Hecht wrote:
>>>
>>>> On 06/21/2021 9:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Can we clarify, is the only problem that the error message is confusing?
>>>
>>> AFAICT there are no ill effects of the retune failing apart from the error message.
>>>
>> 
>> So maybe the simplest thing to do is just amend the message:
>> e.g.
>> 
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 4e52eb14198a..5cbf05e331c4 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -936,13 +936,22 @@ int mmc_execute_tuning(struct mmc_card *card)
>>  		opcode = MMC_SEND_TUNING_BLOCK;
>>  
>>  	err = host->ops->execute_tuning(host, opcode);
>> -
>>  	if (err)
>> -		pr_err("%s: tuning execution failed: %d\n",
>> -			mmc_hostname(host), err);
>> -	else
>> -		mmc_retune_enable(host);
>> +		goto out_err;
>> +
>> +	mmc_retune_enable(host);
>>  
>> +	return 0;
>> +
>> +out_err:
>> +	if (mmc_card_is_removable(host)) {
>> +		if (err != -ENOMEDIUM)
>> +			pr_err("%s: tuning execution failed: %d (this is normal if card removed)\n",
>> +			       mmc_hostname(host), err);
> 
> Hmm, an error message saying "this is normal" doesn't look like a good
> option to me. Can't we surpress the message somehow or even avoid tuning
> somehow if the card is removed? Sorry, I can't look this up myself right
> now, working on another task today.

With the code above, if the host controller knows the card has been
removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress
the message.

Otherwise, you need to introduce a new card state or flag to indicate
that the card may not be present, and use that to suppress the message.

> >> +	} else {
>> +		pr_err("%s: tuning execution failed: %d\n",
>> +		       mmc_hostname(host), err);
>> +	}
>>  	return err;
>>  }
>>  
>> 
>> > 

Re: [PATCH] mmc: disable tuning when checking card presence

[Wolfram Sang]

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

Hi Adrian, Ulf, everyone,

> With the code above, if the host controller knows the card has been
> removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress
> the message.

On second thought, I like the idea with -ENOMEDIUM. Because tuning can
still fail for reasons other than a removed card and we want to see an
error message then.

So, I checked when/how to return -ENOMEDIUM for the SDHI driver but this
lead me to more questions. The few driver which return this error code
all follow a similar pattern:

xxx_request()
{
	if (host->get_cd == 1)
		submit_mrq
	else
		cmd->error = -ENOMEDIUM
		mmc_request_done()
}

So, my first question would be if we can't apply this pattern in the
core before calling the .request callback? A lot of drivers are not
implementing this pattern although it seems useful. Is it required?
Recommended? Nice to have? However, I could imagine an answer for moving
it into the core is "no, that should be checked atomically"? E.g. sdhci
does it, but atmel-mci and s3cmci do not. If I just look at moving the
card detection call into the core, I don't really see the reason for
atomic. Am I missing something?

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] mmc: disable tuning when checking card presence

[Ulf Hansson]

On Sat, 26 Jun 2021 at 20:58, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Adrian, Ulf, everyone,
>
> > With the code above, if the host controller knows the card has been
> > removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress
> > the message.
>
> On second thought, I like the idea with -ENOMEDIUM. Because tuning can
> still fail for reasons other than a removed card and we want to see an
> error message then.
>
> So, I checked when/how to return -ENOMEDIUM for the SDHI driver but this
> lead me to more questions. The few driver which return this error code
> all follow a similar pattern:
>
> xxx_request()
> {
>         if (host->get_cd == 1)
>                 submit_mrq
>         else
>                 cmd->error = -ENOMEDIUM
>                 mmc_request_done()
> }
>
> So, my first question would be if we can't apply this pattern in the
> core before calling the .request callback? A lot of drivers are not
> implementing this pattern although it seems useful. Is it required?

It's required for some sdhci variants, because issuing a command when
a card has been removed can hang (or completes after quite a long
timeout, I don't recall, Adrian?).

> Recommended? Nice to have? However, I could imagine an answer for moving
> it into the core is "no, that should be checked atomically"? E.g. sdhci
> does it, but atmel-mci and s3cmci do not. If I just look at moving the
> card detection call into the core, I don't really see the reason for
> atomic. Am I missing something?

My main concern would be performance/latency, as we would introduce
some overhead for every single request. So, no, we don't want this in
the core in my opinion.

Kind regards
Uffe

Re: [PATCH] mmc: disable tuning when checking card presence

[Adrian Hunter]

On 29/06/21 5:16 pm, Ulf Hansson wrote:
> On Sat, 26 Jun 2021 at 20:58, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>>
>> Hi Adrian, Ulf, everyone,
>>
>>> With the code above, if the host controller knows the card has been
>>> removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress
>>> the message.
>>
>> On second thought, I like the idea with -ENOMEDIUM. Because tuning can
>> still fail for reasons other than a removed card and we want to see an
>> error message then.
>>
>> So, I checked when/how to return -ENOMEDIUM for the SDHI driver but this
>> lead me to more questions. The few driver which return this error code
>> all follow a similar pattern:
>>
>> xxx_request()
>> {
>>         if (host->get_cd == 1)
>>                 submit_mrq
>>         else
>>                 cmd->error = -ENOMEDIUM
>>                 mmc_request_done()
>> }
>>
>> So, my first question would be if we can't apply this pattern in the
>> core before calling the .request callback? A lot of drivers are not
>> implementing this pattern although it seems useful. Is it required?
> 
> It's required for some sdhci variants, because issuing a command when
> a card has been removed can hang (or completes after quite a long
> timeout, I don't recall, Adrian?).

If the host supports SDHCI's own card detect then after the card is
removed requests will not start nor error, until the 10 second
software timeout.  The logic to check SDHCI card present predated
the use of GPIO card-detect but the same approach was copied, although
it should be possible to do without it.

> 
>> Recommended? Nice to have? However, I could imagine an answer for moving
>> it into the core is "no, that should be checked atomically"? E.g. sdhci
>> does it, but atmel-mci and s3cmci do not. If I just look at moving the
>> card detection call into the core, I don't really see the reason for
>> atomic. Am I missing something?
> 
> My main concern would be performance/latency, as we would introduce
> some overhead for every single request. So, no, we don't want this in
> the core in my opinion.

I agree.

I would get rid of it from SDHCI but it looked like we might need to
rely on ->card_event() which won't work because it claims the host,
which will wait for the block driver to release it, which will wait
for the request anyway. That was as far as I got, thinking about it.

Re: [PATCH] mmc: disable tuning when checking card presence

[Wolfram Sang]

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


> Otherwise, you need to introduce a new card state or flag to indicate
> that the card may not be present, and use that to suppress the message.

I now went this route and, for me, 'detect_change' worked. Will send a
patch in a minute.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]