Bug 86521

Since this is a regression, let me ask if you are able to offer a "git bisect" to figure out the culprit commit.

Thanks

# first bad commit: [f6aac652290b31f120fc07c61331e9e0d4b4afb9] toshiba_acpi: Add extra check to backlight code

commit f6aac652290b31f120fc07c61331e9e0d4b4afb9
Author: Azael Avalos <coproscefalo@gmail.com>
Date:   Mon Aug 4 09:21:01 2014 -0600

    toshiba_acpi: Add extra check to backlight code
    
    Some Toshiba models (most notably Qosmios) come with an
    incomplete backlight method where the AML code doesn't
    check for write or read commands and always returns
    HCI_SUCCESS and the actual brightness (and in some
    cases the max brightness), thus allowing the backlight
    interface to be registered without write support.
    
    This patch changes the set_lcd_brightness function,
    checking the returned values for values greater than
    zero to avoid registering a broken backlight interface.
    
    Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
    Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>

My laptop is a Tecra M11 btw.

Please attach your DSDT (or acpidump) so I can take a look at your Toshiba specific backlight code.

What are the contents of "/sys/class/backlight" on your current kernel and also on the previous one you used (if possible)?

What I'm looking for is the presence of the toshiba_acpi entry under that directory. And if its present (again, on both kernels if possible), please do the following as root:

"echo 1 > /sys/class/backlight/toshiba_acpi/brightness"

And see if the actual brightness changes.


Cheers
Azael

On 3.14.22:

# ls -lh /sys/class/backlight
lrwxrwxrwx 1 root root 0 Oct 25  2014 toshiba -> ../../devices/LNXSYSTM:00/device:00/TOS6208:00/backlight/toshiba


# echo 1 > /sys/class/backlight/toshiba_acpi/brightness
bash: /sys/class/backlight/toshiba_acpi/brightness: No such file or directory


The script I use is the following:
echo "brightness: $1" > /proc/acpi/toshiba/lcd

Also, on 3.14.22, this changes brightness:
echo 0 > /sys/class/backlight/toshiba/brightness

Created attachment 155071 [details]
acpidump

(In reply to Nuno Lopes from comment #5)
> On 3.14.22:
> 
> # ls -lh /sys/class/backlight
> lrwxrwxrwx 1 root root 0 Oct 25  2014 toshiba ->
> ../../devices/LNXSYSTM:00/device:00/TOS6208:00/backlight/toshiba
> 
> 
> # echo 1 > /sys/class/backlight/toshiba_acpi/brightness
> bash: /sys/class/backlight/toshiba_acpi/brightness: No such file or directory

Sorry, it should have been "/sys/class/backlight/toshiba/brightness"
I had a brain fart or something :)

> 
> 
> The script I use is the following:
> echo "brightness: $1" > /proc/acpi/toshiba/lcd

Let me create a patch for you to test, unfortunately your Toshiba specific backlight code is hidden, and I'm unable to check what it returns, but with your help, we might be able to figure this out.

@Nuno

http://pastebin.com/k3DpPaPJ <- toshiba_acpi
http://pastebin.com/Jv0YWma3 <- Makefile

Please get the two files from the pastebin links and simply do:

make
sudo make install
sudo make load

Once the modified module is installed and loaded, try to change the brightness and provide me the output of "dmesg | grep toshiba_acpi"


Cheers
Azael

[    0.622333] toshiba_acpi: Toshiba Laptop ACPI Extras version 0.20
[    0.625723] toshiba_acpi: out[0]: 0
[    0.625803] toshiba_acpi: out[1]: 2a
[    0.626255] toshiba_acpi: out[2]: 8000
[    0.626354] toshiba_acpi: out[3]: 0
[    0.626845] toshiba_acpi: out[4]: 0
[    0.626917] toshiba_acpi: out[5]: 0
[    0.629952] toshiba_acpi: Illumination device not available
[    0.632697] toshiba_acpi: Keyboard illumination not available

Created attachment 155281 [details]
Proposed patch

@Nuno

Sorry for the delay, please test the attached patch and see if it solves your issue. Once confirmed I'll send it to the platform mailing list for inclusion.

Cheers
Azael

Created attachment 155291 [details]
Working patch

@Nuno

Sorry, use this patch instead, the other one posted does'n compile.

Cheers
Azael

> The brigthness control of my Toshiba laptop stopped working
> when I upgraded to 3.17.1.  It worked fine at least up to 3.14.22.

Marking as a bisected regression
moved to drivers/platform_x86

