RE: [PATCH v2 4/5] ACPI: button: Always notify kernel space using _LID returning value

["Zheng, Lv" <lv.zheng@intel.com>]

Hi, Benjamin

> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Subject: Re: [PATCH v2 4/5] ACPI: button: Always notify kernel space using _LID returning value
> 
> On Fri, May 12, 2017 at 3:22 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi,
> >
> >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] 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@intel.com> 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@intel.com>
> >> > ---
> >> >  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.

The purpose of PATCH 4-5 is not to fix anything.
They are no-op cleanups, purposing to make some implicit logic explicit.
Please don't try to mess the purpose up.

While what you want is a behavior change.
Our upstreaming rule is always to split a non-no-op change into a separate commit.
Because in the firmware support world and the cross-layered bugging area.
We don't know what's going to happen to the behavior change.
(Actually I know what's going to happen to what you want to change here - new regression report IMO)
And don't want to be false treated as the culprit to cause the regressions.

> 
> >
> > 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.

So that we can do no-ops for i915.
How could we notify nouveau driver of this change?

> 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.

I didn't.
Marking something __deprecated doesn't actually break its users.
They can still use it.

If nouveau driver relies on acpi_lid_open(), then it always uses _LID return value even in button.lid_init_state=open/close modes.
If I changed acpi_lid_open() to what you suggested, the function will return open/close after boot/resume in button.lid_init_state=open/close modes.
This is an actually a real behavior change, possibly will bother nouveau developers of regressions.

I can see that you just want me to cleanup nouveau driver along with i915.
I can try but I have no mean to test.
I don't have any nouveau graphics card in my hand.
That's also why I just mark it as "__deprecated".

On the contrary, if something has a design defect, or layered bugs, will say we shouldn't bother the other communities.
So you are just trying to make an argument to me from your point of view, not mine.

> So no, I still believe this is not the
> best way of solving the issue.

There is no issue for PATCH 4-5 to solve.
They are just no-op cleanups.

Cheers,
Lv

> 
> 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@vger.kernel.org
> >> > 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@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html