LinuxLists.cc - [RFC PATCH] mmc: Enable wakeup

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 13 June 2013 19:56, Zoran Markovic <[email protected]> wrote:
> This is a reworked implementation of wakelocks for the MMC core from
> Android kernel, originally authored by Colin Cross and San Mehat.
> The patch makes sure that whenever a MMC device is inserted/removed,
> the system stays awake until it's reconfigured for the new state.
> It is assumed that 1/2 second is sufficient for the system to start
> the configuration action for the newly detected MMC device, which might
> include e.g. mounting the hosted file system(s).
>
> The implementation uses wakeup_sources instead of wake_locks.
>
> Feedback on the approach is greatly appreciated, in particular for the
> 1/2 second extension peroid.

Hi Zoran,

I am not sure I understand why this patch is needed. When a new card
is inserted/removed and the upper levels gets notification about the
new card, triggering the mounting/un-mounting of the file system, why
should it be the lowest layer (mmc) that prevents the platform from
enter suspend/sleep? Why do we need to prevent it at all?

Note that notifier handling in mmc_pm_notify, was if I remember
correctly, not completely developed when the original version of this
patch was being discussed. mmc_pm_notify prevents cards from being
inserted/removed in the middle of suspend->resume sequence, is that
not enough?

Kind regards
Ulf Hansson

