[PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Removing a card "slowly" can trigger a GPIO irq to be raised far
before the card is actually removed. This means the scheduled
detect work will not find out that the card were removed and thus
the card and the block device will not be unregistered.

Let the mmc_detect_card_removed function trigger a new detect
work immediately when it discovers that a card has been removed.
This will solve the described issue above. Moreover we make sure
the detect work is executed as soon as possible, since there is
no reason for waiting for a "delayed" detect to happen.

Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>
---
 drivers/mmc/core/core.c  |   24 +++++++++++++-----------
 include/linux/mmc/host.h |    1 -
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 4770807..7bc02f4 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host, unsigned long delay)
 	WARN_ON(host->removed);
 	spin_unlock_irqrestore(&host->lock, flags);
 #endif
-	host->detect_change = 1;
 	mmc_schedule_delayed_work(&host->detect, delay);
 }
 
@@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host *host)
 int mmc_detect_card_removed(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
+	int ret = 1;
 
 	WARN_ON(!host->claimed);
-	/*
-	 * The card will be considered unchanged unless we have been asked to
-	 * detect a change or host requires polling to provide card detection.
-	 */
-	if (card && !host->detect_change && !(host->caps & MMC_CAP_NEEDS_POLL))
-		return mmc_card_removed(card);
 
-	host->detect_change = 0;
+	if (card && !mmc_card_removed(card)) {
+		if (_mmc_detect_card_removed(host)) {
+			/*
+			 * Make sure a detect work is always executed and also
+			 * do it as soon as possible.
+			 */
+			cancel_delayed_work(&host->detect);
+			mmc_detect_change(host, 0);
+		}
+		ret = mmc_card_removed(card);
+	}
 
-	return _mmc_detect_card_removed(host);
+	return ret;
 }
 EXPORT_SYMBOL(mmc_detect_card_removed);
 
@@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
 	    && !(host->caps & MMC_CAP_NONREMOVABLE))
 		host->bus_ops->detect(host);
 
-	host->detect_change = 0;
-
 	/*
 	 * Let mmc_bus_put() free the bus/bus_ops if we've found that
 	 * the card is no longer present.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 031d865..09fa5e6 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -305,7 +305,6 @@ struct mmc_host {
 	int			claim_cnt;	/* "claim" nesting count */
 
 	struct delayed_work	detect;
-	int			detect_change;	/* card detect flag */
 	struct mmc_hotplug	hotplug;
 
 	const struct mmc_bus_ops *bus_ops;	/* current bus driver */
-- 
1.7.5.4

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Linus Walleij]

On Tue, Jan 3, 2012 at 11:33 AM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:

> Removing a card "slowly" can trigger a GPIO irq to be raised far
> before the card is actually removed. This means the scheduled
> detect work will not find out that the card were removed and thus
> the card and the block device will not be unregistered.
>
> Let the mmc_detect_card_removed function trigger a new detect
> work immediately when it discovers that a card has been removed.
> This will solve the described issue above. Moreover we make sure
> the detect work is executed as soon as possible, since there is
> no reason for waiting for a "delayed" detect to happen.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@stericsson.com>

This looks way more solid than the simple variable.
The old detect_change variable does not seem to be
protected by any lock either.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 3/01/2012 12:33 p.m., Ulf Hansson wrote:
> Removing a card "slowly" can trigger a GPIO irq to be raised far
> before the card is actually removed. This means the scheduled
> detect work will not find out that the card were removed and thus
> the card and the block device will not be unregistered.

One way around that problem is to error out requests after the GPIO
indicates the card is "removed".  sdhci effectively does that but with
a "present" bit.  Perhaps even simpler - set the card-removed flag.

> Let the mmc_detect_card_removed function trigger a new detect
> work immediately when it discovers that a card has been removed.

This is changing some long-standing functionality i.e. the card is not 
removed
without a card detect event.  It is difficult to know whether that will 
be very
bad for poor quality cards,

> This will solve the described issue above. Moreover we make sure
> the detect work is executed as soon as possible, since there is
> no reason for waiting for a "delayed" detect to happen.
>
> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
> ---
>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>   include/linux/mmc/host.h |    1 -
>   2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 4770807..7bc02f4 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host, unsigned long delay)
>   	WARN_ON(host->removed);
>   	spin_unlock_irqrestore(&host->lock, flags);
>   #endif
> -	host->detect_change = 1;
>   	mmc_schedule_delayed_work(&host->detect, delay);
>   }
>
> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>   int mmc_detect_card_removed(struct mmc_host *host)
>   {
>   	struct mmc_card *card = host->card;
> +	int ret = 1;
>
>   	WARN_ON(!host->claimed);
> -	/*
> -	 * The card will be considered unchanged unless we have been asked to
> -	 * detect a change or host requires polling to provide card detection.
> -	 */
> -	if (card&&  !host->detect_change&&  !(host->caps&  MMC_CAP_NEEDS_POLL))
> -		return mmc_card_removed(card);
>
> -	host->detect_change = 0;
> +	if (card&&  !mmc_card_removed(card)) {
> +		if (_mmc_detect_card_removed(host)) {
> +			/*
> +			 * Make sure a detect work is always executed and also
> +			 * do it as soon as possible.
> +			 */
> +			cancel_delayed_work(&host->detect);
> +			mmc_detect_change(host, 0);
> +		}
> +		ret = mmc_card_removed(card);
> +	}
>
> -	return _mmc_detect_card_removed(host);
> +	return ret;
>   }
>   EXPORT_SYMBOL(mmc_detect_card_removed);
>
> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>   	&&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>   		host->bus_ops->detect(host);
>
> -	host->detect_change = 0;
> -
>   	/*
>   	 * Let mmc_bus_put() free the bus/bus_ops if we've found that
>   	 * the card is no longer present.
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 031d865..09fa5e6 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -305,7 +305,6 @@ struct mmc_host {
>   	int			claim_cnt;	/* "claim" nesting count */
>
>   	struct delayed_work	detect;
> -	int			detect_change;	/* card detect flag */
>   	struct mmc_hotplug	hotplug;
>
>   	const struct mmc_bus_ops *bus_ops;	/* current bus driver */

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 3/01/2012 12:33 p.m., Ulf Hansson wrote:
>> Removing a card "slowly" can trigger a GPIO irq to be raised far
>> before the card is actually removed. This means the scheduled
>> detect work will not find out that the card were removed and thus
>> the card and the block device will not be unregistered.
> 
> One way around that problem is to error out requests after the GPIO
> indicates the card is "removed".  sdhci effectively does that but with
> a "present" bit.  Perhaps even simpler - set the card-removed flag.

I get the idea, but that will only partly solve this issue.

My concern is more about what we actually can trust; either the GPIO irq 
which likely is giving more than one irq when inserting/removing a card 
since the slot is probably not glitch free, or that a "rescan" runs to 
make sure a CMD13 is accepted from the previously inserted card.

Moreover, the issue this patch tries to solve can not be solved without 
doing a "rescan" which must be triggered from the the block layer some 
how. I thought this new function that you previously added 
"mmc_detect_card_remove" was the proper place to do this.

> 
>> Let the mmc_detect_card_removed function trigger a new detect
>> work immediately when it discovers that a card has been removed.
> 
> This is changing some long-standing functionality i.e. the card is not 
> removed
> without a card detect event.  It is difficult to know whether that will 
> be very
> bad for poor quality cards,

Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the 
card to make sure it is still present, that is already done from the 
block layer after each read/write request. So I can not see that "poor 
quality cards" will have any further problem with this patch, but I 
might miss something!?

> 
>> This will solve the described issue above. Moreover we make sure
>> the detect work is executed as soon as possible, since there is
>> no reason for waiting for a "delayed" detect to happen.
>>
>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>> ---
>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>   include/linux/mmc/host.h |    1 -
>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 4770807..7bc02f4 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host, unsigned long delay)
>>   	WARN_ON(host->removed);
>>   	spin_unlock_irqrestore(&host->lock, flags);
>>   #endif
>> -	host->detect_change = 1;
>>   	mmc_schedule_delayed_work(&host->detect, delay);
>>   }
>>
>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>>   int mmc_detect_card_removed(struct mmc_host *host)
>>   {
>>   	struct mmc_card *card = host->card;
>> +	int ret = 1;
>>
>>   	WARN_ON(!host->claimed);
>> -	/*
>> -	 * The card will be considered unchanged unless we have been asked to
>> -	 * detect a change or host requires polling to provide card detection.
>> -	 */
>> -	if (card&&  !host->detect_change&&  !(host->caps&  MMC_CAP_NEEDS_POLL))
>> -		return mmc_card_removed(card);
>>
>> -	host->detect_change = 0;
>> +	if (card&&  !mmc_card_removed(card)) {
>> +		if (_mmc_detect_card_removed(host)) {
>> +			/*
>> +			 * Make sure a detect work is always executed and also
>> +			 * do it as soon as possible.
>> +			 */
>> +			cancel_delayed_work(&host->detect);
>> +			mmc_detect_change(host, 0);
>> +		}
>> +		ret = mmc_card_removed(card);
>> +	}
>>
>> -	return _mmc_detect_card_removed(host);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>
>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>   	&&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>   		host->bus_ops->detect(host);
>>
>> -	host->detect_change = 0;
>> -
>>   	/*
>>   	 * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>   	 * the card is no longer present.
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 031d865..09fa5e6 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -305,7 +305,6 @@ struct mmc_host {
>>   	int			claim_cnt;	/* "claim" nesting count */
>>
>>   	struct delayed_work	detect;
>> -	int			detect_change;	/* card detect flag */
>>   	struct mmc_hotplug	hotplug;
>>
>>   	const struct mmc_bus_ops *bus_ops;	/* current bus driver */
> 
> 

Br
Ulf Hansson

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 09/01/12 13:02, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 3/01/2012 12:33 p.m., Ulf Hansson wrote:
>>> Removing a card "slowly" can trigger a GPIO irq to be raised far
>>> before the card is actually removed. This means the scheduled
>>> detect work will not find out that the card were removed and thus
>>> the card and the block device will not be unregistered.
>>
>> One way around that problem is to error out requests after the GPIO
>> indicates the card is "removed".  sdhci effectively does that but with
>> a "present" bit.  Perhaps even simpler - set the card-removed flag.
> 
> I get the idea, but that will only partly solve this issue.
> 
> My concern is more about what we actually can trust; either the GPIO irq
> which likely is giving more than one irq when inserting/removing a card
> since the slot is probably not glitch free, or that a "rescan" runs to make
> sure a CMD13 is accepted from the previously inserted card.

Yes, I guess you would need to debounce the GPIO if you wanted to rely on it.

> 
> Moreover, the issue this patch tries to solve can not be solved without
> doing a "rescan" which must be triggered from the the block layer some how.
> I thought this new function that you previously added
> "mmc_detect_card_remove" was the proper place to do this.
> 
>>
>>> Let the mmc_detect_card_removed function trigger a new detect
>>> work immediately when it discovers that a card has been removed.
>>
>> This is changing some long-standing functionality i.e. the card is not
>> removed
>> without a card detect event.  It is difficult to know whether that will be
>> very
>> bad for poor quality cards,
> 
> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
> to make sure it is still present, that is already done from the block layer
> after each read/write request. So I can not see that "poor quality cards"
> will have any further problem with this patch, but I might miss something!?

The block driver has never caused a card to be removed before.  That is new
and it is designed to preserve existing behaviour i.e. do not remove a card
without a card detect event.

You are assuming:
	1. that a poor quality card will not return errors for a few
	commands and then resume operation
	2. that removing a card on error is desirable

Both those assumptions may be true, but there is no evidence that they are.


> 
>>
>>> This will solve the described issue above. Moreover we make sure
>>> the detect work is executed as soon as possible, since there is
>>> no reason for waiting for a "delayed" detect to happen.
>>>
>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>> ---
>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>   include/linux/mmc/host.h |    1 -
>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 4770807..7bc02f4 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>> unsigned long delay)
>>>       WARN_ON(host->removed);
>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>   #endif
>>> -    host->detect_change = 1;
>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>   }
>>>
>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>   {
>>>       struct mmc_card *card = host->card;
>>> +    int ret = 1;
>>>
>>>       WARN_ON(!host->claimed);
>>> -    /*
>>> -     * The card will be considered unchanged unless we have been asked to
>>> -     * detect a change or host requires polling to provide card detection.
>>> -     */
>>> -    if (card&&  !host->detect_change&&  !(host->caps&  MMC_CAP_NEEDS_POLL))
>>> -        return mmc_card_removed(card);
>>>
>>> -    host->detect_change = 0;
>>> +    if (card&&  !mmc_card_removed(card)) {
>>> +        if (_mmc_detect_card_removed(host)) {
>>> +            /*
>>> +             * Make sure a detect work is always executed and also
>>> +             * do it as soon as possible.
>>> +             */
>>> +            cancel_delayed_work(&host->detect);
>>> +            mmc_detect_change(host, 0);
>>> +        }
>>> +        ret = mmc_card_removed(card);
>>> +    }
>>>
>>> -    return _mmc_detect_card_removed(host);
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>
>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>           host->bus_ops->detect(host);
>>>
>>> -    host->detect_change = 0;
>>> -
>>>       /*
>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>        * the card is no longer present.
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index 031d865..09fa5e6 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>       int            claim_cnt;    /* "claim" nesting count */
>>>
>>>       struct delayed_work    detect;
>>> -    int            detect_change;    /* card detect flag */
>>>       struct mmc_hotplug    hotplug;
>>>
>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>
>>
> 
> Br
> Ulf Hansson
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

>> My concern is more about what we actually can trust; either the GPIO irq
>> which likely is giving more than one irq when inserting/removing a card
>> since the slot is probably not glitch free, or that a "rescan" runs to make
>> sure a CMD13 is accepted from the previously inserted card.
> 
> Yes, I guess you would need to debounce the GPIO if you wanted to rely on it.
> 
>> Moreover, the issue this patch tries to solve can not be solved without
>> doing a "rescan" which must be triggered from the the block layer some how.
>> I thought this new function that you previously added
>> "mmc_detect_card_remove" was the proper place to do this.
>>
>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>> work immediately when it discovers that a card has been removed.
>>> This is changing some long-standing functionality i.e. the card is not
>>> removed
>>> without a card detect event.  It is difficult to know whether that will be
>>> very
>>> bad for poor quality cards,
>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>> to make sure it is still present, that is already done from the block layer
>> after each read/write request. So I can not see that "poor quality cards"
>> will have any further problem with this patch, but I might miss something!?
> 
> The block driver has never caused a card to be removed before.  That is new
> and it is designed to preserve existing behaviour i.e. do not remove a card
> without a card detect event.

True, but is this a problem!?

Anyway, this is the actual issue this patch is trying to solve. If you 
remove a card "slowly", a "rescan" work, which the GPIO irq has 
triggered to run will run the CMD13 to verify that the card is still 
there. Since it has not completely been removed the CMD13 will succeed 
and the card will not be removed.

Moreover every other new block request will soon start to fail and 
always do; until a new rescan is triggered (which is when you insert a 
new card or do a suspend-resume cycle). In practice I think it is more 
preferred that the card gets removed and it's corresponding block device.

> 
> You are assuming:
> 	1. that a poor quality card will not return errors for a few
> 	commands and then resume operation

I see your point. I did some tests with a bunch of old crappy cards, 
both SD and MMC which I had in my collection. I have found none of these 
to trigger a undesirable removal of the card.

Of course I have only a subset of all cards, so this can not be fully 
tested for all existing cards.

> 	2. that removing a card on error is desirable

Well, we will just fire of a rescan work to check if the card has been 
removed. If it is still there it will of course not be removed.

> 
> Both those assumptions may be true, but there is no evidence that they are.
> 
> 
>>>> This will solve the described issue above. Moreover we make sure
>>>> the detect work is executed as soon as possible, since there is
>>>> no reason for waiting for a "delayed" detect to happen.
>>>>
>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>> ---
>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>   include/linux/mmc/host.h |    1 -
>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 4770807..7bc02f4 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>> unsigned long delay)
>>>>       WARN_ON(host->removed);
>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>   #endif
>>>> -    host->detect_change = 1;
>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>   }
>>>>
>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>   {
>>>>       struct mmc_card *card = host->card;
>>>> +    int ret = 1;
>>>>
>>>>       WARN_ON(!host->claimed);
>>>> -    /*
>>>> -     * The card will be considered unchanged unless we have been asked to
>>>> -     * detect a change or host requires polling to provide card detection.
>>>> -     */
>>>> -    if (card&&  !host->detect_change&&  !(host->caps&  MMC_CAP_NEEDS_POLL))
>>>> -        return mmc_card_removed(card);
>>>>
>>>> -    host->detect_change = 0;
>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>> +        if (_mmc_detect_card_removed(host)) {
>>>> +            /*
>>>> +             * Make sure a detect work is always executed and also
>>>> +             * do it as soon as possible.
>>>> +             */
>>>> +            cancel_delayed_work(&host->detect);
>>>> +            mmc_detect_change(host, 0);
>>>> +        }
>>>> +        ret = mmc_card_removed(card);
>>>> +    }
>>>>
>>>> -    return _mmc_detect_card_removed(host);
>>>> +    return ret;
>>>>   }
>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>
>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>           host->bus_ops->detect(host);
>>>>
>>>> -    host->detect_change = 0;
>>>> -
>>>>       /*
>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>        * the card is no longer present.
>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>> index 031d865..09fa5e6 100644
>>>> --- a/include/linux/mmc/host.h
>>>> +++ b/include/linux/mmc/host.h
>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>
>>>>       struct delayed_work    detect;
>>>> -    int            detect_change;    /* card detect flag */
>>>>       struct mmc_hotplug    hotplug;
>>>>
>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>
>> Br
>> Ulf Hansson
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 09/01/12 15:14, Ulf Hansson wrote:
> 
>>> My concern is more about what we actually can trust; either the GPIO irq
>>> which likely is giving more than one irq when inserting/removing a card
>>> since the slot is probably not glitch free, or that a "rescan" runs to make
>>> sure a CMD13 is accepted from the previously inserted card.
>>
>> Yes, I guess you would need to debounce the GPIO if you wanted to rely on it.
>>
>>> Moreover, the issue this patch tries to solve can not be solved without
>>> doing a "rescan" which must be triggered from the the block layer some how.
>>> I thought this new function that you previously added
>>> "mmc_detect_card_remove" was the proper place to do this.
>>>
>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>> work immediately when it discovers that a card has been removed.
>>>> This is changing some long-standing functionality i.e. the card is not
>>>> removed
>>>> without a card detect event.  It is difficult to know whether that will be
>>>> very
>>>> bad for poor quality cards,
>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>> to make sure it is still present, that is already done from the block layer
>>> after each read/write request. So I can not see that "poor quality cards"
>>> will have any further problem with this patch, but I might miss something!?
>>
>> The block driver has never caused a card to be removed before.  That is new
>> and it is designed to preserve existing behaviour i.e. do not remove a card
>> without a card detect event.
> 
> True, but is this a problem!?

