[PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Marco Trevisan]

Backport for v4.5 and v4.6 of
commit afcedebc6a094224973534f43b396bbbf33fe44e upstream.

Fixes resuming of keyboard backlight for ThinkPad laptops after suspension


Override default LED class suspend/resume handles, by keeping track of
the brightness level before suspending so that it can be automatically
restored on resume by calling default resume handler.

Signed-off-by: Marco Trevisan (Treviño) <mail@3v1n0.net>
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Signed-off-by: Darren Hart <dvhart@linux.intel.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 43 +++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index e305ab5..5f09f16 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5001,6 +5001,8 @@ static int kbdlight_set_level(int level)
 	return 0;
 }
 
+static int kbdlight_set_level_and_update(int level);
+
 static int kbdlight_get_level(void)
 {
 	int status = 0;
@@ -5068,7 +5070,7 @@ static void kbdlight_set_worker(struct work_struct *work)
 			container_of(work, struct tpacpi_led_classdev, work);
 
 	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
-		kbdlight_set_level(data->new_state);
+		kbdlight_set_level_and_update(data->new_state);
 }
 
 static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
@@ -5099,7 +5101,6 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 		.max_brightness	= 2,
 		.brightness_set	= &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
-		.flags		= LED_CORE_SUSPENDRESUME,
 	}
 };
 
@@ -5137,6 +5138,20 @@ static void kbdlight_exit(void)
 	flush_workqueue(tpacpi_wq);
 }
 
+static int kbdlight_set_level_and_update(int level)
+{
+	int ret;
+	struct led_classdev *led_cdev;
+
+	ret = kbdlight_set_level(level);
+	led_cdev = &tpacpi_led_kbdlight.led_classdev;
+
+	if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED))
+		led_cdev->brightness = level;
+
+	return ret;
+}
+
 static int kbdlight_read(struct seq_file *m)
 {
 	int level;
@@ -5177,13 +5192,35 @@ static int kbdlight_write(char *buf)
 	if (level == -1)
 		return -EINVAL;
 
-	return kbdlight_set_level(level);
+	return kbdlight_set_level_and_update(level);
+}
+
+static void kbdlight_suspend(void)
+{
+	struct led_classdev *led_cdev;
+
+	if (!tp_features.kbdlight)
+		return;
+
+	led_cdev = &tpacpi_led_kbdlight.led_classdev;
+	led_update_brightness(led_cdev);
+	led_classdev_suspend(led_cdev);
+}
+
+static void kbdlight_resume(void)
+{
+	if (!tp_features.kbdlight)
+		return;
+
+	led_classdev_resume(&tpacpi_led_kbdlight.led_classdev);
 }
 
 static struct ibm_struct kbdlight_driver_data = {
 	.name = "kbdlight",
 	.read = kbdlight_read,
 	.write = kbdlight_write,
+	.suspend = kbdlight_suspend,
+	.resume = kbdlight_resume,
 	.exit = kbdlight_exit,
 };
 

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Greg KH]

On Tue, May 31, 2016 at 07:02:53PM +0200, Marco Trevisan wrote:
> Backport for v4.5 and v4.6 of
> commit afcedebc6a094224973534f43b396bbbf33fe44e upstream.
> 
> Fixes resuming of keyboard backlight for ThinkPad laptops after suspension
> 
> 
> Override default LED class suspend/resume handles, by keeping track of
> the brightness level before suspending so that it can be automatically
> restored on resume by calling default resume handler.
> 
> Signed-off-by: Marco Trevisan (Trevi�o) <mail@3v1n0.net>
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Signed-off-by: Darren Hart <dvhart@linux.intel.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 43 +++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)

This looks like a new feature to me, which isn't really stable material,
sorry.

greg k-h

[PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Marco Trevisan]

From: Marco Trevisan (Treviño) <mail@3v1n0.net>

Override default LED class suspend/resume handles, by keeping track of
the brightness level before suspending so that it can be automatically
restored on resume by calling default resume handler.

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 drivers/platform/x86/thinkpad_acpi.c | 43 +++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 1f9783c..10111c2 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -5041,6 +5041,8 @@ static int kbdlight_set_level(int level)
 	return 0;
 }
 