>
> Cc: San Mehat <[email protected]>
> Cc: Colin Cross <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Chris Ball <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Johan Rudholm <[email protected]>
> Cc: Jaehoon Chung <[email protected]>
> Cc: Konstantin Dorfman <[email protected]>
> Cc: Guennadi Liakhovetski <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> [<[email protected]>: tweaked commit message, reworked to use
> wakeup_source_register/unregister instead of wakeup_source_init/trash,
> added the missing __pm_relax() for non-removable devices in mmc_rescan().]
> Signed-off-by: Zoran Markovic <[email protected]>
> ---
> drivers/mmc/core/core.c | 31 +++++++++++++++++++++++++------
> drivers/mmc/core/host.c | 7 +++++++
> include/linux/mmc/host.h | 2 ++
> 3 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c40396f..d5230c7 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>
> @@ -1656,6 +1657,7 @@ void mmc_detect_change(struct mmc_host *host, unsigned long delay)
> spin_unlock_irqrestore(&host->lock, flags);
> #endif
> host->detect_change = 1;
> + __pm_stay_awake(host->ws);
> mmc_schedule_delayed_work(&host->detect, delay);
> }
>
> @@ -2351,13 +2353,16 @@ void mmc_rescan(struct work_struct *work)
> struct mmc_host *host =
> container_of(work, struct mmc_host, detect.work);
> int i;
> + bool extend_wakeup = false;
>
> if (host->rescan_disable)
> return;
>
> /* If there is a non-removable card registered, only scan once */
> - if ((host->caps & MMC_CAP_NONREMOVABLE) && host->rescan_entered)
> + if ((host->caps & MMC_CAP_NONREMOVABLE) && host->rescan_entered) {
> + __pm_relax(host->ws);
> return;
> + }
> host->rescan_entered = 1;
>
> mmc_bus_get(host);
> @@ -2400,16 +2405,27 @@ void mmc_rescan(struct work_struct *work)
>
> mmc_claim_host(host);
> for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> - if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> + if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min))) {
> + /* stay awake extra time to process detected device */
> + extend_wakeup = true;
> break;
> + }
> if (freqs[i] <= host->f_min)
> break;
> }
> mmc_release_host(host);
>
> out:
> - if (host->caps & MMC_CAP_NEEDS_POLL)
> + if (extend_wakeup)
> + /* extra 1/2 second should be enough, hopefully */
> + __pm_wakeup_event(host->ws, MSEC_PER_SEC/2);
> + else
> + __pm_relax(host->ws);
> +
> + if (host->caps & MMC_CAP_NEEDS_POLL) {
> + __pm_stay_awake(host->ws);
> mmc_schedule_delayed_work(&host->detect, HZ);
> + }
> }
>
> void mmc_start_host(struct mmc_host *host)
> @@ -2433,7 +2449,8 @@ void mmc_stop_host(struct mmc_host *host)
> #endif
>
> host->rescan_disable = 1;
> - cancel_delayed_work_sync(&host->detect);
> + if (cancel_delayed_work_sync(&host->detect))
> + __pm_relax(host->ws);
> mmc_flush_scheduled_work();
>
> /* clear pm flags now and let card drivers set them as needed */
> @@ -2628,7 +2645,8 @@ int mmc_suspend_host(struct mmc_host *host)
> {
> int err = 0;
>
> - cancel_delayed_work(&host->detect);
> + if (cancel_delayed_work(&host->detect))
> + __pm_relax(host->ws);
> mmc_flush_scheduled_work();
>
> mmc_bus_get(host);
> @@ -2741,7 +2759,8 @@ int mmc_pm_notify(struct notifier_block *notify_block,
> spin_lock_irqsave(&host->lock, flags);
> host->rescan_disable = 1;
> spin_unlock_irqrestore(&host->lock, flags);
> - cancel_delayed_work_sync(&host->detect);
> + if (cancel_delayed_work_sync(&host->detect))
> + __pm_relax(host->ws);
>
> if (!host->bus_ops || host->bus_ops->suspend)
> break;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 2a3593d..3cbb3d7 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -452,6 +452,11 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
> host->class_dev.class = &mmc_host_class;
> device_initialize(&host->class_dev);
>
> + host->ws = wakeup_source_register(
> + kasprintf(GFP_KERNEL, "%s_detect", mmc_hostname(host)));
> + if (!host->ws)
> + goto free;
> +
> mmc_host_clk_init(host);
>
> mutex_init(&host->slot.lock);
> @@ -555,6 +560,8 @@ void mmc_free_host(struct mmc_host *host)
> spin_lock(&mmc_host_lock);
> idr_remove(&mmc_host_idr, host->index);
> spin_unlock(&mmc_host_lock);
> + wakeup_source_unregister(host->ws);
> + host->ws = NULL;
>
> put_device(&host->class_dev);
> }
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index e326ae2..9dc2dd6 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -15,6 +15,7 @@
> #include <linux/sched.h>
> #include <linux/device.h>
> #include <linux/fault-inject.h>
> +#include <linux/pm_wakeup.h>
>
> #include <linux/mmc/core.h>
> #include <linux/mmc/pm.h>
> @@ -329,6 +330,7 @@ struct mmc_host {
> int claim_cnt; /* "claim" nesting count */
>
> struct delayed_work detect;
> + struct wakeup_source *ws;
> int detect_change; /* card detect flag */
> struct mmc_slot slot;
>
> --
> 1.7.9.5
>

2013-06-14 18:42:36

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

> I am not sure I understand why this patch is needed. When a new card
> is inserted/removed and the upper levels gets notification about the
> new card, triggering the mounting/un-mounting of the file system, why
> should it be the lowest layer (mmc) that prevents the platform from
> enter suspend/sleep? Why do we need to prevent it at all?
>
> Note that notifier handling in mmc_pm_notify, was if I remember
> correctly, not completely developed when the original version of this
> patch was being discussed. mmc_pm_notify prevents cards from being
> inserted/removed in the middle of suspend->resume sequence, is that
> not enough?

I will try to speak on behalf of the original implementers in a hope
they would provide the original motivation for the patch.

My understanding is that any preemption in the procedure could be an
opportunity to suspend, as there may be a suspend request racing with
this code. This is why the calls to __pm_stay_awake() and
queue_delayed_work() are so tightly coupled. It would be up to the
delayed work procedure (mmc_rescan()) to decide whether or not it is
safe to suspend. If there are no changes in the MMC state or all
changes can be handled by mmc_rescan(), it is safe to call
__pm_relax(). Otherwise, userland may take over processing of this
event, and this is why the awake state needs to be extended by 1/2
second.

Regards, Zoran

2013-06-14 20:52:31

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On Fri, Jun 14, 2013 at 11:42 AM, Zoran Markovic
<[email protected]> wrote:
>> I am not sure I understand why this patch is needed. When a new card
>> is inserted/removed and the upper levels gets notification about the
>> new card, triggering the mounting/un-mounting of the file system, why
>> should it be the lowest layer (mmc) that prevents the platform from
>> enter suspend/sleep? Why do we need to prevent it at all?
>>
>> Note that notifier handling in mmc_pm_notify, was if I remember
>> correctly, not completely developed when the original version of this
>> patch was being discussed. mmc_pm_notify prevents cards from being
>> inserted/removed in the middle of suspend->resume sequence, is that
>> not enough?
>
> I will try to speak on behalf of the original implementers in a hope
> they would provide the original motivation for the patch.
>
> My understanding is that any preemption in the procedure could be an
> opportunity to suspend, as there may be a suspend request racing with
> this code. This is why the calls to __pm_stay_awake() and
> queue_delayed_work() are so tightly coupled. It would be up to the
> delayed work procedure (mmc_rescan()) to decide whether or not it is
> safe to suspend. If there are no changes in the MMC state or all
> changes can be handled by mmc_rescan(), it is safe to call
> __pm_relax(). Otherwise, userland may take over processing of this
> event, and this is why the awake state needs to be extended by 1/2
> second.

The __pm_stay_awake() is required to prevent autosleep during the time
between the card detect interrupt and when the userspace process that
gets the notification runs. The 1/2 second delay is used because it
is easier than trying to detect when the userspace process has
received the notification, at which time it should hold its own
wakelock and the mmc subsystem can call __pm_relax().

2013-06-17 14:22:31

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 14 June 2013 22:52, Colin Cross <[email protected]> wrote:
> On Fri, Jun 14, 2013 at 11:42 AM, Zoran Markovic
> <[email protected]> wrote:
>>> I am not sure I understand why this patch is needed. When a new card
>>> is inserted/removed and the upper levels gets notification about the
>>> new card, triggering the mounting/un-mounting of the file system, why
>>> should it be the lowest layer (mmc) that prevents the platform from
>>> enter suspend/sleep? Why do we need to prevent it at all?
>>>
>>> Note that notifier handling in mmc_pm_notify, was if I remember
>>> correctly, not completely developed when the original version of this
>>> patch was being discussed. mmc_pm_notify prevents cards from being
>>> inserted/removed in the middle of suspend->resume sequence, is that
>>> not enough?
>>
>> I will try to speak on behalf of the original implementers in a hope
>> they would provide the original motivation for the patch.
>>
>> My understanding is that any preemption in the procedure could be an
>> opportunity to suspend, as there may be a suspend request racing with
>> this code. This is why the calls to __pm_stay_awake() and
>> queue_delayed_work() are so tightly coupled. It would be up to the
>> delayed work procedure (mmc_rescan()) to decide whether or not it is
>> safe to suspend. If there are no changes in the MMC state or all
>> changes can be handled by mmc_rescan(), it is safe to call
>> __pm_relax(). Otherwise, userland may take over processing of this
>> event, and this is why the awake state needs to be extended by 1/2
>> second.
>
> The __pm_stay_awake() is required to prevent autosleep during the time
> between the card detect interrupt and when the userspace process that
> gets the notification runs. The 1/2 second delay is used because it
> is easier than trying to detect when the userspace process has
> received the notification, at which time it should hold its own
> wakelock and the mmc subsystem can call __pm_relax().

Hi Colin,

I don't have the in-depth knowledge about how the userspace deamons
handles the event notifications, so please bare with me while I am
trying to understand more here.

First of all, are we trying to solve an issue here or just improving
some specific situation, that is not clear to me.

I might have misunderstood this patch, but it seems like your concern
is that you believe the event notification can get lost - if userspace
are about to trigger a suspend while a card is being inserted/removed.
If that is the case, could you elaborate on what level the
notification can get lost?

Kind regards
Ulf Hansson

2013-06-17 18:33:51

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On Mon, Jun 17, 2013 at 7:22 AM, Ulf Hansson <[email protected]> wrote:
> On 14 June 2013 22:52, Colin Cross <[email protected]> wrote:
>> On Fri, Jun 14, 2013 at 11:42 AM, Zoran Markovic
>> <[email protected]> wrote:
>>>> I am not sure I understand why this patch is needed. When a new card
>>>> is inserted/removed and the upper levels gets notification about the
>>>> new card, triggering the mounting/un-mounting of the file system, why
>>>> should it be the lowest layer (mmc) that prevents the platform from
>>>> enter suspend/sleep? Why do we need to prevent it at all?
>>>>
>>>> Note that notifier handling in mmc_pm_notify, was if I remember
>>>> correctly, not completely developed when the original version of this
>>>> patch was being discussed. mmc_pm_notify prevents cards from being
>>>> inserted/removed in the middle of suspend->resume sequence, is that
>>>> not enough?
>>>
>>> I will try to speak on behalf of the original implementers in a hope
>>> they would provide the original motivation for the patch.
>>>
>>> My understanding is that any preemption in the procedure could be an
>>> opportunity to suspend, as there may be a suspend request racing with
>>> this code. This is why the calls to __pm_stay_awake() and
>>> queue_delayed_work() are so tightly coupled. It would be up to the
>>> delayed work procedure (mmc_rescan()) to decide whether or not it is
>>> safe to suspend. If there are no changes in the MMC state or all
>>> changes can be handled by mmc_rescan(), it is safe to call
>>> __pm_relax(). Otherwise, userland may take over processing of this
>>> event, and this is why the awake state needs to be extended by 1/2
>>> second.
>>
>> The __pm_stay_awake() is required to prevent autosleep during the time
>> between the card detect interrupt and when the userspace process that
>> gets the notification runs. The 1/2 second delay is used because it
>> is easier than trying to detect when the userspace process has
>> received the notification, at which time it should hold its own
>> wakelock and the mmc subsystem can call __pm_relax().
>
> Hi Colin,
>
> I don't have the in-depth knowledge about how the userspace deamons
> handles the event notifications, so please bare with me while I am
> trying to understand more here.
>
> First of all, are we trying to solve an issue here or just improving
> some specific situation, that is not clear to me.
>
> I might have misunderstood this patch, but it seems like your concern
> is that you believe the event notification can get lost - if userspace
> are about to trigger a suspend while a card is being inserted/removed.
> If that is the case, could you elaborate on what level the
> notification can get lost?
>
> Kind regards
> Ulf Hansson

This is a generic requirement for using a kernel with autosleep
enabled. Autosleep will enter suspend whenever there is no wakeup
source/wakelock held. Consider the following sequence:

Kernel is suspended
Card is inserted, triggering a wakeup interrupt, which is an implicit
wakeup source until it is handled
Kernel starts resuming, resumes the mmc driver
The mmc driver enables its interrupt, which is immediately handled and
queues an event to be handled by userspace
At this point the wakeup interrupt is handled and gone, and no wakeup
sources are being held, so the kernel can choose to go back to
suspend, so userspace can't handle the insertion event until the
kernel wakes up for another reason.

In general, an event that is triggered by a wakeup interrupt that is
being passed from the kernel to userspace needs to have a wakeup
source held while the event is queued.

2013-06-18 13:17:16

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 17 June 2013 20:33, Colin Cross <[email protected]> wrote:
> On Mon, Jun 17, 2013 at 7:22 AM, Ulf Hansson <[email protected]> wrote:
>> On 14 June 2013 22:52, Colin Cross <[email protected]> wrote:
>>> On Fri, Jun 14, 2013 at 11:42 AM, Zoran Markovic
>>> <[email protected]> wrote:
>>>>> I am not sure I understand why this patch is needed. When a new card
>>>>> is inserted/removed and the upper levels gets notification about the
>>>>> new card, triggering the mounting/un-mounting of the file system, why
>>>>> should it be the lowest layer (mmc) that prevents the platform from
>>>>> enter suspend/sleep? Why do we need to prevent it at all?
>>>>>
>>>>> Note that notifier handling in mmc_pm_notify, was if I remember
>>>>> correctly, not completely developed when the original version of this
>>>>> patch was being discussed. mmc_pm_notify prevents cards from being
>>>>> inserted/removed in the middle of suspend->resume sequence, is that
>>>>> not enough?
>>>>
>>>> I will try to speak on behalf of the original implementers in a hope
>>>> they would provide the original motivation for the patch.
>>>>
>>>> My understanding is that any preemption in the procedure could be an
>>>> opportunity to suspend, as there may be a suspend request racing with
>>>> this code. This is why the calls to __pm_stay_awake() and
>>>> queue_delayed_work() are so tightly coupled. It would be up to the
>>>> delayed work procedure (mmc_rescan()) to decide whether or not it is
>>>> safe to suspend. If there are no changes in the MMC state or all
>>>> changes can be handled by mmc_rescan(), it is safe to call
>>>> __pm_relax(). Otherwise, userland may take over processing of this
>>>> event, and this is why the awake state needs to be extended by 1/2
>>>> second.
>>>
>>> The __pm_stay_awake() is required to prevent autosleep during the time
>>> between the card detect interrupt and when the userspace process that
>>> gets the notification runs. The 1/2 second delay is used because it
>>> is easier than trying to detect when the userspace process has
>>> received the notification, at which time it should hold its own
>>> wakelock and the mmc subsystem can call __pm_relax().
>>
>> Hi Colin,
>>
>> I don't have the in-depth knowledge about how the userspace deamons
>> handles the event notifications, so please bare with me while I am
>> trying to understand more here.
>>
>> First of all, are we trying to solve an issue here or just improving
>> some specific situation, that is not clear to me.
>>
>> I might have misunderstood this patch, but it seems like your concern
>> is that you believe the event notification can get lost - if userspace
>> are about to trigger a suspend while a card is being inserted/removed.
>> If that is the case, could you elaborate on what level the
>> notification can get lost?
>>
>> Kind regards
>> Ulf Hansson
>
> This is a generic requirement for using a kernel with autosleep
> enabled. Autosleep will enter suspend whenever there is no wakeup
> source/wakelock held. Consider the following sequence:
>
> Kernel is suspended
> Card is inserted, triggering a wakeup interrupt, which is an implicit
> wakeup source until it is handled

I don't think a card insert/remove irq need to be configured as a
wakeup interrupt. As you say, it will force a resume to detect the
card, but for what reason?
Instead, I think it it better to leave the card detection to be
handled at the next resume, thus not resuming the system when not
needed.

> Kernel starts resuming, resumes the mmc driver
> The mmc driver enables its interrupt, which is immediately handled and
> queues an event to be handled by userspace
> At this point the wakeup interrupt is handled and gone, and no wakeup
> sources are being held, so the kernel can choose to go back to
> suspend, so userspace can't handle the insertion event until the
> kernel wakes up for another reason.

Is this a problem? From my point of view it should be perfectly
acceptable to let userspace handle the event at the next resume/wakeup
instead. Don't you think so?

>
> In general, an event that is triggered by a wakeup interrupt that is
> being passed from the kernel to userspace needs to have a wakeup
> source held while the event is queued.

That's sounds reasonable. Would it then make sense to hold a generic
wakeup source in the "suspend/resume core", once a wakeup interrupt is
fetched?

Kind regards
Ulf Hansson

2013-06-18 17:15:34

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On Tue, Jun 18, 2013 at 6:17 AM, Ulf Hansson <[email protected]> wrote:
> On 17 June 2013 20:33, Colin Cross <[email protected]> wrote:
>> This is a generic requirement for using a kernel with autosleep
>> enabled. Autosleep will enter suspend whenever there is no wakeup
>> source/wakelock held. Consider the following sequence:
>>
>> Kernel is suspended
>> Card is inserted, triggering a wakeup interrupt, which is an implicit
>> wakeup source until it is handled
>
> I don't think a card insert/remove irq need to be configured as a
> wakeup interrupt. As you say, it will force a resume to detect the
> card, but for what reason?
> Instead, I think it it better to leave the card detection to be
> handled at the next resume, thus not resuming the system when not
> needed.

That decision is going to be different on each device. On Android
devices it has been configured as a wakeup interrupt. This patch is
necessary on devices that want to handle the event as a wakeup event,
and has negligible impact on those that do not.

>> Kernel starts resuming, resumes the mmc driver
>> The mmc driver enables its interrupt, which is immediately handled and
>> queues an event to be handled by userspace
>> At this point the wakeup interrupt is handled and gone, and no wakeup
>> sources are being held, so the kernel can choose to go back to
>> suspend, so userspace can't handle the insertion event until the
>> kernel wakes up for another reason.
>
> Is this a problem? From my point of view it should be perfectly
> acceptable to let userspace handle the event at the next resume/wakeup
> instead. Don't you think so?

Depends what userspace is doing. If userspace would like to provide
the user some feedback on the event, then it has to be a wakeup
interrupt, and it has to hold a wakelock until it has passed the event
to userspace.

>>
>> In general, an event that is triggered by a wakeup interrupt that is
>> being passed from the kernel to userspace needs to have a wakeup
>> source held while the event is queued.
>
> That's sounds reasonable. Would it then make sense to hold a generic
> wakeup source in the "suspend/resume core", once a wakeup interrupt is
> fetched?

No, the suspend/resume core can only hold a wakeup source until it has
handed the event off to the driver, at which point it is the driver's
responsibility to hold a wakeup source. The suspend/resume core
cannot tell if the event was handled by the driver or will be passed
to userspace.

2013-06-19 14:21:31

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 18 June 2013 19:15, Colin Cross <[email protected]> wrote:
> On Tue, Jun 18, 2013 at 6:17 AM, Ulf Hansson <[email protected]> wrote:
>> On 17 June 2013 20:33, Colin Cross <[email protected]> wrote:
>>> This is a generic requirement for using a kernel with autosleep
>>> enabled. Autosleep will enter suspend whenever there is no wakeup
>>> source/wakelock held. Consider the following sequence:
>>>
>>> Kernel is suspended
>>> Card is inserted, triggering a wakeup interrupt, which is an implicit
>>> wakeup source until it is handled
>>
>> I don't think a card insert/remove irq need to be configured as a
>> wakeup interrupt. As you say, it will force a resume to detect the
>> card, but for what reason?
>> Instead, I think it it better to leave the card detection to be
>> handled at the next resume, thus not resuming the system when not
>> needed.
>
> That decision is going to be different on each device. On Android
> devices it has been configured as a wakeup interrupt. This patch is
> necessary on devices that want to handle the event as a wakeup event,
> and has negligible impact on those that do not.
>
>>> Kernel starts resuming, resumes the mmc driver
>>> The mmc driver enables its interrupt, which is immediately handled and
>>> queues an event to be handled by userspace
>>> At this point the wakeup interrupt is handled and gone, and no wakeup
>>> sources are being held, so the kernel can choose to go back to
>>> suspend, so userspace can't handle the insertion event until the
>>> kernel wakes up for another reason.
>>
>> Is this a problem? From my point of view it should be perfectly
>> acceptable to let userspace handle the event at the next resume/wakeup
>> instead. Don't you think so?
>
> Depends what userspace is doing. If userspace would like to provide
> the user some feedback on the event, then it has to be a wakeup
> interrupt, and it has to hold a wakelock until it has passed the event
> to userspace.

It seems like a bad idea that an insertion of an SD card should
trigger the display to be light up. That is indirectly in principle
what you suggest should happen from user space once a new SD card is
found. Right?

I have been working with Android for several years, we never used this
kind of setup. Instead we wait for the user to press the "display on"
button. At that time the confirmation will be received. Not saying
that this is the only way of doing it, but it seems to be an accepted
solution for all our customers.

I agree to that this patch should have negligible impact though - if
we get things right. I will try to review the code in more detail
soon.

Kind regards
Ulf Hansson

>
>>>
>>> In general, an event that is triggered by a wakeup interrupt that is
>>> being passed from the kernel to userspace needs to have a wakeup
>>> source held while the event is queued.
>>
>> That's sounds reasonable. Would it then make sense to hold a generic
>> wakeup source in the "suspend/resume core", once a wakeup interrupt is
>> fetched?
>
> No, the suspend/resume core can only hold a wakeup source until it has
> handed the event off to the driver, at which point it is the driver's
> responsibility to hold a wakeup source. The suspend/resume core
> cannot tell if the event was handled by the driver or will be passed
> to userspace.

2013-06-19 17:29:09

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On Wed, Jun 19, 2013 at 7:21 AM, Ulf Hansson <[email protected]> wrote:
> It seems like a bad idea that an insertion of an SD card should
> trigger the display to be light up. That is indirectly in principle
> what you suggest should happen from user space once a new SD card is
> found. Right?

Most likely what will happen is the system will mount the sdcard, and
if necessary start the media scanner so that the user can see their
media on the sd card when they turn the screen on. But that is mostly
irrelevant, the point is that the event needs to be passed to
userspace to allow it to make the decision in a timely fashion.

> I have been working with Android for several years, we never used this
> kind of setup. Instead we wait for the user to press the "display on"
> button. At that time the confirmation will be received. Not saying
> that this is the only way of doing it, but it seems to be an accepted
> solution for all our customers.

This patch is ported from the Android common tree, so you've probably
been using it.

> I agree to that this patch should have negligible impact though - if
> we get things right. I will try to review the code in more detail
> soon.
>
> Kind regards
> Ulf Hansson
>

2013-06-23 09:28:09

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 19 June 2013 19:29, Colin Cross <[email protected]> wrote:
> On Wed, Jun 19, 2013 at 7:21 AM, Ulf Hansson <[email protected]> wrote:
>> It seems like a bad idea that an insertion of an SD card should
>> trigger the display to be light up. That is indirectly in principle
>> what you suggest should happen from user space once a new SD card is
>> found. Right?
>
> Most likely what will happen is the system will mount the sdcard, and
> if necessary start the media scanner so that the user can see their
> media on the sd card when they turn the screen on. But that is mostly
> irrelevant, the point is that the event needs to be passed to
> userspace to allow it to make the decision in a timely fashion.
>
>> I have been working with Android for several years, we never used this
>> kind of setup. Instead we wait for the user to press the "display on"
>> button. At that time the confirmation will be received. Not saying
>> that this is the only way of doing it, but it seems to be an accepted
>> solution for all our customers.
>
> This patch is ported from the Android common tree, so you've probably
> been using it.

We removed more or less all Android code in the mmc subsystem, since
it just didn't work. :-)

The "deferred resume" was very useful though, so after some rework we
kept it and could then improve the system resume time significantly.

>
>> I agree to that this patch should have negligible impact though - if
>> we get things right. I will try to review the code in more detail
>> soon.
>>
>> Kind regards
>> Ulf Hansson
>>

2013-06-24 19:58:42

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

>> This patch is ported from the Android common tree, so you've probably
>> been using it.
>
> We removed more or less all Android code in the mmc subsystem, since
> it just didn't work. :-)
>
> The "deferred resume" was very useful though, so after some rework we
> kept it and could then improve the system resume time significantly.

For what it's worth, I fixed one bug I noticed in the Android kernel:
if a system has a non-removable MMC device, a suspend/resume cycle on
this device would hold a wake lock forever. This was a visible issue
on the panda board I am using.

If there are doubts on whether or not the system should stay awake
during a MMC mount, we have the option to make the calls to
wakeup_source_register/unregister configurable. Skipping these calls
would leave the .ws field NULL, in which case
__pm_stay_awake/__pm_relax/__pm_wakeup_event would do nothing.

Thoughts?

- Zoran

2013-06-26 20:57:16

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 24 June 2013 21:58, Zoran Markovic <[email protected]> wrote:
>>> This patch is ported from the Android common tree, so you've probably
>>> been using it.
>>
>> We removed more or less all Android code in the mmc subsystem, since
>> it just didn't work. :-)
>>
>> The "deferred resume" was very useful though, so after some rework we
>> kept it and could then improve the system resume time significantly.
>
> For what it's worth, I fixed one bug I noticed in the Android kernel:
> if a system has a non-removable MMC device, a suspend/resume cycle on
> this device would hold a wake lock forever. This was a visible issue
> on the panda board I am using.
>
> If there are doubts on whether or not the system should stay awake
> during a MMC mount, we have the option to make the calls to
> wakeup_source_register/unregister configurable. Skipping these calls
> would leave the .ws field NULL, in which case
> __pm_stay_awake/__pm_relax/__pm_wakeup_event would do nothing.

Even if we make this feature configurable, I can't see any host driver
that would benefit from it as of today. The reason is simply that host
drivers do not configure it's card detect irq as a wakeup irq. Myself
is also having quite hard to see the benefit of doing that, but I
don't know all the use cases.

Let's see if we can get someone else to provide input...

>
> Thoughts?
>
> - Zoran

Kind regards
Ulf Hansson

2013-08-01 07:42:32

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

Ulf,
I got confirmation from Broadcom that all cell phone reference designs
have card insert/removal configured as a wakeup IRQ. Unless our
customers change that - which I doubt - this results in a considerable
number of products implementing this feature.

Please let me know how you wish to proceed.

Cheers,
Zoran

On 26 June 2013 13:57, Ulf Hansson <[email protected]> wrote:
> On 24 June 2013 21:58, Zoran Markovic <[email protected]> wrote:
>>>> This patch is ported from the Android common tree, so you've probably
>>>> been using it.
>>>
>>> We removed more or less all Android code in the mmc subsystem, since
>>> it just didn't work. :-)
>>>
>>> The "deferred resume" was very useful though, so after some rework we
>>> kept it and could then improve the system resume time significantly.
>>
>> For what it's worth, I fixed one bug I noticed in the Android kernel:
>> if a system has a non-removable MMC device, a suspend/resume cycle on
>> this device would hold a wake lock forever. This was a visible issue
>> on the panda board I am using.
>>
>> If there are doubts on whether or not the system should stay awake
>> during a MMC mount, we have the option to make the calls to
>> wakeup_source_register/unregister configurable. Skipping these calls
>> would leave the .ws field NULL, in which case
>> __pm_stay_awake/__pm_relax/__pm_wakeup_event would do nothing.
>
> Even if we make this feature configurable, I can't see any host driver
> that would benefit from it as of today. The reason is simply that host
> drivers do not configure it's card detect irq as a wakeup irq. Myself
> is also having quite hard to see the benefit of doing that, but I
> don't know all the use cases.
>
> Let's see if we can get someone else to provide input...
>
>>
>> Thoughts?
>>
>> - Zoran
>
>
> Kind regards
> Ulf Hansson

2013-08-22 17:08:05

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

Ulf,
> I got confirmation from Broadcom that all cell phone reference designs
> have card insert/removal configured as a wakeup IRQ. Unless our
> customers change that - which I doubt - this results in a considerable
> number of products implementing this feature.
>
> Please let me know how you wish to proceed.

I think this patch would be useful for all mobile applications. What
are the chances of getting this in the next kernel version?

Thanks,
Zoran

2013-08-23 07:15:13

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

On 22 August 2013 19:08, Zoran Markovic <[email protected]> wrote:
> Ulf,
>> I got confirmation from Broadcom that all cell phone reference designs
>> have card insert/removal configured as a wakeup IRQ. Unless our
>> customers change that - which I doubt - this results in a considerable
>> number of products implementing this feature.
>>
>> Please let me know how you wish to proceed.
>
> I think this patch would be useful for all mobile applications. What
> are the chances of getting this in the next kernel version?
>

Hi Zoran,

Recently got back from vacation, sorry for the delayed response.

I am about to set up test using this patch, to validate this for a
ux500 Android build. Please give me a few days to provide you with
some results and input.

Kind regards
Ulf Hansson

> Thanks,
> Zoran

2013-08-23 08:12:57

[permalink] [raw]

Subject: Re: [RFC PATCH] mmc: Enable wakeup_sources for mmc core

Hi Zoran,

On 13 June 2013 19:56, Zoran Markovic <[email protected]> wrote:
> This is a reworked implementation of wakelocks for the MMC core from
> Android kernel, originally authored by Colin Cross and San Mehat.
> The patch makes sure that whenever a MMC device is inserted/removed,
> the system stays awake until it's reconfigured for the new state.
> It is assumed that 1/2 second is sufficient for the system to start
> the configuration action for the newly detected MMC device, which might
> include e.g. mounting the hosted file system(s).
>
> The implementation uses wakeup_sources instead of wake_locks.
>
> Feedback on the approach is greatly appreciated, in particular for the
> 1/2 second extension peroid.
>
> Cc: San Mehat <[email protected]>
> Cc: Colin Cross <[email protected]>
> Cc: John Stultz <[email protected]>
> Cc: Chris Ball <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Johan Rudholm <[email protected]>
> Cc: Jaehoon Chung <[email protected]>
> Cc: Konstantin Dorfman <[email protected]>
> Cc: Guennadi Liakhovetski <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> [<[email protected]>: tweaked commit message, reworked to use
> wakeup_source_register/unregister instead of wakeup_source_init/trash,
> added the missing __pm_relax() for non-removable devices in mmc_rescan().]
> Signed-off-by: Zoran Markovic <[email protected]>
> ---
> drivers/mmc/core/core.c | 31 +++++++++++++++++++++++++------
> drivers/mmc/core/host.c | 7 +++++++
> include/linux/mmc/host.h | 2 ++
> 3 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c40396f..d5230c7 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>
> @@ -1656,6 +1657,7 @@ void mmc_detect_change(struct mmc_host *host, unsigned long delay)
> spin_unlock_irqrestore(&host->lock, flags);
> #endif
> host->detect_change = 1;
> + __pm_stay_awake(host->ws);

There are some scenarios I want to be sure you have thought of. Please
comment on them.

1. mmc_detect_change does obviously not have to be run the same number
of times as the mmc_rescan function. In other words, the calls to
__pm_stay_awake is not paired with __pm_relay, I suppose this does not
matter?

2. mmc_detect_change can for example be called while the device
suspend sequence is progressing. At this point the rescan work is
disabled, thus __pm_relax will not be called, until the next rescan
work as executed which is after the complete resume cycle
(mmc_pm_notify:PM_POST_SUSPEND). Is that an issue?

> mmc_schedule_delayed_work(&host->detect, delay);
> }
>
> @@ -2351,13 +2353,16 @@ void mmc_rescan(struct work_struct *work)
> struct mmc_host *host =
> container_of(work, struct mmc_host, detect.work);
> int i;
> + bool extend_wakeup = false;
>
> if (host->rescan_disable)
> return;
>
> /* If there is a non-removable card registered, only scan once */
> - if ((host->caps & MMC_CAP_NONREMOVABLE) && host->rescan_entered)
> + if ((host->caps & MMC_CAP_NONREMOVABLE) && host->rescan_entered) {
> + __pm_relax(host->ws);

By calling __pm_relax here, this indicates to me that is seems like
you might have prevented, even for a very small timeslot, with a
MMC_CAP_NONREMOVABLE card/host from the system to suspend.

For sure, you must not prevent the suspend even for small timeslots,
when MMC_CAP_NONREMOVABLE is set.

> return;
> + }
> host->rescan_entered = 1;
>
> mmc_bus_get(host);
> @@ -2400,16 +2405,27 @@ void mmc_rescan(struct work_struct *work)
>
> mmc_claim_host(host);
> for (i = 0; i < ARRAY_SIZE(freqs); i++) {
> - if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min)))
> + if (!mmc_rescan_try_freq(host, max(freqs[i], host->f_min))) {
> + /* stay awake extra time to process detected device */
> + extend_wakeup = true;
> break;
> + }
> if (freqs[i] <= host->f_min)
> break;
> }
> mmc_release_host(host);
>
> out:
> - if (host->caps & MMC_CAP_NEEDS_POLL)
> + if (extend_wakeup)
> + /* extra 1/2 second should be enough, hopefully */
> + __pm_wakeup_event(host->ws, MSEC_PER_SEC/2);
> + else
> + __pm_relax(host->ws);
> +
> + if (host->caps & MMC_CAP_NEEDS_POLL) {
> + __pm_stay_awake(host->ws);

This does not make sense.

So when using polling mode to detect card insert/remove, you will
prevent suspend forever? Maybe I missed a point somewhere?

> mmc_schedule_delayed_work(&host->detect, HZ);
> + }
> }
>
> void mmc_start_host(struct mmc_host *host)
> @@ -2433,7 +2449,8 @@ void mmc_stop_host(struct mmc_host *host)
> #endif
>
> host->rescan_disable = 1;
> - cancel_delayed_work_sync(&host->detect);
> + if (cancel_delayed_work_sync(&host->detect))
> + __pm_relax(host->ws);
> mmc_flush_scheduled_work();
>
> /* clear pm flags now and let card drivers set them as needed */
> @@ -2628,7 +2645,8 @@ int mmc_suspend_host(struct mmc_host *host)
> {

This function has become deprecated. You need to rebase this patch and
please do not add some new code in here.

> int err = 0;
>
> - cancel_delayed_work(&host->detect);
> + if (cancel_delayed_work(&host->detect))
> + __pm_relax(host->ws);
> mmc_flush_scheduled_work();
>
> mmc_bus_get(host);
> @@ -2741,7 +2759,8 @@ int mmc_pm_notify(struct notifier_block *notify_block,
> spin_lock_irqsave(&host->lock, flags);
> host->rescan_disable = 1;
> spin_unlock_irqrestore(&host->lock, flags);
> - cancel_delayed_work_sync(&host->detect);
> + if (cancel_delayed_work_sync(&host->detect))
> + __pm_relax(host->ws);
>
> if (!host->bus_ops || host->bus_ops->suspend)
> break;
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 2a3593d..3cbb3d7 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -452,6 +452,11 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
> host->class_dev.class = &mmc_host_class;
> device_initialize(&host->class_dev);
>
> + host->ws = wakeup_source_register(
> + kasprintf(GFP_KERNEL, "%s_detect", mmc_hostname(host)));
> + if (!host->ws)
> + goto free;
> +
> mmc_host_clk_init(host);
>
> mutex_init(&host->slot.lock);
> @@ -555,6 +560,8 @@ void mmc_free_host(struct mmc_host *host)
> spin_lock(&mmc_host_lock);
> idr_remove(&mmc_host_idr, host->index);
> spin_unlock(&mmc_host_lock);
> + wakeup_source_unregister(host->ws);
> + host->ws = NULL;
>
> put_device(&host->class_dev);
> }
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index e326ae2..9dc2dd6 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -15,6 +15,7 @@
> #include <linux/sched.h>
> #include <linux/device.h>
> #include <linux/fault-inject.h>
> +#include <linux/pm_wakeup.h>
>
> #include <linux/mmc/core.h>
> #include <linux/mmc/pm.h>
> @@ -329,6 +330,7 @@ struct mmc_host {
> int claim_cnt; /* "claim" nesting count */
>
> struct delayed_work detect;
> + struct wakeup_source *ws;
> int detect_change; /* card detect flag */
> struct mmc_slot slot;
>
> --
> 1.7.9.5
>

Kind regards
Ulf Hansson

2013-09-05 21:54:17