[RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Ulf Hansson]

We want to give user space provision to fully consume a card
insert/remove event, when the event was caused by a wakeup irq.

By signaling the wakeup event for a time of 5 s for devices configured
as wakeup capable, we likely will be prevent a sleep long enough to let
user space consume the event.

To enable this feature, host drivers must thus configure their devices
as wakeup capable.

This is a reworked implementation of the old wakelocks for the mmc
subsystem, originally authored by Colin Cross and San Mehat for the
Android kernel. Zoran Markovic shall also be given cred for recently
re-trying to upstream this feature.

Cc: San Mehat <san@google.com>
Cc: Colin Cross <ccross@android.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Zoran Markovic <zoran.markovic@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

This patch has just been compile tested, since I at very moment did not
have a good board to test it on. I am working on that.

Any help in testing this patch is thus greatly appreciated. While
testing also don't forget to enable the host device as wakeup capable.
Use "device_init_wakeup" from the host probe function.

---
 drivers/mmc/core/core.c |   39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index bf18b6b..3e8229e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -23,6 +23,7 @@
 #include <linux/log2.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeup.h>
 #include <linux/suspend.h>
 #include <linux/fault-inject.h>
 #include <linux/random.h>
@@ -1723,6 +1724,28 @@ void mmc_detach_bus(struct mmc_host *host)
 	mmc_bus_put(host);
 }
 
+static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
+				bool cd_irq)
+{
+#ifdef CONFIG_MMC_DEBUG
+	unsigned long flags;
+	spin_lock_irqsave(&host->lock, flags);
+	WARN_ON(host->removed);
+	spin_unlock_irqrestore(&host->lock, flags);
+#endif
+
+	/*
+	 * If the device is configured as wakeup, we prevent a new sleep for
+	 * 5 s to give provision for user space to consume the event.
+	 */
+	if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
+		device_can_wakeup(mmc_dev(host)))
+		pm_wakeup_event(mmc_dev(host), 5000);
+
+	host->detect_change = 1;
+	mmc_schedule_delayed_work(&host->detect, delay);
+}
+
 /**
  *	mmc_detect_change - process change of state on a MMC socket
  *	@host: host which changed state.
@@ -1735,16 +1758,8 @@ void mmc_detach_bus(struct mmc_host *host)
  */
 void mmc_detect_change(struct mmc_host *host, unsigned long delay)
 {
-#ifdef CONFIG_MMC_DEBUG
-	unsigned long flags;
-	spin_lock_irqsave(&host->lock, flags);
-	WARN_ON(host->removed);
-	spin_unlock_irqrestore(&host->lock, flags);
-#endif
-	host->detect_change = 1;
-	mmc_schedule_delayed_work(&host->detect, delay);
+	_mmc_detect_change(host, delay, true);
 }
-
 EXPORT_SYMBOL(mmc_detect_change);
 
 void mmc_init_erase(struct mmc_card *card)
@@ -2423,7 +2438,7 @@ int mmc_detect_card_removed(struct mmc_host *host)
 			 * rescan handle the card removal.
 			 */
 			cancel_delayed_work(&host->detect);
-			mmc_detect_change(host, 0);
+			_mmc_detect_change(host, 0, false);
 		}
 	}
 
@@ -2505,7 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
 		mmc_power_off(host);
 	else
 		mmc_power_up(host);
-	mmc_detect_change(host, 0);
+	_mmc_detect_change(host, 0, false);
 }
 
 void mmc_stop_host(struct mmc_host *host)
@@ -2724,7 +2739,7 @@ int mmc_pm_notify(struct notifier_block *notify_block,
 		spin_lock_irqsave(&host->lock, flags);
 		host->rescan_disable = 0;
 		spin_unlock_irqrestore(&host->lock, flags);
-		mmc_detect_change(host, 0);
+		_mmc_detect_change(host, 0, false);
 
 	}
 
-- 
1.7.9.5

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Jaehoon Chung]

Hi Ulf,