(In reply to Azael Avalos from comment #12)
> Created attachment 155291 [details]
> Working patch
> 
> @Nuno
> 
> Sorry, use this patch instead, the other one posted does'n compile.
> 
> Cheers
> Azael

Sorry for the delay; was on travel.
I confirm that this patch fixes the regression.

Thank you!
Nuno

commit a39f46df33c6847399f9b41b74ef09a4095fb996
Author: Azael Avalos <coproscefalo@gmail.com>
Date: Mon, 24 Nov 2014 19:29:36 -0700

toshiba_acpi: Fix regression caused by backlight extra check code

Hi,

I'm afraid that simply reverting the extra check seems to break the backlight on other laptops, see:
https://bugzilla.redhat.com/show_bug.cgi?id=1206036

I'm still waiting for confirmation from the user that this is the case, but this is the most likely culprit. TBH I'm a bit surprised that the original commit was simply reverted, since the original commit clearly states that the extra check is needed on some models such as the "Qosmios" model, and it seems also the Z30.

I've asked the user to also provide the exact acpi_device_id his laptop has, maybe we can get away with disabling the check on TOS6208 models only, but if the Z30 has an id of TOS6208 too we've a bigger problem.

Regards,

Hans

Hi there,

(In reply to Hans de Goede from comment #16)
> Hi,
> 
> I'm afraid that simply reverting the extra check seems to break the
> backlight on other laptops, see:
> https://bugzilla.redhat.com/show_bug.cgi?id=1206036
> 
> I'm still waiting for confirmation from the user that this is the case, but
> this is the most likely culprit. TBH I'm a bit surprised that the original
> commit was simply reverted, since the original commit clearly states that
> the extra check is needed on some models such as the "Qosmios" model, and it
> seems also the Z30.
> 
> I've asked the user to also provide the exact acpi_device_id his laptop has,
> maybe we can get away with disabling the check on TOS6208 models only, but
> if the Z30 has an id of TOS6208 too we've a bigger problem.

Ughh, I saw this one comming...

The original patch was intended for TOS1900 devices only, as all of them have a read-only backlight interface, however, the laptop on this bug report got bailed out too, causing to revert the commit as my proposed solution was to check for a DSDT call that only TOS1900 devices have, and thus, didn't got accepted in mainline with concerns that it might be prone to breakage.

It seems Toshiba is phasing out its backlight interface in favor of generic ones (ACPI or Windows dictated specs), even recent TOS1900 devices return 0x8000 (Not Supported) if Windows 8 is loaded.

If the Z30 happens to be a TOS62XX device, as you say, "we've a bigger problem".
An immediate solution is to add the discovered devices to the (ever growing) DMI matching list in video.c, but I wanted to have a solution in-house instead of relaying on external code.

I'll wait for updates here and will try to work something out in case of a confirmation of a TOS62XX device.

> 
> Regards,
> 
> Hans


Cheers
Azael

Hi,

Thanks for the quick response, so I've just got word back from the user and reverting the revert does indeed fix things for him, so this is indeed the culprit.

And the Z30 has a TOS6208 interface...

One thing which I noticed while looking into this is that the toshiba driver always registers a backlight interface, in which it is (AFAIK) unique for all drivers in platform/x86 all other vendor specific drivers only register their backlight interface if there is no acpi-video backlight interface.

Specfically they contain code like this:

        if (dmi_check_system(video_vendor_dmi_table))
                acpi_video_dmi_promote_vendor();

        if (acpi_video_backlight_support())
                return 0;

        acpi_video_unregister_backlight();

The first call is to check a dmi-list which is stored in the "vendor" driver which models which need to use the vendor interface even though they have a acpi-video backlight interface, the second call checks if acpi-video is active, note that this is an "if" and not an "else if" as the choice me be overridden from the  kernel cmdline, the last call is to remove the acpi-video-backlight call, as that may have been loaded before the acpi_video_dmi_promote_vendor() call, if acpi-video has not registered a backlight interface it is a simple nop.

This is what pretty much all platform/x86 backlight drivers do, except the Toshiba one, so when I first got this bug report I wanted to send you a patch adding this, but thinking about it more I'm afraid that that may cause regressions which is why I did not do that, and upon looking further I found the revert which seems to be the underlying cause of the specific issue for the Z30.

I guess we could add the above standard code, with an extra check maybe for transflective displays, as I do think we will want to use the toshiba driver over the acpi-video on there ?

Regards,

Hans

Hi,

Sure thing, make a patch containing that code and I can help testing it on my devices, chances are that it might land in 4.1 ('tho not sure).

And yes, check for transflective support and make sure it uses the toshiba interface, otherwise use and/or check for generic one.

Cheers
Azael

Created attachment 174001 [details]
[PATCH] toshiba_acpi: Do not register vendor backlight when acpi_video bl is available

Hi,

(In reply to Azael Avalos from comment #19)
> Hi,
> 
> Sure thing, make a patch containing that code and I can help testing it on
> my devices, chances are that it might land in 4.1 ('tho not sure).
> 
> And yes, check for transflective support and make sure it uses the toshiba
> interface, otherwise use and/or check for generic one.
> 
> Cheers
> Azael

Ok, here is a patch implementing this, I'll submit this upstream as soon as I hear back from the reporter of the problem and he confirms that this fixes the problem.

Regards,

Hans

Hi again,

The user has confirmed that the attached patch fixes this, so I'm now officially submitting it upstream, I'll Cc you on the patch submission.

Regards,

Hans

commit 358d6a2c3ecae2b22c7d7e61f9d5672557446dfb
Author: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 21 Apr 2015 12:01:32 +0200

toshiba_acpi: Do not register vendor backlight when acpi_video bl is available