From 684683857688324594e7cd4178f128abd977f06f Mon Sep 17 00:00:00 2001 From: Andre Brait Date: Sun, 4 Oct 2026 03:12:19 +0000 Subject: [PATCH 1/7] fanctrlplus2: find renumbered USB fan controllers when migrating pwm paths A USB HID controller gets a new instance suffix (0003:VID:PID.NNNN) each time it enumerates, and the hwmonN index follows probe order. The startup migration resolved such paths through /sys/class/hwmon/hwmonN, which can belong to another chip after renumbering, so labels and fan groups for the controller were left pointing at a path that no longer exists. Resolve a saved /sys/devices path by wildcarding only those two volatile parts first, keeping the chip-name lookup as the fallback. Refs #11 --- .../plugins/fanctrlplus2/include/Common.php | 49 ++++++++++++------- tests/pwm_path_migration_test.php | 46 +++++++++++++++++ 2 files changed, 76 insertions(+), 19 deletions(-) create mode 100644 tests/pwm_path_migration_test.php diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php index 2e60d7e..36b14d0 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php @@ -124,6 +124,30 @@ function extract_chip_and_pwm_from_path(string $old_path): ?array { return null; } +// Find where a saved /sys/devices pwm path lives now. The hwmonN index follows +// probe order and a USB HID device gets a new instance suffix +// (0003:VID:PID.NNNN) each time it enumerates, so both are wildcarded; the +// rest of the path (bus topology, VID:PID, pwmN) still pins the same device. +function find_moved_pwm_path(string $old_path): ?string { + if (str_contains($old_path, '/class/hwmon/')) return null; // no device identity + $pattern = preg_replace( + ['#/hwmon/hwmon\d+/#', '#(/[0-9A-Fa-f]{4}:[0-9A-Fa-f]{4}:[0-9A-Fa-f]{4})\.[0-9A-Fa-f]+/#'], + ['/hwmon/hwmon*/', '$1.*/'], + $old_path + ); + $matches = glob($pattern) ?: []; + return count($matches) === 1 ? $matches[0] : null; +} + +// Current path for a saved pwm path: the same device first, then any chip with +// the same driver name and pwmN. +function current_pwm_path(string $old_path, array $pwm_map): ?string { + $moved = find_moved_pwm_path($old_path); + if ($moved !== null) return $moved; + $pair = extract_chip_and_pwm_from_path($old_path); + return $pair ? ($pwm_map[$pair[0].':'.$pair[1]] ?? null) : null; +} + function log_migrate(string $msg): void { // Write the plugin log. @file_put_contents("/var/log/fanctrlplus2-migrate.log", @@ -161,13 +185,8 @@ function migrate_cfg_and_labels(string $plugin): void { continue; } - $pair = extract_chip_and_pwm_from_path($old_path); - if (!$pair) { log_migrate("migrate label: skip (unparsable) $old_path"); $out[]=$line; continue; } - [$chip,$pwmN] = $pair; - $key = "$chip:$pwmN"; - if (!isset($pwm_map[$key])) { log_migrate("migrate label: no match for $chip:$pwmN, keep $old_path"); $out[]=$line; continue; } - - $new_path = $pwm_map[$key]; + $new_path = current_pwm_path($old_path, $pwm_map); + if ($new_path === null) { log_migrate("migrate label: no match, keep $old_path"); $out[]=$line; continue; } if ($new_path !== $old_path) { if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { log_migrate("migrate label: $old_path → $new_path ({$o[1]} → {$n[1]})"); @@ -194,19 +213,11 @@ function migrate_cfg_and_labels(string $plugin): void { continue; } - $pair = extract_chip_and_pwm_from_path($old_path); - if (!$pair) { - log_migrate("migrate cfg: skip (unparsable) $cfgfile controller=$old_path"); - continue; - } - [$chip,$pwmN] = $pair; - $key = "$chip:$pwmN"; - if (!isset($pwm_map[$key])) { - log_migrate("migrate cfg: no match for $cfgfile ($chip:$pwmN), keep $old_path"); - continue; + $new_path = current_pwm_path($old_path, $pwm_map); + if ($new_path === null) { + log_migrate("migrate cfg: no match for $cfgfile, keep $old_path"); + continue; } - - $new_path = $pwm_map[$key]; if ($new_path === $old_path) continue; if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { diff --git a/tests/pwm_path_migration_test.php b/tests/pwm_path_migration_test.php new file mode 100644 index 0000000..ac3dd7a --- /dev/null +++ b/tests/pwm_path_migration_test.php @@ -0,0 +1,46 @@ + Date: Sun, 4 Oct 2026 03:15:21 +0000 Subject: [PATCH 2/7] fanctrlplus2: keep unresolved USB controller paths instead of guessing Address review: avoid str_contains on the PHP 7.4 startup path, keep paths that still exist, refuse ambiguous or glob-like matches, and do not fall back to the hwmonN name lookup for a USB HID path that was not found. --- .../plugins/fanctrlplus2/include/Common.php | 14 ++++++++++---- tests/pwm_path_migration_test.php | 17 ++++++++++++++++- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php index 36b14d0..ec3dc30 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php @@ -128,10 +128,15 @@ function extract_chip_and_pwm_from_path(string $old_path): ?array { // probe order and a USB HID device gets a new instance suffix // (0003:VID:PID.NNNN) each time it enumerates, so both are wildcarded; the // rest of the path (bus topology, VID:PID, pwmN) still pins the same device. +const FCP_HID_INSTANCE = '#(/[0-9A-Fa-f]{4}:[0-9A-Fa-f]{4}:[0-9A-Fa-f]{4})\.[0-9A-Fa-f]+/#'; + function find_moved_pwm_path(string $old_path): ?string { - if (str_contains($old_path, '/class/hwmon/')) return null; // no device identity + // A /sys/class path carries no device identity; glob characters would + // turn the saved path into an arbitrary pattern. + if (strpos($old_path, '/class/hwmon/') !== false || strpbrk($old_path, '*?[') !== false) return null; + if (file_exists($old_path)) return $old_path; $pattern = preg_replace( - ['#/hwmon/hwmon\d+/#', '#(/[0-9A-Fa-f]{4}:[0-9A-Fa-f]{4}:[0-9A-Fa-f]{4})\.[0-9A-Fa-f]+/#'], + ['#/hwmon/hwmon\d+/#', FCP_HID_INSTANCE], ['/hwmon/hwmon*/', '$1.*/'], $old_path ); @@ -140,10 +145,11 @@ function find_moved_pwm_path(string $old_path): ?string { } // Current path for a saved pwm path: the same device first, then any chip with -// the same driver name and pwmN. +// the same driver name and pwmN. A USB device that was not found is absent or +// ambiguous; guessing by name would hand its settings to another controller. function current_pwm_path(string $old_path, array $pwm_map): ?string { $moved = find_moved_pwm_path($old_path); - if ($moved !== null) return $moved; + if ($moved !== null || preg_match(FCP_HID_INSTANCE, $old_path)) return $moved; $pair = extract_chip_and_pwm_from_path($old_path); return $pair ? ($pwm_map[$pair[0].':'.$pair[1]] ?? null) : null; } diff --git a/tests/pwm_path_migration_test.php b/tests/pwm_path_migration_test.php index ac3dd7a..43dacd9 100644 --- a/tests/pwm_path_migration_test.php +++ b/tests/pwm_path_migration_test.php @@ -32,9 +32,24 @@ if (find_moved_pwm_path("$port/0003:3904:F001.0005/hwmon/hwmon5/pwm3") !== null) { $failures[] = 'A channel the device no longer has must not match.'; } -if (find_moved_pwm_path('/sys/class/hwmon/hwmon5/pwm2') !== null) { +// Two instances of the same device on one port: nothing pins either one. +$twin = "$root/devices/pci0000:00/0000:00:14.0/usb2/2-1/2-1:1.0"; +$make("$twin/0003:3904:F001.000A/hwmon/hwmon9/pwm1"); +$make("$twin/0003:3904:F001.000B/hwmon/hwmon10/pwm1"); +if (find_moved_pwm_path("$twin/0003:3904:F001.0002/hwmon/hwmon3/pwm1") !== null) { + $failures[] = 'An ambiguous match must not be picked.'; +} +if (find_moved_pwm_path("$twin/0003:3904:F001.000A/hwmon/hwmon9/pwm1") !== "$twin/0003:3904:F001.000A/hwmon/hwmon9/pwm1") { + $failures[] = 'A path that still exists must be kept even when it has siblings.'; +} + +$make("$root/class/hwmon/hwmon3/pwm2"); +if (find_moved_pwm_path("$root/class/hwmon/hwmon5/pwm2") !== null) { $failures[] = 'A /sys/class path carries no device identity and must be left to the chip-name fallback.'; } +if (find_moved_pwm_path("$root/devices/platform/nct6775.*/hwmon/hwmon4/pwm2") !== null) { + $failures[] = 'A saved path must not be used as a glob pattern.'; +} exec('rm -rf ' . escapeshellarg($root)); From a6cc18d05b8fc9eb9361192aaf68ac4dba7fbbaf Mon Sep 17 00:00:00 2001 From: Andre Brait Date: Sun, 4 Oct 2026 04:32:13 +0000 Subject: [PATCH 3/7] fanctrlplus2: don't let migrated paths collide with re-entered ones A user who re-labeled and re-assigned fans after a renumbering has entries for the current path next to stale ones for the old instance. Migrating the stale ones made duplicate label keys, where savelabel edits the first line while the readers use the last, and could point two fans at one PWM. The label already saved for the current path now wins, and a fan is not moved onto a PWM another fan controls. --- .../plugins/fanctrlplus2/include/Common.php | 34 +++++++++++++++---- tests/pwm_path_migration_test.php | 32 +++++++++++++++++ 2 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php index ec3dc30..2d53fba 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php @@ -172,15 +172,18 @@ function safe_rewrite(string $file, string $content): bool { return true; } -function migrate_cfg_and_labels(string $plugin): void { - $cfgpath = "/boot/config/plugins/$plugin"; +function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { + $cfgpath = $cfgpath ?: "/boot/config/plugins/$plugin"; $labelFile = "$cfgpath/pwm_labels.cfg"; $pwm_map = build_pwm_map(); // --- labels --- if (is_file($labelFile)) { $lines = file($labelFile, FILE_IGNORE_NEW_LINES) ?: []; - $changed = false; $out = []; + // A label already saved for the current path (re-entered after the + // device moved) wins over a stale one migrated onto the same path; + // the label readers take the last line and savelabel edits the first. + $changed = false; $out = []; $pos = []; foreach ($lines as $line) { if (!preg_match('/^(.+?)=(.*)$/', $line, $m)) { $out[]=$line; continue; } $old_path = trim($m[1], " \t\n\r\0\x0B\"'"); @@ -194,22 +197,34 @@ function migrate_cfg_and_labels(string $plugin): void { $new_path = current_pwm_path($old_path, $pwm_map); if ($new_path === null) { log_migrate("migrate label: no match, keep $old_path"); $out[]=$line; continue; } if ($new_path !== $old_path) { + $changed = true; + if (isset($pos[$new_path])) { log_migrate("migrate label: drop $old_path, $new_path already labeled"); continue; } if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { log_migrate("migrate label: $old_path → $new_path ({$o[1]} → {$n[1]})"); } else { log_migrate("migrate label: $old_path → $new_path"); } + } elseif (isset($pos[$new_path])) { $changed = true; - $out[] = $new_path.'='.$label; - } else { - $out[] = $line; + $out[$pos[$new_path]] = $line; + continue; } + $pos[$new_path] = count($out); + $out[] = $new_path.'='.$label; } if ($changed) safe_rewrite($labelFile, implode("\n", $out)); } // --- cfgs --- - foreach (glob("$cfgpath/{$plugin}_*.cfg") ?: [] as $cfgfile) { + // A fan assigned to the current path while an old one still pointed at + // the vanished path keeps it: two loops must not drive the same PWM. + $cfgfiles = glob("$cfgpath/{$plugin}_*.cfg") ?: []; + $in_use = []; + foreach ($cfgfiles as $cfgfile) { + $controller = trim((string)((@parse_ini_file($cfgfile) ?: [])['controller'] ?? ''), " \t\n\r\0\x0B\"'"); + if ($controller !== '' && file_exists($controller)) $in_use[$controller] = true; + } + foreach ($cfgfiles as $cfgfile) { $ini = @parse_ini_file($cfgfile); if (!$ini) continue; @@ -225,6 +240,11 @@ function migrate_cfg_and_labels(string $plugin): void { continue; } if ($new_path === $old_path) continue; + if (isset($in_use[$new_path])) { + log_migrate("migrate cfg: keep $old_path in $cfgfile, $new_path is already controlled"); + continue; + } + $in_use[$new_path] = true; if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { log_migrate("migrate cfg: $cfgfile controller: $old_path → $new_path ({$o[1]} → {$n[1]})"); diff --git a/tests/pwm_path_migration_test.php b/tests/pwm_path_migration_test.php index 43dacd9..42eb13b 100644 --- a/tests/pwm_path_migration_test.php +++ b/tests/pwm_path_migration_test.php @@ -51,6 +51,38 @@ $failures[] = 'A saved path must not be used as a glob pattern.'; } +// The reporter's state after re-assigning by hand: stale lines for the old +// instance next to new ones for the current path. +$make("$port/0003:3904:F001.0008/hwmon/hwmon7/pwm8"); +$old = "$port/0003:3904:F001.0005/hwmon/hwmon5"; +$new = "$port/0003:3904:F001.0008/hwmon/hwmon7"; +$cfg = "$root/cfg"; +@mkdir($cfg); +file_put_contents("$cfg/pwm_labels.cfg", implode("\n", [ + '__FCP_HISTORY__=1', + "$old/pwm9=JBOD_old", + "$old/pwm8=Rear", + "$new/pwm9=JBOD", + "$port/0003:3904:F001.0003/hwmon/hwmon4/pwm9=JBOD_older", +]) . "\n"); +file_put_contents("$cfg/fanctrlplus2_old.cfg", "custom=\"old\"\ncontroller=\"$old/pwm9\"\n"); +file_put_contents("$cfg/fanctrlplus2_new.cfg", "custom=\"new\"\ncontroller=\"$new/pwm9\"\n"); +file_put_contents("$cfg/fanctrlplus2_rear.cfg", "custom=\"rear\"\ncontroller=\"$old/pwm8\"\n"); + +migrate_cfg_and_labels('fanctrlplus2', $cfg); + +$labels = file("$cfg/pwm_labels.cfg", FILE_IGNORE_NEW_LINES); +if ($labels !== ['__FCP_HISTORY__=1', "$new/pwm9=JBOD", "$new/pwm8=Rear"]) { + $failures[] = "A label saved for the current path must win over a stale one migrated onto it, once. Got:\n" . implode("\n", $labels); +} +$controller = fn(string $name) => parse_ini_file("$cfg/fanctrlplus2_$name.cfg")['controller']; +if ($controller('old') !== "$old/pwm9") { + $failures[] = 'A fan must not be migrated onto a PWM another fan already controls.'; +} +if ($controller('rear') !== "$new/pwm8") { + $failures[] = 'A fan on a re-enumerated controller must be migrated to its current path.'; +} + exec('rm -rf ' . escapeshellarg($root)); if ($failures) { From e3210cd2ac0116dd274d41423205d0e4d375b2b8 Mon Sep 17 00:00:00 2001 From: Andre Brait Date: Sun, 4 Oct 2026 04:36:28 +0000 Subject: [PATCH 4/7] fanctrlplus2: keep the newest stale label and quote migration log lines Among stale lines landing on one path, keep the last, as the label readers do, rather than the first. Leave empty labels alone, and pass log messages, which now carry saved paths, to logger as one quoted argument. --- .../plugins/fanctrlplus2/include/Common.php | 36 +++++++++++-------- tests/pwm_path_migration_test.php | 20 +++++++---- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php index 2d53fba..38abe60 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php @@ -159,7 +159,7 @@ function log_migrate(string $msg): void { @file_put_contents("/var/log/fanctrlplus2-migrate.log", date("c")." ".$msg."\n", FILE_APPEND); // Mirror the entry to syslog. - @exec("logger -t fanctrlplus2 '$msg'"); + @exec('logger -t fanctrlplus2 ' . escapeshellarg($msg)); } function safe_rewrite(string $file, string $content): bool { @@ -180,37 +180,45 @@ function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { // --- labels --- if (is_file($labelFile)) { $lines = file($labelFile, FILE_IGNORE_NEW_LINES) ?: []; - // A label already saved for the current path (re-entered after the - // device moved) wins over a stale one migrated onto the same path; - // the label readers take the last line and savelabel edits the first. - $changed = false; $out = []; $pos = []; + // Several lines can land on one path: a label re-entered for the + // current path after the device moved, or stale ones from several + // moves. Keep one line, as the readers see it (the last one), except + // that a line saved for the current path beats any migrated one. + $changed = false; $out = []; $pos = []; $current = []; foreach ($lines as $line) { if (!preg_match('/^(.+?)=(.*)$/', $line, $m)) { $out[]=$line; continue; } $old_path = trim($m[1], " \t\n\r\0\x0B\"'"); $label = $m[2]; - if (preg_match('/^__FCP_[A-Z0-9_]+__$/', $old_path)) { + // Flags, and empty labels, which the readers ignore. + if (preg_match('/^__FCP_[A-Z0-9_]+__$/', $old_path) || $label === '') { $out[] = $line; continue; } $new_path = current_pwm_path($old_path, $pwm_map); if ($new_path === null) { log_migrate("migrate label: no match, keep $old_path"); $out[]=$line; continue; } - if ($new_path !== $old_path) { + $is_current = $new_path === $old_path; + if (!$is_current) { $changed = true; - if (isset($pos[$new_path])) { log_migrate("migrate label: drop $old_path, $new_path already labeled"); continue; } if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { log_migrate("migrate label: $old_path → $new_path ({$o[1]} → {$n[1]})"); } else { log_migrate("migrate label: $old_path → $new_path"); } - } elseif (isset($pos[$new_path])) { + } + if (!isset($pos[$new_path])) { + $pos[$new_path] = count($out); + $out[] = $new_path.'='.$label; + } else { $changed = true; - $out[$pos[$new_path]] = $line; - continue; + if (!$is_current && isset($current[$new_path])) { + log_migrate("migrate label: drop $old_path, $new_path already labeled"); + continue; + } + $out[$pos[$new_path]] = $new_path.'='.$label; } - $pos[$new_path] = count($out); - $out[] = $new_path.'='.$label; + if ($is_current) $current[$new_path] = true; } if ($changed) safe_rewrite($labelFile, implode("\n", $out)); } @@ -222,7 +230,7 @@ function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { $in_use = []; foreach ($cfgfiles as $cfgfile) { $controller = trim((string)((@parse_ini_file($cfgfile) ?: [])['controller'] ?? ''), " \t\n\r\0\x0B\"'"); - if ($controller !== '' && file_exists($controller)) $in_use[$controller] = true; + if ($controller !== '') $in_use[$controller] = true; } foreach ($cfgfiles as $cfgfile) { $ini = @parse_ini_file($cfgfile); diff --git a/tests/pwm_path_migration_test.php b/tests/pwm_path_migration_test.php index 42eb13b..b55d477 100644 --- a/tests/pwm_path_migration_test.php +++ b/tests/pwm_path_migration_test.php @@ -51,9 +51,11 @@ $failures[] = 'A saved path must not be used as a glob pattern.'; } -// The reporter's state after re-assigning by hand: stale lines for the old -// instance next to new ones for the current path. +// The reporter's state after renumbering twice and re-assigning by hand: +// stale lines for older instances next to new ones for the current path. $make("$port/0003:3904:F001.0008/hwmon/hwmon7/pwm8"); +$make("$port/0003:3904:F001.0008/hwmon/hwmon7/pwm7"); +$older = "$port/0003:3904:F001.0003/hwmon/hwmon4"; $old = "$port/0003:3904:F001.0005/hwmon/hwmon5"; $new = "$port/0003:3904:F001.0008/hwmon/hwmon7"; $cfg = "$root/cfg"; @@ -62,25 +64,29 @@ '__FCP_HISTORY__=1', "$old/pwm9=JBOD_old", "$old/pwm8=Rear", + "$new/pwm8=", "$new/pwm9=JBOD", - "$port/0003:3904:F001.0003/hwmon/hwmon4/pwm9=JBOD_older", + "$older/pwm9=JBOD_older", + "$older/pwm7=Top_older", + "$old/pwm7=Top", ]) . "\n"); file_put_contents("$cfg/fanctrlplus2_old.cfg", "custom=\"old\"\ncontroller=\"$old/pwm9\"\n"); file_put_contents("$cfg/fanctrlplus2_new.cfg", "custom=\"new\"\ncontroller=\"$new/pwm9\"\n"); file_put_contents("$cfg/fanctrlplus2_rear.cfg", "custom=\"rear\"\ncontroller=\"$old/pwm8\"\n"); +file_put_contents("$cfg/fanctrlplus2_rear2.cfg", "custom=\"rear2\"\ncontroller=\"$older/pwm8\"\n"); migrate_cfg_and_labels('fanctrlplus2', $cfg); $labels = file("$cfg/pwm_labels.cfg", FILE_IGNORE_NEW_LINES); -if ($labels !== ['__FCP_HISTORY__=1', "$new/pwm9=JBOD", "$new/pwm8=Rear"]) { - $failures[] = "A label saved for the current path must win over a stale one migrated onto it, once. Got:\n" . implode("\n", $labels); +if ($labels !== ['__FCP_HISTORY__=1', "$new/pwm9=JBOD", "$new/pwm8=Rear", "$new/pwm8=", "$new/pwm7=Top"]) { + $failures[] = "Each current path must keep one label: the one saved for it, else the last stale one. Got:\n" . implode("\n", $labels); } $controller = fn(string $name) => parse_ini_file("$cfg/fanctrlplus2_$name.cfg")['controller']; if ($controller('old') !== "$old/pwm9") { $failures[] = 'A fan must not be migrated onto a PWM another fan already controls.'; } -if ($controller('rear') !== "$new/pwm8") { - $failures[] = 'A fan on a re-enumerated controller must be migrated to its current path.'; +if ($controller('rear') !== "$new/pwm8" || $controller('rear2') !== "$older/pwm8") { + $failures[] = 'Exactly one of two stale fans on the same PWM must be migrated to its current path.'; } exec('rm -rf ' . escapeshellarg($root)); From 2465139991a96fc0bdcb8ab4f9c945d4cb7a4e59 Mon Sep 17 00:00:00 2001 From: Andre Brait Date: Mon, 5 Oct 2026 02:45:04 +0000 Subject: [PATCH 5/7] fanctrlplus2: persist USB serial identity across port moves Capture VID:PID, serial and controller-relative PWM channel at save and startup, then resolve labels and runtime controller paths from that binding. A saved serial follows the controller even when another identical model occupies the old port; missing or ambiguous bindings remain saved but cannot drive a replacement. Only capture unique serials. Existing duplicated-serial controllers retain their separate port bindings. Current manual assignments take precedence over stale migrated configurations, and losing live assignments stop without discarding their saved binding. Rewrite only controller fields so migration preserves unrelated escaped configuration data. Verified with 25 test files and an isolated mount/PID namespace exercising real label/config save endpoints, getpwm and service startup. --- README.md | 28 +++ .../plugins/fanctrlplus2/include/Common.php | 162 ++++++++++++------ .../fanctrlplus2/include/FanBlockRender.php | 10 +- .../fanctrlplus2/include/FanctrlLogic.php | 44 +---- .../fanctrlplus2/include/FcpAirflow.php | 18 +- .../include/update.fanctrlplus2.php | 1 + .../fanctrlplus2/scripts/rc.fanctrlplus2 | 4 + tests/usb_identity_test.php | 143 ++++++++++++++++ 8 files changed, 302 insertions(+), 108 deletions(-) create mode 100644 tests/usb_identity_test.php diff --git a/README.md b/README.md index 8b773ae..35c9912 100644 --- a/README.md +++ b/README.md @@ -94,6 +94,34 @@ echo "$temp" Remember to make the file executable (`chmod +x`). +## Fan identity and USB port changes + +For USB controllers that expose a unique serial number, fan labels and +configurations are identified by VID:PID, serial number, interface, and PWM +channel. The plugin records this identity when a label or configuration is +saved, and automatically upgrades existing entries at service startup while +their controller can still be identified. Labels use an encoded `usb:` key in +`pwm_labels.cfg`; `controller_identity` stores the same key in each fan +configuration, or a path binding when no unique serial is available. The +runtime `controller` path is resolved again whenever the service starts. + +This preserves assignments when a controller moves to another USB port, even +when an identical model occupies the old port. Missing or ambiguous identities +(including duplicate serial numbers for the same channel) remain saved but +unassigned until they can be resolved; the plugin does not guess another device. + +Controllers without a serial number, controllers whose serial is already +duplicated when first saved, and older entries whose serial has not yet been +recorded use the same-port path migration. Their existing port assignments +remain separate. Keep those controllers on the same port. If an unrecorded +controller has already moved, reassign it once so its serial can be saved. +Restart the service after a driver reload or port move. If multiple +configurations resolve to one channel, the already-current assignment wins; +blocked bindings remain saved for recovery when the conflicting assignment +is removed. +The identity identifies the controller's header, not the physical fan attached +to it; replacing the fan does not change the saved assignment. + Support / Issues - https://github.com/andrebrait/fanctrlplus/issues diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php index 38abe60..9628252 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/Common.php @@ -71,15 +71,6 @@ function normalize_chip_name(string $chip): string { return $chip; } -function build_pwm_map(): array { - $map = []; - foreach (list_pwm() as $pwm) { - if ($pwm['chip'] === '') continue; - $map[normalize_chip_name($pwm['chip']).':'.$pwm['name']] = $pwm['sensor']; - } - return $map; -} - function extract_chip_and_pwm_from_path(string $old_path): ?array { $old_path = trim($old_path, " \t\n\r\0\x0B\"'"); @@ -144,16 +135,66 @@ function find_moved_pwm_path(string $old_path): ?string { return count($matches) === 1 ? $matches[0] : null; } -// Current path for a saved pwm path: the same device first, then any chip with -// the same driver name and pwmN. A USB device that was not found is absent or -// ambiguous; guessing by name would hand its settings to another controller. +// Path-only entries cannot recover a serial after the device has moved. +// Keep the same-port migration for those legacy/no-serial entries. function current_pwm_path(string $old_path, array $pwm_map): ?string { $moved = find_moved_pwm_path($old_path); - if ($moved !== null || preg_match(FCP_HID_INSTANCE, $old_path)) return $moved; + if ($moved !== null || preg_match(FCP_HID_INSTANCE, $old_path) || preg_match('#/usb\d+/#', $old_path)) return $moved; $pair = extract_chip_and_pwm_from_path($old_path); return $pair ? ($pwm_map[$pair[0].':'.$pair[1]] ?? null) : null; } +// A fan is a USB controller plus its interface/hwmon channel, not a USB port. +// Hex-encoded JSON is safe as an unquoted label key and a shell/INI value. +function fcp_pwm_identity(string $path): ?string { + $path = realpath($path); + if ($path === false || !preg_match('#/pwm\d+$#', $path)) return null; + for ($dir = dirname($path); $dir !== dirname($dir); $dir = dirname($dir)) { + if (!is_file("$dir/idVendor") || !is_file("$dir/idProduct")) continue; + $vid = strtolower(trim((string)@file_get_contents("$dir/idVendor"))); + $pid = strtolower(trim((string)@file_get_contents("$dir/idProduct"))); + $serial = trim((string)@file_get_contents("$dir/serial")); + if (!preg_match('/^[0-9a-f]{4}$/', $vid) || !preg_match('/^[0-9a-f]{4}$/', $pid) || $serial === '') return null; + $channel = substr($path, strlen($dir)); + $channel = preg_replace('#^/[^/]+:(\d+\.\d+)/#', '/interface:$1/', $channel); + $channel = preg_replace([FCP_HID_INSTANCE, '#/hwmon/hwmon\d+/#'], ['$1.*/', '/hwmon/hwmon*/'], $channel); + $encoded = json_encode([$vid, $pid, $serial, $channel]); + return $encoded === false ? null : 'usb:'.bin2hex($encoded); + } + return null; +} + +function fcp_pwm_identity_map(array $pwms): array { + $map = []; + foreach ($pwms as $pwm) { + $key = fcp_pwm_identity($pwm['sensor']); + if ($key === null) continue; + // Duplicate serials/channels cannot safely identify either device. + $map[$key] = array_key_exists($key, $map) ? null : $pwm['sensor']; + } + return $map; +} + +// Capture a serial key only while it uniquely identifies this channel. +// Devices with duplicated/placeholder serials retain their port identity. +function fcp_unique_pwm_identity(string $path, ?array $identities = null): ?string { + $key = fcp_pwm_identity($path); + if ($key === null) return null; + $identities = $identities ?? fcp_pwm_identity_map(list_pwm()); + return ($identities[$key] ?? null) === realpath($path) ? $key : null; +} + +function fcp_load_pwm_labels(string $file, string $hwmon_glob = '/sys/class/hwmon/hwmon*'): array { + $identities = fcp_pwm_identity_map(list_pwm($hwmon_glob)); + $labels = []; + foreach (is_file($file) ? file($file, FILE_IGNORE_NEW_LINES) : [] as $line) { + if (!preg_match('/^(.+?)=(.+)$/', $line, $m)) continue; + $path = strpos($m[1], 'usb:') === 0 ? ($identities[$m[1]] ?? null) : $m[1]; + if ($path !== null && preg_match('#/pwm\d+$#', $path)) $labels[$path] = $m[2]; + } + return $labels; +} + function log_migrate(string $msg): void { // Write the plugin log. @file_put_contents("/var/log/fanctrlplus2-migrate.log", @@ -172,10 +213,13 @@ function safe_rewrite(string $file, string $content): bool { return true; } -function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { +function migrate_cfg_and_labels(string $plugin, string $cfgpath = '', string $hwmon_glob = '/sys/class/hwmon/hwmon*'): void { $cfgpath = $cfgpath ?: "/boot/config/plugins/$plugin"; $labelFile = "$cfgpath/pwm_labels.cfg"; - $pwm_map = build_pwm_map(); + $pwms = list_pwm($hwmon_glob); + $pwm_map = []; + foreach ($pwms as $pwm) $pwm_map[normalize_chip_name($pwm['chip']).':'.$pwm['name']] = $pwm['sensor']; + $identities = fcp_pwm_identity_map($pwms); // --- labels --- if (is_file($labelFile)) { @@ -196,9 +240,12 @@ function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { continue; } - $new_path = current_pwm_path($old_path, $pwm_map); + $stable = strpos($old_path, 'usb:') === 0; + $new_path = $stable ? ($identities[$old_path] ?? null) : current_pwm_path($old_path, $pwm_map); if ($new_path === null) { log_migrate("migrate label: no match, keep $old_path"); $out[]=$line; continue; } - $is_current = $new_path === $old_path; + $is_current = $stable || $new_path === $old_path; + $key = fcp_unique_pwm_identity($new_path, $identities) ?? $new_path; + if ($key !== $old_path) $changed = true; if (!$is_current) { $changed = true; if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { @@ -207,18 +254,18 @@ function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { log_migrate("migrate label: $old_path → $new_path"); } } - if (!isset($pos[$new_path])) { - $pos[$new_path] = count($out); - $out[] = $new_path.'='.$label; + if (!isset($pos[$key])) { + $pos[$key] = count($out); + $out[] = $key.'='.$label; } else { $changed = true; - if (!$is_current && isset($current[$new_path])) { + if (!$is_current && isset($current[$key])) { log_migrate("migrate label: drop $old_path, $new_path already labeled"); continue; } - $out[$pos[$new_path]] = $new_path.'='.$label; + $out[$pos[$key]] = $key.'='.$label; } - if ($is_current) $current[$new_path] = true; + if ($is_current) $current[$key] = true; } if ($changed) safe_rewrite($labelFile, implode("\n", $out)); } @@ -227,41 +274,56 @@ function migrate_cfg_and_labels(string $plugin, string $cfgpath = ''): void { // A fan assigned to the current path while an old one still pointed at // the vanished path keeps it: two loops must not drive the same PWM. $cfgfiles = glob("$cfgpath/{$plugin}_*.cfg") ?: []; - $in_use = []; - foreach ($cfgfiles as $cfgfile) { - $controller = trim((string)((@parse_ini_file($cfgfile) ?: [])['controller'] ?? ''), " \t\n\r\0\x0B\"'"); - if ($controller !== '') $in_use[$controller] = true; - } + $configs = []; $in_use = []; foreach ($cfgfiles as $cfgfile) { $ini = @parse_ini_file($cfgfile); if (!$ini) continue; - + $configs[$cfgfile] = $ini; + $path = (string)($ini['controller'] ?? ''); + $identity = (string)($ini['controller_identity'] ?? ''); + $source = $identity !== '' ? $identity : $path; + $resolved = strpos($source, 'usb:') === 0 ? ($identities[$source] ?? null) : (is_file($source) ? $source : null); + // Already-current manual assignments win over stale migrated ones. + if ($resolved === $path && $path !== '' && !isset($in_use[$path])) $in_use[$path] = $cfgfile; + } + foreach ($configs as $cfgfile => $ini) { + $original = $ini; $old_path = trim((string)($ini['controller'] ?? ''), " \t\n\r\0\x0B\"'"); - - if ($old_path === '' || !preg_match('#/hwmon\d+/pwm\d+$#', $old_path)) { - continue; - } - - $new_path = current_pwm_path($old_path, $pwm_map); - if ($new_path === null) { - log_migrate("migrate cfg: no match for $cfgfile, keep $old_path"); - continue; - } - if ($new_path === $old_path) continue; - if (isset($in_use[$new_path])) { + $identity = (string)($ini['controller_identity'] ?? ''); + if ($identity === '' && !preg_match('#/pwm\d+$#', $old_path)) continue; + $source = $identity !== '' ? $identity : $old_path; + $new_path = strpos($source, 'usb:') === 0 ? ($identities[$source] ?? null) : current_pwm_path($source, $pwm_map); + if ($new_path !== null && isset($in_use[$new_path]) && $in_use[$new_path] !== $cfgfile) { log_migrate("migrate cfg: keep $old_path in $cfgfile, $new_path is already controlled"); - continue; + if ($identity === '' && !is_file($old_path)) continue; + // A live losing path must also stop. Retain its binding for + // recovery when the winning assignment is removed. + if ($identity === '') $identity = fcp_unique_pwm_identity($old_path, $identities) ?? $old_path; + $ini['controller_identity'] = $identity; + $new_path = null; } - $in_use[$new_path] = true; - - if (preg_match('#/(hwmon\d+)/#', $old_path, $o) && preg_match('#/(hwmon\d+)/#', $new_path, $n)) { - log_migrate("migrate cfg: $cfgfile controller: $old_path → $new_path ({$o[1]} → {$n[1]})"); + if ($new_path === null) { + log_migrate("migrate cfg: no match for $cfgfile, identity=$identity"); + // Never drive a replacement at the old path. Keep the identity + // so a disconnected/ambiguous controller can recover later. + if ($identity === '') continue; + $ini['controller'] = ''; } else { - log_migrate("migrate cfg: $cfgfile controller: $old_path → $new_path"); + $in_use[$new_path] = $cfgfile; + $ini['controller'] = $new_path; + $ini['controller_identity'] = fcp_unique_pwm_identity($new_path, $identities) ?? $new_path; + if ($new_path !== $old_path) log_migrate("migrate cfg: $cfgfile controller: $old_path → $new_path"); + } + if ($ini === $original) continue; + // Only these two fields changed. Re-serializing the entire INI would + // re-escape free-text values that are also sourced by the shell. + $buf = (string)file_get_contents($cfgfile); + foreach (['controller', 'controller_identity'] as $k) { + $v = str_replace(["\\", '"', '$', '`', "\r", "\n"], ["\\\\", '\\"', '\\$', '\\`', '', ''], (string)$ini[$k]); + $line = "$k=\"$v\""; + $buf = preg_replace_callback('/^[ \t]*'.$k.'[ \t]*=.*$/m', function () use ($line) { return $line; }, $buf, -1, $count); + if ($count === 0) $buf = rtrim($buf, "\n")."\n".$line."\n"; } - - $ini['controller'] = $new_path; - $buf=''; foreach ($ini as $k=>$v){ $v=str_replace('"','',(string)$v); $buf.=$k.'="'.$v."\"\n"; } safe_rewrite($cfgfile, $buf); } } diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanBlockRender.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanBlockRender.php index 61755cf..b7d7ef1 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanBlockRender.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanBlockRender.php @@ -1,13 +1,7 @@ 0 -> read disk_group_{g}_* keys. Otherwise fall back to ONE group diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php index 4b85c3c..2ac355f 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php @@ -110,38 +110,19 @@ function scan_dir($dir) { $pwm = $_POST['pwm'] ?? ''; $label = $_POST['label'] ?? ''; - $label_file = "/boot/config/plugins/fanctrlplus2/pwm_labels.cfg"; - // Read existing labels. - $lines = is_file($label_file) ? file($label_file, FILE_IGNORE_NEW_LINES) : []; - $found = false; - if (!$pwm) { json_response(['status' => 'error', 'message' => 'Missing pwm']); break; } - - // An empty label means delete it. - if ($label === '') { - $new_lines = []; - foreach ($lines as $line) { - if (strpos($line, "$pwm=") !== 0) $new_lines[] = $line; - } - file_put_contents($label_file, implode("\n", $new_lines) . "\n"); - json_response(['status' => 'ok', 'message' => 'Label removed']); - break; - } - - // Write the label. - foreach ($lines as &$line) { - if (strpos($line, "$pwm=") === 0) { - $line = "$pwm=$label"; - $found = true; - break; - } - } - if (!$found) $lines[] = "$pwm=$label"; + $key = fcp_unique_pwm_identity($pwm) ?? $pwm; + $lines = is_file($label_file) ? file($label_file, FILE_IGNORE_NEW_LINES) : []; + // Remove both the legacy path and stable key before saving one entry. + $lines = array_values(array_filter($lines, function ($line) use ($key, $pwm) { + return strpos($line, "$key=") !== 0 && strpos($line, "$pwm=") !== 0; + })); + if ($label !== '') $lines[] = "$key=$label"; file_put_contents($label_file, implode("\n", $lines) . "\n"); - json_response(['status' => 'ok', 'message' => 'Label saved']); + json_response(['status' => 'ok', 'message' => $label === '' ? 'Label removed' : 'Label saved']); break; case 'newtemp': @@ -324,14 +305,7 @@ function scan_dir($dir) { case 'getpwm': $pwms = list_pwm(); $label_file = "/boot/config/plugins/fanctrlplus2/pwm_labels.cfg"; - $labels = []; - if (is_file($label_file)) { - foreach (file($label_file, FILE_IGNORE_NEW_LINES) as $line) { - if (preg_match('/^(.+?)=(.+)$/', $line, $m)) { - $labels[$m[1]] = $m[2]; - } - } - } + $labels = fcp_load_pwm_labels($label_file); foreach ($pwms as &$pwm) { $pwm['label'] = $labels[$pwm['sensor']] ?? ''; } diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FcpAirflow.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FcpAirflow.php index 003e82e..f98fa97 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FcpAirflow.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FcpAirflow.php @@ -1,27 +1,15 @@ label - if (!is_file($cfg)) return $dirN_to_label; - - $lines = file($cfg, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES); - foreach ($lines as $line) { - $line = trim($line); - if ($line === '' || $line[0] === '#') continue; - - $eq = strpos($line, '='); - if ($eq === false) continue; - - $pwm_path = trim(substr($line, 0, $eq)); - $label = trim(substr($line, $eq+1)); - if ($label === '') continue; + foreach (fcp_load_pwm_labels($cfg) as $pwm_path => $label) { if (preg_match('~/pwm(\d+)$~i', $pwm_path, $m)) { $n = (int)$m[1]; diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php index 76bbd84..6b49d0e 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php @@ -247,6 +247,7 @@ 'label' => $custom, 'service' => $_POST['service'][$i] ?? '0', 'controller' => $controller, + 'controller_identity' => fcp_unique_pwm_identity($controller) ?? $controller, 'pwm' => $pwm, 'max' => $max_pwm, 'idle' => (string)$idle_abs, diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/scripts/rc.fanctrlplus2 b/src/usr/local/emhttp/plugins/fanctrlplus2/scripts/rc.fanctrlplus2 index 434b0e1..0541630 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/scripts/rc.fanctrlplus2 +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/scripts/rc.fanctrlplus2 @@ -36,6 +36,10 @@ fanctrlplus2.start() { service=$(grep -Po '^service="\K[^"]+' "$cfg") [[ "$service" != "1" ]] && continue + # Serial resolution can leave an absent/ambiguous controller unassigned. + controller=$(grep -Po '^controller="\K[^"]+' "$cfg") + [[ -n "$controller" && -w "$controller" ]] || continue + # Sanitize the custom name before using it in a PID filename. raw_custom=$(grep -Po '^custom="\K[^"]+' "$cfg") clean_custom=$(echo "$raw_custom" | tr -d '\r\n\t ') diff --git a/tests/usb_identity_test.php b/tests/usb_identity_test.php new file mode 100644 index 0000000..333a5fe --- /dev/null +++ b/tests/usb_identity_test.php @@ -0,0 +1,143 @@ + 'JBOD'], 'Labels must follow the serial and keep channel 10.'); + $check(parse_ini_file($file)['controller_identity'] === $identity, 'Re-enumeration must not change the saved serial/channel identity.'); + $check(strpos(file_get_contents($file), $metadata) !== false, 'A port move must preserve unrelated configuration bytes.'); + $check(strpos(file_get_contents("$cfg/pwm_labels.cfg"), '__FCP_HISTORY__=1') !== false, 'Serial migration must preserve dashboard flags.'); + + // Duplicate serial numbers cannot safely identify either controller. + $duplicate = $device('3-2', 'hwmon12', '2080376E4548'); + $migrate(); + $check(parse_ini_file($file)['controller'] === '', 'Duplicate serials must disable the runtime controller, not guess.'); + $check(parse_ini_file($file)['controller_identity'] === $identity, 'Unresolved identity must remain saved for recovery.'); + $check(fcp_load_pwm_labels("$cfg/pwm_labels.cfg", "$hwmon/hwmon*") === [], 'Ambiguous serial labels must not attach to either device.'); + + unlink("$hwmon/hwmon12"); + $migrate(); + $check(parse_ini_file($file)['controller'] === $moved, 'A resolved serial must restore its runtime path after ambiguity clears.'); + $check(fcp_load_pwm_labels("$cfg/pwm_labels.cfg", "$hwmon/hwmon*") === [$moved => 'JBOD'], 'Labels must survive ambiguity and reappear on recovery.'); + unlink("$hwmon/hwmon9"); + $migrate(); + $check(parse_ini_file($file)['controller'] === '', 'An absent serial must not fall back to an identical replacement.'); + + // No serial: keep the legacy same-port behavior, not a VID:PID guess. + $noSerial = $device('4-1', 'hwmon15', ''); + $check(fcp_pwm_identity($noSerial) === null, 'A device without a serial must not get a serial identity.'); + $legacyFile = "$cfg/fanctrlplus2_legacy.cfg"; + file_put_contents($legacyFile, "custom=\"legacy\"\ncontroller=\"$noSerial\"\n"); + $migrate(); + $check(strpos((string)(parse_ini_file($legacyFile)['controller_identity'] ?? ''), 'usb:') !== 0, 'A serial-less configuration must retain its path identity.'); + rename("$root/devices/4-1", "$root/removed_no_serial"); + unlink("$hwmon/hwmon15"); + $noSerialMoved = $device('4-2', 'hwmon16', ''); + $migrate(); + $check(parse_ini_file($legacyFile)['controller'] !== $noSerialMoved, 'A serial-less port move must not be guessed by VID:PID.'); + + // Duplicate serials present at upgrade must not collapse working port + // assignments into an ambiguous key. + $a = $device('5-1', 'hwmon20', 'FAKE'); + $b = $device('5-2', 'hwmon21', 'FAKE'); + $dupCfg = "$root/duplicates"; + mkdir($dupCfg); + file_put_contents("$dupCfg/pwm_labels.cfg", "$a=Front\n$b=Rear\n"); + file_put_contents("$dupCfg/fanctrlplus2_a.cfg", "controller=\"$a\"\n"); + file_put_contents("$dupCfg/fanctrlplus2_b.cfg", "controller=\"$b\"\n"); + for ($i = 0; $i < 2; $i++) migrate_cfg_and_labels('fanctrlplus2', $dupCfg, "$hwmon/hwmon*"); + $check(fcp_load_pwm_labels("$dupCfg/pwm_labels.cfg", "$hwmon/hwmon*") === [$a=>'Front', $b=>'Rear'], 'Ambiguous serials must leave both existing path labels intact.'); + $check(parse_ini_file("$dupCfg/fanctrlplus2_a.cfg")['controller'] === $a && parse_ini_file("$dupCfg/fanctrlplus2_b.cfg")['controller'] === $b, 'Ambiguous serials at capture must not disable existing port assignments.'); + + // A manually reassigned current path wins over a moved identity, + // independent of the two filenames. + foreach ([['aa', 'zz'], ['zz', 'aa']] as [$serialName, $liveName]) { + $collision = "$root/collision_$serialName"; + mkdir($collision); + $serialFile = "$collision/fanctrlplus2_$serialName.cfg"; + $liveFile = "$collision/fanctrlplus2_$liveName.cfg"; + $known = fcp_pwm_identity($replacement); + file_put_contents($serialFile, "controller=\"$root/gone/hwmon1/pwm10\"\ncontroller_identity=\"$known\"\n"); + file_put_contents($liveFile, "controller=\"$replacement\"\n"); + migrate_cfg_and_labels('fanctrlplus2', $collision, "$hwmon/hwmon*"); + $check(parse_ini_file($liveFile)['controller'] === $replacement && parse_ini_file($serialFile)['controller'] === '', 'A current manual assignment must win without two loops, in either filename order.'); + unlink($liveFile); + migrate_cfg_and_labels('fanctrlplus2', $collision, "$hwmon/hwmon*"); + $check(parse_ini_file($serialFile)['controller'] === $replacement, 'A blocked identity must recover when the other assignment is removed.'); + } + + // Two path-only configs on one live channel also cannot launch two + // loops. The losing path binding remains available for later recovery. + $same = "$root/same_channel"; + mkdir($same); + $first = "$same/fanctrlplus2_a.cfg"; + $second = "$same/fanctrlplus2_b.cfg"; + file_put_contents($first, "controller=\"$noSerialMoved\"\n"); + file_put_contents($second, "controller=\"$noSerialMoved\"\n"); + migrate_cfg_and_labels('fanctrlplus2', $same, "$hwmon/hwmon*"); + $check(parse_ini_file($first)['controller'] === $noSerialMoved && parse_ini_file($second)['controller'] === '', 'Two live path-only assignments must leave exactly one runtime controller.'); + unlink($first); + migrate_cfg_and_labels('fanctrlplus2', $same, "$hwmon/hwmon*"); + $check(parse_ini_file($second)['controller'] === $noSerialMoved, 'A blocked path binding must recover when its owner is removed.'); + + // USB drivers need not be HID drivers: the relative path does not + // itself carry VID:PID, so the identity must include both explicitly. + $nonHid = []; + foreach ([['6-1', 'hwmon30', '1111', '2222'], ['6-2', 'hwmon31', '3333', '4444']] as [$port, $index, $vid, $pid]) { + $usb = "$root/devices/$port"; + $dir = "$usb/$port:1.0/hwmon/$index"; + mkdir($dir, 0777, true); + foreach (['idVendor'=>$vid, 'idProduct'=>$pid, 'serial'=>'SHARED'] as $key=>$value) file_put_contents("$usb/$key", "$value\n"); + file_put_contents("$dir/name", "usb_fan\n"); + touch("$dir/pwm10"); + symlink($dir, "$hwmon/$index"); + $nonHid[] = "$dir/pwm10"; + } + $keyA = fcp_pwm_identity($nonHid[0]); + $keyB = fcp_pwm_identity($nonHid[1]); + $check($keyA !== null && $keyB !== null && $keyA !== $keyB, 'VID:PID must distinguish devices reporting the same serial.'); + $map = fcp_pwm_identity_map(list_pwm("$hwmon/hwmon*")); + $check(($map[$keyA] ?? null) === $nonHid[0] && ($map[$keyB] ?? null) === $nonHid[1], 'Different VID:PID pairs must not become an ambiguous serial.'); +} + +exec('rm -rf '.escapeshellarg($root)); +if ($failures) { fwrite(STDERR, implode("\n", $failures)."\n"); exit(1); } +echo "USB serial identity tests passed\n"; From 9c3c0c53d9d41441f6f3df227449fa9fbe4fb7f1 Mon Sep 17 00:00:00 2001 From: Andre Brait Date: Mon, 5 Oct 2026 08:00:12 +0000 Subject: [PATCH 6/7] fanctrlplus2: retain unresolved controller bindings when saving An empty posted runtime path is not an explicit reassignment. Read the prior configuration before renaming or overwriting it and preserve its serial identity or legacy path binding when unresolved. A nonempty selection still captures the replacement controller. Verified failing-before/passing-after through the real save endpoint in an isolated mount/PID namespace: empty save and rename preserve the identity, unrelated interval changes persist, reconnect restores control, explicit reassignment replaces the identity, and legacy path bindings survive. All 25 test files pass. --- README.md | 2 ++ .../plugins/fanctrlplus2/include/update.fanctrlplus2.php | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 35c9912..e31183f 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,8 @@ Restart the service after a driver reload or port move. If multiple configurations resolve to one channel, the already-current assignment wins; blocked bindings remain saved for recovery when the conflicting assignment is removed. +Saving a configuration without a resolved controller, including a rename, +retains its binding. Selecting a different controller replaces the binding. The identity identifies the controller's header, not the physical fan attached to it; replacing the fan does not change the saved assignment. diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php index 6b49d0e..d98dc95 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/update.fanctrlplus2.php @@ -47,6 +47,12 @@ $expected_file = $plugin . '_' . $custom . '.cfg'; $old_path = "$cfgpath/$old_file"; $new_path = "$cfgpath/$expected_file"; + // Read before rename/overwrite: an empty runtime controller means the + // saved binding is unresolved, not that the user selected another device. + $previous = is_file($old_path) ? (@parse_ini_file($old_path) ?: []) : []; + $controller_identity = $controller === '' + ? (string)($previous['controller_identity'] ?? $previous['controller'] ?? '') + : (fcp_unique_pwm_identity($controller) ?? $controller); // Read the raw values first. $pwm_percent_raw = $_POST['pwm_percent'][$i] ?? ''; @@ -247,7 +253,7 @@ 'label' => $custom, 'service' => $_POST['service'][$i] ?? '0', 'controller' => $controller, - 'controller_identity' => fcp_unique_pwm_identity($controller) ?? $controller, + 'controller_identity' => $controller_identity, 'pwm' => $pwm, 'max' => $max_pwm, 'idle' => (string)$idle_abs, From 322007c214901d7b22b40e5b1f1c25c573023ced Mon Sep 17 00:00:00 2001 From: Andre Brait Date: Mon, 5 Oct 2026 08:14:19 +0000 Subject: [PATCH 7/7] fanctrlplus2: remove dormant USB identity when clearing a label Apply CodeRabbit's suggested deletion rule: also obtain the raw PWM identity and remove its stored entry when the label is cleared, even while that serial is ambiguous. Preserve identity entries for nonempty ambiguous saves and when no identity is available. Verified failing-before/passing-after through the real label endpoint in an isolated mount/PID namespace. Cleared labels stay cleared after ambiguity resolves; nonempty ambiguous saves, unrelated labels, dashboard flags and no-serial deletion remain unchanged. All 25 test files pass. --- README.md | 2 ++ .../emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e31183f..00e62a9 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,8 @@ blocked bindings remain saved for recovery when the conflicting assignment is removed. Saving a configuration without a resolved controller, including a rename, retains its binding. Selecting a different controller replaces the binding. +Clearing a label also removes its dormant USB identity entry, so it cannot +reappear after serial ambiguity is resolved. The identity identifies the controller's header, not the physical fan attached to it; replacing the fan does not change the saved assignment. diff --git a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php index 2ac355f..0363fce 100644 --- a/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php +++ b/src/usr/local/emhttp/plugins/fanctrlplus2/include/FanctrlLogic.php @@ -114,11 +114,13 @@ function scan_dir($dir) { json_response(['status' => 'error', 'message' => 'Missing pwm']); break; } + $identity = fcp_pwm_identity($pwm); $key = fcp_unique_pwm_identity($pwm) ?? $pwm; $lines = is_file($label_file) ? file($label_file, FILE_IGNORE_NEW_LINES) : []; // Remove both the legacy path and stable key before saving one entry. - $lines = array_values(array_filter($lines, function ($line) use ($key, $pwm) { - return strpos($line, "$key=") !== 0 && strpos($line, "$pwm=") !== 0; + $lines = array_values(array_filter($lines, function ($line) use ($key, $pwm, $identity, $label) { + return strpos($line, "$key=") !== 0 && strpos($line, "$pwm=") !== 0 + && ($label !== '' || $identity === null || strpos($line, "$identity=") !== 0); })); if ($label !== '') $lines[] = "$key=$label"; file_put_contents($label_file, implode("\n", $lines) . "\n");