On 09/20/2013 06:48 PM, Ulf Hansson wrote:
> We want to give user space provision to fully consume a card
> insert/remove event, when the event was caused by a wakeup irq.
> 
> By signaling the wakeup event for a time of 5 s for devices configured
> as wakeup capable, we likely will be prevent a sleep long enough to let
> user space consume the event.
> 
> To enable this feature, host drivers must thus configure their devices
> as wakeup capable.
> 
> This is a reworked implementation of the old wakelocks for the mmc
> subsystem, originally authored by Colin Cross and San Mehat for the
> Android kernel. Zoran Markovic shall also be given cred for recently
> re-trying to upstream this feature.
> 
> Cc: San Mehat <san@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Zoran Markovic <zoran.markovic@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> This patch has just been compile tested, since I at very moment did not
> have a good board to test it on. I am working on that.
> 
> Any help in testing this patch is thus greatly appreciated. While
> testing also don't forget to enable the host device as wakeup capable.
> Use "device_init_wakeup" from the host probe function.

I used the device_init_wakeup(&pdev->dev, 1) into host controller. (Also enabled the irq for wakeup)
It didn't work when device is suspended.
Well, i might missed something.
As my understanding, it's helpful that wakeup-event is used when device is suspended.
How do you call mmc_detect_change() during suspended?

Best Regards,
Jaehoon Chung
> 
> ---
>  drivers/mmc/core/core.c |   39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index bf18b6b..3e8229e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/log2.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeup.h>
>  #include <linux/suspend.h>
>  #include <linux/fault-inject.h>
>  #include <linux/random.h>
> @@ -1723,6 +1724,28 @@ void mmc_detach_bus(struct mmc_host *host)
>  	mmc_bus_put(host);
>  }
>  
> +static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
> +				bool cd_irq)
> +{
> +#ifdef CONFIG_MMC_DEBUG
> +	unsigned long flags;
> +	spin_lock_irqsave(&host->lock, flags);
> +	WARN_ON(host->removed);
> +	spin_unlock_irqrestore(&host->lock, flags);
> +#endif
> +
> +	/*
> +	 * If the device is configured as wakeup, we prevent a new sleep for
> +	 * 5 s to give provision for user space to consume the event.
> +	 */
> +	if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
> +		device_can_wakeup(mmc_dev(host)))
> +		pm_wakeup_event(mmc_dev(host), 5000);
> +
> +	host->detect_change = 1;
> +	mmc_schedule_delayed_work(&host->detect, delay);
> +}
> +
>  /**
>   *	mmc_detect_change - process change of state on a MMC socket
>   *	@host: host which changed state.
> @@ -1735,16 +1758,8 @@ void mmc_detach_bus(struct mmc_host *host)
>   */
>  void mmc_detect_change(struct mmc_host *host, unsigned long delay)
>  {
> -#ifdef CONFIG_MMC_DEBUG
> -	unsigned long flags;
> -	spin_lock_irqsave(&host->lock, flags);
> -	WARN_ON(host->removed);
> -	spin_unlock_irqrestore(&host->lock, flags);
> -#endif
> -	host->detect_change = 1;
> -	mmc_schedule_delayed_work(&host->detect, delay);
> +	_mmc_detect_change(host, delay, true);
>  }
> -
>  EXPORT_SYMBOL(mmc_detect_change);
>  
>  void mmc_init_erase(struct mmc_card *card)
> @@ -2423,7 +2438,7 @@ int mmc_detect_card_removed(struct mmc_host *host)
>  			 * rescan handle the card removal.
>  			 */
>  			cancel_delayed_work(&host->detect);
> -			mmc_detect_change(host, 0);
> +			_mmc_detect_change(host, 0, false);
>  		}
>  	}
>  
> @@ -2505,7 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
>  		mmc_power_off(host);
>  	else
>  		mmc_power_up(host);
> -	mmc_detect_change(host, 0);
> +	_mmc_detect_change(host, 0, false);
>  }
>  
>  void mmc_stop_host(struct mmc_host *host)
> @@ -2724,7 +2739,7 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>  		spin_lock_irqsave(&host->lock, flags);
>  		host->rescan_disable = 0;
>  		spin_unlock_irqrestore(&host->lock, flags);
> -		mmc_detect_change(host, 0);
> +		_mmc_detect_change(host, 0, false);
>  
>  	}
>  
> 

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Ulf Hansson]