Better not to find out.

> 
> Anyway, this is the actual issue this patch is trying to solve. If you
> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
> run will run the CMD13 to verify that the card is still there. Since it has
> not completely been removed the CMD13 will succeed and the card will not be
> removed.
> 
> Moreover every other new block request will soon start to fail and always
> do; until a new rescan is triggered (which is when you insert a new card or
> do a suspend-resume cycle). In practice I think it is more preferred that
> the card gets removed and it's corresponding block device.

There are other ways to solve that problem.  Apart from my previous
suggestion, there is also the possibility to make use of ->get_cd
instead of CMD13, someone already posted a patch for that
"[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
but it should probably be selected on a per driver basis (i.e. add a
MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
though.

> 
>>
>> You are assuming:
>>     1. that a poor quality card will not return errors for a few
>>     commands and then resume operation
> 
> I see your point. I did some tests with a bunch of old crappy cards, both SD
> and MMC which I had in my collection. I have found none of these to trigger
> a undesirable removal of the card.
> 
> Of course I have only a subset of all cards, so this can not be fully tested
> for all existing cards.
> 
>>     2. that removing a card on error is desirable
> 
> Well, we will just fire of a rescan work to check if the card has been
> removed. If it is still there it will of course not be removed.

Not if it has stopped responding.  Again, this is a change in behaviour.
Previously, a card that stopped responding was not removed.

Perhaps in the future someone will want to try to recover cards that
stop responding, for example by power-cycling.  That would be in
conflict with your approach because it would power cycle on every single
card removal.

> 
>>
>> Both those assumptions may be true, but there is no evidence that they are.
>>
>>
>>>>> This will solve the described issue above. Moreover we make sure
>>>>> the detect work is executed as soon as possible, since there is
>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>
>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>> ---
>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>   include/linux/mmc/host.h |    1 -
>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>> index 4770807..7bc02f4 100644
>>>>> --- a/drivers/mmc/core/core.c
>>>>> +++ b/drivers/mmc/core/core.c
>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>> unsigned long delay)
>>>>>       WARN_ON(host->removed);
>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>   #endif
>>>>> -    host->detect_change = 1;
>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>   }
>>>>>
>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>> *host)
>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>   {
>>>>>       struct mmc_card *card = host->card;
>>>>> +    int ret = 1;
>>>>>
>>>>>       WARN_ON(!host->claimed);
>>>>> -    /*
>>>>> -     * The card will be considered unchanged unless we have been asked to
>>>>> -     * detect a change or host requires polling to provide card
>>>>> detection.
>>>>> -     */
>>>>> -    if (card&&  !host->detect_change&&  !(host->caps& 
>>>>> MMC_CAP_NEEDS_POLL))
>>>>> -        return mmc_card_removed(card);
>>>>>
>>>>> -    host->detect_change = 0;
>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>> +            /*
>>>>> +             * Make sure a detect work is always executed and also
>>>>> +             * do it as soon as possible.
>>>>> +             */
>>>>> +            cancel_delayed_work(&host->detect);
>>>>> +            mmc_detect_change(host, 0);
>>>>> +        }
>>>>> +        ret = mmc_card_removed(card);
>>>>> +    }
>>>>>
>>>>> -    return _mmc_detect_card_removed(host);
>>>>> +    return ret;
>>>>>   }
>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>
>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>           host->bus_ops->detect(host);
>>>>>
>>>>> -    host->detect_change = 0;
>>>>> -
>>>>>       /*
>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>        * the card is no longer present.
>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>> index 031d865..09fa5e6 100644
>>>>> --- a/include/linux/mmc/host.h
>>>>> +++ b/include/linux/mmc/host.h
>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>
>>>>>       struct delayed_work    detect;
>>>>> -    int            detect_change;    /* card detect flag */
>>>>>       struct mmc_hotplug    hotplug;
>>>>>
>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>
>>> Br
>>> Ulf Hansson
>>>
>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 09/01/12 15:14, Ulf Hansson wrote:
>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>> which likely is giving more than one irq when inserting/removing a card
>>>> since the slot is probably not glitch free, or that a "rescan" runs to make
>>>> sure a CMD13 is accepted from the previously inserted card.
>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely on it.
>>>
>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>> doing a "rescan" which must be triggered from the the block layer some how.
>>>> I thought this new function that you previously added
>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>
>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>> work immediately when it discovers that a card has been removed.
>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>> removed
>>>>> without a card detect event.  It is difficult to know whether that will be
>>>>> very
>>>>> bad for poor quality cards,
>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>>> to make sure it is still present, that is already done from the block layer
>>>> after each read/write request. So I can not see that "poor quality cards"
>>>> will have any further problem with this patch, but I might miss something!?
>>> The block driver has never caused a card to be removed before.  That is new
>>> and it is designed to preserve existing behaviour i.e. do not remove a card
>>> without a card detect event.
>> True, but is this a problem!?
> 
> Better not to find out.

:-)

Then there is lot of other things around mmc we also should not change.

> 
>> Anyway, this is the actual issue this patch is trying to solve. If you
>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>> run will run the CMD13 to verify that the card is still there. Since it has
>> not completely been removed the CMD13 will succeed and the card will not be
>> removed.
>>
>> Moreover every other new block request will soon start to fail and always
>> do; until a new rescan is triggered (which is when you insert a new card or
>> do a suspend-resume cycle). In practice I think it is more preferred that
>> the card gets removed and it's corresponding block device.
> 
> There are other ways to solve that problem.  Apart from my previous
> suggestion, there is also the possibility to make use of ->get_cd
> instead of CMD13, someone already posted a patch for that
> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
> but it should probably be selected on a per driver basis (i.e. add a
> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
> though.
> 

Unfortunately that wont help to solve this issue either. That patch will 
only prevent you from executing a CMD13 if the get_cd function says the 
card is still there. I kind of micro optimization I think, unless you 
very often encounters errors in the block layer.

The key in this patch is that a rescan work is triggered to fully verify 
that the card is still there and if not, it can remove it. I don't think 
this is such a big matter, but of course this is my own opinion. :-)

