mba6x_bl: init at 2016-02-12 #13688

simonvandel · 2016-03-05T11:11:13Z

Things done:

Tested using sandboxing (nix-build --option build-use-chroot true or nix.useChroot on NixOS)
Built on platform(s): NixOS
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

This driver fixes backlight after suspend/resume on Macbook Air 6,1
and 6,2.

Added a workaround for patjak/mba6x_bl#43 in
form of a systemd service.

Has been working great on my machine for at least a month now.

mention-bot · 2016-03-05T11:11:23Z

By analyzing the blame information on this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers

joachifm · 2016-03-05T13:07:07Z

nixos/modules/hardware/video/backlight/mba6x_bl.nix

+    services.mba6x_bl = {
+      enable = mkEnableOption "mba6x_bl";
+    };
+  };


This will appear somewhat cryptic to readers of the man page, is it possible to provide a more descriptive name/docs?

simonvandel · 2016-03-05T17:51:40Z

Oops, i screwed up. Let med restore this..

simonvandel · 2016-03-05T17:56:52Z

@joachifm : I changed the description based on your feedback.

joachifm · 2016-03-05T20:38:28Z

Great. I'd like to get some feedback from others before merging this, though. Feel free to ping me if it ends up lingering for too long.

hrdinka · 2016-03-06T18:41:27Z

nixos/modules/hardware/video/backlight/mba6x_bl.nix

