Hi Alex,
On Wednesday, 10 of September 2008, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> >
> > >ACPI: EC: do transaction from interrupt context
> >
> > In the above patch there exist the following problems:
> > a. In the following code the GPE_STORM bit will always be set regardless
> > of whether there exists the EC GPE storm.
> > >if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
> > > /* check if we received SCI during transaction */
> > > ec_check_sci(ec, acpi_ec_read_status(ec));
> > > /* it is safe to enable GPE outside of transaction */
> > > acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> > > } else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
> > > atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD)
> > > pr_debug(PREFIX "GPE storm detected\n");
> > > set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
> >
> This problem is fixed two days ago, it is not completely fair to mention
> it here.
> > b. When EC GPE storm is detected, the EC GPE will be disabled and
> > EC will work in polling mode while doing EC transaction. In such case
> > maybe EC transaction is not finished. But when timeout happens, it
> > will still be regarded as finishing the EC transaction.
> >
> Ok. Fixed, now -ETIME is returned in this case.
> > This error is related with the following source code:
> > > {
> > > delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
> > //Maybe the preemptible schedule happens here.If the current jiffies
> > is greater than the predefined jiffies after the process is returned
> > from preempt_schedule, OS will have no opportunity to do the EC transaction.
> > It means that EC transaction is not finished. But it is still regarded as
> > finishing EC transaction. It is incorrect.
> > > while (time_before(jiffies, delay)) {
> > > gpe_transaction(ec, acpi_ec_read_status(ec));
> > > msleep(1);
> > //as msleep is realized by using the function of schedule_timeout,
> > maybe the current jiffies is already greater than the predefined jiffies
> > before finishing EC transaction. In such case the EC transaction is
> > not finished. But it is also regarded as finishing. It is incorrect.
> > > if (ec_transaction_done(ec))
> > > goto end;
> > > }
> >
> ec_transaction_done() returns true only if there is finished transaction
> stored in
> acpi_ec. It does not matter if it is late to check for this result from
> timeout point of view.
> the same ideology was put into driver before this patch:
> "ACPI: Avoid bogus EC timeout when EC is in Polling mode"
> > c. There is no detailed change log although a lot is changed.(
> > Including the EC work mode, the detect mechanism of EC GPE storm).
> > It is not easy to understand.
> >
> > At the same time this commit can't handle the EC notification
> > event in the following case:
> > EC GPE Interrupt storm is detected and EC GPE will be disabled
> > when doing EC transaction. If EC notification event happens while doing
> > EC transaction(EC GPE is disabled), the SCI_EVT bit of EC status
> > register is cleared before OS issuing query command. In such case the EC
> > notification event will be lost.
> >
> As I said already, there is no way EC will clear SCI bit before driver
> issues
> the Query command to it (thus reading the event).
> Even more, this same behavior was in the driver for ages and it did
> not broke once.
> > Based on the above analysis ,IMO the two patches are not appropriate.
> >
> >
> This is the best analysis I've seen so far :) Thanks, it is really
> entertaining.
> Please use latest version next time, it is attached for your convenience.
Hm, the attached patch conflicts with
acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch currently in
-mm. Should we Andrew to drop that patch?
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
