On 17 June 2013 20:33, Colin Cross <ccross@xxxxxxxxxxx> wrote: > On Mon, Jun 17, 2013 at 7:22 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: >> On 14 June 2013 22:52, Colin Cross <ccross@xxxxxxxxxxx> wrote: >>> On Fri, Jun 14, 2013 at 11:42 AM, Zoran Markovic >>> <zoran.markovic@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