On 23 September 2013 12:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Ulf,
>
> On 09/20/2013 06:48 PM, Ulf Hansson wrote:
>> We want to give user space provision to fully consume a card
>> insert/remove event, when the event was caused by a wakeup irq.
>>
>> By signaling the wakeup event for a time of 5 s for devices configured
>> as wakeup capable, we likely will be prevent a sleep long enough to let
>> user space consume the event.
>>
>> To enable this feature, host drivers must thus configure their devices
>> as wakeup capable.
>>
>> This is a reworked implementation of the old wakelocks for the mmc
>> subsystem, originally authored by Colin Cross and San Mehat for the
>> Android kernel. Zoran Markovic shall also be given cred for recently
>> re-trying to upstream this feature.
>>
>> Cc: San Mehat <san@google.com>
>> Cc: Colin Cross <ccross@android.com>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Zoran Markovic <zoran.markovic@linaro.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> This patch has just been compile tested, since I at very moment did not
>> have a good board to test it on. I am working on that.
>>
>> Any help in testing this patch is thus greatly appreciated. While
>> testing also don't forget to enable the host device as wakeup capable.
>> Use "device_init_wakeup" from the host probe function.
>

Hi Jaehoon,

Thanks for helping out testing this patch!

> I used the device_init_wakeup(&pdev->dev, 1) into host controller. (Also enabled the irq for wakeup)
> It didn't work when device is suspended.
> Well, i might missed something.

Not sure what you mean by didn't work here. Did the card detect irq
wake up the platform and thus triggered a resume? Or you mean that
even that did not work?

> As my understanding, it's helpful that wakeup-event is used when device is suspended.

This feature is typically used for devices using "autosuspend", like
Android devices. Otherwise you likely do not want to wakeup the
platform when inserting/removing a card, but that is another story.
:-)

> How do you call mmc_detect_change() during suspended?

To try to clarify things a bit, the sequence is typically like this:

1. We enter suspend.
2. A card is inserted and a card detect irq is triggered. The
corresponding irq handler gets called which then issue
mmc_detect_change.
3. mmc_detect_change recognize the device as a wakeup capable device
and thus issue a pm_wakeup_event(5s) to prevent a new trigger of
suspend.
4. A rescan work is scheduled.
5. System continues resuming.
6. After PM_POST_SUSPEND notification (mmc_pm_notify), rescan is
enabled and a new work re-scheduled.
7. The rescan work detects the new card and user space soon gets
notified about it.
8. User space gets the notification and can act on it.
9. The timeout of the pm_wakeup_event has elapsed, allowing a new
suspend to be triggered.


Kind regards
Ulf Hansson

>
> Best Regards,
> Jaehoon Chung
>>
>> ---
>>  drivers/mmc/core/core.c |   39 +++++++++++++++++++++++++++------------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index bf18b6b..3e8229e 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/log2.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pm_wakeup.h>
>>  #include <linux/suspend.h>
>>  #include <linux/fault-inject.h>
>>  #include <linux/random.h>
>> @@ -1723,6 +1724,28 @@ void mmc_detach_bus(struct mmc_host *host)
>>       mmc_bus_put(host);
>>  }
>>
>> +static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>> +                             bool cd_irq)
>> +{
>> +#ifdef CONFIG_MMC_DEBUG
>> +     unsigned long flags;
>> +     spin_lock_irqsave(&host->lock, flags);
>> +     WARN_ON(host->removed);
>> +     spin_unlock_irqrestore(&host->lock, flags);
>> +#endif
>> +
>> +     /*
>> +      * If the device is configured as wakeup, we prevent a new sleep for
>> +      * 5 s to give provision for user space to consume the event.
>> +      */
>> +     if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
>> +             device_can_wakeup(mmc_dev(host)))
>> +             pm_wakeup_event(mmc_dev(host), 5000);
>> +
>> +     host->detect_change = 1;
>> +     mmc_schedule_delayed_work(&host->detect, delay);
>> +}
>> +
>>  /**
>>   *   mmc_detect_change - process change of state on a MMC socket
>>   *   @host: host which changed state.
>> @@ -1735,16 +1758,8 @@ void mmc_detach_bus(struct mmc_host *host)
>>   */
>>  void mmc_detect_change(struct mmc_host *host, unsigned long delay)
>>  {
>> -#ifdef CONFIG_MMC_DEBUG
>> -     unsigned long flags;
>> -     spin_lock_irqsave(&host->lock, flags);
>> -     WARN_ON(host->removed);
>> -     spin_unlock_irqrestore(&host->lock, flags);
>> -#endif
>> -     host->detect_change = 1;
>> -     mmc_schedule_delayed_work(&host->detect, delay);
>> +     _mmc_detect_change(host, delay, true);
>>  }
>> -
>>  EXPORT_SYMBOL(mmc_detect_change);
>>
>>  void mmc_init_erase(struct mmc_card *card)
>> @@ -2423,7 +2438,7 @@ int mmc_detect_card_removed(struct mmc_host *host)
>>                        * rescan handle the card removal.
>>                        */
>>                       cancel_delayed_work(&host->detect);
>> -                     mmc_detect_change(host, 0);
>> +                     _mmc_detect_change(host, 0, false);
>>               }
>>       }
>>
>> @@ -2505,7 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
>>               mmc_power_off(host);
>>       else
>>               mmc_power_up(host);
>> -     mmc_detect_change(host, 0);
>> +     _mmc_detect_change(host, 0, false);
>>  }
>>
>>  void mmc_stop_host(struct mmc_host *host)
>> @@ -2724,7 +2739,7 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>>               spin_lock_irqsave(&host->lock, flags);
>>               host->rescan_disable = 0;
>>               spin_unlock_irqrestore(&host->lock, flags);
>> -             mmc_detect_change(host, 0);
>> +             _mmc_detect_change(host, 0, false);
>>
>>       }
>>
>>
>

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Jaehoon Chung]

