On Sunday 07 February 2010, Rafael J. Wysocki wrote: > On Sunday 07 February 2010, Rafael J. Wysocki wrote: > > Hi, > > > > On Wednesday 03 February 2010, Moore, Robert wrote: > > > Matthew, > > > > > > Thanks for your response to my questions. > > > > > > I've been thinking about these interfaces: > > > > > > acpi_ref_runtime_gpe > > > acpi_ref_wakeup_gpe > > > acpi_unref_runtime_gpe > > > acpi_unref_wakeup_gpe > > > > > > While I understand the need for the reference counting mechanism, it is a > > > good idea to support the shared GPEs, I think it may be simpler and cleaner > > > to simply not directly expose this mechanism to GPE users. > > > > Well, it already is exposed to the users and in a way that doesn't look very > > clean to me. Namely, to enable a wakeup GPE for run time, the caller needs to > > set its type to ACPI_GPE_TYPE_WAKE_RUN before calling acpi_enable_gpe(), > > while with the Matthew's interface the only thing the caller would need to do > > would be calling acpi_ref_runtime_gpe(). > > > > > I'm suggesting that we add the reference counting mechanism to the existing > > > AcpiEnableGpe and AcpiDisableGpe interfaces and update their descriptions. > > > We already hide the differences between wake, run, and wake-run GPEs behind > > > these interfaces. > > > > Not really, as I said above. > > > > Moreover, we need two reference counters, because there are cases when a > > GPE should be enabled for wakeup and not enabled for run time and vice versa. > > > > > Adding the reference count semantic to these interfaces changes their > > > behavior in a fairly simple way: > > > > > > Support for shared GPEs: > > > AcpiEnableGpe: For a given GPE, it is actually enabled only on the first call. > > > AcpiDisableGpe: For a given GPE, it is actually disabled only on the last call. > > > > I suppose you mean acpi_enable_gpe() and acpi_disable_gpe(). > > > > That wouldn't work, because sometimes we need to actually hardware-disable > > GPEs and hardware-enable them regardless of the refcount mechanism, like for > > example in the EC suspend and resume routines. > > > > That said, if you are afraid that the new interface may be cumbersome for the > > callers, I think we can introduce just two callbacks, > > > > acpi_get_gpe() > > acpi_put_gpe() > > > > taking 3 arguments each, where the two first arguments are like for the > > Matthew's callbacks and the third argument is a mask of two bits: > > > > ACPI_GPE_TYPE_WAKE > > ACPI_GPE_TYPE_RUNTIME > > > > that will tell the callback whether to use the wakeup or runtime counter for > > reference counting (if called with ACPI_GPE_TYPE_WAKE_RUN, both > > reference counters will be modified at the same time). > > > > Please tell me what you think. > > For easier reference, I reworked the Matthew's patches to implement the above > idea. I noticed that patch [2/3] was actually wrong, because it added the GPE refcounting to drivers/acpi/wakeup.c:acpi_[enable|disable]_wakeup_device(), while in fact it should have added it to drivers/acpi/sleep.c:acpi_pm_device_sleep_wake(). Moreover, patch [3/3] did not really enable the wakeup GPEs after they had been disabled by acpi_disable_all_gpes(). I fixed the two patches and added explanatory comments to patch [1/3]. Updated patches follow, comments welcome. Rafael _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