>>> You are assuming:
>>>     1. that a poor quality card will not return errors for a few
>>>     commands and then resume operation
>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>> and MMC which I had in my collection. I have found none of these to trigger
>> a undesirable removal of the card.
>>
>> Of course I have only a subset of all cards, so this can not be fully tested
>> for all existing cards.
>>
>>>     2. that removing a card on error is desirable
>> Well, we will just fire of a rescan work to check if the card has been
>> removed. If it is still there it will of course not be removed.
> 
> Not if it has stopped responding.  Again, this is a change in behaviour.
> Previously, a card that stopped responding was not removed.
> 
> Perhaps in the future someone will want to try to recover cards that
> stop responding, for example by power-cycling.  That would be in
> conflict with your approach because it would power cycle on every single
> card removal.
> 
>>> Both those assumptions may be true, but there is no evidence that they are.
>>>
>>>
>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>> the detect work is executed as soon as possible, since there is
>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>> ---
>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>> index 4770807..7bc02f4 100644
>>>>>> --- a/drivers/mmc/core/core.c
>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>> unsigned long delay)
>>>>>>       WARN_ON(host->removed);
>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>   #endif
>>>>>> -    host->detect_change = 1;
>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>   }
>>>>>>
>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>> *host)
>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>   {
>>>>>>       struct mmc_card *card = host->card;
>>>>>> +    int ret = 1;
>>>>>>
>>>>>>       WARN_ON(!host->claimed);
>>>>>> -    /*
>>>>>> -     * The card will be considered unchanged unless we have been asked to
>>>>>> -     * detect a change or host requires polling to provide card
>>>>>> detection.
>>>>>> -     */
>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps& 
>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>> -        return mmc_card_removed(card);
>>>>>>
>>>>>> -    host->detect_change = 0;
>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>> +            /*
>>>>>> +             * Make sure a detect work is always executed and also
>>>>>> +             * do it as soon as possible.
>>>>>> +             */
>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>> +            mmc_detect_change(host, 0);
>>>>>> +        }
>>>>>> +        ret = mmc_card_removed(card);
>>>>>> +    }
>>>>>>
>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>> +    return ret;
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>
>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>           host->bus_ops->detect(host);
>>>>>>
>>>>>> -    host->detect_change = 0;
>>>>>> -
>>>>>>       /*
>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>        * the card is no longer present.
>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>> index 031d865..09fa5e6 100644
>>>>>> --- a/include/linux/mmc/host.h
>>>>>> +++ b/include/linux/mmc/host.h
>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>
>>>>>>       struct delayed_work    detect;
>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>
>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>> Br
>>>> Ulf Hansson
>>>>
>>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Sorry, missed one of your comments.

Adrian Hunter wrote:
> On 09/01/12 15:14, Ulf Hansson wrote:
>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>> which likely is giving more than one irq when inserting/removing a card
>>>> since the slot is probably not glitch free, or that a "rescan" runs to make
>>>> sure a CMD13 is accepted from the previously inserted card.
>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely on it.
>>>
>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>> doing a "rescan" which must be triggered from the the block layer some how.
>>>> I thought this new function that you previously added
>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>
>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>> work immediately when it discovers that a card has been removed.
>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>> removed
>>>>> without a card detect event.  It is difficult to know whether that will be
>>>>> very
>>>>> bad for poor quality cards,
>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>>> to make sure it is still present, that is already done from the block layer
>>>> after each read/write request. So I can not see that "poor quality cards"
>>>> will have any further problem with this patch, but I might miss something!?
>>> The block driver has never caused a card to be removed before.  That is new
>>> and it is designed to preserve existing behaviour i.e. do not remove a card
>>> without a card detect event.
>> True, but is this a problem!?
> 
> Better not to find out.
> 
>> Anyway, this is the actual issue this patch is trying to solve. If you
>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>> run will run the CMD13 to verify that the card is still there. Since it has
>> not completely been removed the CMD13 will succeed and the card will not be
>> removed.
>>
>> Moreover every other new block request will soon start to fail and always
>> do; until a new rescan is triggered (which is when you insert a new card or
>> do a suspend-resume cycle). In practice I think it is more preferred that
>> the card gets removed and it's corresponding block device.
> 
> There are other ways to solve that problem.  Apart from my previous
> suggestion, there is also the possibility to make use of ->get_cd
> instead of CMD13, someone already posted a patch for that
> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
> but it should probably be selected on a per driver basis (i.e. add a
> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
> though.
> 
>>> You are assuming:
>>>     1. that a poor quality card will not return errors for a few
>>>     commands and then resume operation
>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>> and MMC which I had in my collection. I have found none of these to trigger
>> a undesirable removal of the card.
>>
>> Of course I have only a subset of all cards, so this can not be fully tested
>> for all existing cards.
>>
>>>     2. that removing a card on error is desirable
>> Well, we will just fire of a rescan work to check if the card has been
>> removed. If it is still there it will of course not be removed.
> 
> Not if it has stopped responding.  Again, this is a change in behaviour.
> Previously, a card that stopped responding was not removed.
> 
> Perhaps in the future someone will want to try to recover cards that
> stop responding, for example by power-cycling.  That would be in
> conflict with your approach because it would power cycle on every single
> card removal.

This is pure hypothetical and the simple solution to such an idea would 
just be to do a "power-cycle attempt" before considering scheduling the 
rescan work in the mmc_detect_card_removed function.

> 
>>> Both those assumptions may be true, but there is no evidence that they are.
>>>
>>>
>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>> the detect work is executed as soon as possible, since there is
>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>> ---
>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>> index 4770807..7bc02f4 100644
>>>>>> --- a/drivers/mmc/core/core.c
>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>> unsigned long delay)
>>>>>>       WARN_ON(host->removed);
>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>   #endif
>>>>>> -    host->detect_change = 1;
>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>   }
>>>>>>
>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>> *host)
>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>   {
>>>>>>       struct mmc_card *card = host->card;
>>>>>> +    int ret = 1;
>>>>>>
>>>>>>       WARN_ON(!host->claimed);
>>>>>> -    /*
>>>>>> -     * The card will be considered unchanged unless we have been asked to
>>>>>> -     * detect a change or host requires polling to provide card
>>>>>> detection.
>>>>>> -     */
>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps& 
>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>> -        return mmc_card_removed(card);
>>>>>>
>>>>>> -    host->detect_change = 0;
>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>> +            /*
>>>>>> +             * Make sure a detect work is always executed and also
>>>>>> +             * do it as soon as possible.
>>>>>> +             */
>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>> +            mmc_detect_change(host, 0);
>>>>>> +        }
>>>>>> +        ret = mmc_card_removed(card);
>>>>>> +    }
>>>>>>
>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>> +    return ret;
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>
>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>           host->bus_ops->detect(host);
>>>>>>
>>>>>> -    host->detect_change = 0;
>>>>>> -
>>>>>>       /*
>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>        * the card is no longer present.
>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>> index 031d865..09fa5e6 100644
>>>>>> --- a/include/linux/mmc/host.h
>>>>>> +++ b/include/linux/mmc/host.h
>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>
>>>>>>       struct delayed_work    detect;
>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>
>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>> Br
>>>> Ulf Hansson
>>>>
>>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 09/01/12 16:27, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>> make
>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>> on it.
>>>>
>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>> how.
>>>>> I thought this new function that you previously added
>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>
>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>> removed
>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>> will be
>>>>>> very
>>>>>> bad for poor quality cards,
>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>>>> to make sure it is still present, that is already done from the block
>>>>> layer
>>>>> after each read/write request. So I can not see that "poor quality cards"
>>>>> will have any further problem with this patch, but I might miss
>>>>> something!?
>>>> The block driver has never caused a card to be removed before.  That is new
>>>> and it is designed to preserve existing behaviour i.e. do not remove a card
>>>> without a card detect event.
>>> True, but is this a problem!?
>>
>> Better not to find out.
> 
> :-)
> 
> Then there is lot of other things around mmc we also should not change.

Can you give an example of a change in existing functionality?  Isn't
everything either a bug fix or new functionality?

> 
>>
>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>>> run will run the CMD13 to verify that the card is still there. Since it has
>>> not completely been removed the CMD13 will succeed and the card will not be
>>> removed.
>>>
>>> Moreover every other new block request will soon start to fail and always
>>> do; until a new rescan is triggered (which is when you insert a new card or
>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>> the card gets removed and it's corresponding block device.
>>
>> There are other ways to solve that problem.  Apart from my previous
>> suggestion, there is also the possibility to make use of ->get_cd
>> instead of CMD13, someone already posted a patch for that
>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>> but it should probably be selected on a per driver basis (i.e. add a
>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>> though.
>>
> 
> Unfortunately that wont help to solve this issue either. That patch will
> only prevent you from executing a CMD13 if the get_cd function says the card
> is still there. I kind of micro optimization I think, unless you very often
> encounters errors in the block layer.

No, the rescan calls that code, so if get_cd() returns 0 the card will be
removed irrespective of whether it has been pulled out slowly or not.

> 
> The key in this patch is that a rescan work is triggered to fully verify
> that the card is still there and if not, it can remove it. I don't think
> this is such a big matter, but of course this is my own opinion. :-)

Another issue with your patch is that the card will not be removed unless
there is subsequent I/O to cause an I/O error and subsequent rescan.

> 
>>>> You are assuming:
>>>>     1. that a poor quality card will not return errors for a few
>>>>     commands and then resume operation
>>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>>> and MMC which I had in my collection. I have found none of these to trigger
>>> a undesirable removal of the card.
>>>
>>> Of course I have only a subset of all cards, so this can not be fully tested
>>> for all existing cards.
>>>
>>>>     2. that removing a card on error is desirable
>>> Well, we will just fire of a rescan work to check if the card has been
>>> removed. If it is still there it will of course not be removed.
>>
>> Not if it has stopped responding.  Again, this is a change in behaviour.
>> Previously, a card that stopped responding was not removed.
>>
>> Perhaps in the future someone will want to try to recover cards that
>> stop responding, for example by power-cycling.  That would be in
>> conflict with your approach because it would power cycle on every single
>> card removal.
>
> This is pure hypothetical and the simple solution to such an idea would
> just be to do a "power-cycle attempt" before considering scheduling the
> rescan work in the mmc_detect_card_removed function.

Nevertheless, in your case a power-cycle would be done for every card removal.

>
>>
>>>> Both those assumptions may be true, but there is no evidence that they are.
>>>>
>>>>
>>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>>> the detect work is executed as soon as possible, since there is
>>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>>
>>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>>> ---
>>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>> index 4770807..7bc02f4 100644
>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>>> unsigned long delay)
>>>>>>>       WARN_ON(host->removed);
>>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>>   #endif
>>>>>>> -    host->detect_change = 1;
>>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>>   }
>>>>>>>
>>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>>> *host)
>>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>>   {
>>>>>>>       struct mmc_card *card = host->card;
>>>>>>> +    int ret = 1;
>>>>>>>
>>>>>>>       WARN_ON(!host->claimed);
>>>>>>> -    /*
>>>>>>> -     * The card will be considered unchanged unless we have been
>>>>>>> asked to
>>>>>>> -     * detect a change or host requires polling to provide card
>>>>>>> detection.
>>>>>>> -     */
>>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps&
>>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>>> -        return mmc_card_removed(card);
>>>>>>>
>>>>>>> -    host->detect_change = 0;
>>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>>> +            /*
>>>>>>> +             * Make sure a detect work is always executed and also
>>>>>>> +             * do it as soon as possible.
>>>>>>> +             */
>>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>>> +            mmc_detect_change(host, 0);
>>>>>>> +        }
>>>>>>> +        ret = mmc_card_removed(card);
>>>>>>> +    }
>>>>>>>
>>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>>> +    return ret;
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>>
>>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>>           host->bus_ops->detect(host);
>>>>>>>
>>>>>>> -    host->detect_change = 0;
>>>>>>> -
>>>>>>>       /*
>>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>>        * the card is no longer present.
>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>> index 031d865..09fa5e6 100644
>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>>
>>>>>>>       struct delayed_work    detect;
>>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>>
>>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>> Br
>>>>> Ulf Hansson
>>>>>
>>>>
>>>
>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 09/01/12 16:27, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>> make
>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>> on it.
>>>>
>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>> how.
>>>>> I thought this new function that you previously added
>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>
>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>> removed
>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>> will be
>>>>>> very
>>>>>> bad for poor quality cards,
>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>>>> to make sure it is still present, that is already done from the block
>>>>> layer
>>>>> after each read/write request. So I can not see that "poor quality cards"
>>>>> will have any further problem with this patch, but I might miss
>>>>> something!?
>>>> The block driver has never caused a card to be removed before.  That is new
>>>> and it is designed to preserve existing behaviour i.e. do not remove a card
>>>> without a card detect event.
>>> True, but is this a problem!?
>>
>> Better not to find out.
> 
> :-)
> 
> Then there is lot of other things around mmc we also should not change.
> 
>>
>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>>> run will run the CMD13 to verify that the card is still there. Since it has
>>> not completely been removed the CMD13 will succeed and the card will not be
>>> removed.
>>>
>>> Moreover every other new block request will soon start to fail and always
>>> do; until a new rescan is triggered (which is when you insert a new card or
>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>> the card gets removed and it's corresponding block device.
>>
>> There are other ways to solve that problem.  Apart from my previous
>> suggestion, there is also the possibility to make use of ->get_cd
>> instead of CMD13, someone already posted a patch for that
>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>> but it should probably be selected on a per driver basis (i.e. add a
>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>> though.
>>
> 
> Unfortunately that wont help to solve this issue either. That patch will
> only prevent you from executing a CMD13 if the get_cd function says the card
> is still there. I kind of micro optimization I think, unless you very often
> encounters errors in the block layer.
> 
> The key in this patch is that a rescan work is triggered to fully verify
> that the card is still there and if not, it can remove it. I don't think
> this is such a big matter, but of course this is my own opinion. :-)

In that case it needs to be selected by the driver e.g.
add MMC_CAP2_RESCAN_ON_ERROR


> 
>>>> You are assuming:
>>>>     1. that a poor quality card will not return errors for a few
>>>>     commands and then resume operation
>>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>>> and MMC which I had in my collection. I have found none of these to trigger
>>> a undesirable removal of the card.
>>>
>>> Of course I have only a subset of all cards, so this can not be fully tested
>>> for all existing cards.
>>>
>>>>     2. that removing a card on error is desirable
>>> Well, we will just fire of a rescan work to check if the card has been
>>> removed. If it is still there it will of course not be removed.
>>
>> Not if it has stopped responding.  Again, this is a change in behaviour.
>> Previously, a card that stopped responding was not removed.
>>
>> Perhaps in the future someone will want to try to recover cards that
>> stop responding, for example by power-cycling.  That would be in
>> conflict with your approach because it would power cycle on every single
>> card removal.
>>
>>>> Both those assumptions may be true, but there is no evidence that they are.
>>>>
>>>>
>>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>>> the detect work is executed as soon as possible, since there is
>>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>>
>>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>>> ---
>>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>> index 4770807..7bc02f4 100644
>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>>> unsigned long delay)
>>>>>>>       WARN_ON(host->removed);
>>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>>   #endif
>>>>>>> -    host->detect_change = 1;
>>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>>   }
>>>>>>>
>>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>>> *host)
>>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>>   {
>>>>>>>       struct mmc_card *card = host->card;
>>>>>>> +    int ret = 1;
>>>>>>>
>>>>>>>       WARN_ON(!host->claimed);
>>>>>>> -    /*
>>>>>>> -     * The card will be considered unchanged unless we have been
>>>>>>> asked to
>>>>>>> -     * detect a change or host requires polling to provide card
>>>>>>> detection.
>>>>>>> -     */
>>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps&
>>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>>> -        return mmc_card_removed(card);
>>>>>>>
>>>>>>> -    host->detect_change = 0;
>>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>>> +            /*
>>>>>>> +             * Make sure a detect work is always executed and also
>>>>>>> +             * do it as soon as possible.
>>>>>>> +             */
>>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>>> +            mmc_detect_change(host, 0);
>>>>>>> +        }
>>>>>>> +        ret = mmc_card_removed(card);
>>>>>>> +    }
>>>>>>>
>>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>>> +    return ret;
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>>
>>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>>           host->bus_ops->detect(host);
>>>>>>>
>>>>>>> -    host->detect_change = 0;
>>>>>>> -
>>>>>>>       /*
>>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>>        * the card is no longer present.
>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>> index 031d865..09fa5e6 100644
>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>>
>>>>>>>       struct delayed_work    detect;
>>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>>
>>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>> Br
>>>>> Ulf Hansson
>>>>>
>>>>
>>>
>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 09/01/12 16:27, Ulf Hansson wrote:
>> Adrian Hunter wrote:
>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>> make
>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>> on it.
>>>>>
>>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>> how.
>>>>>> I thought this new function that you previously added
>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>
>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>>> removed
>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>> will be
>>>>>>> very
>>>>>>> bad for poor quality cards,
>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>>>>> to make sure it is still present, that is already done from the block
>>>>>> layer
>>>>>> after each read/write request. So I can not see that "poor quality cards"
>>>>>> will have any further problem with this patch, but I might miss
>>>>>> something!?
>>>>> The block driver has never caused a card to be removed before.  That is new
>>>>> and it is designed to preserve existing behaviour i.e. do not remove a card
>>>>> without a card detect event.
>>>> True, but is this a problem!?
>>> Better not to find out.
>> :-)
>>
>> Then there is lot of other things around mmc we also should not change.
> 
> Can you give an example of a change in existing functionality?  Isn't
> everything either a bug fix or new functionality?
> 
>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>>>> run will run the CMD13 to verify that the card is still there. Since it has
>>>> not completely been removed the CMD13 will succeed and the card will not be
>>>> removed.
>>>>
>>>> Moreover every other new block request will soon start to fail and always
>>>> do; until a new rescan is triggered (which is when you insert a new card or
>>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>>> the card gets removed and it's corresponding block device.
>>> There are other ways to solve that problem.  Apart from my previous
>>> suggestion, there is also the possibility to make use of ->get_cd
>>> instead of CMD13, someone already posted a patch for that
>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>> but it should probably be selected on a per driver basis (i.e. add a
>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>> though.
>>>
>> Unfortunately that wont help to solve this issue either. That patch will
>> only prevent you from executing a CMD13 if the get_cd function says the card
>> is still there. I kind of micro optimization I think, unless you very often
>> encounters errors in the block layer.
> 
> No, the rescan calls that code, so if get_cd() returns 0 the card will be
> removed irrespective of whether it has been pulled out slowly or not.

That is not correct. The rescan uses the get_cd function to find out if
it really make sense to try to initialize a new card. It is not used for
removing existing cards.

You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
detect card". This patch will prevent the bus_ops->alive function to be
called if the get_cd function indicates that the card is still there. I
can not see how this on it's own will help out to solve the issue my
patch is trying to solve.

> 
>> The key in this patch is that a rescan work is triggered to fully verify
>> that the card is still there and if not, it can remove it. I don't think
>> this is such a big matter, but of course this is my own opinion. :-)
> 
> Another issue with your patch is that the card will not be removed unless
> there is subsequent I/O to cause an I/O error and subsequent rescan.
> 

This is exactly the problem this patch is trying to solve. Instead of
"forever" keeping the card inserted and thus returning errors for every
new I/O request, we trigger a rescan to fully remove the card.

>>>>> You are assuming:
>>>>>     1. that a poor quality card will not return errors for a few
>>>>>     commands and then resume operation
>>>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>>>> and MMC which I had in my collection. I have found none of these to trigger
>>>> a undesirable removal of the card.
>>>>
>>>> Of course I have only a subset of all cards, so this can not be fully tested
>>>> for all existing cards.
>>>>
>>>>>     2. that removing a card on error is desirable
>>>> Well, we will just fire of a rescan work to check if the card has been
>>>> removed. If it is still there it will of course not be removed.
>>> Not if it has stopped responding.  Again, this is a change in behaviour.
>>> Previously, a card that stopped responding was not removed.
>>>
>>> Perhaps in the future someone will want to try to recover cards that
>>> stop responding, for example by power-cycling.  That would be in
>>> conflict with your approach because it would power cycle on every single
>>> card removal.
>> This is pure hypothetical and the simple solution to such an idea would
>> just be to do a "power-cycle attempt" before considering scheduling the
>> rescan work in the mmc_detect_card_removed function.
> 
> Nevertheless, in your case a power-cycle would be done for every card removal.

Do quite follow, what do you mean by every card removal? We detect that
the card has been removed (by using _mmc_detect_card_removed) and only
at that time a rescan (power-cycle) is triggered.

> 
>>>>> Both those assumptions may be true, but there is no evidence that they are.
>>>>>
>>>>>
>>>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>>>> the detect work is executed as soon as possible, since there is
>>>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>>>
>>>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>>>> ---
>>>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>>> index 4770807..7bc02f4 100644
>>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>>>> unsigned long delay)
>>>>>>>>       WARN_ON(host->removed);
>>>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>>>   #endif
>>>>>>>> -    host->detect_change = 1;
>>>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>>>   }
>>>>>>>>
>>>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>>>> *host)
>>>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>>>   {
>>>>>>>>       struct mmc_card *card = host->card;
>>>>>>>> +    int ret = 1;
>>>>>>>>
>>>>>>>>       WARN_ON(!host->claimed);
>>>>>>>> -    /*
>>>>>>>> -     * The card will be considered unchanged unless we have been
>>>>>>>> asked to
>>>>>>>> -     * detect a change or host requires polling to provide card
>>>>>>>> detection.
>>>>>>>> -     */
>>>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps&
>>>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>>>> -        return mmc_card_removed(card);
>>>>>>>>
>>>>>>>> -    host->detect_change = 0;
>>>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>>>> +            /*
>>>>>>>> +             * Make sure a detect work is always executed and also
>>>>>>>> +             * do it as soon as possible.
>>>>>>>> +             */
>>>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>>>> +            mmc_detect_change(host, 0);
>>>>>>>> +        }
>>>>>>>> +        ret = mmc_card_removed(card);
>>>>>>>> +    }
>>>>>>>>
>>>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>>>> +    return ret;
>>>>>>>>   }
>>>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>>>
>>>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>>>           host->bus_ops->detect(host);
>>>>>>>>
>>>>>>>> -    host->detect_change = 0;
>>>>>>>> -
>>>>>>>>       /*
>>>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>>>        * the card is no longer present.
>>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>>> index 031d865..09fa5e6 100644
>>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>>>
>>>>>>>>       struct delayed_work    detect;
>>>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>>>
>>>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>>> Br
>>>>>> Ulf Hansson
>>>>>>
>>>
>>
> 
> 

BR
Ulf Hansson

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 09/01/12 16:27, Ulf Hansson wrote:
>> Adrian Hunter wrote:
>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>> make
>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>> on it.
>>>>>
>>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>> how.
>>>>>> I thought this new function that you previously added
>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>
>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>>> removed
>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>> will be
>>>>>>> very
>>>>>>> bad for poor quality cards,
>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the card
>>>>>> to make sure it is still present, that is already done from the block
>>>>>> layer
>>>>>> after each read/write request. So I can not see that "poor quality cards"
>>>>>> will have any further problem with this patch, but I might miss
>>>>>> something!?
>>>>> The block driver has never caused a card to be removed before.  That is new
>>>>> and it is designed to preserve existing behaviour i.e. do not remove a card
>>>>> without a card detect event.
>>>> True, but is this a problem!?
>>> Better not to find out.
>> :-)
>>
>> Then there is lot of other things around mmc we also should not change.
>>
>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has triggered to
>>>> run will run the CMD13 to verify that the card is still there. Since it has
>>>> not completely been removed the CMD13 will succeed and the card will not be
>>>> removed.
>>>>
>>>> Moreover every other new block request will soon start to fail and always
>>>> do; until a new rescan is triggered (which is when you insert a new card or
>>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>>> the card gets removed and it's corresponding block device.
>>> There are other ways to solve that problem.  Apart from my previous
>>> suggestion, there is also the possibility to make use of ->get_cd
>>> instead of CMD13, someone already posted a patch for that
>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>> but it should probably be selected on a per driver basis (i.e. add a
>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>> though.
>>>
>> Unfortunately that wont help to solve this issue either. That patch will
>> only prevent you from executing a CMD13 if the get_cd function says the card
>> is still there. I kind of micro optimization I think, unless you very often
>> encounters errors in the block layer.
>>
>> The key in this patch is that a rescan work is triggered to fully verify
>> that the card is still there and if not, it can remove it. I don't think
>> this is such a big matter, but of course this is my own opinion. :-)
> 
> In that case it needs to be selected by the driver e.g.
> add MMC_CAP2_RESCAN_ON_ERROR
> 

That could be an option. Maybe better to have it default turned on (ie 
MMC_CAP2_NO_RESCAN_ON_ERROR) to see if we encounter any problems with 
crappy cards. Otherwise we will never know. What do you think?

> 
>>>>> You are assuming:
>>>>>     1. that a poor quality card will not return errors for a few
>>>>>     commands and then resume operation
>>>> I see your point. I did some tests with a bunch of old crappy cards, both SD
>>>> and MMC which I had in my collection. I have found none of these to trigger
>>>> a undesirable removal of the card.
>>>>
>>>> Of course I have only a subset of all cards, so this can not be fully tested
>>>> for all existing cards.
>>>>
>>>>>     2. that removing a card on error is desirable
>>>> Well, we will just fire of a rescan work to check if the card has been
>>>> removed. If it is still there it will of course not be removed.
>>> Not if it has stopped responding.  Again, this is a change in behaviour.
>>> Previously, a card that stopped responding was not removed.
>>>
>>> Perhaps in the future someone will want to try to recover cards that
>>> stop responding, for example by power-cycling.  That would be in
>>> conflict with your approach because it would power cycle on every single
>>> card removal.
>>>
>>>>> Both those assumptions may be true, but there is no evidence that they are.
>>>>>
>>>>>
>>>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>>>> the detect work is executed as soon as possible, since there is
>>>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>>>
>>>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>>>> ---
>>>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>>> index 4770807..7bc02f4 100644
>>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>>>> unsigned long delay)
>>>>>>>>       WARN_ON(host->removed);
>>>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>>>   #endif
>>>>>>>> -    host->detect_change = 1;
>>>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>>>   }
>>>>>>>>
>>>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>>>> *host)
>>>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>>>   {
>>>>>>>>       struct mmc_card *card = host->card;
>>>>>>>> +    int ret = 1;
>>>>>>>>
>>>>>>>>       WARN_ON(!host->claimed);
>>>>>>>> -    /*
>>>>>>>> -     * The card will be considered unchanged unless we have been
>>>>>>>> asked to
>>>>>>>> -     * detect a change or host requires polling to provide card
>>>>>>>> detection.
>>>>>>>> -     */
>>>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps&
>>>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>>>> -        return mmc_card_removed(card);
>>>>>>>>
>>>>>>>> -    host->detect_change = 0;
>>>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>>>> +            /*
>>>>>>>> +             * Make sure a detect work is always executed and also
>>>>>>>> +             * do it as soon as possible.
>>>>>>>> +             */
>>>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>>>> +            mmc_detect_change(host, 0);
>>>>>>>> +        }
>>>>>>>> +        ret = mmc_card_removed(card);
>>>>>>>> +    }
>>>>>>>>
>>>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>>>> +    return ret;
>>>>>>>>   }
>>>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>>>
>>>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>>>           host->bus_ops->detect(host);
>>>>>>>>
>>>>>>>> -    host->detect_change = 0;
>>>>>>>> -
>>>>>>>>       /*
>>>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>>>        * the card is no longer present.
>>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>>> index 031d865..09fa5e6 100644
>>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>>>
>>>>>>>>       struct delayed_work    detect;
>>>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>>>
>>>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>>> Br
>>>>>> Ulf Hansson
>>>>>>
>>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 10/01/12 12:59, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 09/01/12 16:27, Ulf Hansson wrote:
>>> Adrian Hunter wrote:
>>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>>> make
>>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>>> on it.
>>>>>>
>>>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>>> how.
>>>>>>> I thought this new function that you previously added
>>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>>
>>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>>>> removed
>>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>>> will be
>>>>>>>> very
>>>>>>>> bad for poor quality cards,
>>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the
>>>>>>> card
>>>>>>> to make sure it is still present, that is already done from the block
>>>>>>> layer
>>>>>>> after each read/write request. So I can not see that "poor quality
>>>>>>> cards"
>>>>>>> will have any further problem with this patch, but I might miss
>>>>>>> something!?
>>>>>> The block driver has never caused a card to be removed before.  That
>>>>>> is new
>>>>>> and it is designed to preserve existing behaviour i.e. do not remove a
>>>>>> card
>>>>>> without a card detect event.
>>>>> True, but is this a problem!?
>>>> Better not to find out.
>>> :-)
>>>
>>> Then there is lot of other things around mmc we also should not change.
>>
>> Can you give an example of a change in existing functionality?  Isn't
>> everything either a bug fix or new functionality?
>>
>>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has
>>>>> triggered to
>>>>> run will run the CMD13 to verify that the card is still there. Since it
>>>>> has
>>>>> not completely been removed the CMD13 will succeed and the card will
>>>>> not be
>>>>> removed.
>>>>>
>>>>> Moreover every other new block request will soon start to fail and always
>>>>> do; until a new rescan is triggered (which is when you insert a new
>>>>> card or
>>>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>>>> the card gets removed and it's corresponding block device.
>>>> There are other ways to solve that problem.  Apart from my previous
>>>> suggestion, there is also the possibility to make use of ->get_cd
>>>> instead of CMD13, someone already posted a patch for that
>>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>>> but it should probably be selected on a per driver basis (i.e. add a
>>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>>> though.
>>>>
>>> Unfortunately that wont help to solve this issue either. That patch will
>>> only prevent you from executing a CMD13 if the get_cd function says the card
>>> is still there. I kind of micro optimization I think, unless you very often
>>> encounters errors in the block layer.
>>
>> No, the rescan calls that code, so if get_cd() returns 0 the card will be
>> removed irrespective of whether it has been pulled out slowly or not.
> 
> That is not correct. The rescan uses the get_cd function to find out if
> it really make sense to try to initialize a new card. It is not used for
> removing existing cards.

mmc_rescan() first calls host->bus_ops->detect() to see if the card is still
there.  If the card does not respond then it is removed.  Then mmc_rescan
attempts to initialize a new card. host->bus_ops->detect() is not used for that.

> 
> You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
> detect card". This patch will prevent the bus_ops->alive function to be
> called if the get_cd function indicates that the card is still there. I
> can not see how this on it's own will help out to solve the issue my
> patch is trying to solve.

Yes it will because it is called by mmc_rescan() and used to remove the card
via host->bus_ops->detect()

> 
>>
>>> The key in this patch is that a rescan work is triggered to fully verify
>>> that the card is still there and if not, it can remove it. I don't think
>>> this is such a big matter, but of course this is my own opinion. :-)
>>
>> Another issue with your patch is that the card will not be removed unless
>> there is subsequent I/O to cause an I/O error and subsequent rescan.
>>
> 
> This is exactly the problem this patch is trying to solve. Instead of
> "forever" keeping the card inserted and thus returning errors for every
> new I/O request, we trigger a rescan to fully remove the card.

If the user pulls out the card slowly so that the rescan sees the card still
there, then if there is no I/O there will be no I/O error and the kernel
will not remove the card - until the user sticks in another card or tries to
access files that are not there.

> 
>>>>>> You are assuming:
>>>>>>     1. that a poor quality card will not return errors for a few
>>>>>>     commands and then resume operation
>>>>> I see your point. I did some tests with a bunch of old crappy cards,
>>>>> both SD
>>>>> and MMC which I had in my collection. I have found none of these to
>>>>> trigger
>>>>> a undesirable removal of the card.
>>>>>
>>>>> Of course I have only a subset of all cards, so this can not be fully
>>>>> tested
>>>>> for all existing cards.
>>>>>
>>>>>>     2. that removing a card on error is desirable
>>>>> Well, we will just fire of a rescan work to check if the card has been
>>>>> removed. If it is still there it will of course not be removed.
>>>> Not if it has stopped responding.  Again, this is a change in behaviour.
>>>> Previously, a card that stopped responding was not removed.
>>>>
>>>> Perhaps in the future someone will want to try to recover cards that
>>>> stop responding, for example by power-cycling.  That would be in
>>>> conflict with your approach because it would power cycle on every single
>>>> card removal.
>>> This is pure hypothetical and the simple solution to such an idea would
>>> just be to do a "power-cycle attempt" before considering scheduling the
>>> rescan work in the mmc_detect_card_removed function.
>>
>> Nevertheless, in your case a power-cycle would be done for every card
>> removal.
> 
> Do quite follow, what do you mean by every card removal? We detect that
> the card has been removed (by using _mmc_detect_card_removed) and only
> at that time a rescan (power-cycle) is triggered.



> 
>>
>>>>>> Both those assumptions may be true, but there is no evidence that they
>>>>>> are.
>>>>>>
>>>>>>
>>>>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>>>>> the detect work is executed as soon as possible, since there is
>>>>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>>>> index 4770807..7bc02f4 100644
>>>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>>>>> unsigned long delay)
>>>>>>>>>       WARN_ON(host->removed);
>>>>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>>>>   #endif
>>>>>>>>> -    host->detect_change = 1;
>>>>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>>>>   }
>>>>>>>>>
>>>>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>>>>> *host)
>>>>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>>>>   {
>>>>>>>>>       struct mmc_card *card = host->card;
>>>>>>>>> +    int ret = 1;
>>>>>>>>>
>>>>>>>>>       WARN_ON(!host->claimed);
>>>>>>>>> -    /*
>>>>>>>>> -     * The card will be considered unchanged unless we have been
>>>>>>>>> asked to
>>>>>>>>> -     * detect a change or host requires polling to provide card
>>>>>>>>> detection.
>>>>>>>>> -     */
>>>>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps&
>>>>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>>>>> -        return mmc_card_removed(card);
>>>>>>>>>
>>>>>>>>> -    host->detect_change = 0;
>>>>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>>>>> +            /*
>>>>>>>>> +             * Make sure a detect work is always executed and also
>>>>>>>>> +             * do it as soon as possible.
>>>>>>>>> +             */
>>>>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>>>>> +            mmc_detect_change(host, 0);
>>>>>>>>> +        }
>>>>>>>>> +        ret = mmc_card_removed(card);
>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>>>>> +    return ret;
>>>>>>>>>   }
>>>>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>>>>
>>>>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>>>>           host->bus_ops->detect(host);
>>>>>>>>>
>>>>>>>>> -    host->detect_change = 0;
>>>>>>>>> -
>>>>>>>>>       /*
>>>>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>>>>        * the card is no longer present.
>>>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>>>> index 031d865..09fa5e6 100644
>>>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>>>>
>>>>>>>>>       struct delayed_work    detect;
>>>>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>>>>
>>>>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>>>> Br
>>>>>>> Ulf Hansson
>>>>>>>
>>>>
>>>
>>
>>
> 
> BR
> Ulf Hansson
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 10/01/12 13:03, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 09/01/12 16:27, Ulf Hansson wrote:
>>> Adrian Hunter wrote:
>>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>>> make
>>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>>> on it.
>>>>>>
>>>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>>> how.
>>>>>>> I thought this new function that you previously added
>>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>>
>>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>>>> removed
>>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>>> will be
>>>>>>>> very
>>>>>>>> bad for poor quality cards,
>>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the
>>>>>>> card
>>>>>>> to make sure it is still present, that is already done from the block
>>>>>>> layer
>>>>>>> after each read/write request. So I can not see that "poor quality
>>>>>>> cards"
>>>>>>> will have any further problem with this patch, but I might miss
>>>>>>> something!?
>>>>>> The block driver has never caused a card to be removed before.  That
>>>>>> is new
>>>>>> and it is designed to preserve existing behaviour i.e. do not remove a
>>>>>> card
>>>>>> without a card detect event.
>>>>> True, but is this a problem!?
>>>> Better not to find out.
>>> :-)
>>>
>>> Then there is lot of other things around mmc we also should not change.
>>>
>>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has
>>>>> triggered to
>>>>> run will run the CMD13 to verify that the card is still there. Since it
>>>>> has
>>>>> not completely been removed the CMD13 will succeed and the card will
>>>>> not be
>>>>> removed.
>>>>>
>>>>> Moreover every other new block request will soon start to fail and always
>>>>> do; until a new rescan is triggered (which is when you insert a new
>>>>> card or
>>>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>>>> the card gets removed and it's corresponding block device.
>>>> There are other ways to solve that problem.  Apart from my previous
>>>> suggestion, there is also the possibility to make use of ->get_cd
>>>> instead of CMD13, someone already posted a patch for that
>>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>>> but it should probably be selected on a per driver basis (i.e. add a
>>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>>> though.
>>>>
>>> Unfortunately that wont help to solve this issue either. That patch will
>>> only prevent you from executing a CMD13 if the get_cd function says the card
>>> is still there. I kind of micro optimization I think, unless you very often
>>> encounters errors in the block layer.
>>>
>>> The key in this patch is that a rescan work is triggered to fully verify
>>> that the card is still there and if not, it can remove it. I don't think
>>> this is such a big matter, but of course this is my own opinion. :-)
>>
>> In that case it needs to be selected by the driver e.g.
>> add MMC_CAP2_RESCAN_ON_ERROR
>>
> 
> That could be an option. Maybe better to have it default turned on (ie
> MMC_CAP2_NO_RESCAN_ON_ERROR) to see if we encounter any problems with crappy
> cards. Otherwise we will never know. What do you think?

MMC_CAP2_RESCAN_ON_ERROR is better to avoid problems for all the drivers
that don't need the change e.g. sdhci

> 
>>
>>>>>> You are assuming:
>>>>>>     1. that a poor quality card will not return errors for a few
>>>>>>     commands and then resume operation
>>>>> I see your point. I did some tests with a bunch of old crappy cards,
>>>>> both SD
>>>>> and MMC which I had in my collection. I have found none of these to
>>>>> trigger
>>>>> a undesirable removal of the card.
>>>>>
>>>>> Of course I have only a subset of all cards, so this can not be fully
>>>>> tested
>>>>> for all existing cards.
>>>>>
>>>>>>     2. that removing a card on error is desirable
>>>>> Well, we will just fire of a rescan work to check if the card has been
>>>>> removed. If it is still there it will of course not be removed.
>>>> Not if it has stopped responding.  Again, this is a change in behaviour.
>>>> Previously, a card that stopped responding was not removed.
>>>>
>>>> Perhaps in the future someone will want to try to recover cards that
>>>> stop responding, for example by power-cycling.  That would be in
>>>> conflict with your approach because it would power cycle on every single
>>>> card removal.
>>>>
>>>>>> Both those assumptions may be true, but there is no evidence that they
>>>>>> are.
>>>>>>
>>>>>>
>>>>>>>>> This will solve the described issue above. Moreover we make sure
>>>>>>>>> the detect work is executed as soon as possible, since there is
>>>>>>>>> no reason for waiting for a "delayed" detect to happen.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ulf Hansson<ulf.hansson@stericsson.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/mmc/core/core.c  |   24 +++++++++++++-----------
>>>>>>>>>   include/linux/mmc/host.h |    1 -
>>>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>>>>>>> index 4770807..7bc02f4 100644
>>>>>>>>> --- a/drivers/mmc/core/core.c
>>>>>>>>> +++ b/drivers/mmc/core/core.c
>>>>>>>>> @@ -1462,7 +1462,6 @@ void mmc_detect_change(struct mmc_host *host,
>>>>>>>>> unsigned long delay)
>>>>>>>>>       WARN_ON(host->removed);
>>>>>>>>>       spin_unlock_irqrestore(&host->lock, flags);
>>>>>>>>>   #endif
>>>>>>>>> -    host->detect_change = 1;
>>>>>>>>>       mmc_schedule_delayed_work(&host->detect, delay);
>>>>>>>>>   }
>>>>>>>>>
>>>>>>>>> @@ -2077,18 +2076,23 @@ int _mmc_detect_card_removed(struct mmc_host
>>>>>>>>> *host)
>>>>>>>>>   int mmc_detect_card_removed(struct mmc_host *host)
>>>>>>>>>   {
>>>>>>>>>       struct mmc_card *card = host->card;
>>>>>>>>> +    int ret = 1;
>>>>>>>>>
>>>>>>>>>       WARN_ON(!host->claimed);
>>>>>>>>> -    /*
>>>>>>>>> -     * The card will be considered unchanged unless we have been
>>>>>>>>> asked to
>>>>>>>>> -     * detect a change or host requires polling to provide card
>>>>>>>>> detection.
>>>>>>>>> -     */
>>>>>>>>> -    if (card&&  !host->detect_change&&  !(host->caps&
>>>>>>>>> MMC_CAP_NEEDS_POLL))
>>>>>>>>> -        return mmc_card_removed(card);
>>>>>>>>>
>>>>>>>>> -    host->detect_change = 0;
>>>>>>>>> +    if (card&&  !mmc_card_removed(card)) {
>>>>>>>>> +        if (_mmc_detect_card_removed(host)) {
>>>>>>>>> +            /*
>>>>>>>>> +             * Make sure a detect work is always executed and also
>>>>>>>>> +             * do it as soon as possible.
>>>>>>>>> +             */
>>>>>>>>> +            cancel_delayed_work(&host->detect);
>>>>>>>>> +            mmc_detect_change(host, 0);
>>>>>>>>> +        }
>>>>>>>>> +        ret = mmc_card_removed(card);
>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> -    return _mmc_detect_card_removed(host);
>>>>>>>>> +    return ret;
>>>>>>>>>   }
>>>>>>>>>   EXPORT_SYMBOL(mmc_detect_card_removed);
>>>>>>>>>
>>>>>>>>> @@ -2112,8 +2116,6 @@ void mmc_rescan(struct work_struct *work)
>>>>>>>>>       &&  !(host->caps&  MMC_CAP_NONREMOVABLE))
>>>>>>>>>           host->bus_ops->detect(host);
>>>>>>>>>
>>>>>>>>> -    host->detect_change = 0;
>>>>>>>>> -
>>>>>>>>>       /*
>>>>>>>>>        * Let mmc_bus_put() free the bus/bus_ops if we've found that
>>>>>>>>>        * the card is no longer present.
>>>>>>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>>>>>>> index 031d865..09fa5e6 100644
>>>>>>>>> --- a/include/linux/mmc/host.h
>>>>>>>>> +++ b/include/linux/mmc/host.h
>>>>>>>>> @@ -305,7 +305,6 @@ struct mmc_host {
>>>>>>>>>       int            claim_cnt;    /* "claim" nesting count */
>>>>>>>>>
>>>>>>>>>       struct delayed_work    detect;
>>>>>>>>> -    int            detect_change;    /* card detect flag */
>>>>>>>>>       struct mmc_hotplug    hotplug;
>>>>>>>>>
>>>>>>>>>       const struct mmc_bus_ops *bus_ops;    /* current bus driver */
>>>>>>> Br
>>>>>>> Ulf Hansson
>>>>>>>
>>>>
>>>
>>
>>
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 10/01/12 12:59, Ulf Hansson wrote:
>> Adrian Hunter wrote:
>>> On 09/01/12 16:27, Ulf Hansson wrote:
>>>> Adrian Hunter wrote:
>>>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>>>> My concern is more about what we actually can trust; either the GPIO irq
>>>>>>>> which likely is giving more than one irq when inserting/removing a card
>>>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>>>> make
>>>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>>>> on it.
>>>>>>>
>>>>>>>> Moreover, the issue this patch tries to solve can not be solved without
>>>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>>>> how.
>>>>>>>> I thought this new function that you previously added
>>>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>>>
>>>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>>>> This is changing some long-standing functionality i.e. the card is not
>>>>>>>>> removed
>>>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>>>> will be
>>>>>>>>> very
>>>>>>>>> bad for poor quality cards,
>>>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the
>>>>>>>> card
>>>>>>>> to make sure it is still present, that is already done from the block
>>>>>>>> layer
>>>>>>>> after each read/write request. So I can not see that "poor quality
>>>>>>>> cards"
>>>>>>>> will have any further problem with this patch, but I might miss
>>>>>>>> something!?
>>>>>>> The block driver has never caused a card to be removed before.  That
>>>>>>> is new
>>>>>>> and it is designed to preserve existing behaviour i.e. do not remove a
>>>>>>> card
>>>>>>> without a card detect event.
>>>>>> True, but is this a problem!?
>>>>> Better not to find out.
>>>> :-)
>>>>
>>>> Then there is lot of other things around mmc we also should not change.
>>> Can you give an example of a change in existing functionality?  Isn't
>>> everything either a bug fix or new functionality?
>>>
>>>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has
>>>>>> triggered to
>>>>>> run will run the CMD13 to verify that the card is still there. Since it
>>>>>> has
>>>>>> not completely been removed the CMD13 will succeed and the card will
>>>>>> not be
>>>>>> removed.
>>>>>>
>>>>>> Moreover every other new block request will soon start to fail and always
>>>>>> do; until a new rescan is triggered (which is when you insert a new
>>>>>> card or
>>>>>> do a suspend-resume cycle). In practice I think it is more preferred that
>>>>>> the card gets removed and it's corresponding block device.
>>>>> There are other ways to solve that problem.  Apart from my previous
>>>>> suggestion, there is also the possibility to make use of ->get_cd
>>>>> instead of CMD13, someone already posted a patch for that
>>>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>>>> but it should probably be selected on a per driver basis (i.e. add a
>>>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>>>> though.
>>>>>
>>>> Unfortunately that wont help to solve this issue either. That patch will
>>>> only prevent you from executing a CMD13 if the get_cd function says the card
>>>> is still there. I kind of micro optimization I think, unless you very often
>>>> encounters errors in the block layer.
>>> No, the rescan calls that code, so if get_cd() returns 0 the card will be
>>> removed irrespective of whether it has been pulled out slowly or not.
>> That is not correct. The rescan uses the get_cd function to find out if
>> it really make sense to try to initialize a new card. It is not used for
>> removing existing cards.
> 
> mmc_rescan() first calls host->bus_ops->detect() to see if the card is still
> there.  If the card does not respond then it is removed.  Then mmc_rescan
> attempts to initialize a new card. host->bus_ops->detect() is not used for that.
> 
>> You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
>> detect card". This patch will prevent the bus_ops->alive function to be
>> called if the get_cd function indicates that the card is still there. I
>> can not see how this on it's own will help out to solve the issue my
>> patch is trying to solve.
> 
> Yes it will because it is called by mmc_rescan() and used to remove the card
> via host->bus_ops->detect()
> 

In principles this means the following sequence:

We will rely on that the get_cd function will return 0 (indicating card 
is removed) when the card is "slowly" removed at the point when the 
rescan function is calling it through the bus_ops->detect --> 
_mmc_detect_card_removed function.

This then becomes a race, meaning that the rescan function must be 
executing at the same time the get_cd function will returns 0. Otherwise 
the rescan function will not remove the card.

Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback function 
to detect card" will likely improve behavior but is not the safe 
solution to handle "slowly" removed cards.

Again, to be sure, we must let the mmc_detect_card_remove function 
trigger a rescan when _mmc_detect_card_removed has detected that the 
card is removed. This should be safe in all circumstances.


>>>> The key in this patch is that a rescan work is triggered to fully verify
>>>> that the card is still there and if not, it can remove it. I don't think
>>>> this is such a big matter, but of course this is my own opinion. :-)
>>> Another issue with your patch is that the card will not be removed unless
>>> there is subsequent I/O to cause an I/O error and subsequent rescan.
>>>
>> This is exactly the problem this patch is trying to solve. Instead of
>> "forever" keeping the card inserted and thus returning errors for every
>> new I/O request, we trigger a rescan to fully remove the card.
> 
> If the user pulls out the card slowly so that the rescan sees the card still
> there, then if there is no I/O there will be no I/O error and the kernel
> will not remove the card - until the user sticks in another card or tries to
> access files that are not there.
> 