+static int kbdlight_set_level_and_update(int level);
+
 static int kbdlight_get_level(void)
 {
 	int status = 0;
@@ -5108,7 +5110,7 @@ static void kbdlight_set_worker(struct work_struct *work)
 			container_of(work, struct tpacpi_led_classdev, work);
 
 	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
-		kbdlight_set_level(data->new_state);
+		kbdlight_set_level_and_update(data->new_state);
 }
 
 static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
@@ -5139,7 +5141,6 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
 		.max_brightness	= 2,
 		.brightness_set	= &kbdlight_sysfs_set,
 		.brightness_get	= &kbdlight_sysfs_get,
-		.flags		= LED_CORE_SUSPENDRESUME,
 	}
 };
 
@@ -5177,6 +5178,20 @@ static void kbdlight_exit(void)
 	flush_workqueue(tpacpi_wq);
 }
 
+static int kbdlight_set_level_and_update(int level)
+{
+	int ret;
+	struct led_classdev *led_cdev;
+
+	ret = kbdlight_set_level(level);
+	led_cdev = &tpacpi_led_kbdlight.led_classdev;
+
+	if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED))
+		led_cdev->brightness = level;
+
+	return ret;
+}
+
 static int kbdlight_read(struct seq_file *m)
 {
 	int level;
@@ -5217,13 +5232,35 @@ static int kbdlight_write(char *buf)
 	if (level == -1)
 		return -EINVAL;
 
-	return kbdlight_set_level(level);
+	return kbdlight_set_level_and_update(level);
+}
+
+static void kbdlight_suspend(void)
+{
+	struct led_classdev *led_cdev;
+
+	if (!tp_features.kbdlight)
+		return;
+
+	led_cdev = &tpacpi_led_kbdlight.led_classdev;
+	led_update_brightness(led_cdev);
+	led_classdev_suspend(led_cdev);
+}
+
+static void kbdlight_resume(void)
+{
+	if (!tp_features.kbdlight)
+		return;
+
+	led_classdev_resume(&tpacpi_led_kbdlight.led_classdev);
 }
 
 static struct ibm_struct kbdlight_driver_data = {
 	.name = "kbdlight",
 	.read = kbdlight_read,
 	.write = kbdlight_write,
+	.suspend = kbdlight_suspend,
+	.resume = kbdlight_resume,
 	.exit = kbdlight_exit,
 };
 
-- 
1.9.1

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Pali Rohár]

On Thursday 19 May 2016 03:23:02 Marco Trevisan wrote:
> From: Marco Trevisan (Treviño) <mail@3v1n0.net>
> 
> Override default LED class suspend/resume handles, by keeping track of
> the brightness level before suspending so that it can be automatically
> restored on resume by calling default resume handler.
> 
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 43 +++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 1f9783c..10111c2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -5041,6 +5041,8 @@ static int kbdlight_set_level(int level)
>  	return 0;
>  }
>  
> +static int kbdlight_set_level_and_update(int level);
> +
>  static int kbdlight_get_level(void)
>  {
>  	int status = 0;
> @@ -5108,7 +5110,7 @@ static void kbdlight_set_worker(struct work_struct *work)
>  			container_of(work, struct tpacpi_led_classdev, work);
>  
>  	if (likely(tpacpi_lifecycle == TPACPI_LIFE_RUNNING))
> -		kbdlight_set_level(data->new_state);
> +		kbdlight_set_level_and_update(data->new_state);
>  }
>  
>  static void kbdlight_sysfs_set(struct led_classdev *led_cdev,
> @@ -5139,7 +5141,6 @@ static struct tpacpi_led_classdev tpacpi_led_kbdlight = {
>  		.max_brightness	= 2,
>  		.brightness_set	= &kbdlight_sysfs_set,
>  		.brightness_get	= &kbdlight_sysfs_get,
> -		.flags		= LED_CORE_SUSPENDRESUME,
>  	}
>  };
>  
> @@ -5177,6 +5178,20 @@ static void kbdlight_exit(void)
>  	flush_workqueue(tpacpi_wq);
>  }
>  
> +static int kbdlight_set_level_and_update(int level)
> +{
> +	int ret;
> +	struct led_classdev *led_cdev;
> +
> +	ret = kbdlight_set_level(level);
> +	led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +
> +	if (ret == 0 && !(led_cdev->flags & LED_SUSPENDED))
> +		led_cdev->brightness = level;
> +
> +	return ret;
> +}
> +
>  static int kbdlight_read(struct seq_file *m)
>  {
>  	int level;
> @@ -5217,13 +5232,35 @@ static int kbdlight_write(char *buf)
>  	if (level == -1)
>  		return -EINVAL;
>  
> -	return kbdlight_set_level(level);
> +	return kbdlight_set_level_and_update(level);
> +}
> +
> +static void kbdlight_suspend(void)
> +{
> +	struct led_classdev *led_cdev;
> +
> +	if (!tp_features.kbdlight)
> +		return;
> +
> +	led_cdev = &tpacpi_led_kbdlight.led_classdev;
> +	led_update_brightness(led_cdev);
> +	led_classdev_suspend(led_cdev);
> +}
> +
> +static void kbdlight_resume(void)
> +{
> +	if (!tp_features.kbdlight)
> +		return;
> +
> +	led_classdev_resume(&tpacpi_led_kbdlight.led_classdev);
>  }
>  
>  static struct ibm_struct kbdlight_driver_data = {
>  	.name = "kbdlight",
>  	.read = kbdlight_read,
>  	.write = kbdlight_write,
> +	.suspend = kbdlight_suspend,
> +	.resume = kbdlight_resume,
>  	.exit = kbdlight_exit,
>  };
>  

