Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx] > Sent: Thursday, January 22, 2015 6:17 AM > > On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote: > > This patch moves transaction wakeup code into advance_transaction(). > > No functional changes. > > > > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > > --- > > drivers/acpi/ec.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c > > index 1b5853f..3e19123 100644 > > --- a/drivers/acpi/ec.c > > +++ b/drivers/acpi/ec.c > > @@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec) > > return ret; > > } > > > > -static bool advance_transaction(struct acpi_ec *ec) > > +static void advance_transaction(struct acpi_ec *ec) > > { > > struct transaction *t; > > u8 status; > > @@ -235,7 +235,7 @@ static bool advance_transaction(struct acpi_ec *ec) > > t->flags |= ACPI_EC_COMMAND_COMPLETE; > > wakeup = true; > > } > > - return wakeup; > > + goto out; > > } else { > > if (EC_FLAGS_QUERY_HANDSHAKE && > > !(status & ACPI_EC_FLAG_SCI) && > > @@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec) > > t->flags |= ACPI_EC_COMMAND_POLL; > > } else > > goto err; > > - return wakeup; > > + goto out; > > } > > err: > > /* > > @@ -262,14 +262,16 @@ err: > > if (in_interrupt() && t) > > ++t->irq_count; > > } > > - return wakeup; > > +out: > > + if (wakeup && in_interrupt()) > > + wake_up(&ec->wait); > > } > > > > static void start_transaction(struct acpi_ec *ec) > > { > > ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0; > > ec->curr->flags = 0; > > - (void)advance_transaction(ec); > > + advance_transaction(ec); > > Well, this looks like a functional change, because we wouldn't call > wake_up(&ec->wait) here before. Ah, Yes. But here, since the only advancement that can happen here is to send the EC command and there are always further advancements until transaction completion, the wake_up() won't be invoked at this point. So IMO, there is no functional changes here. > > > } > > > > static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data); > > @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec) > > return 0; > > } > > spin_lock_irqsave(&ec->lock, flags); > > - (void)advance_transaction(ec); > > + advance_transaction(ec); > > Ditto. Yes. I changed this logic. By invoking wake_up() here, we can break the ec_poll() loop. Because if the transaction has completed due to the polling advancement, we really don't need to wait for another interrupt. So this new logic makes this patch more like a race fix, not a simple cleanup. And IMO, it might also be a stable material. I didn't notice this race bug because there is no bug report against this. Should I change the description around this and re-send the patch? Thanks and best regards -Lv > > Or am I missing anything? > > > spin_unlock_irqrestore(&ec->lock, flags); > > } while (time_before(jiffies, delay)); > > pr_debug("controller reset, restart transaction\n"); > > @@ -688,8 +690,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, > > struct acpi_ec *ec = data; > > > > spin_lock_irqsave(&ec->lock, flags); > > - if (advance_transaction(ec)) > > - wake_up(&ec->wait); > > + advance_transaction(ec); > > spin_unlock_irqrestore(&ec->lock, flags); > > ec_check_sci(ec, acpi_ec_read_status(ec)); > > return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE; > > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