Br
Ulf Hansson

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 13/01/12 12:04, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 10/01/12 12:59, Ulf Hansson wrote:
>>> Adrian Hunter wrote:
>>>> On 09/01/12 16:27, Ulf Hansson wrote:
>>>>> Adrian Hunter wrote:
>>>>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>>>>> My concern is more about what we actually can trust; either the
>>>>>>>>> GPIO irq
>>>>>>>>> which likely is giving more than one irq when inserting/removing a
>>>>>>>>> card
>>>>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>>>>> make
>>>>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>>>>> on it.
>>>>>>>>
>>>>>>>>> Moreover, the issue this patch tries to solve can not be solved
>>>>>>>>> without
>>>>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>>>>> how.
>>>>>>>>> I thought this new function that you previously added
>>>>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>>>>
>>>>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>>>>> This is changing some long-standing functionality i.e. the card is
>>>>>>>>>> not
>>>>>>>>>> removed
>>>>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>>>>> will be
>>>>>>>>>> very
>>>>>>>>>> bad for poor quality cards,
>>>>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the
>>>>>>>>> card
>>>>>>>>> to make sure it is still present, that is already done from the block
>>>>>>>>> layer
>>>>>>>>> after each read/write request. So I can not see that "poor quality
>>>>>>>>> cards"
>>>>>>>>> will have any further problem with this patch, but I might miss
>>>>>>>>> something!?
>>>>>>>> The block driver has never caused a card to be removed before.  That
>>>>>>>> is new
>>>>>>>> and it is designed to preserve existing behaviour i.e. do not remove a
>>>>>>>> card
>>>>>>>> without a card detect event.
>>>>>>> True, but is this a problem!?
>>>>>> Better not to find out.
>>>>> :-)
>>>>>
>>>>> Then there is lot of other things around mmc we also should not change.
>>>> Can you give an example of a change in existing functionality?  Isn't
>>>> everything either a bug fix or new functionality?
>>>>
>>>>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has
>>>>>>> triggered to
>>>>>>> run will run the CMD13 to verify that the card is still there. Since it
>>>>>>> has
>>>>>>> not completely been removed the CMD13 will succeed and the card will
>>>>>>> not be
>>>>>>> removed.
>>>>>>>
>>>>>>> Moreover every other new block request will soon start to fail and
>>>>>>> always
>>>>>>> do; until a new rescan is triggered (which is when you insert a new
>>>>>>> card or
>>>>>>> do a suspend-resume cycle). In practice I think it is more preferred
>>>>>>> that
>>>>>>> the card gets removed and it's corresponding block device.
>>>>>> There are other ways to solve that problem.  Apart from my previous
>>>>>> suggestion, there is also the possibility to make use of ->get_cd
>>>>>> instead of CMD13, someone already posted a patch for that
>>>>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>>>>> but it should probably be selected on a per driver basis (i.e. add a
>>>>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>>>>> though.
>>>>>>
>>>>> Unfortunately that wont help to solve this issue either. That patch will
>>>>> only prevent you from executing a CMD13 if the get_cd function says the
>>>>> card
>>>>> is still there. I kind of micro optimization I think, unless you very
>>>>> often
>>>>> encounters errors in the block layer.
>>>> No, the rescan calls that code, so if get_cd() returns 0 the card will be
>>>> removed irrespective of whether it has been pulled out slowly or not.
>>> That is not correct. The rescan uses the get_cd function to find out if
>>> it really make sense to try to initialize a new card. It is not used for
>>> removing existing cards.
>>
>> mmc_rescan() first calls host->bus_ops->detect() to see if the card is still
>> there.  If the card does not respond then it is removed.  Then mmc_rescan
>> attempts to initialize a new card. host->bus_ops->detect() is not used for
>> that.
>>
>>> You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
>>> detect card". This patch will prevent the bus_ops->alive function to be
>>> called if the get_cd function indicates that the card is still there. I
>>> can not see how this on it's own will help out to solve the issue my
>>> patch is trying to solve.
>>
>> Yes it will because it is called by mmc_rescan() and used to remove the card
>> via host->bus_ops->detect()
>>
> 
> In principles this means the following sequence:
> 
> We will rely on that the get_cd function will return 0 (indicating card is
> removed) when the card is "slowly" removed at the point when the rescan
> function is calling it through the bus_ops->detect -->
> _mmc_detect_card_removed function.
> 
> This then becomes a race, meaning that the rescan function must be executing
> at the same time the get_cd function will returns 0. Otherwise the rescan
> function will not remove the card.
> 
> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback function to
> detect card" will likely improve behavior but is not the safe solution to
> handle "slowly" removed cards.
> 
> Again, to be sure, we must let the mmc_detect_card_remove function trigger a
> rescan when _mmc_detect_card_removed has detected that the card is removed.
> This should be safe in all circumstances.

sdhci has no problem because it does this:

	- the host controller debounces the card detect line
	- the host controller records whether or not the card is present
	- the sdhci driver prevents (errors out) requests when the card is
	not present

So it should work if you:

	- debounce the gpio line
	- record whether or not the card is present based on the debounced
	gpio line
	- either error out requests when the card is not present
	or
	- use the get_cd patch (still ought to be driver selected)
	and implement get_cd based on whether you have recorded the card
	present or not


> 
> 
>>>>> The key in this patch is that a rescan work is triggered to fully verify
>>>>> that the card is still there and if not, it can remove it. I don't think
>>>>> this is such a big matter, but of course this is my own opinion. :-)
>>>> Another issue with your patch is that the card will not be removed unless
>>>> there is subsequent I/O to cause an I/O error and subsequent rescan.
>>>>
>>> This is exactly the problem this patch is trying to solve. Instead of
>>> "forever" keeping the card inserted and thus returning errors for every
>>> new I/O request, we trigger a rescan to fully remove the card.
>>
>> If the user pulls out the card slowly so that the rescan sees the card still
>> there, then if there is no I/O there will be no I/O error and the kernel
>> will not remove the card - until the user sticks in another card or tries to
>> access files that are not there.
>>
> 
> Br
> Ulf Hansson
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 13/01/12 12:04, Ulf Hansson wrote:
>> Adrian Hunter wrote:
>>> On 10/01/12 12:59, Ulf Hansson wrote:
>>>> Adrian Hunter wrote:
>>>>> On 09/01/12 16:27, Ulf Hansson wrote:
>>>>>> Adrian Hunter wrote:
>>>>>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>>>>>> My concern is more about what we actually can trust; either the
>>>>>>>>>> GPIO irq
>>>>>>>>>> which likely is giving more than one irq when inserting/removing a
>>>>>>>>>> card
>>>>>>>>>> since the slot is probably not glitch free, or that a "rescan" runs to
>>>>>>>>>> make
>>>>>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to rely
>>>>>>>>> on it.
>>>>>>>>>
>>>>>>>>>> Moreover, the issue this patch tries to solve can not be solved
>>>>>>>>>> without
>>>>>>>>>> doing a "rescan" which must be triggered from the the block layer some
>>>>>>>>>> how.
>>>>>>>>>> I thought this new function that you previously added
>>>>>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>>>>>
>>>>>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>>>>>> This is changing some long-standing functionality i.e. the card is
>>>>>>>>>>> not
>>>>>>>>>>> removed
>>>>>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>>>>>> will be
>>>>>>>>>>> very
>>>>>>>>>>> bad for poor quality cards,
>>>>>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to the
>>>>>>>>>> card
>>>>>>>>>> to make sure it is still present, that is already done from the block
>>>>>>>>>> layer
>>>>>>>>>> after each read/write request. So I can not see that "poor quality
>>>>>>>>>> cards"
>>>>>>>>>> will have any further problem with this patch, but I might miss
>>>>>>>>>> something!?
>>>>>>>>> The block driver has never caused a card to be removed before.  That
>>>>>>>>> is new
>>>>>>>>> and it is designed to preserve existing behaviour i.e. do not remove a
>>>>>>>>> card
>>>>>>>>> without a card detect event.
>>>>>>>> True, but is this a problem!?
>>>>>>> Better not to find out.
>>>>>> :-)
>>>>>>
>>>>>> Then there is lot of other things around mmc we also should not change.
>>>>> Can you give an example of a change in existing functionality?  Isn't
>>>>> everything either a bug fix or new functionality?
>>>>>
>>>>>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>>>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has
>>>>>>>> triggered to
>>>>>>>> run will run the CMD13 to verify that the card is still there. Since it
>>>>>>>> has
>>>>>>>> not completely been removed the CMD13 will succeed and the card will
>>>>>>>> not be
>>>>>>>> removed.
>>>>>>>>
>>>>>>>> Moreover every other new block request will soon start to fail and
>>>>>>>> always
>>>>>>>> do; until a new rescan is triggered (which is when you insert a new
>>>>>>>> card or
>>>>>>>> do a suspend-resume cycle). In practice I think it is more preferred
>>>>>>>> that
>>>>>>>> the card gets removed and it's corresponding block device.
>>>>>>> There are other ways to solve that problem.  Apart from my previous
>>>>>>> suggestion, there is also the possibility to make use of ->get_cd
>>>>>>> instead of CMD13, someone already posted a patch for that
>>>>>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>>>>>> but it should probably be selected on a per driver basis (i.e. add a
>>>>>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>>>>>> though.
>>>>>>>
>>>>>> Unfortunately that wont help to solve this issue either. That patch will
>>>>>> only prevent you from executing a CMD13 if the get_cd function says the
>>>>>> card
>>>>>> is still there. I kind of micro optimization I think, unless you very
>>>>>> often
>>>>>> encounters errors in the block layer.
>>>>> No, the rescan calls that code, so if get_cd() returns 0 the card will be
>>>>> removed irrespective of whether it has been pulled out slowly or not.
>>>> That is not correct. The rescan uses the get_cd function to find out if
>>>> it really make sense to try to initialize a new card. It is not used for
>>>> removing existing cards.
>>> mmc_rescan() first calls host->bus_ops->detect() to see if the card is still
>>> there.  If the card does not respond then it is removed.  Then mmc_rescan
>>> attempts to initialize a new card. host->bus_ops->detect() is not used for
>>> that.
>>>
>>>> You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
>>>> detect card". This patch will prevent the bus_ops->alive function to be
>>>> called if the get_cd function indicates that the card is still there. I
>>>> can not see how this on it's own will help out to solve the issue my
>>>> patch is trying to solve.
>>> Yes it will because it is called by mmc_rescan() and used to remove the card
>>> via host->bus_ops->detect()
>>>
>> In principles this means the following sequence:
>>
>> We will rely on that the get_cd function will return 0 (indicating card is
>> removed) when the card is "slowly" removed at the point when the rescan
>> function is calling it through the bus_ops->detect -->
>> _mmc_detect_card_removed function.
>>
>> This then becomes a race, meaning that the rescan function must be executing
>> at the same time the get_cd function will returns 0. Otherwise the rescan
>> function will not remove the card.
>>
>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback function to
>> detect card" will likely improve behavior but is not the safe solution to
>> handle "slowly" removed cards.
>>
>> Again, to be sure, we must let the mmc_detect_card_remove function trigger a
>> rescan when _mmc_detect_card_removed has detected that the card is removed.
>> This should be safe in all circumstances.
> 
> sdhci has no problem because it does this:
> 
> 	- the host controller debounces the card detect line
> 	- the host controller records whether or not the card is present
> 	- the sdhci driver prevents (errors out) requests when the card is
> 	not present

Debouncing will just be a way of triggering the problem more seldom. Or 
in worst case, state the card has been removed even if it has not.

Just because you get a GPIO irq on the detect line does not mean the 
card is removed, debouncing or not. I consider this as pure mechanical 
switch which likely has glitches and I don't see that we should trust it 
fully. We only want to trigger a detect work, which is exactly what is 
done in the patch from Guennadi Liakhovetski "mmc: add a generic GPIO 
card-detect helper".

If each host driver that supports GPIO card detect makes use of the 
card-detect helper and if we accept a version of this patch, I think the 
situation should be safe in all cases. Moreover GPIO debouncing will 
never be needed for GPIO card detect for your sdhci driver either.

> 
> So it should work if you:
> 
> 	- debounce the gpio line
> 	- record whether or not the card is present based on the debounced
> 	gpio line
> 	- either error out requests when the card is not present
> 	or
> 	- use the get_cd patch (still ought to be driver selected)
> 	and implement get_cd based on whether you have recorded the card
> 	present or not
> 
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 13/01/12 13:31, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 13/01/12 12:04, Ulf Hansson wrote:
>>> Adrian Hunter wrote:
>>>> On 10/01/12 12:59, Ulf Hansson wrote:
>>>>> Adrian Hunter wrote:
>>>>>> On 09/01/12 16:27, Ulf Hansson wrote:
>>>>>>> Adrian Hunter wrote:
>>>>>>>> On 09/01/12 15:14, Ulf Hansson wrote:
>>>>>>>>>>> My concern is more about what we actually can trust; either the
>>>>>>>>>>> GPIO irq
>>>>>>>>>>> which likely is giving more than one irq when inserting/removing a
>>>>>>>>>>> card
>>>>>>>>>>> since the slot is probably not glitch free, or that a "rescan"
>>>>>>>>>>> runs to
>>>>>>>>>>> make
>>>>>>>>>>> sure a CMD13 is accepted from the previously inserted card.
>>>>>>>>>> Yes, I guess you would need to debounce the GPIO if you wanted to
>>>>>>>>>> rely
>>>>>>>>>> on it.
>>>>>>>>>>
>>>>>>>>>>> Moreover, the issue this patch tries to solve can not be solved
>>>>>>>>>>> without
>>>>>>>>>>> doing a "rescan" which must be triggered from the the block layer
>>>>>>>>>>> some
>>>>>>>>>>> how.
>>>>>>>>>>> I thought this new function that you previously added
>>>>>>>>>>> "mmc_detect_card_remove" was the proper place to do this.
>>>>>>>>>>>
>>>>>>>>>>>>> Let the mmc_detect_card_removed function trigger a new detect
>>>>>>>>>>>>> work immediately when it discovers that a card has been removed.
>>>>>>>>>>>> This is changing some long-standing functionality i.e. the card is
>>>>>>>>>>>> not
>>>>>>>>>>>> removed
>>>>>>>>>>>> without a card detect event.  It is difficult to know whether that
>>>>>>>>>>>> will be
>>>>>>>>>>>> very
>>>>>>>>>>>> bad for poor quality cards,
>>>>>>>>>>> Doing a mmc_detect (rescan) will in the end just issue a CMD13 to
>>>>>>>>>>> the
>>>>>>>>>>> card
>>>>>>>>>>> to make sure it is still present, that is already done from the
>>>>>>>>>>> block
>>>>>>>>>>> layer
>>>>>>>>>>> after each read/write request. So I can not see that "poor quality
>>>>>>>>>>> cards"
>>>>>>>>>>> will have any further problem with this patch, but I might miss
>>>>>>>>>>> something!?
>>>>>>>>>> The block driver has never caused a card to be removed before.  That
>>>>>>>>>> is new
>>>>>>>>>> and it is designed to preserve existing behaviour i.e. do not
>>>>>>>>>> remove a
>>>>>>>>>> card
>>>>>>>>>> without a card detect event.
>>>>>>>>> True, but is this a problem!?
>>>>>>>> Better not to find out.
>>>>>>> :-)
>>>>>>>
>>>>>>> Then there is lot of other things around mmc we also should not change.
>>>>>> Can you give an example of a change in existing functionality?  Isn't
>>>>>> everything either a bug fix or new functionality?
>>>>>>
>>>>>>>>> Anyway, this is the actual issue this patch is trying to solve. If you
>>>>>>>>> remove a card "slowly", a "rescan" work, which the GPIO irq has
>>>>>>>>> triggered to
>>>>>>>>> run will run the CMD13 to verify that the card is still there.
>>>>>>>>> Since it
>>>>>>>>> has
>>>>>>>>> not completely been removed the CMD13 will succeed and the card will
>>>>>>>>> not be
>>>>>>>>> removed.
>>>>>>>>>
>>>>>>>>> Moreover every other new block request will soon start to fail and
>>>>>>>>> always
>>>>>>>>> do; until a new rescan is triggered (which is when you insert a new
>>>>>>>>> card or
>>>>>>>>> do a suspend-resume cycle). In practice I think it is more preferred
>>>>>>>>> that
>>>>>>>>> the card gets removed and it's corresponding block device.
>>>>>>>> There are other ways to solve that problem.  Apart from my previous
>>>>>>>> suggestion, there is also the possibility to make use of ->get_cd
>>>>>>>> instead of CMD13, someone already posted a patch for that
>>>>>>>> "[PATCH 2/4 v4] MMC/SD: Add callback function to detect card"
>>>>>>>> but it should probably be selected on a per driver basis (i.e. add a
>>>>>>>> MMC_CAP2 for it).  I guess you would still need to debounce the GPIO
>>>>>>>> though.
>>>>>>>>
>>>>>>> Unfortunately that wont help to solve this issue either. That patch will
>>>>>>> only prevent you from executing a CMD13 if the get_cd function says the
>>>>>>> card
>>>>>>> is still there. I kind of micro optimization I think, unless you very
>>>>>>> often
>>>>>>> encounters errors in the block layer.
>>>>>> No, the rescan calls that code, so if get_cd() returns 0 the card will be
>>>>>> removed irrespective of whether it has been pulled out slowly or not.
>>>>> That is not correct. The rescan uses the get_cd function to find out if
>>>>> it really make sense to try to initialize a new card. It is not used for
>>>>> removing existing cards.
>>>> mmc_rescan() first calls host->bus_ops->detect() to see if the card is
>>>> still
>>>> there.  If the card does not respond then it is removed.  Then mmc_rescan
>>>> attempts to initialize a new card. host->bus_ops->detect() is not used for
>>>> that.
>>>>
>>>>> You were referring to "[PATCH 2/4 v4] MMC/SD: Add callback function to
>>>>> detect card". This patch will prevent the bus_ops->alive function to be
>>>>> called if the get_cd function indicates that the card is still there. I
>>>>> can not see how this on it's own will help out to solve the issue my
>>>>> patch is trying to solve.
>>>> Yes it will because it is called by mmc_rescan() and used to remove the
>>>> card
>>>> via host->bus_ops->detect()
>>>>
>>> In principles this means the following sequence:
>>>
>>> We will rely on that the get_cd function will return 0 (indicating card is
>>> removed) when the card is "slowly" removed at the point when the rescan
>>> function is calling it through the bus_ops->detect -->
>>> _mmc_detect_card_removed function.
>>>
>>> This then becomes a race, meaning that the rescan function must be executing
>>> at the same time the get_cd function will returns 0. Otherwise the rescan
>>> function will not remove the card.
>>>
>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback function to
>>> detect card" will likely improve behavior but is not the safe solution to
>>> handle "slowly" removed cards.
>>>
>>> Again, to be sure, we must let the mmc_detect_card_remove function trigger a
>>> rescan when _mmc_detect_card_removed has detected that the card is removed.
>>> This should be safe in all circumstances.
>>
>> sdhci has no problem because it does this:
>>
>>     - the host controller debounces the card detect line
>>     - the host controller records whether or not the card is present
>>     - the sdhci driver prevents (errors out) requests when the card is
>>     not present
> 
> Debouncing will just be a way of triggering the problem more seldom. Or in
> worst case, state the card has been removed even if it has not.