On 09/23/2013 09:11 PM, Ulf Hansson wrote:
> On 23 September 2013 12:55, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi Ulf,
>>
>> On 09/20/2013 06:48 PM, Ulf Hansson wrote:
>>> We want to give user space provision to fully consume a card
>>> insert/remove event, when the event was caused by a wakeup irq.
>>>
>>> By signaling the wakeup event for a time of 5 s for devices configured
>>> as wakeup capable, we likely will be prevent a sleep long enough to let
>>> user space consume the event.
>>>
>>> To enable this feature, host drivers must thus configure their devices
>>> as wakeup capable.
>>>
>>> This is a reworked implementation of the old wakelocks for the mmc
>>> subsystem, originally authored by Colin Cross and San Mehat for the
>>> Android kernel. Zoran Markovic shall also be given cred for recently
>>> re-trying to upstream this feature.
>>>
>>> Cc: San Mehat <san@google.com>
>>> Cc: Colin Cross <ccross@android.com>
>>> Cc: John Stultz <john.stultz@linaro.org>
>>> Cc: Zoran Markovic <zoran.markovic@linaro.org>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>
>>> This patch has just been compile tested, since I at very moment did not
>>> have a good board to test it on. I am working on that.
>>>
>>> Any help in testing this patch is thus greatly appreciated. While
>>> testing also don't forget to enable the host device as wakeup capable.
>>> Use "device_init_wakeup" from the host probe function.
>>
> 
> Hi Jaehoon,
> 
> Thanks for helping out testing this patch!
> 
>> I used the device_init_wakeup(&pdev->dev, 1) into host controller. (Also enabled the irq for wakeup)
>> It didn't work when device is suspended.
>> Well, i might missed something.
> 
> Not sure what you mean by didn't work here. Did the card detect irq
> wake up the platform and thus triggered a resume? Or you mean that
> even that did not work?
> 
>> As my understanding, it's helpful that wakeup-event is used when device is suspended.
> 
> This feature is typically used for devices using "autosuspend", like
> Android devices. Otherwise you likely do not want to wakeup the
> platform when inserting/removing a card, but that is another story.
> :-)
Hi Ulf,

Then, my case should be another story. I will re-test with using "autosuspend".
I didn't use the autosuspend.
> 
>> How do you call mmc_detect_change() during suspended?

In my case,
1, entered suspended.
2. card is inserted..then didn't get the triggered irq.
If get the triggered irq, need to set irq_set_irq_wake(cd_irq, 1).

So i have asked to you how called mmc_detect_change().
In my-case, didn't call mmc_detect_change().

I will re-test and share the result.

