amd-hybrid-graphics: fix race condition #21227
Conversation
|
@lheckemann, thanks for your PR! By analyzing the history of the files in this pull request, we identified @wkennington to be a potential reviewer. |
| ExecStart = "${pkgs.bash}/bin/sh -c 'echo -e \"IGD\\nOFF\" > /sys/kernel/debug/vgaswitcheroo/switch; exit 0'"; | ||
| ExecStop = "${pkgs.bash}/bin/sh -c 'echo ON >/sys/kernel/debug/vgaswitcheroo/switch; exit 0'"; | ||
| ExecStart = "${pkgs.bash}/bin/sh -c 'echo -e \"IGD\\nOFF\" > /sys/kernel/debug/vgaswitcheroo/switch'"; | ||
| ExecStop = "${pkgs.bash}/bin/sh -c 'echo ON >/sys/kernel/debug/vgaswitcheroo/switch'"; |
There was a problem hiding this comment.
Do you know why the exit 0's were there? / Why did you remove them?
There was a problem hiding this comment.
#19775 — they caused the script to fail silently. I don't know why they were there.
|
I removed them because they hid the fact that it was failing, which made the actual problem less evident. I don't know why they were there.
|
|
LGTM, @wkennington can you take a look? OR someone with this hardware? |
| }; | ||
| }; | ||
| systemd.paths."vgaswitcheroo" = { |
There was a problem hiding this comment.
Can you elaborate on the motivation for this versus using unitConfig.ConditionPathExists in the above unit?
There was a problem hiding this comment.
Sorry, missed this — we want to start the unit once the path appears, not if it exists.
| @@ -25,15 +25,22 @@ | |||
| path = [ pkgs.bash ]; | |||
| description = "Disable AMD Card"; | |||
| after = [ "sys-kernel-debug.mount" ]; | |||
| requires = [ "sys-kernel-debug.mount" ]; | |||
| wantedBy = [ "multi-user.target" ]; | |||
| before = [ "systemd-vconsole-setup.service" "display-manager.service" ]; | |||
There was a problem hiding this comment.
Hm, I'm a litt confused about the dependency graph here; previously this unit would be pulled in by the default target, but what pulls it in now?
There was a problem hiding this comment.
Oh, nevermind; I see it's supposed to be pulled in by the path unit below :)
|
Since nobody else seems to be using this module and able to review the change, perhaps it would make sense to remove the module entirely? Nobody else has complained about it being broken, so either nobody else has encountered the race condition (seems unlikely to me) or nobody else is using it and it would be better off living only in my personal config. |
Motivation for this change
#19775
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)I'm really not sure about the correctness of my implementation. Feedback is very welcome!