If a delay is used with mmc_detect_change, debouncing is not necessary.

> 
> Just because you get a GPIO irq on the detect line does not mean the card is
> removed, debouncing or not. I consider this as pure mechanical switch which
> likely has glitches and I don't see that we should trust it fully. We only
> want to trigger a detect work, which is exactly what is done in the patch
> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".

The original problem was "slow card removal".  "Unreliable card detect"
is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have discussed.

> 
> If each host driver that supports GPIO card detect makes use of the
> card-detect helper and if we accept a version of this patch, I think the
> situation should be safe in all cases. Moreover GPIO debouncing will never
> be needed for GPIO card detect for your sdhci driver either.

Safe in all cases, except at least the 3 already given:
	- card is buggy and sometimes fails several commands in a row
	- upper layers want to attempt to recover an unresponsive card
	- even in the case of slow removal, the vendor wants the card
	to show as removed immediately whether or not there is any I/O

> 
>>
>> So it should work if you:
>>
>>     - debounce the gpio line
>>     - record whether or not the card is present based on the debounced
>>     gpio line
>>     - either error out requests when the card is not present
>>     or
>>     - use the get_cd patch (still ought to be driver selected)
>>     and implement get_cd based on whether you have recorded the card
>>     present or not

In fact the get_cd approach is flawed.  If a new card has been inserted
then get_cd will say the old card is present whereas CMD13 would fail for a
new card because it has not been initialized.