Best Regards,
Jaehoon Chung
> 
> To try to clarify things a bit, the sequence is typically like this:
> 
> 1. We enter suspend.
> 2. A card is inserted and a card detect irq is triggered. The
> corresponding irq handler gets called which then issue
> mmc_detect_change.
> 3. mmc_detect_change recognize the device as a wakeup capable device
> and thus issue a pm_wakeup_event(5s) to prevent a new trigger of
> suspend.
> 4. A rescan work is scheduled.
> 5. System continues resuming.
> 6. After PM_POST_SUSPEND notification (mmc_pm_notify), rescan is
> enabled and a new work re-scheduled.
> 7. The rescan work detects the new card and user space soon gets
> notified about it.
> 8. User space gets the notification and can act on it.
> 9. The timeout of the pm_wakeup_event has elapsed, allowing a new
> suspend to be triggered.
> 
> 
> Kind regards
> Ulf Hansson
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>>
>>> ---
>>>  drivers/mmc/core/core.c |   39 +++++++++++++++++++++++++++------------
>>>  1 file changed, 27 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index bf18b6b..3e8229e 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -23,6 +23,7 @@
>>>  #include <linux/log2.h>
>>>  #include <linux/regulator/consumer.h>
>>>  #include <linux/pm_runtime.h>
>>> +#include <linux/pm_wakeup.h>
>>>  #include <linux/suspend.h>
>>>  #include <linux/fault-inject.h>
>>>  #include <linux/random.h>
>>> @@ -1723,6 +1724,28 @@ void mmc_detach_bus(struct mmc_host *host)
>>>       mmc_bus_put(host);
>>>  }
>>>
>>> +static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>> +                             bool cd_irq)
>>> +{
>>> +#ifdef CONFIG_MMC_DEBUG
>>> +     unsigned long flags;
>>> +     spin_lock_irqsave(&host->lock, flags);
>>> +     WARN_ON(host->removed);
>>> +     spin_unlock_irqrestore(&host->lock, flags);
>>> +#endif
>>> +
>>> +     /*
>>> +      * If the device is configured as wakeup, we prevent a new sleep for
>>> +      * 5 s to give provision for user space to consume the event.
>>> +      */
>>> +     if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
>>> +             device_can_wakeup(mmc_dev(host)))
>>> +             pm_wakeup_event(mmc_dev(host), 5000);
>>> +
>>> +     host->detect_change = 1;
>>> +     mmc_schedule_delayed_work(&host->detect, delay);
>>> +}
>>> +
>>>  /**
>>>   *   mmc_detect_change - process change of state on a MMC socket
>>>   *   @host: host which changed state.
>>> @@ -1735,16 +1758,8 @@ void mmc_detach_bus(struct mmc_host *host)
>>>   */
>>>  void mmc_detect_change(struct mmc_host *host, unsigned long delay)
>>>  {
>>> -#ifdef CONFIG_MMC_DEBUG
>>> -     unsigned long flags;
>>> -     spin_lock_irqsave(&host->lock, flags);
>>> -     WARN_ON(host->removed);
>>> -     spin_unlock_irqrestore(&host->lock, flags);
>>> -#endif
>>> -     host->detect_change = 1;
>>> -     mmc_schedule_delayed_work(&host->detect, delay);
>>> +     _mmc_detect_change(host, delay, true);
>>>  }
>>> -
>>>  EXPORT_SYMBOL(mmc_detect_change);
>>>
>>>  void mmc_init_erase(struct mmc_card *card)
>>> @@ -2423,7 +2438,7 @@ int mmc_detect_card_removed(struct mmc_host *host)
>>>                        * rescan handle the card removal.
>>>                        */
>>>                       cancel_delayed_work(&host->detect);
>>> -                     mmc_detect_change(host, 0);
>>> +                     _mmc_detect_change(host, 0, false);
>>>               }
>>>       }
>>>
>>> @@ -2505,7 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
>>>               mmc_power_off(host);
>>>       else
>>>               mmc_power_up(host);
>>> -     mmc_detect_change(host, 0);
>>> +     _mmc_detect_change(host, 0, false);
>>>  }
>>>
>>>  void mmc_stop_host(struct mmc_host *host)
>>> @@ -2724,7 +2739,7 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>>>               spin_lock_irqsave(&host->lock, flags);
>>>               host->rescan_disable = 0;
>>>               spin_unlock_irqrestore(&host->lock, flags);
>>> -             mmc_detect_change(host, 0);
>>> +             _mmc_detect_change(host, 0, false);
>>>
>>>       }
>>>
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Zoran Markovic]

Hi Ulf,
I like the fact that wakeups are now quite simplified. A couple of
comments below:

> By signaling the wakeup event for a time of 5 s for devices configured
> as wakeup capable, we likely will be prevent a sleep long enough to let
> user space consume the event.
Given that there is no pm_relax(), 5 seconds is probably too long.
User space should grab a wakeup_source of its own if it needs to
extend the awake state. I recommend putting just enough to start the
mount process - probably 0.5-1 second - but Android guys would know
better.

