From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benjamin Tissoires Subject: Re: [RFC PATCH v6 2/5] ACPI: button: Add an optional workaround to fix an event missing issue for old userspace Date: Fri, 23 Jun 2017 16:03:08 +0200 Message-ID: <20170623140308.GP26073@mail.corp.redhat.com> References: <2a779ae8c280c968b3237ac4a3d9580df7262a46.1493951798.git.lv.zheng@intel.com> <14729dcd394f41facb871bd3cd3faf7fbf14231e.1498034513.git.lv.zheng@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <14729dcd394f41facb871bd3cd3faf7fbf14231e.1498034513.git.lv.zheng@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: systemd-devel-bounces@lists.freedesktop.org Sender: "systemd-devel" To: Lv Zheng Cc: Len Brown , systemd-devel@lists.freedesktop.org, "Rafael J . Wysocki" , "Rafael J . Wysocki" , linux-kernel@vger.kernel.org, Lv Zheng , linux-acpi@vger.kernel.org List-Id: linux-acpi@vger.kernel.org T24gSnVuIDIxIDIwMTcgb3IgdGhlcmVhYm91dHMsIEx2IFpoZW5nIHdyb3RlOgo+IFRoZXJlIGFy ZSBwbGF0Zm9ybSB2YXJpYXRpb25zIGltcGxlbWVudGluZyBBQ1BJIGxpZCBkZXZpY2UgaW4gZGlm ZmVyZW50Cj4gd2F5czoKPiAxLiBTb21lIHBsYXRmb3JtcyBzZW5kICJvcGVuIiBldmVudHMgdG8g T1MgYW5kIHRoZSBldmVudHMgYXJyaXZlIGJlZm9yZQo+ICAgIGJ1dHRvbiBkcml2ZXIgaXMgcmVz dW1lZDsKPiAyLiBTb21lIHBsYXRmb3JtcyBzZW5kICJvcGVuIiBldmVudHMgdG8gT1MsIGJ1dCB0 aGUgZXZlbnRzIGFycml2ZSBhZnRlcgo+ICAgIGJ1dHRvbiBkcml2ZXIgaXMgcmVzdW1lZCwgZXgu LCBTYW1zdW5nIE4yMTArOwo+IDMuIFNvbWUgcGxhdGZvcm1zIG5ldmVyIHNlbmQgIm9wZW4iIGV2 ZW50cyB0byBPUywgYnV0IHNlbmQgIm9wZW4iIGV2ZW50cyB0bwo+ICAgIHVwZGF0ZSB0aGUgY2Fj aGVkIF9MSUQgcmV0dXJuIHZhbHVlLCBhbmQgdGhlIHVwZGF0ZSBldmVudHMgYXJyaXZlIGJlZm9y ZQo+ICAgIGJ1dHRvbiBkcml2ZXIgaXMgcmVzdW1lZDsKPiA0LiBTb21lIHBsYXRmb3JtcyBuZXZl ciBzZW5kICJvcGVuIiBldmVudHMgdG8gT1MsIGJ1dCBzZW5kICJvcGVuIiBldmVudHMgdG8KPiAg ICB1cGRhdGUgdGhlIGNhY2hlZCBfTElEIHJldHVybiB2YWx1ZSwgYnV0IHRoZSB1cGRhdGUgZXZl bnRzIGFycml2ZSBhZnRlcgo+ICAgIGJ1dHRvbiBkcml2ZXIgaXMgcmVzdW1lZCwgZXguLCBTdXJm YWNlIFBybyAzOwo+IDUuIFNvbWUgcGxhdGZvcm1zIG5ldmVyIHNlbmQgIm9wZW4iIGV2ZW50cywg X0xJRCByZXR1cm5zIHZhbHVlIHN0aWNrcyB0bwo+ICAgICJjbG9zZSIsIGV4LiwgU3VyZmFjZSBQ cm8gMS4KPiBDdXJyZW50bHksIGFsbCBjYXNlcyB3b3JrIGZpbmUgd2l0aCBzeXN0ZW1kIDIzMywg YnV0IG9ubHkgY2FzZSAxLDIgd29yawo+IGZpbmUgd2l0aCBzeXN0ZW1kIDIyOS4KPiAKPiBDYXNl IDMsNCBjYW4gYmUgdHJlYXRlZCBhcyBhbiBldmVudCBtaXNzaW5nIGlzc3VlOgo+ICAgIEFmdGVy IHNlZWluZyBhIExJRCAiY2xvc2UiIGV2ZW50LCBzeXN0ZW1kIDIyOSB3aWxsIHdhaXQgc2V2ZXJh bCBzZWNvbmRzCj4gICAgKEhvbGRvZmZUaW1lb3V0U2VjKSBiZWZvcmUgc3VzcGVuZGluZyB0aGUg cGxhdGZvcm0uIFRodXMgb24gY2FzZSA0Cj4gICAgcGxhdGZvcm1zLCBpZiB1c2VycyBjbG9zZSBs aWQsIGFuZCByZS1vcGVuIGl0IGR1cmluZyB0aGUKPiAgICBIb2xkb2ZmVGltZW91dFNlYyBwZXJp b2QsIHRoZXJlIGlzIHN0aWxsIG5vICJvcGVuIiBldmVudHMgc2VlbiBieSB0aGUKPiAgICB1c2Vy c3BhY2UuIFRodXMgc3lzdGVtZCBzdGlsbCBjb25zaWRlcnMgdGhlIGxhc3Qgc3RhdGUgYXMgImNs b3NlIiBhbmQKPiAgICBzdXNwZW5kcyB0aGUgcGxhdGZvcm0gYWZ0ZXIgdGhlIEhvbGRvZmZUaW1l b3V0U2VjIHRpbWVzIG91dC4KPiAKPiBOb3RlIHRoYXQgbm90IG9ubHkgc3lzdGVtZCAyMjksIGRl c2t0b3AgbWFuYWdlcnMgKGV4LiwKPiBnbm9tZS1zZXR0aW5ncy1kYWVtb24pIGFsc28gc3VmZmVy IGZyb20gdGhpcyBpc3N1ZS4KPiAKPiBUaGlzIHBhdGNoIHRyaWVzIHRvIGZpeCB0aGlzIGlzc3Vl IGJ5IHBlcmlvZGljYWxseSBzZW5kaW5nIF9MSUQgcmV0dXJuCj4gdmFsdWUgdG8gdXNlcnNwYWNl LCB3aGljaCBlbnN1cmVzIHRvIHRyaWdnZXIgYSBTV19MSUQgZXZlbnQgd2hlbiB0aGUKPiB1bmRl cmx5aW5nIGhhcmR3YXJlIHN0YXRlIGhhcyBjaGFuZ2VkLiBBcyBhZGRpbmcgYSBwZXJpb2RpYyB0 aW1lciBpcyBub3QgYQo+IHBvd2VyIGZyaWVuZGx5IHdheSwgdGhpcyBwYXRjaCBwcmVwYXJlcyBh biBvcHRpb24gZm9yIHVzZXJzIHRvIGVuYWJsZSBvbgo+IGZhaWx1cmUgcGxhdGZvcm1zIGZvciBv bGQgdXNlcnNwYWNlIHByb2dyYW1zLgo+IAo+IFVzZXJzIGNhbiBjb25maWd1cmUgdXBkYXRlIGlu dGVydmFsIHZpYSBidXR0b24ubGlkX3VwZGF0ZV9pbnRlcnZhbC4KPiBUaGlzIHNob3VsZCBiZSBj b25maWd1cmVkIHRvIGEgc21hbGxlciB2YWx1ZSB0aGFuIEhvbGRvZmZUaW1lb3V0U2VjIGluCj4g L2V0Yy9zeXN0ZW1kL2xvZ2luZC5jb25mLgo+IAo+IENjOiA8c3lzdGVtZC1kZXZlbEBsaXN0cy5m cmVlZGVza3RvcC5vcmc+Cj4gQ2M6IEJlbmphbWluIFRpc3NvaXJlcyA8YmVuamFtaW4udGlzc29p cmVzQHJlZGhhdC5jb20+Cj4gQ2M6IFBldGVyIEh1dHRlcmVyIDxwZXRlci5odXR0ZXJlckB3aG8t dC5uZXQ+Cj4gU2lnbmVkLW9mZi1ieTogTHYgWmhlbmcgPGx2LnpoZW5nQGludGVsLmNvbT4KPiAt LS0KCk5BQ0s6IHdoYXQgaXMgdGhlIHBvaW50IGlmIHlvdSBqdXN0IHdhbnQgdG8gcHJvdmlkZSBh biBldmVudCB0byB1c2VyCnNwYWNlPyBXZSBhbHJlYWR5IGV4cGxhaW5lZCB0byB5b3UgdGhhdCB0 aGUgZXZlbnRzIGRvIG5vdCBtYXR0ZXIsIG9ubHkKdGhlIHN0YXRlIGlzLiBJZiB1c2VyIHNwYWNl IGhhcyBhbiBpc3N1ZSB3aXRoIGEgc3RhdGUgbm90IGJlaW5nIGluIHN5bmMsCml0J3MgYSB1c2Vy IHNwYWNlIGJ1ZyBhbmQgaXQgaGFzIHRvIGJlIGZpeGVkIGluIHVzZXIgc3BhY2UuCgpUaGlzIHBh dGNoIGNvdWxkIGJlIHVzZWZ1bCBpZiB0aGUgcHVycG9zZSB3YXMgdG8gbW9uaXRvciB0aGUgY2hh bmdlcwp3aGlsZSB0aGUgc3RhdGUgaXMgdW5yZWxpYWJsZSAoaW4gY2FzZSA0KToKLSBpZiBfTElE IHJldHVybnMgYSBkaWZmZXJlbnQgdmFsdWUgdGhhbiByaWdodCBhZnRlciBzdXNwZW5kLCBpdCBp cwogIGV4cGVjdGVkIHRvIGJlIHNvbWV0aGluZyByZWxpYWJsZSwgYW5kIHNvIHdlIGNvdWxkIHRo ZW4gcmUtY3JlYXRlIHRoZQogIGlucHV0IG5vZGUgYW5kIHByZXNlbnQgaXQgdG8gdXNlciBzcGFj ZQotIGlmIF9MSUQgc3RpbGwgcmV0dXJucyB0aGUgc2FtZSB2YWx1ZSBhZnRlciBhIHNvbWV3aGF0 IGxvbmcgZGVsYXkKICAoMTAtMjAgc2Vjb25kcyksIHRoZW4gd2UgY2FuIGNvbnNpZGVyIHRoZSB2 YWx1ZSB0byBiZSBjb3JyZWN0IGFuZAogIHJlLWNyZWF0ZSB0aGUgaW5wdXQgbm9kZS4KClBvbGxp bmcgZm9yIHBvbGxpbmcgYW5kIGhvcGluZyB1c2VyIHNwYWNlIHdpbGwgaGF2ZSBtb3JlIGNoYW5j ZXMgb2YKc2VlaW5nIGFuIGV2ZW50IGZvciBhbiBFVl9TVyBpcyBtb290LgoKQ2hlZXJzLApCZW5q YW1pbgoKPiAgZHJpdmVycy9hY3BpL2J1dHRvbi5jIHwgMTcgKysrKysrKysrKysrKysrLS0KPiAg MSBmaWxlIGNoYW5nZWQsIDE1IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCj4gCj4gZGlm ZiAtLWdpdCBhL2RyaXZlcnMvYWNwaS9idXR0b24uYyBiL2RyaXZlcnMvYWNwaS9idXR0b24uYwo+ IGluZGV4IDY3YTBkNzguLmE4YjExOWUgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9hY3BpL2J1dHRv bi5jCj4gKysrIGIvZHJpdmVycy9hY3BpL2J1dHRvbi5jCj4gQEAgLTEyNiw2ICsxMjYsMTQgQEAg c3RhdGljIHVuc2lnbmVkIGxvbmcgbGlkX25vdGlmeV90aW1lb3V0IF9fcmVhZF9tb3N0bHkgPSAx MDsKPiAgbW9kdWxlX3BhcmFtKGxpZF9ub3RpZnlfdGltZW91dCwgdWxvbmcsIDA2NDQpOwo+ICBN T0RVTEVfUEFSTV9ERVNDKGxpZF9ub3RpZnlfdGltZW91dCwgIlRpbWVvdXQgKHMpIGJlZm9yZSBy ZWNlaXZpbmcgbGlkIG5vdGlmaWNhdGlvbiIpOwo+ICAKPiArc3RhdGljIGJvb2wgbGlkX3Blcmlv ZGljX3VwZGF0ZSBfX3JlYWRfbW9zdGx5ID0gZmFsc2U7Cj4gK21vZHVsZV9wYXJhbShsaWRfcGVy aW9kaWNfdXBkYXRlLCBib29sLCAwNjQ0KTsKPiArTU9EVUxFX1BBUk1fREVTQyhsaWRfcGVyaW9k aWNfdXBkYXRlLCAiUGVyaW9kaWNhbGx5IHNlbmRpbmcgbGlkIHN0YXRlIHVwZGF0ZXMiKTsKPiAr Cj4gK3N0YXRpYyB1bnNpZ25lZCBsb25nIGxpZF91cGRhdGVfaW50ZXJ2YWwgX19yZWFkX21vc3Rs eSA9IDEgKiBNU0VDX1BFUl9TRUM7Cj4gK21vZHVsZV9wYXJhbShsaWRfdXBkYXRlX2ludGVydmFs LCB1bG9uZywgMDY0NCk7Cj4gK01PRFVMRV9QQVJNX0RFU0MobGlkX3VwZGF0ZV9pbnRlcnZhbCwg IkludGVydmFsIChtcykgYmV0d2VlbiBsaWQgcGVyaW9kaWMgdXBkYXRlcyIpOwo+ICsKPiAgLyog LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0KPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgRlMgSW50 ZXJmYWNlICgvcHJvYykKPiAgICAgLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0gKi8KPiBAQCAtMzk1LDYgKzQw Myw4IEBAIHN0YXRpYyB2b2lkIGFjcGlfbGlkX2luaXRpYWxpemVfc3RhdGUoc3RydWN0IGFjcGlf ZGV2aWNlICpkZXZpY2UpCj4gIAkJYnJlYWs7Cj4gIAljYXNlIEFDUElfQlVUVE9OX0xJRF9JTklU X01FVEhPRDoKPiAgCQkodm9pZClhY3BpX2xpZF91cGRhdGVfc3RhdGUoZGV2aWNlKTsKPiArCQlp ZiAobGlkX3BlcmlvZGljX3VwZGF0ZSkKPiArCQkJYWNwaV9saWRfc3RhcnRfdGltZXIoZGV2aWNl LCBsaWRfdXBkYXRlX2ludGVydmFsKTsKPiAgCQlicmVhazsKPiAgCWNhc2UgQUNQSV9CVVRUT05f TElEX0lOSVRfSUdOT1JFOgo+ICAJZGVmYXVsdDoKPiBAQCAtNTYwLDggKzU3MCwxMSBAQCBzdGF0 aWMgaW50IGFjcGlfYnV0dG9uX2FkZChzdHJ1Y3QgYWNwaV9kZXZpY2UgKmRldmljZSkKPiAgCQkg KiBtb3JlIHdlIG9ubHkgY2FyZSBhYm91dCB0aGUgbGFzdCBvbmUuLi4KPiAgCQkgKi8KPiAgCQls aWRfZGV2aWNlID0gZGV2aWNlOwo+IC0JCWFjcGlfbGlkX3N0YXJ0X3RpbWVyKGRldmljZSwKPiAt CQkJbGlkX25vdGlmeV90aW1lb3V0ICogTVNFQ19QRVJfU0VDKTsKPiArCQlpZiAobGlkX3Blcmlv ZGljX3VwZGF0ZSkKPiArCQkJYWNwaV9saWRfaW5pdGlhbGl6ZV9zdGF0ZShkZXZpY2UpOwo+ICsJ CWVsc2UKPiArCQkJYWNwaV9saWRfc3RhcnRfdGltZXIoZGV2aWNlLAo+ICsJCQkJbGlkX25vdGlm eV90aW1lb3V0ICogTVNFQ19QRVJfU0VDKTsKPiAgCX0KPiAgCj4gIAlwcmludGsoS0VSTl9JTkZP IFBSRUZJWCAiJXMgWyVzXVxuIiwgbmFtZSwgYWNwaV9kZXZpY2VfYmlkKGRldmljZSkpOwo+IC0t IAo+IDIuNy40Cj4gCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCnN5c3RlbWQtZGV2ZWwgbWFpbGluZyBsaXN0CnN5c3RlbWQtZGV2ZWxAbGlzdHMuZnJlZWRl c2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8v c3lzdGVtZC1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754325AbdFWODX (ORCPT ); Fri, 23 Jun 2017 10:03:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32940 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754275AbdFWODU (ORCPT ); Fri, 23 Jun 2017 10:03:20 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 17B2730C445 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=benjamin.tissoires@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 17B2730C445 Date: Fri, 23 Jun 2017 16:03:08 +0200 From: Benjamin Tissoires To: Lv Zheng Cc: "Rafael J . Wysocki" , "Rafael J . Wysocki" , Len Brown , Lv Zheng , linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, systemd-devel@lists.freedesktop.org, Peter Hutterer Subject: Re: [RFC PATCH v6 2/5] ACPI: button: Add an optional workaround to fix an event missing issue for old userspace Message-ID: <20170623140308.GP26073@mail.corp.redhat.com> References: <2a779ae8c280c968b3237ac4a3d9580df7262a46.1493951798.git.lv.zheng@intel.com> <14729dcd394f41facb871bd3cd3faf7fbf14231e.1498034513.git.lv.zheng@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <14729dcd394f41facb871bd3cd3faf7fbf14231e.1498034513.git.lv.zheng@intel.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 23 Jun 2017 14:03:15 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Jun 21 2017 or thereabouts, Lv Zheng wrote: > There are platform variations implementing ACPI lid device in different > ways: > 1. Some platforms send "open" events to OS and the events arrive before > button driver is resumed; > 2. Some platforms send "open" events to OS, but the events arrive after > button driver is resumed, ex., Samsung N210+; > 3. Some platforms never send "open" events to OS, but send "open" events to > update the cached _LID return value, and the update events arrive before > button driver is resumed; > 4. Some platforms never send "open" events to OS, but send "open" events to > update the cached _LID return value, but the update events arrive after > button driver is resumed, ex., Surface Pro 3; > 5. Some platforms never send "open" events, _LID returns value sticks to > "close", ex., Surface Pro 1. > Currently, all cases work fine with systemd 233, but only case 1,2 work > fine with systemd 229. > > Case 3,4 can be treated as an event missing issue: > After seeing a LID "close" event, systemd 229 will wait several seconds > (HoldoffTimeoutSec) before suspending the platform. Thus on case 4 > platforms, if users close lid, and re-open it during the > HoldoffTimeoutSec period, there is still no "open" events seen by the > userspace. Thus systemd still considers the last state as "close" and > suspends the platform after the HoldoffTimeoutSec times out. > > Note that not only systemd 229, desktop managers (ex., > gnome-settings-daemon) also suffer from this issue. > > This patch tries to fix this issue by periodically sending _LID return > value to userspace, which ensures to trigger a SW_LID event when the > underlying hardware state has changed. As adding a periodic timer is not a > power friendly way, this patch prepares an option for users to enable on > failure platforms for old userspace programs. > > Users can configure update interval via button.lid_update_interval. > This should be configured to a smaller value than HoldoffTimeoutSec in > /etc/systemd/logind.conf. > > Cc: > Cc: Benjamin Tissoires > Cc: Peter Hutterer > Signed-off-by: Lv Zheng > --- NACK: what is the point if you just want to provide an event to user space? We already explained to you that the events do not matter, only the state is. If user space has an issue with a state not being in sync, it's a user space bug and it has to be fixed in user space. This patch could be useful if the purpose was to monitor the changes while the state is unreliable (in case 4): - if _LID returns a different value than right after suspend, it is expected to be something reliable, and so we could then re-create the input node and present it to user space - if _LID still returns the same value after a somewhat long delay (10-20 seconds), then we can consider the value to be correct and re-create the input node. Polling for polling and hoping user space will have more chances of seeing an event for an EV_SW is moot. Cheers, Benjamin > drivers/acpi/button.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 67a0d78..a8b119e 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -126,6 +126,14 @@ static unsigned long lid_notify_timeout __read_mostly = 10; > module_param(lid_notify_timeout, ulong, 0644); > MODULE_PARM_DESC(lid_notify_timeout, "Timeout (s) before receiving lid notification"); > > +static bool lid_periodic_update __read_mostly = false; > +module_param(lid_periodic_update, bool, 0644); > +MODULE_PARM_DESC(lid_periodic_update, "Periodically sending lid state updates"); > + > +static unsigned long lid_update_interval __read_mostly = 1 * MSEC_PER_SEC; > +module_param(lid_update_interval, ulong, 0644); > +MODULE_PARM_DESC(lid_update_interval, "Interval (ms) between lid periodic updates"); > + > /* -------------------------------------------------------------------------- > FS Interface (/proc) > -------------------------------------------------------------------------- */ > @@ -395,6 +403,8 @@ static void acpi_lid_initialize_state(struct acpi_device *device) > break; > case ACPI_BUTTON_LID_INIT_METHOD: > (void)acpi_lid_update_state(device); > + if (lid_periodic_update) > + acpi_lid_start_timer(device, lid_update_interval); > break; > case ACPI_BUTTON_LID_INIT_IGNORE: > default: > @@ -560,8 +570,11 @@ static int acpi_button_add(struct acpi_device *device) > * more we only care about the last one... > */ > lid_device = device; > - acpi_lid_start_timer(device, > - lid_notify_timeout * MSEC_PER_SEC); > + if (lid_periodic_update) > + acpi_lid_initialize_state(device); > + else > + acpi_lid_start_timer(device, > + lid_notify_timeout * MSEC_PER_SEC); > } > > printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device)); > -- > 2.7.4 >