>>
>>
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

>>>> In principles this means the following sequence:
>>>>
>>>> We will rely on that the get_cd function will return 0 (indicating card is
>>>> removed) when the card is "slowly" removed at the point when the rescan
>>>> function is calling it through the bus_ops->detect -->
>>>> _mmc_detect_card_removed function.
>>>>
>>>> This then becomes a race, meaning that the rescan function must be executing
>>>> at the same time the get_cd function will returns 0. Otherwise the rescan
>>>> function will not remove the card.
>>>>
>>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback function to
>>>> detect card" will likely improve behavior but is not the safe solution to
>>>> handle "slowly" removed cards.
>>>>
>>>> Again, to be sure, we must let the mmc_detect_card_remove function trigger a
>>>> rescan when _mmc_detect_card_removed has detected that the card is removed.
>>>> This should be safe in all circumstances.
>>> sdhci has no problem because it does this:
>>>
>>>     - the host controller debounces the card detect line
>>>     - the host controller records whether or not the card is present
>>>     - the sdhci driver prevents (errors out) requests when the card is
>>>     not present
>> Debouncing will just be a way of triggering the problem more seldom. Or in
>> worst case, state the card has been removed even if it has not.
> 
> If a delay is used with mmc_detect_change, debouncing is not necessary.
> 
>> Just because you get a GPIO irq on the detect line does not mean the card is
>> removed, debouncing or not. I consider this as pure mechanical switch which
>> likely has glitches and I don't see that we should trust it fully. We only
>> want to trigger a detect work, which is exactly what is done in the patch
>> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".
> 
> The original problem was "slow card removal".  "Unreliable card detect"
> is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
> for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have discussed.

