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.

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

rfkill code refactoring #1015

Merged
merged 6 commits into from
Feb 11, 2021
Merged

rfkill code refactoring #1015

merged 6 commits into from
Feb 11, 2021

Conversation

alebastr
Copy link
Contributor

@alebastr alebastr commented Feb 3, 2021

I only wanted to fix that one event size check in rfkill.cpp. Should have more willpower to stop and ignore the rest 😭

Summary:

  • Reopening "/dev/rfkill" for each event isn't optimal and could miss some events. Especially since we were ignoring ADD event with the initial state of the device.
  • Rfkill doesn't even need a thread. Glib::MainLoop is fast enough to handle events on a couple of file descriptors.
  • Bluetooth::interval_thread_ was not doing anything useful because it had no way to get changed status value. Now we have 3 less threads 😄
  • I'm actually ashamed of the last commit, but untangling the locks and threads in network.cpp was beyond an evening coding exercise. Although I'm quite curious why we have to sleep for 5 seconds while holding the mutex and will check that on another evening.

Fixes #994

Kernel 5.11 added one more field to the `struct rfkill_event` and broke
unnecessarily strict check in `rfkill.cpp`. According to `linux/rfkill.h`,
we must accept events at least as large as v1 event size and should be
prepared to get additional fields at the end of a v1 event structure.
Open rfkill device only once per module.
Remove rfkill threads and use `Glib::signal_io` as a more efficient way
to poll the rfkill device.
Handle runtime errors from rfkill and stop polling of the device instead
of crashing waybar.
The string was always overwritten in `update()`; don't need to store
temporary value in the class.
The timer thread was always reading the same value from Rfkill state.
Moving rfkill to the main event loop had unexpected side-effects.
Notably, the network module mutex can block all the main thread events
for several seconds while the network worker thread is sleeping.

Instead of waiting for the mutex let's hope that the worker thread
succeeds and schedule timer thread wakeup just in case.
@alebastr alebastr marked this pull request as ready for review February 10, 2021 05:28
@alebastr
Copy link
Contributor Author

I haven't found time for a better look at the network.cpp, but let's not make that block the review. Suggestions on dealing with rfkill wakeup in network module would be appreciated.


@rxguy, @pvalena, do you mind trying this PR? Only the first commit is required to resolve #994 and that's the only commit I'm planning to cherry-pick for Fedora package, but it'll be nice to test the rest of the changes here on another system.

@pvalena
Copy link

pvalena commented Feb 10, 2021

@alebastr Sure I'd try... can you build the package in COPR for rawhide?

@alebastr
Copy link
Contributor Author

src/util/rfkill.cpp Outdated Show resolved Hide resolved
@Alexays
Copy link
Owner

Alexays commented Feb 10, 2021

Just a small comment, otherwise LGTM, no suggestion for the network part, relying on the timer thread is a good idea.

@Alexays
Copy link
Owner

Alexays commented Feb 11, 2021

LGTM thanks Aleksei!

@Alexays Alexays merged commit 08e1960 into Alexays:master Feb 11, 2021
@alebastr alebastr deleted the rfkill-events branch February 11, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on start - 'std::runtime_error' - Wrong size of RFKILL event
3 participants