Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

displaylink: manually activate dlm.service #107294

Merged
merged 1 commit into from Dec 27, 2020
Merged

displaylink: manually activate dlm.service #107294

merged 1 commit into from Dec 27, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Dec 20, 2020

The shell script coming with the vendor-provided udev rule simply
starts dlm.service (and sets up some symlinks), and stops dlm.service if
that was the last card plugged in.

On NixOS, some of the cat/grep/sed commands are not available, causing
the script to fail.

Turns out, the symlinks aren't needed at all. Archlinux ships their own
script
(https://aur.archlinux.org/cgit/aur.git/plain/udev.sh?h=displaylink),
which only starts and stops dlm.service, depending on whether there's
cards left or not.

We can further optimize this by simply starting dlm.service on the first
card, and not stopping it at all. Considering dlm won't get stopped if
one of multiple cards is unplugged, it seems to handle disconnects.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@flokli
displaylink: manually activate dlm.service
43f7479 
The shell script coming with the vendor-provided udev rule simply
starts dlm.service (and sets up some symlinks), and stops dlm.service if
that was the last card plugged in.

On NixOS, some of the cat/grep/sed commands are not available, causing
the script to fail.

Turns out, the symlinks aren't needed at all. Archlinux ships their own
script
(https://aur.archlinux.org/cgit/aur.git/plain/udev.sh?h=displaylink),
which only starts and stops dlm.service, depending on whether there's
cards left or not.

We can further optimize this by simply starting dlm.service on the first
card, and not stopping it at all. Considering dlm won't get stopped if
one of multiple cards is unplugged, it seems to handle disconnects.
@flokli flokli requested a review from eyJhb December 20, 2020 23:36
@eyJhb
Copy link
Member

eyJhb commented Dec 21, 2020

So instead of rolling it the Arch way, we just start the dlm.service once a card is inserted?
Seems like we could just grab the one that arch use, and then roll with that.

I will of course test this soon!

@flokli
Copy link
Contributor Author

flokli commented Dec 21, 2020

The arch one might also work, but looking at the script itself, it really only starts dlm (and stops it later).

The udev rule in this PR simply pulls in/starts dlm.service when the first displaylink card is plugged in, without any scary shellscripts that could go wrong. It doesn't bother stopping the service when that was the last card in the system.

I saw a bunch of errors when executing our script (or theirs), and fear this is due to things like grep (and others) not being in $PATH - and patching it in nixpkgs would be a bit laborious.

IMHO, that udev rule oneliner is much cleaner, and we should point the maintainer of the AUR package to it, too.

@peterhoeg
Copy link
Member

This is definitely the cleaner way to do this.

One thing though - the ACTION part should be ACTION!="remove" ref https://lwn.net/Articles/837033/

@flokli
Copy link
Contributor Author

flokli commented Dec 22, 2020 •
edited

From my understanding of the article, udev rules should match on specific events they care about ("adding the device", in our case), and the linked problem was mostly due to assumptions on a specific set of events. By now matching on ACTION!="remove", we would make it worse, because it'll fire on any other (new) event by default - including unbind, when we surely shouldn't start dlm.service.

IMHO, matching on the add event only seems fine. We start dlm when a new device is plugged in (and don't care if it's dlm loading firmware, the kernel driver, or if it needs no firmware and emits a bind immediately afterwards).

bind events shouldn't matter in our case, cause they happen after the add, a we don't stop dlm.service on unbind/bind either.

The only case I could see how this might be incomplete would be if users manually unbind the device and stop dlm. On a manual bind afterwards, dlm won't be started. But that's a very theoretic scenario, and if users manually stop dlm, we can expect them to start it as well (or just replug the device, which should fire the add event, and get things back to normal).

@flokli
Copy link
Contributor Author

flokli commented Dec 25, 2020

@peterhoeg do you agree? Is this good to merge?

@flokli
Copy link
Contributor Author

flokli commented Dec 27, 2020

Okay, let's get this in. The current udev rules also only did listen on add (and remove).

@flokli flokli merged commit b19ae92 into NixOS:master Dec 27, 2020
@flokli flokli deleted the displaylink-simplify branch December 27, 2020 21:31
@eyJhb
Copy link
Member

eyJhb commented Dec 27, 2020

@flokli sorry for not testing, Christmas holidays got me away from home :( But seems like a fine solution. But with this change, it should now work to hotplug a Displaylink device?

@flokli
Copy link
Contributor Author

flokli commented Dec 27, 2020

Yes. We had the udev rule before, but it was just a shell script checking if a card is connected (in addition to the udev rule already doing that ;-)), that then started dlm.service. Some of the binaries in that shell script weren't accessible, so it failed halfway.

I asked someone to verify this works with a displaylink docking station. Not precisely that commit, but that commit cherry-picked to 20.09.

@peterhoeg
Copy link
Member

Thanks for getting this done @flokli!

@flokli flokli mentioned this pull request Dec 28, 2020
Merged
10 tasks
@flokli
Copy link
Contributor Author

flokli commented Dec 28, 2020

I sent a backport PR in #107795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eyJhb eyJhb Awaiting requested review from eyJhb

@peterhoeg peterhoeg Awaiting requested review from peterhoeg

@nshalman nshalman Awaiting requested review from nshalman

@abbradar abbradar Awaiting requested review from abbradar

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@flokli @eyJhb @peterhoeg