+      wantedBy = [ "multi-user.target" ];
+      serviceConfig = {
+        Type = "oneshot";
+        ExecStart = "/bin/sh -c \"${modprobe} -r mba6x_bl && ${modprobe} mba6x_bl\"";


Please use script instead of calling sh yourself. It's documented over here: https://nixos.org/nixos/manual/options.html#opt-systemd.services._name_.script

hrdinka · 2016-03-06T18:46:14Z

Adding a whole module for a temporary workaround feels kinda wrong. Does this bug also occur when the module isn't auto loaded (by blacklisting it) but loaded manually or does it always need to be loaded twice to work?

simonvandel · 2016-03-06T19:22:44Z

@hrdinka Thanks for your feedback. I will add fixes soon.

I don't know if it can be justified to make it a module. The driver can be loaded by users by setting boot.extraModulePackages = [ kernel.mba6x_bl ]; in their config file. I feel it is nicer for a module to do this. The workaround was easily added as a systemd unit to the module, making it work as expected without exposing this to the user.

hrdinka · 2016-03-06T19:50:29Z

Ok I see. So it's not only for the workaround. Then it might fit better as an extra option below programs.light (@puffnfresh) something like programs.light.mba6xSupport. @joachifm what's your opinion on that?

simonvandel · 2016-03-06T20:01:19Z

It seems weird to relate the mba6x driver to the light utility. How do they relate other than being in the domain of background light?

joachifm · 2016-03-06T20:05:50Z

Hm, what about the hardware. option namespace? Another possibility is to forgo the module and instead add a small snippet to the nixos-hardware repository. In the end I suppose we should do whatever makes the most sense for potential users; not being a user, I don't have much to contribute.

hrdinka · 2016-03-06T20:22:52Z

@joachifm that was my second idea. This option shouldn't be placed in services.

@simonvandel yes you are right. I fast fired. Technically it does not refer to the light program at all. I just like the idea of having it somewhere where it belongs instead of adding another top level option for a specific device that might be unnecessary in future. Something like:

hardware.backlight.enable # enable generic backlight control
hardware.backlight.macbookAir6xSupport # enables specific device drivers
hardware.backlight.samsungWhateverSupport # or workarounds
...

programs.light seemed natural to me because it is our only backlight control related option in nixos and might be the right backend for a more general backlight option.

simonvandel · 2016-03-06T20:23:10Z

Hm, what about the hardware. option namespace?

This seems more appropriate.

simonvandel · 2016-03-06T20:31:07Z

Something like:

hardware.backlight.enable # enable generic backlight control
hardware.backlight.macbookAir6xSupport # enables specific device drivers
hardware.backlight.samsungWhateverSupport # or workarounds

It would be nice to have a general framework for these kind of hardware specific packages. Another alternative is to leave the actual driver be in nixpkgs, but drop the module support. The use of the driver could then be documented in something like https://github.com/ehmry/nixos-hardware.

hrdinka · 2016-03-06T20:31:15Z

Or a more general hardware support and fixing scheme:

hardware.support.macbookAir6x # enables macbook air 6,x related drivers/fixs
hardware.support.dellXY
hardware.support.hpXY

hrdinka · 2016-03-06T20:33:05Z

@simonvandel thank's for the link, didn't know that. That's actually what my last propose would do under the hood.

hrdinka · 2016-03-06T21:42:07Z

My previous idea is a bad one. An entry for every device out there isn't practical and is absolutely impossible to maintain. I guess for your cause your last post shows the best solution. Just keep the package and add some documentation about its existence and the workaround. The bug itself is hopefully fixed soon upstream.

joachifm · 2016-03-07T12:12:20Z

Suggestion: if the driver is useful on its own, submit a separate PR for it, then we can figure out how to best expose it to users later.

This driver fixes backlight after suspend/resume on Macbook Air 6,1 and 6,2. Added a workaround for patjak/mba6x_bl#43 in form of a systemd service. Has been working great on my machine for at least a month now.

simonvandel · 2016-03-07T18:09:58Z

I updated this PR to only include the packaging of the driver, and implemented the feedback received. Is this merge-able?

I will add some documentation to https://github.com/ehmry/nixos-hardware when I get the time.

joachifm · 2016-03-07T22:13:33Z

It looks to be in keeping with similar packages already in the tree. If it's useful to you, it's sure to be useful to someone else. Thank you for your efforts.

mba6x_bl: init at 2016-02-12

huacayacauh · 2016-03-24T23:57:23Z

Hello,
May I ask how to use this package ? I have the exact problem it is supposed to fix (I am running nixos 15.09 on a macbook air 6.2), but can't manage to find out how to include it in my configuration...
Thank you for the work!

joachifm · 2016-03-25T00:02:31Z

@huacayacauh did you try #13688 (comment)

huacayacauh · 2016-03-25T07:27:49Z

@joachifm thanks. Yes, in this case nixos-rebuild tells me that kernel variable is undefined. I don't know what I should substitute kernel for, my tries gave "attribute mba6x_bl missing".

joachifm · 2016-03-25T07:36:37Z

I think you'd need to refer to boot.kernelPackages.mba6x_bl (untested)

huacayacauh · 2016-03-25T07:43:16Z

@joachifm
boot.kernelPackages.mba6x_bl gives undefined variable ‘boot’
config.boot.kernelPackages.mba6x_bl gives attribute ‘mba6x_bl’ missing

joachifm · 2016-03-25T07:45:54Z

Oh, I see you're running 15.09; this is not available there.

joachifm · 2016-03-25T07:52:06Z

It won't be in a release channel for a while. The earliest you'll get it is when the nixos-unstable channel becomes unstuck. Alternatively, you can clone NixOS/nixpkgs-channels at the release branch you want and cherry-pick the commit onto it. I won't go into details here, see the manual.

huacayacauh · 2016-03-25T08:16:26Z

@joachifm Ok, thank you very much for the quick answer.

joachifm added 8.has: package (new) This PR adds a new package 8.has: module (new) This PR adds a module in `nixos/` labels Mar 5, 2016

joachifm reviewed Mar 5, 2016
View reviewed changes

simonvandel closed this Mar 5, 2016

simonvandel force-pushed the mba6x_bl branch from 70c31f3 to 4a6602c Compare March 5, 2016 17:47

simonvandel reopened this Mar 5, 2016

hrdinka reviewed Mar 6, 2016
View reviewed changes

joachifm added the 9.needs: reporter feedback This issue needs the person who filed it to respond label Mar 6, 2016

mba6x_bl: init at 2016-02-12

aac309f

This driver fixes backlight after suspend/resume on Macbook Air 6,1 and 6,2. Added a workaround for patjak/mba6x_bl#43 in form of a systemd service. Has been working great on my machine for at least a month now.

simonvandel force-pushed the mba6x_bl branch from fd8f542 to aac309f Compare March 7, 2016 18:07

joachifm added a commit that referenced this pull request Mar 7, 2016

Merge pull request #13688 from simonvandel/mba6x_bl

375226f

mba6x_bl: init at 2016-02-12

joachifm merged commit 375226f into NixOS:master Mar 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mba6x_bl: init at 2016-02-12 #13688

mba6x_bl: init at 2016-02-12 #13688

simonvandel commented Mar 5, 2016

mention-bot commented Mar 5, 2016

joachifm Mar 5, 2016

simonvandel commented Mar 5, 2016

simonvandel commented Mar 5, 2016

joachifm commented Mar 5, 2016

hrdinka Mar 6, 2016

hrdinka commented Mar 6, 2016

simonvandel commented Mar 6, 2016

hrdinka commented Mar 6, 2016

simonvandel commented Mar 6, 2016

joachifm commented Mar 6, 2016

hrdinka commented Mar 6, 2016

simonvandel commented Mar 6, 2016

simonvandel commented Mar 6, 2016

hrdinka commented Mar 6, 2016

hrdinka commented Mar 6, 2016

hrdinka commented Mar 6, 2016

joachifm commented Mar 7, 2016

simonvandel commented Mar 7, 2016

joachifm commented Mar 7, 2016

huacayacauh commented Mar 24, 2016

joachifm commented Mar 25, 2016

huacayacauh commented Mar 25, 2016

joachifm commented Mar 25, 2016

huacayacauh commented Mar 25, 2016

joachifm commented Mar 25, 2016

joachifm commented Mar 25, 2016

huacayacauh commented Mar 25, 2016

mba6x_bl: init at 2016-02-12 #13688

mba6x_bl: init at 2016-02-12 #13688

Conversation

simonvandel commented Mar 5, 2016

Things done:

mention-bot commented Mar 5, 2016

joachifm Mar 5, 2016

Choose a reason for hiding this comment

simonvandel commented Mar 5, 2016

simonvandel commented Mar 5, 2016

joachifm commented Mar 5, 2016

hrdinka Mar 6, 2016

Choose a reason for hiding this comment

hrdinka commented Mar 6, 2016

simonvandel commented Mar 6, 2016

hrdinka commented Mar 6, 2016

simonvandel commented Mar 6, 2016

joachifm commented Mar 6, 2016

hrdinka commented Mar 6, 2016

simonvandel commented Mar 6, 2016

simonvandel commented Mar 6, 2016

hrdinka commented Mar 6, 2016

hrdinka commented Mar 6, 2016

hrdinka commented Mar 6, 2016

joachifm commented Mar 7, 2016

simonvandel commented Mar 7, 2016

joachifm commented Mar 7, 2016

huacayacauh commented Mar 24, 2016

joachifm commented Mar 25, 2016

huacayacauh commented Mar 25, 2016

joachifm commented Mar 25, 2016

huacayacauh commented Mar 25, 2016

joachifm commented Mar 25, 2016

joachifm commented Mar 25, 2016

huacayacauh commented Mar 25, 2016