thinkpad_acpi: don't yell on unsupported brightness interfaces

PATCH In review David Herrmann 4 participants 9 messages Oct 21, 2015 – Jan 9, 2016 x86 platform drivers

Share & tools

David Herrmann

author

2015-10-21 10:46 UTC #0 lore raw

PATCH 1/1 thinkpad_acpi: don't yell on unsupported brightness interfaces

The thinkpad_acpi driver currently emits error messages on unsupported brightness interfaces, giving the impression that someone will implement those. However, this error is spit out on nearly every thinkpad in production since 2 years now. Furthermore, the backlight interfaces on those devices are supported by the i915 driver just fine.

Downgrade the error message to a normal pr_info() and stop telling people to report it to IBM.

Signed-off-by David Herrmann <[email protected]>

---
 drivers/platform/x86/thinkpad_acpi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +1 -2

index 131dd74..0bed473 100644
@@ -6459,8 +6459,7 @@ static void __init tpacpi_detect_brightness_capabilities(void)
 		pr_info("detected a 8-level brightness capable ThinkPad\n");
 		break;
 	default:
-		pr_err("Unsupported brightness interface, "
-		       "please contact %s\n", TPACPI_MAIL);
+		pr_info("Unsupported brightness interface\n");
 		tp_features.bright_unkfw = 1;
 		bright_maxlvl = b - 1;
 	}

Signature · 1 line

2.6.1

Henrique de Moraes Holschuh

Acked-by on v1 1/1

2015-10-21 14:33 UTC #1 lore raw

Quoted 8 lines · David Herrmann

The thinkpad_acpi driver currently emits error messages on unsupported brightness interfaces, giving the impression that someone will implement those. However, this error is spit out on nearly every thinkpad in production since 2 years now. Furthermore, the backlight interfaces on those devices are supported by the i915 driver just fine.
Downgrade the error message to a normal pr_info() and stop telling people to report it to IBM.

IBM? Those reports go directly to me.

Quoted 1 line

Signed-off-by David Herrmann <[email protected]>

Acked-by Henrique de Moraes Holschuh <[email protected]>

Quoted 28 lines

---
 drivers/platform/x86/thinkpad_acpi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +1 -2

index 131dd74..0bed473 100644
@@ -6459,8 +6459,7 @@ static void __init
 tpacpi_detect_brightness_capabilities(void)
 		pr_info("detected a 8-level brightness capable ThinkPad\n");
 		break;
 	default:
-               pr_err("Unsupported brightness interface, "
-                      "please contact %s\n", TPACPI_MAIL);
+               pr_info("Unsupported brightness interface\n");
 		tp_features.bright_unkfw = 1;
 		bright_maxlvl = b - 1;

}

Signature · 7 lines

2.6.1

--
To unsubscribe from this list: send the line "unsubscribe
platform-driver-x86" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

  "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

Darren Hart

2015-10-21 14:44 UTC #2 lore raw

Quoted 7 lines · Henrique de Moraes Holschuh

Quoted 6 lines · David Herrmann
The thinkpad_acpi driver currently emits error messages on unsupported brightness interfaces, giving the impression that someone will implement those. However, this error is spit out on nearly every thinkpad in production since 2 years now. Furthermore, the backlight interfaces on those devices are supported by the i915 driver just fine.

Quoted 8 lines

Quoted 2 lines
Downgrade the error message to a normal pr_info() and stop telling people to report it to IBM.
IBM? Those reports go directly to me.
Quoted 1 line
Signed-off-by David Herrmann <[email protected]>
Acked-by Henrique de Moraes Holschuh <[email protected]>

Thanks, Queued.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

David Herrmann

author

2015-11-06 14:19 UTC #3 lore raw

Hi Darren!

Quoted 11 lines · Darren Hart

Quoted 10 lines · Henrique de Moraes Holschuh
Quoted 8 lines · David Herrmann
The thinkpad_acpi driver currently emits error messages on unsupported brightness interfaces, giving the impression that someone will implement those. However, this error is spit out on nearly every thinkpad in production since 2 years now. Furthermore, the backlight interfaces on those devices are supported by the i915 driver just fine.
Downgrade the error message to a normal pr_info() and stop telling people to report it to IBM.

Quoted 7 lines

Quoted 5 lines
IBM? Those reports go directly to me.
Quoted 1 line
Signed-off-by David Herrmann <[email protected]>
Acked-by Henrique de Moraes Holschuh <[email protected]>
Thanks, Queued.

Any particular reason this didn't show up in:

    [GIT PULL] platform-drivers-x86 for 4.4-1

Just a kind reminder, in case it slipped through the cracks.

Thanks
David

Darren Hart

2015-11-06 17:57 UTC #4 lore raw

Quoted 18 lines · David Herrmann

Hi Darren!
Quoted 15 lines · Darren Hart
Quoted 14 lines · Henrique de Moraes Holschuh
Quoted 8 lines · David Herrmann
The thinkpad_acpi driver currently emits error messages on unsupported brightness interfaces, giving the impression that someone will implement those. However, this error is spit out on nearly every thinkpad in production since 2 years now. Furthermore, the backlight interfaces on those devices are supported by the i915 driver just fine.
Downgrade the error message to a normal pr_info() and stop telling people to report it to IBM.
IBM? Those reports go directly to me.
Quoted 1 line
Signed-off-by David Herrmann <[email protected]>

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Acked-by Henrique de Moraes Holschuh <[email protected]>
Thanks, Queued.
Any particular reason this didn't show up in:
    [GIT PULL] platform-drivers-x86 for 4.4-1
