On Fri, May 12, 2017 at 3:22 AM, Zheng, Lv <lv.zheng@xxxxxxxxx> wrote: > Hi, > >> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] On Behalf Of Benjamin >> Tissoires >> Subject: Re: [PATCH v2 4/5] ACPI: button: Always notify kernel space using _LID returning value >> >> On Tue, May 9, 2017 at 9:02 AM, Lv Zheng <lv.zheng@xxxxxxxxx> wrote: >> > Both nouveau and i915, the only 2 kernel space lid notification listeners, >> > invoke acpi_lid_open() API to obtain _LID returning value instead of using >> > the notified value. >> > >> > So this patch moves this logic from listeners to lid driver, always notify >> > kernel space listeners using _LID returning value. >> > >> > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> >> > --- >> > drivers/acpi/button.c | 31 ++++++++++++++++++++----------- >> > 1 file changed, 20 insertions(+), 11 deletions(-) >> > >> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c >> > index b91ff86..7ff3a75 100644 >> > --- a/drivers/acpi/button.c >> > +++ b/drivers/acpi/button.c >> > @@ -119,6 +119,9 @@ static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN; >> > static unsigned long lid_report_interval __read_mostly = 500; >> > module_param(lid_report_interval, ulong, 0644); >> > MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events"); >> > +static bool lid_fake_events __read_mostly = true; >> > +module_param(lid_fake_events, bool, 0644); >> > +MODULE_PARM_DESC(lid_fake_events, "Deliver fake events to kernel drivers"); >> > >> > /* -------------------------------------------------------------------------- >> > FS Interface (/proc) >> > @@ -139,7 +142,8 @@ static int acpi_lid_evaluate_state(struct acpi_device *device) >> > return lid_state ? 1 : 0; >> > } >> > >> > -static void acpi_lid_notify_state(struct acpi_device *device, int state) >> > +static void acpi_lid_notify_state(struct acpi_device *device, >> > + int state, bool bios_notify) >> > { >> > struct acpi_button *button = acpi_driver_data(device); >> > ktime_t next_report; >> > @@ -219,7 +223,11 @@ static void acpi_lid_notify_state(struct acpi_device *device, int state) >> > if (state) >> > pm_wakeup_event(&device->dev, 0); >> > >> > - atomic_notifier_call_chain(&acpi_lid_notifier, state, device); >> > + if (bios_notify) >> > + atomic_notifier_call_chain(&acpi_lid_notifier, state, device); >> > + else if (lid_fake_events) >> > + atomic_notifier_call_chain(&acpi_lid_notifier, >> > + acpi_lid_evaluate_state(device), device); >> >> I don't quite get the "lid_fake_event" parameter. If set, you are >> forwarding the evaluated method, which should be correct, not faked... > > Fake means the event is generated by kernel. > BIOS didn't generate the event. > Maybe we could name it differently. > >> >> Also, what if bios_notify is false and so is lid_fake_events? Is that >> expected that no events are forwarded? > > When bios_notify=true: > 1. the button driver evaluates _LID upon seeing a BIOS notification and > 2. the lid event notifier just forwards _LID return value to the listeners. > When bios_notify=false: > 1. this is only happen when the platform reboots/resumes, > 2. the button driver just fakes (sorry I used fake again) an event without seeing any BIOS notification, and > 3. the lid event notifier sends the faked event to the listeners. Originally, What has been sent to the listeners: > 3.1. sends "open" to the listeners when button.lid_init_state=open, > 3.2. sends "close" to the listeners when button.lid_init_state=close, > 3.3. sends _LID return value to the listeners when button.lid_init_state=method. > > So let's check the listener side (who invokes acpi_lid_notifier_register()). > The only acpi_lid_notifier_register() invoker is drivers/gpu/drm/i915/intel_lvds.c. > > If you check its implementation (also you can see its code in PATCH 5/5). > It cares no about what we send. > Whatever we send "open/close/eval(_LID)" to it, it just invokes acpi_lid_open() API to obtain the _LID return value. Right, so that's a wrong behavior since c1c7af6089207 "drm/i915: force mode set at lid open time" at around the 2.6.31 kernel. > While button driver's preference is remove acpi_lid_open() and let the listeners use the notified value instead. That's one way of seeing. I would prefer having acpi_lid_open() reporting the cached value, the same as the one we sent in the notifier. Or if you believe there might be races, this could be synced or acpi_lid_open() can be removed, but the races seem to be minimal in term of impact: if the LID is opened, on resume the driver decides to restore the pipes and planes by itself. A race would mean that the notification would only matter for the last state, and that's no problem. > > If the listener do not use the notified value, there is a time slice between lid notification and the _LID evaluation. > We should put some synchronization mechanism around around the 2 actions in button driver to make the result more trustworthy. > So we trend to just send what the listener want to them and leave the possibility of implementing such usage model in button driver. > >> >> Honestly, adding one more kernel parameter doesn't seem to solve >> anything, it just adds more blur on a blurry situation. >> >> How about: >> - you keep the "ignore", "method" and "open" (because they are kernel >> API now) and don't add more >> - you revert the "method" initial lid state, as requested by me and >> others hitting the docking station "bug" > > Let's talk about this in different email. > >> - you just let user space deal with false lid switch values by >> forwarding the bugs to libinput. >> >> IMO, we don't need such patch. > > This patch is developed to eliminate acpi_lid_open() in order to enforce the following usage model: > > All lid event notifier should use the notified value in their callback. Yes, make sense. But you are deprecating the nouveau call in the other patch, while all you could do is fix it in acpi.button without bothering other developers. So no, I still believe this is not the best way of solving the issue. Cheers, Benjamin > > Cheers, > Lv > >> >> Cheers, >> Benjamin >> >> > } >> > >> > static int acpi_button_state_seq_show(struct seq_file *seq, void *offset) >> > @@ -349,7 +357,8 @@ int acpi_lid_open(void) >> > } >> > EXPORT_SYMBOL(acpi_lid_open); >> > >> > -static int acpi_lid_update_state(struct acpi_device *device) >> > +static int acpi_lid_update_state(struct acpi_device *device, >> > + bool bios_notify) >> > { >> > int state; >> > >> > @@ -357,7 +366,7 @@ static int acpi_lid_update_state(struct acpi_device *device) >> > if (state < 0) >> > return state; >> > >> > - acpi_lid_notify_state(device, state); >> > + acpi_lid_notify_state(device, state, bios_notify); >> > return 0; >> > } >> > >> > @@ -365,13 +374,13 @@ static void acpi_lid_initialize_state(struct acpi_device *device) >> > { >> > switch (lid_init_state) { >> > case ACPI_BUTTON_LID_INIT_OPEN: >> > - (void)acpi_lid_notify_state(device, 1); >> > + (void)acpi_lid_notify_state(device, 1, false); >> > break; >> > case ACPI_BUTTON_LID_INIT_CLOSE: >> > - (void)acpi_lid_notify_state(device, 0); >> > + (void)acpi_lid_notify_state(device, 0, false); >> > break; >> > case ACPI_BUTTON_LID_INIT_METHOD: >> > - (void)acpi_lid_update_state(device); >> > + (void)acpi_lid_update_state(device, false); >> > break; >> > case ACPI_BUTTON_LID_INIT_IGNORE: >> > default: >> > @@ -391,7 +400,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) >> > case ACPI_BUTTON_NOTIFY_STATUS: >> > input = button->input; >> > if (button->type == ACPI_BUTTON_TYPE_LID) { >> > - acpi_lid_update_state(device); >> > + acpi_lid_update_state(device, true); >> > } else { >> > int keycode; >> > >> > @@ -555,13 +564,13 @@ static int param_set_lid_init_state(const char *val, struct kernel_param *kp) >> > >> > if (!strncmp(val, "open", sizeof("open") - 1)) { >> > lid_init_state = ACPI_BUTTON_LID_INIT_OPEN; >> > - pr_info("Notify initial lid state as open\n"); >> > + pr_info("Notify initial lid state to users space as open and kernel drivers with >> _LID return value\n"); >> > } else if (!strncmp(val, "close", sizeof("close") - 1)) { >> > lid_init_state = ACPI_BUTTON_LID_INIT_CLOSE; >> > - pr_info("Notify initial lid state as close\n"); >> > + pr_info("Notify initial lid state to user space as close and kernel drivers with >> _LID return value\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"); >> > + pr_info("Notify initial lid state to user/kernel space 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"); >> > -- >> > 2.7.4 >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >> > the body of a message to majordomo@xxxxxxxxxxxxxxx >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
