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

[Ulf Hansson]

On 18 June 2013 19:15, Colin Cross <ccross@xxxxxxxxxxx> wrote:
> On Tue, Jun 18, 2013 at 6:17 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>> On 17 June 2013 20:33, Colin Cross <ccross@xxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/