Hi, Rafael I'm sorry something is wrong in my previous reply. Let me correct it. > From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx] > Sent: Friday, January 23, 2015 12:01 AM > > On Thursday, January 22, 2015 06:37:28 AM Zheng, Lv wrote: > > 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: <skip> > > > > @@ -262,14 +262,16 @@ err: > > > > if (in_interrupt() && t) > > > > ++t->irq_count; > > > > } > > > > - return wakeup; > > > > +out: > > > > + if (wakeup && in_interrupt()) > > > > + wake_up(&ec->wait); > > > > } > > > > I forgot this diff block. The wake_up() is still invoked for the advance_transaction() invoked in the GPE handler because of in_interrupt() check. For the 2 task context invocations, wake_up() won't be invoked. > > > > 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); > > > > } > > > > > > > > static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data); > > > > > > 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. Thus I didn't change the logic here. > > > > @@ -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. And I didn't change the logic here. > > 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. This statement is also wrong. After checking include/linux/wait.h I realized that we originally broke ec_poll() loop because of ec_transaction_complete() check. See include/linux/wait.h: The condition is checked in __wait_cond_timeout() prior than doing __wait_event_timeout() which may put the task in the wait queue. So if the condition is matched, wait_event_timeout() just returns. For the above 2 advance_transaction() invocations, they are both invoked in the task that might be put into the wait queue. But at this point, the task is already in the run queue, so wake_up() takes no effect to this and wait_event_timeout() can break ec_poll() because of the condition check (ec_transaction_complete()). As a conclusion, in fact, even there is no in_interrupt() check made before wake_up(), the modification can still be a no-op. > > 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. There is no bug because ec_poll() is broken with ec_transaction_complete() check. Thus this is not a bug fix and cannot be a stable candidate. > > Should I change the description around this and re-send the patch? Thus I needn't change the description for this patch. > Yes, please. It seems I needn't to change anything. So it might not be suitable to re-send the patchset because of the above discussion. My bad sorry for the mistake. Could you help to check the rest patches of this revision. Thanks in advance. Best regards -Lv ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
