Hi, Rui
> From: Zhang, Rui
> Subject: RE: [RFC PATCH v6 1/3] ACPI / EC: Fix possible driver order issue by
> moving EC event handling
> earlier
>
>
> > From: linux-acpi-ow...@vger.kernel.org [mailto:linux-acpi-
> > Subject: [RFC PATCH v6 1/3] ACPI / EC: Fix possible driver order issue by
> > moving EC event handling earlier
> >
> > This patch tries to detect EC events earlier after resume, so that if an
> > event
> > occurred before invoking acpi_ec_unblock_transactions(), it could be
> > detected by acpi_ec_unblock_transactions() which is the earliest EC driver
> > call after resume.
> >
> > However after the noirq stage, if an event ocurred after
> > acpi_ec_unblock_transactions() and before acpi_ec_resume(), there was no
> > mean to detect and trigger it right then, but can only detect it and handle
> > it
> > after acpi_ec_resume().
> >
> > Now the final logic is:
> > 1. If ec_freeze_events=Y, event handling is stopped in acpi_ec_suspend(),
> > restarted in acpi_ec_resume();
> > 2. If ec_freeze_events=N, event handling is stopped in
> > acpi_ec_block_transactions(), restarted in
> > acpi_ec_unblock_transactions();
> > 3. In order to handling the conflict of the edge-trigger nature of EC IRQ
> > and the Linux noirq stage, advance_transaction() is invoked where the
> > event handling is enabled and the noirq stage is ended.
> >
> > Known issue:
> > 1. Event ocurred between acpi_ec_unblock_transactions() and
> > acpi_ec_resume() may still lead to the order issue. This can only be
> > fixed by adding a periodic detection mechanism during the noirq stage.
> >
> > Signed-off-by: Lv Zheng <lv.zh...@intel.com>
> > Tested-by: Tomislav Ivek <tomislav.i...@gmail.com>
> > Tested-by: Luya Tshimbalanga <l...@fedoraproject.org>
>
> I don't know what issue this patch has been tested for. Lv, can you please
> clarify?
The testers' names are listed here because they have tried the commit and no
regressions can be found on their test platforms.
>
> I agree with lv that it can probably fix some issues brought by the device
> order issue.
> And I'll be glad to push this after we have verified it is really helpful.
> Lv,
> Do you still remember the bug report for the lid issue?
Maybe you should ask Benjamin.
Let me Cc him for further investigation.
Thanks,
Lv
>
> Thanks,
> rui
> > ---
> > drivers/acpi/ec.c | 35 ++++++++++++++++++++++++++---------
> > 1 file changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index df84246..f1f320b
> > 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -249,6 +249,11 @@ static bool acpi_ec_started(struct acpi_ec *ec)
> > !test_bit(EC_FLAGS_STOPPED, &ec->flags); }
> >
> > +static bool acpi_ec_no_sleep_events(void) {
> > + return acpi_sleep_no_ec_events() && ec_freeze_events; }
> > +
> > static bool acpi_ec_event_enabled(struct acpi_ec *ec) {
> > /*
> > @@ -260,14 +265,14 @@ static bool acpi_ec_event_enabled(struct acpi_ec
> > *ec)
> > return false;
> > /*
> > * However, disabling the event handling is experimental for late
> > - * stage (suspend), and is controlled by the boot parameter of
> > - * "ec_freeze_events":
> > + * stage (suspend), and is controlled by
> > + * "acpi_ec_no_sleep_events()":
> > * 1. true: The EC event handling is disabled before entering
> > * the noirq stage.
> > * 2. false: The EC event handling is automatically disabled as
> > * soon as the EC driver is stopped.
> > */
> > - if (ec_freeze_events)
> > + if (acpi_ec_no_sleep_events())
> > return acpi_ec_started(ec);
> > else
> > return test_bit(EC_FLAGS_STARTED, &ec->flags); @@ -524,8
> > +529,8 @@ static bool acpi_ec_query_flushed(struct acpi_ec *ec) static void
> > __acpi_ec_flush_event(struct acpi_ec *ec) {
> > /*
> > - * When ec_freeze_events is true, we need to flush events in
> > - * the proper position before entering the noirq stage.
> > + * When acpi_ec_no_sleep_events() is true, we need to flush events
> > + * in the proper position before entering the noirq stage.
> > */
> > wait_event(ec->wait, acpi_ec_query_flushed(ec));
> > if (ec_query_wq)
> > @@ -948,7 +953,8 @@ static void acpi_ec_start(struct acpi_ec *ec, bool
> > resuming)
> > if (!resuming) {
> > acpi_ec_submit_request(ec);
> > ec_dbg_ref(ec, "Increase driver");
> > - }
> > + } else if (!acpi_ec_no_sleep_events())
> > + __acpi_ec_enable_event(ec);
> > ec_log_drv("EC started");
> > }
> > spin_unlock_irqrestore(&ec->lock, flags); @@ -980,7 +986,7 @@
> > static void acpi_ec_stop(struct acpi_ec *ec, bool suspending)
> > if (!suspending) {
> > acpi_ec_complete_request(ec);
> > ec_dbg_ref(ec, "Decrease driver");
> > - } else if (!ec_freeze_events)
> > + } else if (!acpi_ec_no_sleep_events())
> > __acpi_ec_disable_event(ec);
> > clear_bit(EC_FLAGS_STARTED, &ec->flags);
> > clear_bit(EC_FLAGS_STOPPED, &ec->flags); @@ -1910,7
> > +1916,7 @@ static int acpi_ec_suspend(struct device *dev)
> > struct acpi_ec *ec =
> > acpi_driver_data(to_acpi_device(dev));
> >
> > - if (acpi_sleep_no_ec_events() && ec_freeze_events)
> > + if (acpi_ec_no_sleep_events())
> > acpi_ec_disable_event(ec);
> > return 0;
> > }
> > @@ -1946,7 +1952,18 @@ static int acpi_ec_resume(struct device *dev)
> > struct acpi_ec *ec =
> > acpi_driver_data(to_acpi_device(dev));
> >
> > - acpi_ec_enable_event(ec);
> > + if (acpi_ec_no_sleep_events())
> > + acpi_ec_enable_event(ec);
> > + else {
> > + /*
> > + * Though whether there is an event pending has been
> > + * checked in acpi_ec_unblock_transactions() when
> > + * acpi_ec_no_sleep_events() is false, check it one more
> > + * time after noirq stage to detect events occurred after
> > + * acpi_ec_unblock_transactions().
> > + */
> > + advance_transaction(ec);
> > + }
> > return 0;
> > }
> > #endif
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the
> > body of a message to majord...@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