> +       /*
> +        * If the device is configured as wakeup, we prevent a new sleep for
> +        * 5 s to give provision for user space to consume the event.
> +        */
> +       if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
> +               device_can_wakeup(mmc_dev(host)))
> +               pm_wakeup_event(mmc_dev(host), 5000);
It seems like device_can_wakeup() is redundant here and I'm not sure
what happens if a device is not wakeup capable. Was the intention to
go into autosleep until something else wakes up the system long enough
to complete the mount? Also is it ok if MMCs that require polling
exhibit wakeup behaviour of their own?

Zoran

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Ulf Hansson]

Hi Zoran,

On 23 September 2013 23:14, Zoran Markovic <zoran.markovic@linaro.org> wrote:
> Hi Ulf,
> I like the fact that wakeups are now quite simplified. A couple of
> comments below:
>
>> By signaling the wakeup event for a time of 5 s for devices configured
>> as wakeup capable, we likely will be prevent a sleep long enough to let
>> user space consume the event.
> Given that there is no pm_relax(), 5 seconds is probably too long.
> User space should grab a wakeup_source of its own if it needs to
> extend the awake state. I recommend putting just enough to start the
> mount process - probably 0.5-1 second - but Android guys would know
> better.

The total time before users pace gets notified can be summarized like this:

1.
Total system resume time, which has quite a big span of expected
consumed time. Do note, if other (e)MMC and/or SD-cards are already
inserted, these will be re-initialized as a part the resume and this
affect the resume time significantly. Typically an eMMC takes 200-400
ms and an SD-card 250-1100ms.

2. The scheduled rescan work shall be given priority to execute and
then complete the initialization of the recently inserted card. If we
assume it will be and SD-card, 250-1100 ms.

3. Once the card device is created from the rescan work, notification
is propagated upwards to user space. For this task I have no relevant
estimation on consumed time.

Not sure how to set the time-out value to meet all expectations. I
believe we could also consider that inserting/removing a card is a
quite seldom operation, so using a bit higher value than necessary
might be okay. What do you think?

>
>> +       /*
>> +        * If the device is configured as wakeup, we prevent a new sleep for
>> +        * 5 s to give provision for user space to consume the event.
>> +        */
>> +       if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
>> +               device_can_wakeup(mmc_dev(host)))
>> +               pm_wakeup_event(mmc_dev(host), 5000);
> It seems like device_can_wakeup() is redundant here and I'm not sure
> what happens if a device is not wakeup capable. Was the intention to
> go into autosleep until something else wakes up the system long enough
> to complete the mount? Also is it ok if MMCs that require polling
> exhibit wakeup behaviour of their own?

I did not want to change present behaviour for all host drivers and
platforms, but instead host drivers need take an action of enabling
wakeup. If they consider to use card detect as wakeup irq, they need
adapt anyway, the irq is not default a wakeup irq.

In polling mode, new inserted cards will never be detected in suspend
state, since the entire system needs to be resumed to be able to
"poll". This is just not feasible. :-)

Kind regards
Ulf Hansson

>
> Zoran

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[John Stultz]

On 09/24/2013 12:55 AM, Ulf Hansson wrote:
> Hi Zoran,
>
> On 23 September 2013 23:14, Zoran Markovic <zoran.markovic@linaro.org> wrote:
>> Hi Ulf,
>> I like the fact that wakeups are now quite simplified. A couple of
>> comments below:
>>
>>> By signaling the wakeup event for a time of 5 s for devices configured
>>> as wakeup capable, we likely will be prevent a sleep long enough to let
>>> user space consume the event.
>> Given that there is no pm_relax(), 5 seconds is probably too long.
>> User space should grab a wakeup_source of its own if it needs to
>> extend the awake state. I recommend putting just enough to start the
>> mount process - probably 0.5-1 second - but Android guys would know
>> better.
> The total time before users pace gets notified can be summarized like this:
>
> 1.
> Total system resume time, which has quite a big span of expected
> consumed time. Do note, if other (e)MMC and/or SD-cards are already
> inserted, these will be re-initialized as a part the resume and this
> affect the resume time significantly. Typically an eMMC takes 200-400
> ms and an SD-card 250-1100ms.
>
> 2. The scheduled rescan work shall be given priority to execute and
> then complete the initialization of the recently inserted card. If we
> assume it will be and SD-card, 250-1100 ms.
>
> 3. Once the card device is created from the rescan work, notification
> is propagated upwards to user space. For this task I have no relevant
> estimation on consumed time.

