| Message ID | 22c0b29810135db8762604f216ca8ea1c66fbdf0.1421234254.git.lv.zheng@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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@intel.com> > --- > 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. > } > > 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. 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; >
SGksIFJhZmFlbA0KDQo+IEZyb206IFJhZmFlbCBKLiBXeXNvY2tpIFttYWlsdG86cmp3QHJqd3lz b2NraS5uZXRdDQo+IFNlbnQ6IFRodXJzZGF5LCBKYW51YXJ5IDIyLCAyMDE1IDY6MTcgQU0NCj4g DQo+IE9uIFdlZG5lc2RheSwgSmFudWFyeSAxNCwgMjAxNSAwNzoyODoyMiBQTSBMdiBaaGVuZyB3 cm90ZToNCj4gPiBUaGlzIHBhdGNoIG1vdmVzIHRyYW5zYWN0aW9uIHdha2V1cCBjb2RlIGludG8g YWR2YW5jZV90cmFuc2FjdGlvbigpLg0KPiA+IE5vIGZ1bmN0aW9uYWwgY2hhbmdlcy4NCj4gPg0K PiA+IFNpZ25lZC1vZmYtYnk6IEx2IFpoZW5nIDxsdi56aGVuZ0BpbnRlbC5jb20+DQo+ID4gLS0t DQo+ID4gIGRyaXZlcnMvYWNwaS9lYy5jIHwgICAxNyArKysrKysrKystLS0tLS0tLQ0KPiA+ICAx IGZpbGUgY2hhbmdlZCwgOSBpbnNlcnRpb25zKCspLCA4IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4g ZGlmZiAtLWdpdCBhL2RyaXZlcnMvYWNwaS9lYy5jIGIvZHJpdmVycy9hY3BpL2VjLmMNCj4gPiBp bmRleCAxYjU4NTNmLi4zZTE5MTIzIDEwMDY0NA0KPiA+IC0tLSBhL2RyaXZlcnMvYWNwaS9lYy5j DQo+ID4gKysrIGIvZHJpdmVycy9hY3BpL2VjLmMNCj4gPiBAQCAtMjAwLDcgKzIwMCw3IEBAIHN0 YXRpYyBpbnQgZWNfdHJhbnNhY3Rpb25fY29tcGxldGVkKHN0cnVjdCBhY3BpX2VjICplYykNCj4g PiAgCXJldHVybiByZXQ7DQo+ID4gIH0NCj4gPg0KPiA+IC1zdGF0aWMgYm9vbCBhZHZhbmNlX3Ry YW5zYWN0aW9uKHN0cnVjdCBhY3BpX2VjICplYykNCj4gPiArc3RhdGljIHZvaWQgYWR2YW5jZV90 cmFuc2FjdGlvbihzdHJ1Y3QgYWNwaV9lYyAqZWMpDQo+ID4gIHsNCj4gPiAgCXN0cnVjdCB0cmFu c2FjdGlvbiAqdDsNCj4gPiAgCXU4IHN0YXR1czsNCj4gPiBAQCAtMjM1LDcgKzIzNSw3IEBAIHN0 YXRpYyBib29sIGFkdmFuY2VfdHJhbnNhY3Rpb24oc3RydWN0IGFjcGlfZWMgKmVjKQ0KPiA+ICAJ CQl0LT5mbGFncyB8PSBBQ1BJX0VDX0NPTU1BTkRfQ09NUExFVEU7DQo+ID4gIAkJCXdha2V1cCA9 IHRydWU7DQo+ID4gIAkJfQ0KPiA+IC0JCXJldHVybiB3YWtldXA7DQo+ID4gKwkJZ290byBvdXQ7 DQo+ID4gIAl9IGVsc2Ugew0KPiA+ICAJCWlmIChFQ19GTEFHU19RVUVSWV9IQU5EU0hBS0UgJiYN Cj4gPiAgCQkgICAgIShzdGF0dXMgJiBBQ1BJX0VDX0ZMQUdfU0NJKSAmJg0KPiA+IEBAIC0yNTEs NyArMjUxLDcgQEAgc3RhdGljIGJvb2wgYWR2YW5jZV90cmFuc2FjdGlvbihzdHJ1Y3QgYWNwaV9l YyAqZWMpDQo+ID4gIAkJCXQtPmZsYWdzIHw9IEFDUElfRUNfQ09NTUFORF9QT0xMOw0KPiA+ICAJ CX0gZWxzZQ0KPiA+ICAJCQlnb3RvIGVycjsNCj4gPiAtCQlyZXR1cm4gd2FrZXVwOw0KPiA+ICsJ CWdvdG8gb3V0Ow0KPiA+ICAJfQ0KPiA+ICBlcnI6DQo+ID4gIAkvKg0KPiA+IEBAIC0yNjIsMTQg KzI2MiwxNiBAQCBlcnI6DQo+ID4gIAkJaWYgKGluX2ludGVycnVwdCgpICYmIHQpDQo+ID4gIAkJ CSsrdC0+aXJxX2NvdW50Ow0KPiA+ICAJfQ0KPiA+IC0JcmV0dXJuIHdha2V1cDsNCj4gPiArb3V0 Og0KPiA+ICsJaWYgKHdha2V1cCAmJiBpbl9pbnRlcnJ1cHQoKSkNCj4gPiArCQl3YWtlX3VwKCZl Yy0+d2FpdCk7DQo+ID4gIH0NCj4gPg0KPiA+ICBzdGF0aWMgdm9pZCBzdGFydF90cmFuc2FjdGlv bihzdHJ1Y3QgYWNwaV9lYyAqZWMpDQo+ID4gIHsNCj4gPiAgCWVjLT5jdXJyLT5pcnFfY291bnQg PSBlYy0+Y3Vyci0+d2kgPSBlYy0+Y3Vyci0+cmkgPSAwOw0KPiA+ICAJZWMtPmN1cnItPmZsYWdz ID0gMDsNCj4gPiAtCSh2b2lkKWFkdmFuY2VfdHJhbnNhY3Rpb24oZWMpOw0KPiA+ICsJYWR2YW5j ZV90cmFuc2FjdGlvbihlYyk7DQo+IA0KPiBXZWxsLCB0aGlzIGxvb2tzIGxpa2UgYSBmdW5jdGlv bmFsIGNoYW5nZSwgYmVjYXVzZSB3ZSB3b3VsZG4ndCBjYWxsDQo+IHdha2VfdXAoJmVjLT53YWl0 KSBoZXJlIGJlZm9yZS4NCg0KQWgsIFllcy4NCkJ1dCBoZXJlLCBzaW5jZSB0aGUgb25seSBhZHZh bmNlbWVudCB0aGF0IGNhbiBoYXBwZW4gaGVyZSBpcyB0byBzZW5kIHRoZSBFQyBjb21tYW5kIGFu ZCB0aGVyZSBhcmUgYWx3YXlzIGZ1cnRoZXIgYWR2YW5jZW1lbnRzIHVudGlsIHRyYW5zYWN0aW9u IGNvbXBsZXRpb24sIHRoZSB3YWtlX3VwKCkgd29uJ3QgYmUgaW52b2tlZCBhdCB0aGlzIHBvaW50 Lg0KU28gSU1PLCB0aGVyZSBpcyBubyBmdW5jdGlvbmFsIGNoYW5nZXMgaGVyZS4NCg0KPiANCj4g PiAgfQ0KPiA+DQo+ID4gIHN0YXRpYyBpbnQgYWNwaV9lY19zeW5jX3F1ZXJ5KHN0cnVjdCBhY3Bp X2VjICplYywgdTggKmRhdGEpOw0KPiA+IEBAIC0zMDQsNyArMzA2LDcgQEAgc3RhdGljIGludCBl Y19wb2xsKHN0cnVjdCBhY3BpX2VjICplYykNCj4gPiAgCQkJCQlyZXR1cm4gMDsNCj4gPiAgCQkJ fQ0KPiA+ICAJCQlzcGluX2xvY2tfaXJxc2F2ZSgmZWMtPmxvY2ssIGZsYWdzKTsNCj4gPiAtCQkJ KHZvaWQpYWR2YW5jZV90cmFuc2FjdGlvbihlYyk7DQo+ID4gKwkJCWFkdmFuY2VfdHJhbnNhY3Rp b24oZWMpOw0KPiANCj4gRGl0dG8uDQoNClllcy4gSSBjaGFuZ2VkIHRoaXMgbG9naWMuDQoNCkJ5 IGludm9raW5nIHdha2VfdXAoKSBoZXJlLCB3ZSBjYW4gYnJlYWsgdGhlIGVjX3BvbGwoKSBsb29w Lg0KQmVjYXVzZSBpZiB0aGUgdHJhbnNhY3Rpb24gaGFzIGNvbXBsZXRlZCBkdWUgdG8gdGhlIHBv bGxpbmcgYWR2YW5jZW1lbnQsIHdlIHJlYWxseSBkb24ndCBuZWVkIHRvIHdhaXQgZm9yIGFub3Ro ZXIgaW50ZXJydXB0Lg0KU28gdGhpcyBuZXcgbG9naWMgbWFrZXMgdGhpcyBwYXRjaCBtb3JlIGxp a2UgYSByYWNlIGZpeCwgbm90IGEgc2ltcGxlIGNsZWFudXAuDQpBbmQgSU1PLCBpdCBtaWdodCBh bHNvIGJlIGEgc3RhYmxlIG1hdGVyaWFsLg0KSSBkaWRuJ3Qgbm90aWNlIHRoaXMgcmFjZSBidWcg YmVjYXVzZSB0aGVyZSBpcyBubyBidWcgcmVwb3J0IGFnYWluc3QgdGhpcy4NCg0KU2hvdWxkIEkg Y2hhbmdlIHRoZSBkZXNjcmlwdGlvbiBhcm91bmQgdGhpcyBhbmQgcmUtc2VuZCB0aGUgcGF0Y2g/ DQoNClRoYW5rcyBhbmQgYmVzdCByZWdhcmRzDQotTHYNCg0KPiANCj4gT3IgYW0gSSBtaXNzaW5n IGFueXRoaW5nPw0KPiANCj4gPiAgCQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmZWMtPmxvY2ss IGZsYWdzKTsNCj4gPiAgCQl9IHdoaWxlICh0aW1lX2JlZm9yZShqaWZmaWVzLCBkZWxheSkpOw0K PiA+ICAJCXByX2RlYnVnKCJjb250cm9sbGVyIHJlc2V0LCByZXN0YXJ0IHRyYW5zYWN0aW9uXG4i KTsNCj4gPiBAQCAtNjg4LDggKzY5MCw3IEBAIHN0YXRpYyB1MzIgYWNwaV9lY19ncGVfaGFuZGxl cihhY3BpX2hhbmRsZSBncGVfZGV2aWNlLA0KPiA+ICAJc3RydWN0IGFjcGlfZWMgKmVjID0gZGF0 YTsNCj4gPg0KPiA+ICAJc3Bpbl9sb2NrX2lycXNhdmUoJmVjLT5sb2NrLCBmbGFncyk7DQo+ID4g LQlpZiAoYWR2YW5jZV90cmFuc2FjdGlvbihlYykpDQo+ID4gLQkJd2FrZV91cCgmZWMtPndhaXQp Ow0KPiA+ICsJYWR2YW5jZV90cmFuc2FjdGlvbihlYyk7DQo+ID4gIAlzcGluX3VubG9ja19pcnFy ZXN0b3JlKCZlYy0+bG9jaywgZmxhZ3MpOw0KPiA+ICAJZWNfY2hlY2tfc2NpKGVjLCBhY3BpX2Vj X3JlYWRfc3RhdHVzKGVjKSk7DQo+ID4gIAlyZXR1cm4gQUNQSV9JTlRFUlJVUFRfSEFORExFRCB8 IEFDUElfUkVFTkFCTEVfR1BFOw0KPiA+DQo+IA0KPiAtLQ0KPiBJIHNwZWFrIG9ubHkgZm9yIG15 c2VsZi4NCj4gUmFmYWVsIEouIFd5c29ja2ksIEludGVsIE9wZW4gU291cmNlIFRlY2hub2xvZ3kg Q2VudGVyLg0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, January 22, 2015 06:37:28 AM Zheng, Lv wrote: > Hi, Rafael > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > > 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@intel.com> > > > --- > > > 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? Yes, please.
Hi, Rafael I'm sorry something is wrong in my previous reply. Let me correct it. > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > 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@rjwysocki.net] > > > 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
On Friday, January 23, 2015 02:18:10 AM Zheng, Lv wrote: > Hi, Rafael > > I'm sorry something is wrong in my previous reply. > Let me correct it. > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > > 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@rjwysocki.net] > > > > 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. OK, I see. Thanks for the analysis. > 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. I will, thanks!
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); } 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); 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;
This patch moves transaction wakeup code into advance_transaction(). No functional changes. Signed-off-by: Lv Zheng <lv.zheng@intel.com> --- drivers/acpi/ec.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)