Hi, Benjamin > From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxx] > Subject: Re: [PATCH v3 1/2] ACPI / button: Add KEY_LID_CLOSE for new > usage model > > Hi, > > On Tue, Jul 12, 2016 at 12:17 PM, Lv Zheng <lv.zheng@xxxxxxxxx> wrote: > > There are many AML tables reporting wrong initial lid state, and some of > > them never reports lid open state. As a proxy layer acting between, ACPI > > button driver is not able to handle all such cases, but need to re-define > > the usage model of the ACPI lid. That is: > > 1. It's initial state is not reliable; > > 2. There may not be an open event; > > 3. Userspace should only take action against the close event which is > > reliable, always sent after a real lid close. > > This patch adds a new input key event so that the new userspace > programs > > can use it to handle this usage model correctly. And in the meanwhile, no > > old programs will be broken by the userspace changes. > > This patch also adds a button.lid_event_type parameter to allow the > users > > to switch between the 2 event types. > > I think we are getting closer to what would be acceptable for > user-space, but I still have a few comments: [Lv Zheng] OK. > > > > > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> > > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx> > > Cc: Bastien Nocera: <hadess@xxxxxxxxxx> > > Cc: linux-input@xxxxxxxxxxxxxxx > > --- > > drivers/acpi/button.c | 109 +++++++++++++++++++++++++-- > ----- > > include/uapi/linux/input-event-codes.h | 6 ++ > > 2 files changed, 91 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > > index 148f4e5..1298ef8 100644 > > --- a/drivers/acpi/button.c > > +++ b/drivers/acpi/button.c > > @@ -57,6 +57,9 @@ > > #define ACPI_BUTTON_LID_INIT_OPEN 0x01 > > #define ACPI_BUTTON_LID_INIT_METHOD 0x02 > > > > +#define ACPI_BUTTON_LID_EVENT_KEY 0x00 > > +#define ACPI_BUTTON_LID_EVENT_SWITCH 0x01 > > + > > #define _COMPONENT ACPI_BUTTON_COMPONENT > > ACPI_MODULE_NAME("button"); > > > > @@ -110,6 +113,7 @@ struct acpi_button { > > static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier); > > static struct acpi_device *lid_device; > > static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD; > > +static u8 lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH; > > > > /* -------------------------------------------------------------------------- > > FS Interface (/proc) > > @@ -136,8 +140,17 @@ static int acpi_lid_notify_state(struct > acpi_device *device, int state) > > int ret; > > > > /* input layer checks if event is redundant */ > > - input_report_switch(button->input, SW_LID, !state); > > - input_sync(button->input); > > + if (lid_event_type == ACPI_BUTTON_LID_EVENT_SWITCH) { > > + input_report_switch(button->input, SW_LID, !state); > > + input_sync(button->input); > > + } > > + if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY && > > + !state) { > > + input_report_key(button->input, KEY_LID_CLOSE, 1); > > + input_sync(button->input); > > + input_report_key(button->input, KEY_LID_CLOSE, 0); > > + input_sync(button->input); > > + } > > > > if (state) > > pm_wakeup_event(&device->dev, 0); > > @@ -292,6 +305,9 @@ static int acpi_lid_update_state(struct > acpi_device *device) > > > > static void acpi_lid_initialize_state(struct acpi_device *device) > > { > > + if (lid_event_type == ACPI_BUTTON_LID_EVENT_KEY) > > + return; > > + > > switch (lid_init_state) { > > case ACPI_BUTTON_LID_INIT_OPEN: > > (void)acpi_lid_notify_state(device, 1); > > @@ -436,6 +452,7 @@ static int acpi_button_add(struct acpi_device > *device) > > > > case ACPI_BUTTON_TYPE_LID: > > input_set_capability(input, EV_SW, SW_LID); > > + input_set_capability(input, EV_KEY, KEY_LID_CLOSE); > > Here you are basically exporting the 2 input events but only update > one of the 2. It will confuse userspace and it is generally better not > to export unused input codes. [Lv Zheng] I just do not know if it is proper to clear the capability during runtime via module parameters. > > However, as I'll say below, I think we should keep the code that way here. [Lv Zheng] Great. So we needn't think about input_clear_capability(). > > > > break; > > } > > > > @@ -475,35 +492,49 @@ static int acpi_button_remove(struct > acpi_device *device) > > > > static int param_set_lid_init_state(const char *val, struct kernel_param > *kp) > > { > > - int result = 0; > > - > > - if (!strncmp(val, "open", sizeof("open") - 1)) { > > - lid_init_state = ACPI_BUTTON_LID_INIT_OPEN; > > - pr_info("Notify initial lid state as open\n"); > > - } else if (!strncmp(val, "method", sizeof("method") - 1)) { > > - lid_init_state = ACPI_BUTTON_LID_INIT_METHOD; > > - pr_info("Notify initial lid state with _LID return value\n"); > > - } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) { > > - lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE; > > - pr_info("Do not notify initial lid state\n"); > > - } else > > - result = -EINVAL; > > + int result = -EINVAL; > > + > > + switch (lid_event_type) { > > + case ACPI_BUTTON_LID_EVENT_KEY: > > + if (!strncmp(val, "ignore", sizeof("ignore") - 1)) { > > + lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE; > > + pr_info("Do not notify initial lid state\n"); > > + } > > + break; > > + case ACPI_BUTTON_LID_EVENT_SWITCH: > > + if (!strncmp(val, "open", sizeof("open") - 1)) { > > + lid_init_state = ACPI_BUTTON_LID_INIT_OPEN; > > + pr_info("Notify initial lid state as open\n"); > > + } else if (!strncmp(val, "method", sizeof("method") - 1)) { > > + lid_init_state = ACPI_BUTTON_LID_INIT_METHOD; > > + pr_info("Notify initial lid state" > > + " with _LID return value\n"); > > + } else if (!strncmp(val, "ignore", sizeof("ignore") - 1)) { > > + lid_init_state = ACPI_BUTTON_LID_INIT_IGNORE; > > + pr_info("Do not notify initial lid state\n"); > > + } > > + break; > > + } > > return result; > > } > > > > static int param_get_lid_init_state(char *buffer, struct kernel_param > *kp) > > { > > - switch (lid_init_state) { > > - case ACPI_BUTTON_LID_INIT_OPEN: > > - return sprintf(buffer, "open"); > > - case ACPI_BUTTON_LID_INIT_METHOD: > > - return sprintf(buffer, "method"); > > - case ACPI_BUTTON_LID_INIT_IGNORE: > > + switch (lid_event_type) { > > + case ACPI_BUTTON_LID_EVENT_KEY: > > return sprintf(buffer, "ignore"); > > - default: > > - return sprintf(buffer, "invalid"); > > + case ACPI_BUTTON_LID_EVENT_SWITCH: > > + switch (lid_init_state) { > > + case ACPI_BUTTON_LID_INIT_OPEN: > > + return sprintf(buffer, "open"); > > + case ACPI_BUTTON_LID_INIT_METHOD: > > + return sprintf(buffer, "method"); > > + case ACPI_BUTTON_LID_INIT_IGNORE: > > + return sprintf(buffer, "ignore"); > > + } > > + break; > > } > > - return 0; > > + return sprintf(buffer, "invalid"); > > } > > > > module_param_call(lid_init_state, > > @@ -511,4 +542,34 @@ module_param_call(lid_init_state, > > NULL, 0644); > > MODULE_PARM_DESC(lid_init_state, "Behavior for reporting LID initial > state"); > > > > +static int param_set_lid_event_type(const char *val, struct > kernel_param *kp) > > +{ > > + int result = -EINVAL; > > + > > + if (!strncmp(val, "key", sizeof("key") - 1)) { > > + lid_event_type = ACPI_BUTTON_LID_EVENT_KEY; > > + pr_info("Notify lid state using key event\n"); > > + } else if (!strncmp(val, "switch", sizeof("switch") - 1)) { > > + lid_event_type = ACPI_BUTTON_LID_EVENT_SWITCH; > > + pr_info("Notify lid state using switch event\n"); > > + } > > + return result; > > +} > > + > > +static int param_get_lid_event_type(char *buffer, struct kernel_param > *kp) > > +{ > > + switch (lid_event_type) { > > + case ACPI_BUTTON_LID_EVENT_KEY: > > + return sprintf(buffer, "key"); > > + case ACPI_BUTTON_LID_EVENT_SWITCH: > > + return sprintf(buffer, "switch"); > > + } > > + return sprintf(buffer, "invalid"); > > +} > > + > > +module_param_call(lid_event_type, > > + param_set_lid_event_type, param_get_lid_event_type, > > + NULL, 0644); > > +MODULE_PARM_DESC(lid_event_type, "Event type for reporting LID > state"); > > I don't think this is a good solution to have a kernel parameter. I > thought the final decision were to have userspace decide which event > was valid, and so we just need to export and emit both of events. [Lv Zheng] OK. > > _If_ you export a kernel parameter, it makes sense to have a dmi > blacklist to have a good default experience, which is what you wanted > to avoid. [Lv Zheng] I was thinking that the distribution vendors can configure a default boot parameters for a specific hardware. > So if you just export and use both events at the same time, you will have: > - correct ACPI machines will just have an extra KEY_LID_CLOSE event > emitted, which will not harm logind > - wrong ACPI machines will not have their SW_LID input event updated > because it will be kept closed. But given that logind will ignore it, > there is no harm either [Lv Zheng] I see. > > As Dmitry said, we could also have a KEY_LID_OPEN emitted for > symmetrical purposes, but I am not entirely sure if this will confuse > userspace or not. On the other hand, there are few users of these > states, and we can teach them how to properly use them. > > So in the end, I think you should just get rid of the kernel > parameter, export SW_LID, KEY_LID_CLOSE, KEY_LID_OPEN in the event > node, and only add the KEY_LID_CLOSE|OPEN events on an actual acpi > notification. [Lv Zheng] I'm sorry I didn't notice the KEY_LID_OPEN comment. Thanks for the reminder. > > Then a small hwdb entry set will teach logind/powerd if they need to > ignore the SW_LID event or not. [Lv Zheng] OK, I'll address what you've suggested in this email. Thanks and best regards -Lv > > Cheers, > Benjamin > > > + > > module_acpi_driver(acpi_button_driver); > > diff --git a/include/uapi/linux/input-event-codes.h > b/include/uapi/linux/input-event-codes.h > > index 737fa32..df7c0c0 100644 > > --- a/include/uapi/linux/input-event-codes.h > > +++ b/include/uapi/linux/input-event-codes.h > > @@ -641,6 +641,12 @@ > > * e.g. teletext or data broadcast application (MHEG, MHP, HbbTV, etc.) > > */ > > #define KEY_DATA 0x275 > > +/* > > + * Special event sent by the lid drivers. The drivers may not be able to > > + * issue "open" event, in which case, they send KEY_LID_CLOSE instead > of > > + * SW_LID. > > + */ > > +#define KEY_LID_CLOSE 0x278 > > > > #define BTN_TRIGGER_HAPPY 0x2c0 > > #define BTN_TRIGGER_HAPPY1 0x2c0 > > -- > > 1.7.10 > > ��.n��������+%������w��{.n�����{��)��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