So how does the notification done to userspace? I wonder if you could
use the select/lock/read style method for releasing the kernel space
wakeup_source, as is done in the input layer?


> Not sure how to set the time-out value to meet all expectations. I
> believe we could also consider that inserting/removing a card is a
> quite seldom operation, so using a bit higher value than necessary
> might be okay. What do you think?

As long as its sure to be *very* rare, then slightly longer delays
aren't a major problem.  The trouble with the timeout style wakelocks is
that they are easy to misuse and that hurts power efficiency. (I've
heard stories of badly written wifi drivers that took 5 minute timed
wakelocks on packets, which effectively kept devices from ever sleeping).

So if possible, its much better to have a normal path where the pm_relax
is called in most cases, using the timeout for only the few places where
you really can't get an end point.

thanks
-john

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Ulf Hansson]

Hi John,

> So how does the notification done to userspace? I wonder if you could
> use the select/lock/read style method for releasing the kernel space
> wakeup_source, as is done in the input layer?

If I understand what you mean correctly, it is very similar what Zoran
tried to do before in his patch!?

For the mmc subsystem, especially considering the pitfalls around the
scheduled detect work, I think it will be hard to get this right, if
implementing like this. I fear we might end up in situations were the
wakeup_source is never "relaxed".

>
>
>> Not sure how to set the time-out value to meet all expectations. I
>> believe we could also consider that inserting/removing a card is a
>> quite seldom operation, so using a bit higher value than necessary
>> might be okay. What do you think?
>
> As long as its sure to be *very* rare, then slightly longer delays
> aren't a major problem.  The trouble with the timeout style wakelocks is
> that they are easy to misuse and that hurts power efficiency. (I've
> heard stories of badly written wifi drivers that took 5 minute timed
> wakelocks on packets, which effectively kept devices from ever sleeping).

In this case, I am more confident that this would actually simplify
code that much, so we can get around all the scary pitfalls.

I can only think of one case which could lead to problem. If there is
a host driver, enabled for wakeup, that gets spurious card detect
irqs, outside the window were you expect a card to be
detected/removed, and at the same time do not have a software
mechanism to "debounce" the irqs. But, should we even consider this as
a valid case?

Kind regards
Ulf Hansson

>
> So if possible, its much better to have a normal path where the pm_relax
> is called in most cases, using the timeout for only the few places where
> you really can't get an end point.
>
> thanks
> -john

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Zoran Markovic]

I like the simplified approach taken in this patch. Shortening the
awake time by trying to call __pm_relax() in all corner cases turned
out to be too complex.

Reviewed-by: Zoran Markovic <zoran.markovic@linaro.org>

Regards, Zoran

On 1 October 2013 02:22, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Hi John,
>
>> So how does the notification done to userspace? I wonder if you could
>> use the select/lock/read style method for releasing the kernel space
>> wakeup_source, as is done in the input layer?
>
> If I understand what you mean correctly, it is very similar what Zoran
> tried to do before in his patch!?
>
> For the mmc subsystem, especially considering the pitfalls around the
> scheduled detect work, I think it will be hard to get this right, if
> implementing like this. I fear we might end up in situations were the
> wakeup_source is never "relaxed".
>
>>
>>
>>> Not sure how to set the time-out value to meet all expectations. I
>>> believe we could also consider that inserting/removing a card is a
>>> quite seldom operation, so using a bit higher value than necessary
>>> might be okay. What do you think?
>>
>> As long as its sure to be *very* rare, then slightly longer delays
>> aren't a major problem.  The trouble with the timeout style wakelocks is
>> that they are easy to misuse and that hurts power efficiency. (I've
>> heard stories of badly written wifi drivers that took 5 minute timed
>> wakelocks on packets, which effectively kept devices from ever sleeping).
>
> In this case, I am more confident that this would actually simplify
> code that much, so we can get around all the scary pitfalls.
>
> I can only think of one case which could lead to problem. If there is
> a host driver, enabled for wakeup, that gets spurious card detect
> irqs, outside the window were you expect a card to be
> detected/removed, and at the same time do not have a software
> mechanism to "debounce" the irqs. But, should we even consider this as
> a valid case?
>
> Kind regards
> Ulf Hansson
>
>>
>> So if possible, its much better to have a normal path where the pm_relax
>> is called in most cases, using the timeout for only the few places where
>> you really can't get an end point.
>>
>> thanks
>> -john

