Kernel: T9170: support compressed modules when searching module firmware - #1261
Conversation
Commit 21c7e66 ("Kernel: T5641: enable module compression to save disk space") which enabled module compression using XZ missed that when we search modules for firmware inclusion, the pattern was hardcoded to *.ko instead of *.ko* - this has been fixed.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe firmware build now accepts compressed kernel module filenames when it uses ChangesFirmware metadata discovery
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/package-build/linux-kernel/build-linux-firmware.sh`:
- Line 23: Update the firmware collection command assigned to FW_FILES to quote
the kernel module search path and '*.ko*' pattern, preventing shell expansion,
and replace plain xargs with find’s filename-safe -exec modinfo {} + (or an
equivalent null-delimited pipeline). Preserve the existing grep and awk
extraction behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5a9ba8ac-c56e-433e-b2a9-828af3889b85
📒 Files selected for processing (1)
scripts/package-build/linux-kernel/build-linux-firmware.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: codeql-analysis-call / Analyze (python)
- GitHub Check: Mergify Merge Queue
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
🪛 Shellcheck (0.11.0)
scripts/package-build/linux-kernel/build-linux-firmware.sh
[warning] 23-23: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 23-23: Quote the parameter to -name so the shell won't interpret it.
(SC2061)
[info] 23-23: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🔍 Remote MCP vyos.dev
Relevant task context
- T9170 title: “Kernel: missing firmware files after enabling module compression”
- Status: “In progress”; priority: “Normal”; issue type: bug.
- The task reports
*.kofinds 0 files, while*.ko*finds 272 compressed modules underkernel/drivers/net. It proposes changing thefindpattern to-name '*.ko*', exactly matching this PR’s change. - The task’s only comment links to GitHub PR
#1261, whereas the PR context references PR#1239; this may warrant verification.
|
|
||
| # Retrieve firmware blobs from source files | ||
| FW_FILES=$(find ${KERNEL_DIR}/debian/linux-image-${KERNEL_VERSION}${KERNEL_SUFFIX}/lib/modules/${KERNEL_VERSION}${KERNEL_SUFFIX}/kernel/drivers/net -name *.ko | xargs modinfo | grep "^firmware:" | awk '{print $2}') | ||
| FW_FILES=$(find ${KERNEL_DIR}/debian/linux-image-${KERNEL_VERSION}${KERNEL_SUFFIX}/lib/modules/${KERNEL_VERSION}${KERNEL_SUFFIX}/kernel/drivers/net -name *.ko* | xargs modinfo | grep "^firmware:" | awk '{print $2}') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Quote the pattern and avoid plain xargs.
Bash expands *.ko* before find receives it. If the working directory contains a matching filename, find can receive the filename instead of the pattern. This can omit compressed modules or fail the command. Because scripts/package-build/linux-kernel/build.py:212-216 runs this script with check=True, the firmware build can abort.
Quote the path and pattern. Use -exec modinfo {} + or -print0 | xargs -0 for filename-safe processing.
Proposed fix
-FW_FILES=$(find ${KERNEL_DIR}/debian/linux-image-${KERNEL_VERSION}${KERNEL_SUFFIX}/lib/modules/${KERNEL_VERSION}${KERNEL_SUFFIX}/kernel/drivers/net -name *.ko* | xargs modinfo | grep "^firmware:" | awk '{print $2}')
+FW_FILES=$(find "${KERNEL_DIR}/debian/linux-image-${KERNEL_VERSION}${KERNEL_SUFFIX}/lib/modules/${KERNEL_VERSION}${KERNEL_SUFFIX}/kernel/drivers/net" \
+ -name '*.ko*' -exec modinfo {} + | grep "^firmware:" | awk '{print $2}')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FW_FILES=$(find ${KERNEL_DIR}/debian/linux-image-${KERNEL_VERSION}${KERNEL_SUFFIX}/lib/modules/${KERNEL_VERSION}${KERNEL_SUFFIX}/kernel/drivers/net -name *.ko* | xargs modinfo | grep "^firmware:" | awk '{print $2}') | |
| FW_FILES=$(find "${KERNEL_DIR}/debian/linux-image-${KERNEL_VERSION}${KERNEL_SUFFIX}/lib/modules/${KERNEL_VERSION}${KERNEL_SUFFIX}/kernel/drivers/net" \ | |
| -name '*.ko*' -exec modinfo {} + | grep "^firmware:" | awk '{print $2}') |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 23-23: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 23-23: Quote the parameter to -name so the shell won't interpret it.
(SC2061)
[info] 23-23: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/package-build/linux-kernel/build-linux-firmware.sh` at line 23,
Update the firmware collection command assigned to FW_FILES to quote the kernel
module search path and '*.ko*' pattern, preventing shell expansion, and replace
plain xargs with find’s filename-safe -exec modinfo {} + (or an equivalent
null-delimited pipeline). Preserve the existing grep and awk extraction
behavior.
Source: Linters/SAST tools
hedrok
left a comment
hedrok
left a comment
There was a problem hiding this comment.
Approve. Fix of search pattern.
I think both points from coderabbit are worth fixing (quoting '.ko' and using -exec), but it is not a new problem, so the PR can be merged as is.
|
Tick the box to add this pull request to the merge queue (same as
|
vyos-bot
Bot
added
mirror-initiated
3 participants
Change summary
Commit 21c7e66 ("Kernel: T5641: enable module compression to save disk space") which enabled module compression using XZ missed that when we search modules for firmware inclusion, the pattern was hardcoded to *.ko instead of .ko - this has been fixed.
Types of changes
Related Task(s)
Related PR(s)
Checklist: