Development #170
Development #170
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe PR adds ACPIResetProcedure and AcpiFindTable to the ACPI driver and updates shutdown/reboot flows to invoke reset/unmount steps. Filesystem drivers (EXT2, FAT1x, NTFS) gain public Unmount functions and are wired into a new VfsUmount and VfsUnmountAll API. Scheduler interfaces gain KillAllProcess variants with scheduler-specific implementations. Shell gains Mount/Unmount commands. MemPool.h removes FastAlloc/FastFree. Build changes switch a SataDisk image to FAT12 in CMake. README updates adjust language and build instructions. Public headers updated accordingly. Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
drivers/ACPI.c (1)
178-186: Avoid reinitializing the shutdown values array and guard FADT usage.
- Move the shutdown values array out of the loop.
- Null/zero‑guard g_fadt and pm1a_control_block before outw().
Suggested change:
-// Try multiple shutdown methods -for (int i = 0; i < 4; i++) { - const uint16_t shutdown_values[] = {0x2000, 0x3C00, 0x1400, 0x0000}; - PrintKernel("ACPI: Trying shutdown value 0x"); - PrintKernelHex(shutdown_values[i]); - PrintKernel(" on port 0x"); - PrintKernelHex(g_fadt->pm1a_control_block); - PrintKernel("\n"); - - outw(g_fadt->pm1a_control_block, shutdown_values[i]); - - // Wait a bit - delay(10); -} +// Try multiple shutdown methods via PM1a +static const uint16_t k_pm1_shutdown_values[4] = {0x2000, 0x3C00, 0x1400, 0x0000}; +if (!g_fadt || g_fadt->pm1a_control_block == 0) { + PrintKernelError("ACPI: FADT/PM1a control block unavailable; skipping PM1a shutdown\n"); +} else { + for (int i = 0; i < 4; i++) { + PrintKernel("ACPI: Trying shutdown value 0x"); + PrintKernelHex(k_pm1_shutdown_values[i]); + PrintKernel(" on port 0x"); + PrintKernelHex(g_fadt->pm1a_control_block); + PrintKernel("\n"); + outw(g_fadt->pm1a_control_block, k_pm1_shutdown_values[i]); + delay(10); + } +}fs/EXT/Ext2.c (2)
32-47: Null deref in Ext2SetActive when device not yet mounted.If g_ext2_by_dev[id] is NULL, dereferencing vol->lock will crash. Guard vol before checking its lock.
- Ext2Volume* vol = g_ext2_by_dev[id]; - if (!vol->lock) return; + Ext2Volume* vol = g_ext2_by_dev[id]; + if (!vol || !vol->lock) { return; }
319-334: Based on my verification, the review comment's deadlock concern is valid and well-founded. I've confirmed three critical nested-lock deadlock sites in the codebase:
- Ext2WriteInode (line 295): Acquires write_lock, then calls Ext2WriteBlock (line 328) which tries to acquire write_lock again
- Ext2AllocateInode (line 656): Acquires write_lock, then calls Ext2WriteBlock (lines 673, 676) which try to acquire write_lock again
- Ext2AllocateBlock (line 693): Acquires write_lock, then calls Ext2WriteBlock (lines 710, 713) which try to acquire write_lock again
The diff provided in the review correctly addresses the Ext2WriteInode case by using BlockDeviceRead/Write directly while holding the write_lock. However, the review correctly notes that similar fixes are needed for Ext2AllocateInode and Ext2AllocateBlock as well.
Fix nested write-lock deadlocks in Ext2WriteInode, Ext2AllocateInode, and Ext2AllocateBlock.
Three functions acquire write_lock on volume.lock, then call Ext2WriteBlock (and Ext2ReadBlock in some cases) which attempt to acquire locks again, causing deadlock:
- Ext2WriteInode (line 295): write_lock → Ext2WriteBlock (328)
- Ext2AllocateInode (line 656): write_lock → Ext2WriteBlock (673, 676)
- Ext2AllocateBlock (line 693): write_lock → Ext2WriteBlock (710, 713)
Use BlockDeviceRead/Write directly (with
num_sectors = block_size / 512; lba = block * num_sectors) instead of Ext2ReadBlock/Ext2WriteBlock while holding write_lock. Apply this pattern to all three functions to eliminate nested locking.- // Read-modify-write the block containing the inode - if (Ext2ReadBlock(inode_table_block + block_offset, block_buffer) != 0) { + // Read-modify-write the block containing the inode (avoid nested locking) + uint32_t num_sectors = volume.block_size / 512; + uint32_t lba = (inode_table_block + block_offset) * num_sectors; + if (BlockDeviceRead(volume.device->id, lba, num_sectors, block_buffer) != 0) { KernelFree(block_buffer); rust_rwlock_write_unlock(volume.lock); return -1; } FastMemcpy(block_buffer + offset_in_block, inode, sizeof(Ext2Inode)); - if (Ext2WriteBlock(inode_table_block + block_offset, block_buffer) != 0) { + if (BlockDeviceWrite(volume.device->id, lba, num_sectors, block_buffer) != 0) { KernelFree(block_buffer); rust_rwlock_write_unlock(volume.lock); return -1; }fs/VFS.c (1)
376-399: VfsIsFile should not infer “file” from size > 0 (FAT).This misclassifies zero-length files as non-files. Align with VfsIsDir pattern and FS-specific IsFile hooks. Based on learnings.
- if (FastStrCmp(mount->fs_driver->name, "FAT1x") == 0) { - return Fat1xGetFileSize(local_path) > 0; + if (FastStrCmp(mount->fs_driver->name, "FAT1x") == 0) { + Fat1xSetActive(mount->device); + return Fat1xIsFile(local_path);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
drivers/ACPI.c(3 hunks)fs/EXT/Ext2.c(2 hunks)fs/EXT/Ext2.h(1 hunks)fs/FAT/FAT1x.c(3 hunks)fs/FAT/FAT1x.h(1 hunks)fs/FileSystem.h(1 hunks)fs/NTFS/NTFS.c(2 hunks)fs/NTFS/NTFS.h(1 hunks)fs/VFS.c(2 hunks)fs/VFS.h(2 hunks)include/Scheduler.h(1 hunks)kernel/etc/Shell.c(5 hunks)kernel/sched/EEVDF.c(1 hunks)kernel/sched/EEVDF.h(1 hunks)kernel/sched/MLFQ.c(1 hunks)kernel/sched/MLFQ.h(1 hunks)kernel/sched/Scheduler.c(1 hunks)mm/MemPool.h(0 hunks)
💤 Files with no reviewable changes (1)
- mm/MemPool.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-01T02:35:28.353Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: fs/VFS.c:331-335
Timestamp: 2025-09-01T02:35:28.353Z
Learning: VfsIsFile function was initially implemented as a placeholder that only checked for mount existence. The proper implementation should follow the same pattern as VfsIsDir: find mount, strip mount path, then use filesystem-specific functions to check if the path points to a file (FS_FILE for RAMFS, Fat12 functions for FAT12).
Applied to files:
fs/VFS.c
🧬 Code graph analysis (13)
fs/EXT/Ext2.h (1)
fs/EXT/Ext2.c (1)
Ext2Unmount(230-254)
kernel/sched/Scheduler.c (1)
kernel/sched/EEVDF.c (1)
EEVDFKillAllProcesses(1321-1328)
fs/FAT/FAT1x.h (1)
fs/FAT/FAT1x.c (1)
Fat1xUnmount(102-133)
kernel/sched/EEVDF.h (1)
kernel/sched/EEVDF.c (1)
EEVDFKillAllProcesses(1321-1328)
fs/NTFS/NTFS.c (1)
kernel/atomic/rust/src/ffi.rs (1)
rust_rwlock_free(168-179)
drivers/ACPI.c (4)
fs/VFS.c (1)
VfsUnmountAll(79-86)kernel/etc/Console.c (1)
PrintKernelSuccess(225-230)kernel/sched/Scheduler.c (1)
KillAllProcess(88-96)include/Io.h (2)
outb(6-8)outw(16-18)
kernel/sched/MLFQ.h (1)
kernel/sched/MLFQ.c (1)
MLFQKillAllProcesses(1922-1929)
fs/VFS.c (5)
fs/VFRFS.c (1)
FsDelete(321-331)fs/NTFS/NTFS.c (1)
NtfsUnmount(151-171)fs/FileSystem.c (1)
FileSystemRegister(13-22)fs/FAT/FAT1x.c (1)
Fat1xUnmount(102-133)fs/EXT/Ext2.c (1)
Ext2Unmount(230-254)
include/Scheduler.h (1)
kernel/sched/Scheduler.c (1)
KillAllProcess(88-96)
fs/NTFS/NTFS.h (1)
fs/NTFS/NTFS.c (1)
NtfsUnmount(151-171)
kernel/etc/Shell.c (8)
kernel/etc/Console.c (1)
PrintKernel(198-211)fs/BlockDevice.c (1)
SearchBlockDevice(104-111)kernel/etc/Format.c (1)
FormatA(233-239)kernel/etc/StringOps.c (1)
FastStrCmp(39-49)fs/FAT/FAT1x.c (1)
Fat1xMount(46-100)fs/EXT/Ext2.c (1)
Ext2Mount(123-228)fs/NTFS/NTFS.c (1)
NtfsMount(48-149)fs/VFS.c (1)
VfsUmount(61-77)
fs/VFS.h (1)
fs/VFS.c (3)
VfsUmount(61-77)VfsStripMount(155-175)VfsUnmountAll(79-86)
fs/EXT/Ext2.c (2)
kernel/etc/Console.c (1)
PrintKernelSuccess(225-230)kernel/atomic/rust/src/ffi.rs (2)
rust_rwlock_write_unlock(203-207)rust_rwlock_free(168-179)
🪛 Clang (14.0.6)
fs/NTFS/NTFS.c
[warning] 46-46: variable 'ntfs_driver' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 152-152: statement should be inside braces
(readability-braces-around-statements)
[warning] 153-153: variable name 'id' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 154-154: statement should be inside braces
(readability-braces-around-statements)
[warning] 156-156: variable 'vol' is not initialized
(cppcoreguidelines-init-variables)
[warning] 157-157: statement should be inside braces
(readability-braces-around-statements)
drivers/ACPI.c
[warning] 192-192: 0xB004 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 192-192: 0x2000 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
fs/FAT/FAT1x.c
[warning] 44-44: variable 'fat_driver' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 102-102: parameter 'device' is unused
(misc-unused-parameters)
[warning] 103-103: statement should be inside braces
(readability-braces-around-statements)
[warning] 104-104: variable 'id' is not initialized
(cppcoreguidelines-init-variables)
[warning] 104-104: variable name 'id' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 105-105: statement should be inside braces
(readability-braces-around-statements)
[warning] 107-107: variable 'vol' is not initialized
(cppcoreguidelines-init-variables)
[warning] 108-108: statement should be inside braces
(readability-braces-around-statements)
kernel/etc/Shell.c
[warning] 1203-1203: statement should be inside braces
(readability-braces-around-statements)
[warning] 1204-1204: statement should be inside braces
(readability-braces-around-statements)
[warning] 1216-1216: 64 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 1219-1219: repeated branch in conditional chain
(bugprone-branch-clone)
[note] 1221-1221: end of the original
(clang)
[note] 1221-1221: clone 1 starts here
(clang)
[note] 1223-1223: clone 2 starts here
(clang)
[warning] 1240-1240: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 1241-1241: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
kernel/sched/MLFQ.c
[warning] 1924-1924: variable name 'p' is too short, expected at least 3 characters
(readability-identifier-length)
fs/EXT/Ext2.c
[warning] 121-121: variable 'ext2_driver' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 231-231: statement should be inside braces
(readability-braces-around-statements)
[warning] 232-232: variable name 'id' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 233-233: statement should be inside braces
(readability-braces-around-statements)
[warning] 236-236: statement should be inside braces
(readability-braces-around-statements)
🔇 Additional comments (6)
drivers/ACPI.c (1)
182-186: Guard all uses of g_fadt and confirm call context for ACPIResetProcedure in reboot.
- Add NULL checks before dereferencing g_fadt in both shutdown and reboot paths.
- Ensure ACPIResetProcedure is invoked from a safe (non‑terminatable) kernel context or exclude the current PID from kill‑all.
Would you like a patch to exclude the current PID from KillAll and to early‑out to Bochs fallback when g_fadt is NULL/invalid?
Also applies to: 198-200
kernel/sched/EEVDF.h (1)
204-205: LGTM; consistent with EEVDF.c implementation.kernel/sched/EEVDF.c (1)
1321-1328: Verify termination queue capacity for bulk termination.The review comment is accurate. The termination queue implementation uses a circular buffer with capacity check
if (term_queue_count >= EEVDF_MAX_PROCESSES)triggering a panic, which constrains usable capacity to 63 items. However,EEVDFKillAllProcessesiterates through allEEVDF_MAX_PROCESSES(64) slots and callsEEVDFASTerminatefor each active process, andEEVDFASTerminatecallsAddToTerminationQueueAtomic(slot)to enqueue each terminating process.If all 64 process slots are active during shutdown and no concurrent dequeuing occurs, the 64th enqueue will trigger a panic. This edge case requires confirmation: verify whether cleanup runs concurrently during
EEVDFKillAllProcesses, whether graceful degradation is preferable to panic, and that your shutdown/reboot sequence handles this scenario safely.fs/NTFS/NTFS.c (1)
151-171: LGTM!The
NtfsUnmountimplementation correctly handles resource cleanup: validates the device, clears the active mount reference, frees the lock, deallocates the volume structure, and clears the registry entry. The implementation follows the same pattern as other filesystem unmount functions.fs/EXT/Ext2.c (1)
225-225: LGTM: clearer success signal on mount.The success badge improves UX and aligns with Console helpers.
fs/VFS.c (1)
61-77: Based on my verification, I can now provide the rewritten review comment:
Handle FsDelete failure and clean up stale mount fields; clarify mount point deletion semantics.
Two issues:
- FsDelete return value ignored at line 72—can fail silently
- Mount struct fields remain uncleared after unmount, inconsistent with VfsInit which zeroes only the active flag
Regarding mount point deletion: the proposed change moves FsDelete outside the driver conditional (currently only called when driver has unmount hook), which changes behavior for virtual filesystems like DevFS. This is an architectural decision—clarify whether mount point directories should always be cleaned up, or only for drivers providing unmount logic.
int VfsUmount(const char* path) { VfsMountStruct* mount = VfsFindMount(path); if (!mount) { return -1; } if (mount->fs_driver && mount->fs_driver->unmount) { int result = mount->fs_driver->unmount(mount->device); if (result != 0) { return result; } + int del_res = FsDelete(mount->mount_point); + if (del_res != 0) { + PrintKernel("VFS: Warning: failed to remove mount point\n"); + } } mount->active = 0; + mount->device = NULL; + mount->fs_driver = NULL; + mount->mount_point[0] = '\0'; return 0; }Before proceeding, confirm:
- Should mount point directories be deleted for all unmounts, or only when driver provides unmount cleanup?
- Should DevFS (/Devices) be exempted from deletion at shutdown?
ACPI graceful shutdown and runtime mounts and umounts
Summary by CodeRabbit
New Features
Chores