Re: [RFC/PATCH] mmc: core: Signal wakeup event at card insert/removal

[Ulf Hansson]

On 20 September 2013 11:48, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> We want to give user space provision to fully consume a card
> insert/remove event, when the event was caused by a wakeup irq.
>
> By signaling the wakeup event for a time of 5 s for devices configured
> as wakeup capable, we likely will be prevent a sleep long enough to let
> user space consume the event.
>
> To enable this feature, host drivers must thus configure their devices
> as wakeup capable.
>
> This is a reworked implementation of the old wakelocks for the mmc
> subsystem, originally authored by Colin Cross and San Mehat for the
> Android kernel. Zoran Markovic shall also be given cred for recently
> re-trying to upstream this feature.
>
> Cc: San Mehat <san@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Zoran Markovic <zoran.markovic@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> This patch has just been compile tested, since I at very moment did not
> have a good board to test it on. I am working on that.
>
> Any help in testing this patch is thus greatly appreciated. While
> testing also don't forget to enable the host device as wakeup capable.
> Use "device_init_wakeup" from the host probe function.

Testing now done on a ux500 SoC. Changes done to configuring card
detect irq as wakeup. Sysfs nodes is behaving as expected. :-)

>
> ---
>  drivers/mmc/core/core.c |   39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index bf18b6b..3e8229e 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/log2.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeup.h>
>  #include <linux/suspend.h>
>  #include <linux/fault-inject.h>
>  #include <linux/random.h>
> @@ -1723,6 +1724,28 @@ void mmc_detach_bus(struct mmc_host *host)
>         mmc_bus_put(host);
>  }
>
> +static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
> +                               bool cd_irq)
> +{
> +#ifdef CONFIG_MMC_DEBUG
> +       unsigned long flags;
> +       spin_lock_irqsave(&host->lock, flags);
> +       WARN_ON(host->removed);
> +       spin_unlock_irqrestore(&host->lock, flags);
> +#endif
> +
> +       /*
> +        * If the device is configured as wakeup, we prevent a new sleep for
> +        * 5 s to give provision for user space to consume the event.
> +        */
> +       if (cd_irq && !(host->caps & MMC_CAP_NEEDS_POLL) &&
> +               device_can_wakeup(mmc_dev(host)))
> +               pm_wakeup_event(mmc_dev(host), 5000);
> +
> +       host->detect_change = 1;
> +       mmc_schedule_delayed_work(&host->detect, delay);
> +}
> +
>  /**
>   *     mmc_detect_change - process change of state on a MMC socket
>   *     @host: host which changed state.
> @@ -1735,16 +1758,8 @@ void mmc_detach_bus(struct mmc_host *host)
>   */
>  void mmc_detect_change(struct mmc_host *host, unsigned long delay)
>  {
> -#ifdef CONFIG_MMC_DEBUG
> -       unsigned long flags;
> -       spin_lock_irqsave(&host->lock, flags);
> -       WARN_ON(host->removed);
> -       spin_unlock_irqrestore(&host->lock, flags);
> -#endif
> -       host->detect_change = 1;
> -       mmc_schedule_delayed_work(&host->detect, delay);
> +       _mmc_detect_change(host, delay, true);
>  }
> -
>  EXPORT_SYMBOL(mmc_detect_change);
>
>  void mmc_init_erase(struct mmc_card *card)
> @@ -2423,7 +2438,7 @@ int mmc_detect_card_removed(struct mmc_host *host)
>                          * rescan handle the card removal.
>                          */
>                         cancel_delayed_work(&host->detect);
> -                       mmc_detect_change(host, 0);
> +                       _mmc_detect_change(host, 0, false);
>                 }
>         }
>
> @@ -2505,7 +2520,7 @@ void mmc_start_host(struct mmc_host *host)
>                 mmc_power_off(host);
>         else
>                 mmc_power_up(host);
> -       mmc_detect_change(host, 0);
> +       _mmc_detect_change(host, 0, false);
>  }
>
>  void mmc_stop_host(struct mmc_host *host)
> @@ -2724,7 +2739,7 @@ int mmc_pm_notify(struct notifier_block *notify_block,
>                 spin_lock_irqsave(&host->lock, flags);
>                 host->rescan_disable = 0;
>                 spin_unlock_irqrestore(&host->lock, flags);
> -               mmc_detect_change(host, 0);
> +               _mmc_detect_change(host, 0, false);
>
>         }
>
> --
> 1.7.9.5
>