For me whole patch looks like a big hack because LED_CORE_SUSPENDRESUME
does not work correctly... I would rather see fixed support for flag
LED_CORE_SUSPENDRESUME, not adding another suspend/resume hook if
possible. Any idea?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Marco Trevisan (Treviño)]

2016-05-19 9:52 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>:
> For me whole patch looks like a big hack because LED_CORE_SUSPENDRESUME
> does not work correctly... I would rather see fixed support for flag
> LED_CORE_SUSPENDRESUME, not adding another suspend/resume hook if
> possible. Any idea?

As I wrote you in private mail I think the only way we have is to keep
the led_cdev->brightness in sync with actual state. And to do this, we
can listen the ACPI event, keeping this like a private event that we
just consume to update led brightness value, without delivering it to
userspace.

However, the custom code needed for getting this done, is probably way
more than the one I added here.
Maybe it could be useful to others to read the brightness level before
suspending, and in this case we could add a new flag to the led
subsystem, but not sure it's really worth.

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Darren Hart]

On Fri, May 20, 2016 at 05:03:52AM +0200, Marco Trevisan (Treviño) wrote:
> 2016-05-19 9:52 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>:
> > For me whole patch looks like a big hack because LED_CORE_SUSPENDRESUME
> > does not work correctly... I would rather see fixed support for flag
> > LED_CORE_SUSPENDRESUME, not adding another suspend/resume hook if
> > possible. Any idea?
> 
> As I wrote you in private mail I think the only way we have is to keep
> the led_cdev->brightness in sync with actual state. And to do this, we
> can listen the ACPI event, keeping this like a private event that we
> just consume to update led brightness value, without delivering it to
> userspace.
> 
> However, the custom code needed for getting this done, is probably way
> more than the one I added here.
> Maybe it could be useful to others to read the brightness level before
> suspending, and in this case we could add a new flag to the led
> subsystem, but not sure it's really worth.
> 

At the risk of contributing to "the platform platform", I agree with a local fix
here in the near term. If this becomes a pattern, we can look to move this into
the LED subsystem. As it is, this is working as designed, overriding when
necessary.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Darren Hart]

On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote:
> From: Marco Trevisan (Treviño) <mail@3v1n0.net>
> 
> Override default LED class suspend/resume handles, by keeping track of
> the brightness level before suspending so that it can be automatically
> restored on resume by calling default resume handler.
> 
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

The patch requires a Signed-off-by. I presume that should be from Marco?

Henrique, is this Acked-by from you accurate? I need to see maintainers respond
on list as they would likely object to me just taking other people's word for it
in other scenarios ;-)

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 972 bytes --]

On Monday 23 May 2016 23:57:52 Darren Hart wrote:
> On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote:
> > From: Marco Trevisan (Treviño) <mail@3v1n0.net>
> > 
> > Override default LED class suspend/resume handles, by keeping track
> > of the brightness level before suspending so that it can be
> > automatically restored on resume by calling default resume
> > handler.
> > 
> > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> 
> The patch requires a Signed-off-by. I presume that should be from
> Marco?
> 
> Henrique, is this Acked-by from you accurate? I need to see
> maintainers respond on list as they would likely object to me just
> taking other people's word for it in other scenarios ;-)