Just a kind reminder, in case it slipped through the cracks.

Thank you, appears to be an error on my part, possibly due to some tooling changes on my end. Apologies, I'll get 4.4-2 out within the merge window and be sure to include this.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Eric Curtin

2016-01-07 01:03 UTC #5 lore raw

Quoted 33 lines · Darren Hart

Quoted 18 lines · David Herrmann
Hi Darren!
Quoted 15 lines · Darren Hart
Quoted 14 lines · Henrique de Moraes Holschuh
Quoted 8 lines · David Herrmann
The thinkpad_acpi driver currently emits error messages on unsupported brightness interfaces, giving the impression that someone will implement those. However, this error is spit out on nearly every thinkpad in production since 2 years now. Furthermore, the backlight interfaces on those devices are supported by the i915 driver just fine.
Downgrade the error message to a normal pr_info() and stop telling people to report it to IBM.
IBM? Those reports go directly to me.
Quoted 1 line
Signed-off-by David Herrmann <[email protected]>
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Acked-by Henrique de Moraes Holschuh <[email protected]>
Thanks, Queued.
Any particular reason this didn't show up in:
    [GIT PULL] platform-drivers-x86 for 4.4-1
Just a kind reminder, in case it slipped through the cracks.
Thank you, appears to be an error on my part, possibly due to some tooling changes on my end. Apologies, I'll get 4.4-2 out within the merge window and be sure to include this.

Quoted 8 lines

--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi Guys,

Actually could we just remove this log message altogether I don't think it is very useful. The likelihood of this log indicating an error or "Unsupported brightness interface" is very very low I imagine. This appears on my Lenovo E540 (b variable in switch statement is 101 in my case), I have been using Linux on this machine for well over a year and the brightness interface is definitely very well supported. From talking to this guy on a completely different thread it seems to happen on his machine also:

https://github.com/mate-desktop/mate-power-manager/issues/179#issuecomment-169085313

Or else I can add a 101 switch case to the statement?

Also another query, could be a mini-side project for me as I'm a beginner kernel dev when I have some spare time. Could we change the minimum brightness value not to be off? 0 does not have to mean backlight off from reading various things around the web:

http://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/backlight.pdf

And to be honest having the ability to completely turn off the backlight is a useless function and is probably unintended as discussed in the github thread above also. You would need night vision googles to read the screen in this state! I'm pretty sure the Windows driver does not completely turn off the screen on minimum brightness either, no need to have different functionality on different platforms especially when this functionality isn't very useful.

As suggested on the GitHub thread also, on other Lenovo machines (T60), minimum brightness (0) does not mean off, so it would be nice to have some consistency here.

Let me know what you think, I'd be happy to implement both these things if you guys agree. I don't see much point in writing code if it won't be accepted!

David Herrmann

author

2016-01-08 11:55 UTC #6 lore raw

Quoted 7 lines · Eric Curtin

Also another query, could be a mini-side project for me as I'm a beginner kernel dev when I have some spare time. Could we change the minimum brightness value not to be off? 0 does not have to mean backlight off from reading various things around the web:
http://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/backlight.pdf

Quoted 7 lines

And to be honest having the ability to completely turn off the backlight is a useless function and is probably unintended as discussed in the github thread above also. You would need night vision googles to read the screen in this state! I'm pretty sure the Windows driver does not completely turn off the screen on minimum brightness either, no need to have different functionality on different platforms especially when this functionality isn't very useful.

Whether '0' means 'off' or 'lowest brightness level' has been discussed several times. Sadly, we cannot change semantics without breaking user-space, so you better work around it. There have been several proposals to fix it, but I guess no-one cared so far.

Anyway, both features (turning backlight off *and* lowest backlight level) _are_ used actively by user-space. Hence, both should be supported.

Thanks
David

Eric Curtin

2016-01-08 17:26 UTC #7 lore raw

Quoted 23 lines · David Herrmann

Hi
Quoted 7 lines · Eric Curtin
Also another query, could be a mini-side project for me as I'm a beginner kernel dev when I have some spare time. Could we change the minimum brightness value not to be off? 0 does not have to mean backlight off from reading various things around the web:
http://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/backlight.pdf
Quoted 7 lines
And to be honest having the ability to completely turn off the backlight is a useless function and is probably unintended as discussed in the github thread above also. You would need night vision googles to read the screen in this state! I'm pretty sure the Windows driver does not completely turn off the screen on minimum brightness either, no need to have different functionality on different platforms especially when this functionality isn't very useful.
Whether '0' means 'off' or 'lowest brightness level' has been discussed several times. Sadly, we cannot change semantics without breaking user-space, so you better work around it. There have been several proposals to fix it, but I guess no-one cared so far.

Quoted 6 lines

Anyway, both features (turning backlight off *and* lowest backlight level) _are_ used actively by user-space. Hence, both should be supported.
Thanks
David

As regards the "Unsupported brightness interface" log message will I add 101 to the switch statement so it doesn't get reported on my device or just remove it? This seems to be reporting many false positives (or negatives whatever way you want to look at it) on different machines so can I remove this logging?

Henrique de Moraes Holschuh

2016-01-09 17:32 UTC #8 lore raw

Quoted 5 lines · Eric Curtin

As regards the "Unsupported brightness interface" log message will I add 101 to the switch statement so it doesn't get reported on my device or just remove it? This seems to be reporting many false positives (or negatives whatever way you want to look at it) on different machines so can I remove this logging?

Either remove the message, or leave it at debug level. Do not add 101 to the switch case, please.

Signature · 4 lines

  "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