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

From: "Zheng, Lv" <lv.zheng@intel.com>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: "Wysocki, Rafael J" <rafael.j.wysocki@intel.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Brown, Len" <len.brown@intel.com>, Lv Zheng <zetalog@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
	Peter Hutterer <peter.hutterer@who-t.net>
Subject: RE: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
Date: Wed, 31 May 2017 08:59:56 +0000	[thread overview]
Message-ID: <1AE640813FDE7649BE1B193DEA596E886CEC0827@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20170529160809.GB28287@mail.corp.redhat.com>

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Subject: Re: [RFC PATCH v3 5/5] ACPI: button: Always notify kernel space using _LID returning value
> 
> Hi Lv,
> 
> On May 27 2017 or thereabouts, Lv Zheng 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.
> >
> > This is a no-op cleanup, but facilitates administrators to configure to
> > notify kernel drivers with faked lid init states via command line
> > "button.lid_notify_init_state=Y".
> >
> > Cc: <intel-gfx@lists.freedesktop.org>
> > Cc: <nouveau@lists.freedesktop.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Peter Hutterer <peter.hutterer@who-t.net>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/button.c             | 16 ++++++++++++++--
> >  drivers/gpu/drm/i915/intel_lvds.c |  2 +-
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 4abf8ae..e047d34 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_notify_init_state __read_mostly = false;
> > +module_param(lid_notify_init_state, bool, 0644);
> > +MODULE_PARM_DESC(lid_notify_init_state, "Notify init lid state to kernel drivers after
> boot/resume");
> >
> >  /* --------------------------------------------------------------------------
> >                                FS Interface (/proc)
> > @@ -224,6 +227,15 @@ static void acpi_lid_notify_state(struct acpi_device *device,
> >  	if (state)
> >  		pm_wakeup_event(&device->dev, 0);
> >
> > +	if (!lid_notify_init_state) {
> > +		/*
> > +		 * There are cases "state" is not a _LID return value, so
> > +		 * correct it before notification.
> > +		 */
> > +		if (!bios_notify &&
> > +		    lid_init_state != ACPI_BUTTON_LID_INIT_METHOD)
> > +			state = acpi_lid_evaluate_state(device);
> > +	}
> >  	acpi_lid_notifier_call(device, state);
> >  }
> >
> > @@ -572,10 +584,10 @@ 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, "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");
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 9ca4dc4..8ca9080 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -548,7 +548,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  	/* Don't force modeset on machines where it causes a GPU lockup */
> >  	if (dmi_check_system(intel_no_modeset_on_lid))
> >  		goto exit;
> > -	if (!acpi_lid_open()) {
> > +	if (!val) {
> >  		/* do modeset on next lid open event */
> >  		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> >  		goto exit;
> 
> This last hunk should really be in its own patch because the intel GPU
> folks would need to apply the rest of the series for their CI suite, and
> also because there is no reason for this change to be alongside any
> other acpi/button.c change.

OK, I'll drop i915 related changes.
However I can see cleanup chances in button.c.
I feel I should at least do minimal tunings in button driver to allow future improvements.

Cheers,
Lv

> Cheers,
> Benjamin