diff mbox series

mmc: disable tuning when checking card presence

Message ID 20210618082317.58408-1-wsa+renesas@sang-engineering.com (mailing list archive)
State New, archived
Headers show
Series mmc: disable tuning when checking card presence | expand

Commit Message

Wolfram Sang June 18, 2021, 8:23 a.m. UTC
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(+)

Comments

Ulrich Hecht June 18, 2021, 10:34 a.m. UTC | #1
> 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
Ulf Hansson June 18, 2021, 10:42 a.m. UTC | #2
+ 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
Adrian Hunter June 21, 2021, 7:15 a.m. UTC | #3
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
>
Ulrich Hecht June 21, 2021, 7:32 a.m. UTC | #4
> 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
Adrian Hunter June 21, 2021, 7:54 a.m. UTC | #5
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;
 }
Wolfram Sang June 21, 2021, 8:11 a.m. UTC | #6
> +			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.
Adrian Hunter June 21, 2021, 8:26 a.m. UTC | #7
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;
>>  }
>>  
>> 
>> >
Wolfram Sang June 26, 2021, 6:58 p.m. UTC | #8
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
Ulf Hansson June 29, 2021, 2:16 p.m. UTC | #9
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
Adrian Hunter June 29, 2021, 4:01 p.m. UTC | #10
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.
Wolfram Sang June 30, 2021, 4:08 a.m. UTC | #11
> 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.
diff mbox series

Patch

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;
 }