I do not understand why you mention "Unreliable card detect"? That has 
nothing to do with this patch.

So to conclude the discussion, do you believe that this patch is 
acceptable as long as we add a CAPS2 option "MMC_CAP2_RESCAN_ON_ERROR", 
which if not set will prevent the detect work from being scheduled from 
mmc_detect_card_removed?

> 
>> If each host driver that supports GPIO card detect makes use of the
>> card-detect helper and if we accept a version of this patch, I think the
>> situation should be safe in all cases. Moreover GPIO debouncing will never
>> be needed for GPIO card detect for your sdhci driver either.
> 
> Safe in all cases, except at least the 3 already given:
> 	- card is buggy and sometimes fails several commands in a row
I doubt this will become a real problem. If a card fails several times 
in a row, upper FS layers wont be happy either. So likely we are screwed 
anyway. Don't you think?
> 	- upper layers want to attempt to recover an unresponsive card
This is not implemented as of right now. I see no problem that my patch 
will prevent this from being implemented in the future.
> 	- even in the case of slow removal, the vendor wants the card
> 	to show as removed immediately whether or not there is any I/O
That is correct and can not be fully solved unless you use 
MMC_CAP_NEEDS_POLL. I doubt that one would like to use polling in favor 
of card detect only to take care of this issue though. So I believe it 
is more likely you want to trigger a card removal when receiving I/O. 
Kind of "Better late than never". :-)


By the way, thanks for keeping up the frequency in this quite long 
discussion.