Darren, I'm not sure if this is really correct approach in current 
patch... Anyway as Marco wrote, there is ACPI event when keyboard 
backlight change and maybe we should handle it...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Marco Trevisan (Treviño)]

2016-05-24 0:04 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>:
> Darren, I'm not sure if this is really correct approach in current
> patch... Anyway as Marco wrote, there is ACPI event when keyboard
> backlight change and maybe we should handle it...

I'll be happy to work on that as well, but I've not much time at the moment.
However I think this could be an ok way to handle things in the mean time.

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Darren Hart]

On Tue, May 24, 2016 at 12:10:44AM +0200, Marco Trevisan (Treviño) wrote:
> 2016-05-24 0:04 GMT+02:00 Pali Rohár <pali.rohar@gmail.com>:
> > Darren, I'm not sure if this is really correct approach in current
> > patch... Anyway as Marco wrote, there is ACPI event when keyboard
> > backlight change and maybe we should handle it...
> 
> I'll be happy to work on that as well, but I've not much time at the moment.
> However I think this could be an ok way to handle things in the mean time.
> 

I believe so, and Henrique has approved.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Marco Trevisan (Treviño)]

2016-05-23 23:57 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote:
>> From: Marco Trevisan (Treviño) <mail@3v1n0.net>
>>
>> Override default LED class suspend/resume handles, by keeping track of
>> the brightness level before suspending so that it can be automatically
>> restored on resume by calling default resume handler.
>>
>> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>
> The patch requires a Signed-off-by. I presume that should be from Marco?

I'm quite new to this, so please let me know how I should proceed.

> Henrique, is this Acked-by from you accurate? I need to see maintainers respond
> on list as they would likely object to me just taking other people's word for it
> in other scenarios ;-)

I've added it based on this reply (on ibm-acpi ML):
 - http://thread.gmane.org/gmane.linux.acpi.ibm-acpi.devel/4006/focus=4007

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Darren Hart]

On Tue, May 24, 2016 at 12:08:46AM +0200, Marco Trevisan (Treviño) wrote:
> 2016-05-23 23:57 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> > On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote:
> >> From: Marco Trevisan (Treviño) <mail@3v1n0.net>
> >>
> >> Override default LED class suspend/resume handles, by keeping track of
> >> the brightness level before suspending so that it can be automatically
> >> restored on resume by calling default resume handler.
> >>
> >> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> >
> > The patch requires a Signed-off-by. I presume that should be from Marco?
> 
> I'm quite new to this, so please let me know how I should proceed.

Any time you submit a patch, you must provide a Signed-off-by in the commit
message. Git will do this for you with "git commit --signoff". See:

Documentation/SubmittingPatches, Section 11: Sign your work

> 
> > Henrique, is this Acked-by from you accurate? I need to see maintainers respond
> > on list as they would likely object to me just taking other people's word for it
> > in other scenarios ;-)
> 
> I've added it based on this reply (on ibm-acpi ML):
>  - http://thread.gmane.org/gmane.linux.acpi.ibm-acpi.devel/4006/focus=4007
> 

Ah, excellent. I didn't have the original since I wasn't on Cc initially. That's
all I need from Henrique.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] thinkpad_acpi: save kbdlight state on suspend and restore it on resume

[Henrique de Moraes Holschuh]

On Mon, May 23, 2016, at 18:57, Darren Hart wrote:
> On Thu, May 19, 2016 at 03:23:02AM +0200, Marco Trevisan wrote:
> > From: Marco Trevisan (Treviño) <mail@3v1n0.net>
> > 
> > Override default LED class suspend/resume handles, by keeping track of
> > the brightness level before suspending so that it can be automatically
> > restored on resume by calling default resume handler.
> > 
> > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> 
> The patch requires a Signed-off-by. I presume that should be from Marco?
> 
> Henrique, is this Acked-by from you accurate? I need to see maintainers
> respond
> on list as they would likely object to me just taking other people's word
> for it
> in other scenarios ;-)

Yeah, I acked it.  Sorry it took a few days to reply to this email.

This is not the "best" fix, but it is by far the less troublesome one:
hooking to the internal event from the thinkpad DSDT and updating the
brightness shadow register in the driver should work, but that will
require testing as we cannot really take for granted that we will get
that event on every firmware out there without going over the DSDT AML
of a lot of models.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh