WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@prahal
Copy link
Collaborator

@prahal prahal commented Nov 13, 2025

Debian fancontrol service PrivateDevices=yes default prevents access to these /dev symlinks.

Description

Trixie fancontrol has PrivateDevices=yes, which prevents access to host /dev. But helios64 sets up
symlinks to thermal and hwmon sysfs entries into /dev via udev rules and uses these links in fancontrol config.
Add a fancontrol systems service drop-in top set PrivateDevices=no in helios64 bsp package to allow
access to these symlinks.

Reported in forum: https://forum.armbian.com/topic/30074-helios64-armbian-2308-bookworm-issues-solved/page/12/#findComment-228274

Likely that also affects helios4, which has the same udev hacks to prepare symlinks to sysfs files for fancontrol.

Mind that I question the requirements for these udev symlinks; this needs to be sorted out if they are needed or were hacks.
Also, there are only 3 boards using fancontrol. I question the use of fancontrol. Maybe we should ship a thermal policy in dts instead.
But for now, let's fix the existing stack.

How Has This Been Tested?

  • build armbian-bsp-cli for trixie current, install it on target board, reload systemd daemon, restart fancontrol service

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

Summary by CodeRabbit

  • Bug Fixes
    • Updated fan control service configuration on Helios64 systems by installing a service override to permit device access, ensuring the fan control daemon can operate correctly with hardware devices.

@prahal prahal requested a review from a team as a code owner November 13, 2025 02:44
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 13, 2025

Walkthrough

Adds a systemd override file disabling PrivateDevices for fancontrol.service on Helios64 and installs it into /etc/systemd/system/fancontrol.service.d/privatedevices.conf from the board BSP tweaks during build.

Changes

Cohort / File(s) Change Summary
Helios64 fancontrol systemd override
packages/bsp/helios64/fancontrol.service.privatedevices-override
New systemd override file containing [Service]\nPrivateDevices=no
Helios64 board configuration
config/boards/helios64.conf
Added install step in post_family_tweaks_bsp__helios64 to copy the override to /etc/systemd/system/fancontrol.service.d/privatedevices.conf and an explanatory comment

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Small, localized configuration addition
  • Check the installed path and override filename are correct
  • Verify the comment accurately describes purpose

Suggested reviewers

  • Heisath
  • chainsx

Poem

🐰 I hopped into code with a curious glance,
Gave fancontrol room to spin and dance,
One tiny file, a gentle decree,
Let cooling breath live systemd-free.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a workaround for fancontrol's /dev access restriction on Helios64 by overriding systemd's PrivateDevices setting to allow access to udev-created /dev symlinks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between aa64b2d and 202587d.

📒 Files selected for processing (2)
  • config/boards/helios64.conf (1 hunks)
  • packages/bsp/helios64/fancontrol.service.privatedevices-override (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/bsp/helios64/fancontrol.service.privatedevices-override
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: igorpecovnik
Repo: armbian/build PR: 8720
File: lib/functions/rootfs/distro-specific.sh:38-47
Timestamp: 2025-11-09T22:30:27.163Z
Learning: In lib/functions/rootfs/distro-specific.sh, the systemd sleep.conf.d override that disables suspend/hibernation is intentionally applied system-wide to all Armbian images (desktop, CLI, and minimal), not gated to desktop-only builds, because suspend/resume is fragile on most boards.
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.

Applied to files:

  • config/boards/helios64.conf
📚 Learning: 2025-08-03T15:21:20.148Z
Learnt from: pyavitz
Repo: armbian/build PR: 8455
File: config/sources/families/sun50iw1.conf:19-24
Timestamp: 2025-08-03T15:21:20.148Z
Learning: In the Armbian build system, when copying firmware files during family_tweaks_s(), use /lib/firmware/updates/ instead of /lib/firmware/ to avoid conflicts with the Armbian firmware package. The /lib/firmware/updates directory takes precedence in Linux firmware loading hierarchy and is the proper location for user-installed firmware files.

Applied to files:

  • config/boards/helios64.conf
🔇 Additional comments (1)
config/boards/helios64.conf (1)

44-45: LGTM! Implementation is correct and override file verified.

The installation of the fancontrol privatedevices override is properly implemented and follows the established pattern from line 42. The override file exists in the PR at packages/bsp/helios64/fancontrol.service.privatedevices-override.

Security consideration: Setting PrivateDevices=no weakens systemd isolation but is necessary for the service to access udev-created /dev symlinks. This tradeoff is acknowledged in the PR.

Regarding Helios4: While Helios4 does have similar hwmon udev rules and fancontrol configuration, the PR description explicitly notes "Likely that also affects helios4... But for now, let's fix the existing stack." The Helios4 fix is intentionally scoped for a separate effort.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added size/small PR with less then 50 lines 11 Milestone: Fourth quarter release Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... BSP Board Support Packages labels Nov 13, 2025
…v symlinks

Debian fancontrol service PrivateDevices=yes default prevents access
to these /dev symlinks.
@prahal prahal force-pushed the fix-helios64-trixie-fancontrol branch from aa64b2d to 202587d Compare November 13, 2025 02:50
@prahal prahal changed the title Helios64: Helios64 uses udev /dev symlinks to target thermal/hwmon dirs Helios64: workaround fancontrol /dev restriction vs Helios64 udev /dev symlinks Nov 13, 2025
@prahal
Copy link
Collaborator Author

prahal commented Nov 13, 2025

v25.08 is also affected, but I believe there won't be any more fix releases for 25.08.

@igorpecovnik
Copy link
Member

v25.08 is also affected, but I believe there won't be any more fix releases for 25.08.

No. This will get there anyway.

@github-actions github-actions bot added the Ready to merge Reviewed, tested and ready for merge label Nov 13, 2025
@github-actions
Copy link
Contributor

✅ This PR has been reviewed and approved — all set for merge!

@github-actions github-actions bot removed the Needs review Seeking for review label Nov 13, 2025
@igorpecovnik igorpecovnik merged commit 0e52814 into armbian:main Nov 13, 2025
10 checks passed
@leggewie
Copy link
Collaborator

leggewie commented Nov 16, 2025

You know it is the wrong thing to do, merged it nonetheless. Again.

In your defense, I have to admit, I am not aware of the proper fix but we should have consulted with the Debian maintainer of the package to find out.

@igorpecovnik
Copy link
Member

You know it is the wrong

I don’t claim to be perfect or to have superhuman oversight. When a contributor confirms that something is ready, I generally trust their judgement and proceed with the merge. This is how collaborative projects typically operate.

we should have consulted with the Debian maintainer

If you feel that additional coordination is needed, please feel free to reach out to them or handle it in the way you believe is appropriate. If you confirm that the result works, I will merge it based on your assessment, without requiring further checks or external confirmation.

If the current process feels too dependent on my decisions, I’m also open to enabling auto-merge so it’s clear that this isn’t about personal preference but about streamlining our workflow.

@leggewie
Copy link
Collaborator

leggewie commented Nov 16, 2025

It's not really about superhuman oversight.

But you were made aware that Debian specifically introduced PrivateDevices=yes as a hardening measure. More info at https://bugs.debian.org/944808. I trust the Debian maintainer to know best what is appropriate for this package and they made a very specific choice here and you knew this full well, overriding and reverting that choice without understanding the implications. This kind of approach is a common topic in Armbian, unfortunately.

This is not about Armbian processes but about decisions. This was a poor decision. It made things worse by going against a specifc upstream decision. And it was known for this to be the case before the merge but as usual it was merged regardless overriding a clear veto once more.

@leggewie
Copy link
Collaborator

Just to be clear, it is absolutely OK to override upstream. I do it all the time. But then you should have a clear understanding of the what and why. There's no understanding here at all.

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

Labels

11 Milestone: Fourth quarter release BSP Board Support Packages Hardware Hardware related like kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

3 participants