BR
Ulf Hansson

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 13/01/12 15:14, Ulf Hansson wrote:
>>>>> In principles this means the following sequence:
>>>>>
>>>>> We will rely on that the get_cd function will return 0 (indicating card is
>>>>> removed) when the card is "slowly" removed at the point when the rescan
>>>>> function is calling it through the bus_ops->detect -->
>>>>> _mmc_detect_card_removed function.
>>>>>
>>>>> This then becomes a race, meaning that the rescan function must be
>>>>> executing
>>>>> at the same time the get_cd function will returns 0. Otherwise the rescan
>>>>> function will not remove the card.
>>>>>
>>>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback
>>>>> function to
>>>>> detect card" will likely improve behavior but is not the safe solution to
>>>>> handle "slowly" removed cards.
>>>>>
>>>>> Again, to be sure, we must let the mmc_detect_card_remove function
>>>>> trigger a
>>>>> rescan when _mmc_detect_card_removed has detected that the card is
>>>>> removed.
>>>>> This should be safe in all circumstances.
>>>> sdhci has no problem because it does this:
>>>>
>>>>     - the host controller debounces the card detect line
>>>>     - the host controller records whether or not the card is present
>>>>     - the sdhci driver prevents (errors out) requests when the card is
>>>>     not present
>>> Debouncing will just be a way of triggering the problem more seldom. Or in
>>> worst case, state the card has been removed even if it has not.
>>
>> If a delay is used with mmc_detect_change, debouncing is not necessary.
>>
>>> Just because you get a GPIO irq on the detect line does not mean the card is
>>> removed, debouncing or not. I consider this as pure mechanical switch which
>>> likely has glitches and I don't see that we should trust it fully. We only
>>> want to trigger a detect work, which is exactly what is done in the patch
>>> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".
>>
>> The original problem was "slow card removal".  "Unreliable card detect"
>> is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
>> for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have
>> discussed.
> 
> I do not understand why you mention "Unreliable card detect"? That has
> nothing to do with this patch.
> 
> So to conclude the discussion, do you believe that this patch is acceptable
> as long as we add a CAPS2 option "MMC_CAP2_RESCAN_ON_ERROR", which if not
> set will prevent the detect work from being scheduled from
> mmc_detect_card_removed?

Yes

> 
>>
>>> If each host driver that supports GPIO card detect makes use of the
>>> card-detect helper and if we accept a version of this patch, I think the
>>> situation should be safe in all cases. Moreover GPIO debouncing will never
>>> be needed for GPIO card detect for your sdhci driver either.
>>
>> Safe in all cases, except at least the 3 already given:
>>     - card is buggy and sometimes fails several commands in a row
> I doubt this will become a real problem. If a card fails several times in a
> row, upper FS layers wont be happy either. So likely we are screwed anyway.
> Don't you think?
>>     - upper layers want to attempt to recover an unresponsive card
> This is not implemented as of right now. I see no problem that my patch will
> prevent this from being implemented in the future.
>>     - even in the case of slow removal, the vendor wants the card
>>     to show as removed immediately whether or not there is any I/O
> That is correct and can not be fully solved unless you use
> MMC_CAP_NEEDS_POLL. I doubt that one would like to use polling in favor of
> card detect only to take care of this issue though. So I believe it is more
> likely you want to trigger a card removal when receiving I/O. Kind of
> "Better late than never". :-)
> 
> 
> By the way, thanks for keeping up the frequency in this quite long discussion.
> 
> BR
> Ulf Hansson
> 

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 13/01/12 15:14, Ulf Hansson wrote:
>>>>>> In principles this means the following sequence:
>>>>>>
>>>>>> We will rely on that the get_cd function will return 0 (indicating card is
>>>>>> removed) when the card is "slowly" removed at the point when the rescan
>>>>>> function is calling it through the bus_ops->detect -->
>>>>>> _mmc_detect_card_removed function.
>>>>>>
>>>>>> This then becomes a race, meaning that the rescan function must be
>>>>>> executing
>>>>>> at the same time the get_cd function will returns 0. Otherwise the rescan
>>>>>> function will not remove the card.
>>>>>>
>>>>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback
>>>>>> function to
>>>>>> detect card" will likely improve behavior but is not the safe solution to
>>>>>> handle "slowly" removed cards.
>>>>>>
>>>>>> Again, to be sure, we must let the mmc_detect_card_remove function
>>>>>> trigger a
>>>>>> rescan when _mmc_detect_card_removed has detected that the card is
>>>>>> removed.
>>>>>> This should be safe in all circumstances.
>>>>> sdhci has no problem because it does this:
>>>>>
>>>>>     - the host controller debounces the card detect line
>>>>>     - the host controller records whether or not the card is present
>>>>>     - the sdhci driver prevents (errors out) requests when the card is
>>>>>     not present
>>>> Debouncing will just be a way of triggering the problem more seldom. Or in
>>>> worst case, state the card has been removed even if it has not.
>>> If a delay is used with mmc_detect_change, debouncing is not necessary.
>>>
>>>> Just because you get a GPIO irq on the detect line does not mean the card is
>>>> removed, debouncing or not. I consider this as pure mechanical switch which
>>>> likely has glitches and I don't see that we should trust it fully. We only
>>>> want to trigger a detect work, which is exactly what is done in the patch
>>>> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".
>>> The original problem was "slow card removal".  "Unreliable card detect"
>>> is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
>>> for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have
>>> discussed.
>> I do not understand why you mention "Unreliable card detect"? That has
>> nothing to do with this patch.
>>
>> So to conclude the discussion, do you believe that this patch is acceptable
>> as long as we add a CAPS2 option "MMC_CAP2_RESCAN_ON_ERROR", which if not
>> set will prevent the detect work from being scheduled from
>> mmc_detect_card_removed?
> 
> Yes
> 

OK, but.. :-)

I were just about to update the patch according to your recommendation 
when I realized the following:

Once _mmc_detect_card_removed has set the card state as removed 
("mmc_card_set_removed"), the card will never be accessible for I/O 
requests any more, all I/O will "silently" be thrown away in the block 
layer. This leads to that there should definitely be no reason for _not_ 
letting a scheduled rescan remove the card as soon as possible. In other 
words the CAP2 should not be needed.

Did I miss something?

Agree?


BR
Ulf Hansson

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Adrian Hunter]

On 13/01/12 16:35, Ulf Hansson wrote:
> Adrian Hunter wrote:
>> On 13/01/12 15:14, Ulf Hansson wrote:
>>>>>>> In principles this means the following sequence:
>>>>>>>
>>>>>>> We will rely on that the get_cd function will return 0 (indicating
>>>>>>> card is
>>>>>>> removed) when the card is "slowly" removed at the point when the rescan
>>>>>>> function is calling it through the bus_ops->detect -->
>>>>>>> _mmc_detect_card_removed function.
>>>>>>>
>>>>>>> This then becomes a race, meaning that the rescan function must be
>>>>>>> executing
>>>>>>> at the same time the get_cd function will returns 0. Otherwise the
>>>>>>> rescan
>>>>>>> function will not remove the card.
>>>>>>>
>>>>>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback
>>>>>>> function to
>>>>>>> detect card" will likely improve behavior but is not the safe
>>>>>>> solution to
>>>>>>> handle "slowly" removed cards.
>>>>>>>
>>>>>>> Again, to be sure, we must let the mmc_detect_card_remove function
>>>>>>> trigger a
>>>>>>> rescan when _mmc_detect_card_removed has detected that the card is
>>>>>>> removed.
>>>>>>> This should be safe in all circumstances.
>>>>>> sdhci has no problem because it does this:
>>>>>>
>>>>>>     - the host controller debounces the card detect line
>>>>>>     - the host controller records whether or not the card is present
>>>>>>     - the sdhci driver prevents (errors out) requests when the card is
>>>>>>     not present
>>>>> Debouncing will just be a way of triggering the problem more seldom. Or in
>>>>> worst case, state the card has been removed even if it has not.
>>>> If a delay is used with mmc_detect_change, debouncing is not necessary.
>>>>
>>>>> Just because you get a GPIO irq on the detect line does not mean the
>>>>> card is
>>>>> removed, debouncing or not. I consider this as pure mechanical switch
>>>>> which
>>>>> likely has glitches and I don't see that we should trust it fully. We only
>>>>> want to trigger a detect work, which is exactly what is done in the patch
>>>>> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".
>>>> The original problem was "slow card removal".  "Unreliable card detect"
>>>> is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
>>>> for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have
>>>> discussed.
>>> I do not understand why you mention "Unreliable card detect"? That has
>>> nothing to do with this patch.
>>>
>>> So to conclude the discussion, do you believe that this patch is acceptable
>>> as long as we add a CAPS2 option "MMC_CAP2_RESCAN_ON_ERROR", which if not
>>> set will prevent the detect work from being scheduled from
>>> mmc_detect_card_removed?
>>
>> Yes
>>
> 
> OK, but.. :-)
> 
> I were just about to update the patch according to your recommendation when
> I realized the following:
> 
> Once _mmc_detect_card_removed has set the card state as removed
> ("mmc_card_set_removed"), the card will never be accessible for I/O requests
> any more, all I/O will "silently" be thrown away in the block layer. This
> leads to that there should definitely be no reason for _not_ letting a
> scheduled rescan remove the card as soon as possible. In other words the
> CAP2 should not be needed.
> 
> Did I miss something?
> 
> Agree?

No.  mmc_detect_card_removed() will not check/set the card removed
unless there has been a call to mmc_detect_change() to set the
host->detect_change flag.

MMC_CAP2_RESCAN_ON_ERROR is definitely needed.

Do not confuse mmc_detect_card_removed() with _mmc_detect_card_removed().
The former is called by block.c.  The latter is only called by mmc_rescan()
via the ->detect method.

Re: [PATCH] mmc: core: Force a "detect" to handle non-properly removed cards

[Ulf Hansson]

Adrian Hunter wrote:
> On 13/01/12 16:35, Ulf Hansson wrote:
>> Adrian Hunter wrote:
>>> On 13/01/12 15:14, Ulf Hansson wrote:
>>>>>>>> In principles this means the following sequence:
>>>>>>>>
>>>>>>>> We will rely on that the get_cd function will return 0 (indicating
>>>>>>>> card is
>>>>>>>> removed) when the card is "slowly" removed at the point when the rescan
>>>>>>>> function is calling it through the bus_ops->detect -->
>>>>>>>> _mmc_detect_card_removed function.
>>>>>>>>
>>>>>>>> This then becomes a race, meaning that the rescan function must be
>>>>>>>> executing
>>>>>>>> at the same time the get_cd function will returns 0. Otherwise the
>>>>>>>> rescan
>>>>>>>> function will not remove the card.
>>>>>>>>
>>>>>>>> Thus my conclusion is that "[PATCH 2/4 v4] MMC/SD: Add callback
>>>>>>>> function to
>>>>>>>> detect card" will likely improve behavior but is not the safe
>>>>>>>> solution to
>>>>>>>> handle "slowly" removed cards.
>>>>>>>>
>>>>>>>> Again, to be sure, we must let the mmc_detect_card_remove function
>>>>>>>> trigger a
>>>>>>>> rescan when _mmc_detect_card_removed has detected that the card is
>>>>>>>> removed.
>>>>>>>> This should be safe in all circumstances.
>>>>>>> sdhci has no problem because it does this:
>>>>>>>
>>>>>>>     - the host controller debounces the card detect line
>>>>>>>     - the host controller records whether or not the card is present
>>>>>>>     - the sdhci driver prevents (errors out) requests when the card is
>>>>>>>     not present
>>>>>> Debouncing will just be a way of triggering the problem more seldom. Or in
>>>>>> worst case, state the card has been removed even if it has not.
>>>>> If a delay is used with mmc_detect_change, debouncing is not necessary.
>>>>>
>>>>>> Just because you get a GPIO irq on the detect line does not mean the
>>>>>> card is
>>>>>> removed, debouncing or not. I consider this as pure mechanical switch
>>>>>> which
>>>>>> likely has glitches and I don't see that we should trust it fully. We only
>>>>>> want to trigger a detect work, which is exactly what is done in the patch
>>>>>> from Guennadi Liakhovetski "mmc: add a generic GPIO card-detect helper".
>>>>> The original problem was "slow card removal".  "Unreliable card detect"
>>>>> is a separate problem.  Currently there is polling (MMC_CAP_NEEDS_POLL)
>>>>> for that.  Alternatively there is MMC_CAP2_RESCAN_ON_ERROR as we have
>>>>> discussed.
>>>> I do not understand why you mention "Unreliable card detect"? That has
>>>> nothing to do with this patch.
>>>>
>>>> So to conclude the discussion, do you believe that this patch is acceptable
>>>> as long as we add a CAPS2 option "MMC_CAP2_RESCAN_ON_ERROR", which if not
>>>> set will prevent the detect work from being scheduled from
>>>> mmc_detect_card_removed?
>>> Yes
>>>
>> OK, but.. :-)
>>
>> I were just about to update the patch according to your recommendation when
>> I realized the following:
>>
>> Once _mmc_detect_card_removed has set the card state as removed
>> ("mmc_card_set_removed"), the card will never be accessible for I/O requests
>> any more, all I/O will "silently" be thrown away in the block layer. This
>> leads to that there should definitely be no reason for _not_ letting a
>> scheduled rescan remove the card as soon as possible. In other words the
>> CAP2 should not be needed.
>>
>> Did I miss something?
>>
>> Agree?
> 
> No.  mmc_detect_card_removed() will not check/set the card removed
> unless there has been a call to mmc_detect_change() to set the
> host->detect_change flag.

That were before this patch. This patch removes the detect_change flag 
since it is used as you say to prevent "mmc_detect_card_removed" from 
calling _mmc_detect_card_removed and thus possibly setting the card 
state to removed.

The use of the detect_flag is a bit strange I think. It means simply 
that after getting one GPIO cd irq and then an I/O error we will only 
try at _most_ _one_ time from mmc_detect_card_removed to see if the card 
really has been removed. If the mmc_detect_card_removed the first time 
does not detect that the card is removed it will have to wait for the 
rescan the cover it, which is likely not what we want!?

I will see if I can figure out a way of keeping the old scenario in 
parallel with having MMC_CAP2_RESCAN_ON_ERROR... I will post a new patch.

> 
> MMC_CAP2_RESCAN_ON_ERROR is definitely needed.
> 
> Do not confuse mmc_detect_card_removed() with _mmc_detect_card_removed().
> The former is called by block.c.  The latter is only called by mmc_rescan()
> via the ->detect method.
>