[26.04_linux-nvidia-bos] firmware: tegra: bpmp: grow debugfs read buffer - #512
Conversation
PR Validation ReportPatchscan ✅ No Missing FixesAll cherry-picked commits checked — no missing upstream fixes found. PR Lint ❌ Errors foundDetailsChecking 1 commits... Cherry-pick digest: ┌──────────────┬──────────────────────────────────────────────────────────────────┬────────────┬─────────┬───────────────────────────┐ │ Local │ Referenced upstream / Patch subject │ Patch-ID │ Subject │ SoB chain │ ├──────────────┼──────────────────────────────────────────────────────────────────┼────────────┼─────────┼───────────────────────────┤ │ 08eb251b260a │ [SAUCE] firmware: tegra: bpmp: grow debugfs read buffer │ N/A │ N/A │ kobak │ └──────────────┴──────────────────────────────────────────────────────────────────┴────────────┴─────────┴───────────────────────────┘ Lint: all checks passed. PR metadata: E: PR targets 26.04_linux-nvidia-bos but body has no https://bugs.launchpad.net/... link |
|
@kobak2026 - I see this is still in draft, please move out of draft state when it is ready for review. From a cursory look, two questions / comments:
|
I agree with these, and I think the commit message could use a "Fixes" tag as well: It also looks like there's a pending patch that's trying to fix |
5d9af51 to
2c0e174
Compare
|
@nvmochs @jamieNguyenNVIDIA, thanks. I checked directly with Aniruddha. His current view is that, if this fix is Agreed on the commit message. I wrapped the body and added: The updated commit is I also reviewed the linked I will keep this PR in draft until the required Launchpad metadata is ready, |
BaseOS Kernel ReviewSummaryNo issues found across the reviewed commits. Findings: no problems found Latest watcher review: open review Generated test plan: open test plan Kernel deb build: successful (download debs, 4 files) Head: This comment is maintained by nv-pr-bot. It is updated when the GitHub watcher publishes a newer review. |
2c0e174 to
221331c
Compare
|
@kobak2026 as reported by boro review, would it make sense to add a max size limit before invoking kvmalloc()? mrq_debug_open() obtains the u32 len directly from the firmware, so a malformed response could trigger a very large allocation attempt (i.e., a negative value reported by the firmware would be interpreted as a large u32). Maybe something like the following (I'd add also __GFP_NORETRY, so the read just fails instead of triggering the OOM killer, this is debugging stuff, right?): What do you think? |
|
@nirmoy @arighi, thanks for reviewing this. I agree that the I updated the PR in I did not add Strict commit-level checkpatch and the mandatory exact-SHA The maintained Boro report still references old head |
|
@kobak2026 looks good now, thanks! |
|
|
|
Thanks for fixing the commit message and providing context with regard to Anirudhha's series. If we are to pursue this as a separate fix, can you post the patch to LKML and then include it in this SAUCE patch as a reference via pick tag? That will help future tracking. Also, my review with Codex had one finding: The new budget accounting does not actually enforce the stated whole-tree limit for in-band population. bpmp_populate_debugfs_inband() subtracts the current directory listing once: but then passes that same child_budget value to every child directory. Since the budget is passed by value, any listing data consumed by one child subtree is not reflected when processing the next sibling. For example, with a 4 KiB root listing, every child directory gets an independent ~508 KiB budget. Multiple sibling directories can each be accepted under that limit, so the total firmware listing data consumed while mirroring the tree can exceed BPMP_DEBUGFS_FIRMWARE_DATA_MAX by a large amount. That does not match the commit message’s claim that recursive in-band population is bounded to the existing 512 KiB MRQ_DEBUGFS whole-tree size. If the intended bound is the total mirrored tree listing size, the remaining budget needs to be shared across recursion, for example by passing a size_t *remaining_budget and decrementing it after each successful mrq_debug_read(): Alternatively, each recursive call could return its total consumed size and the parent could subtract it before processing the next sibling. If the intended limit is only peak retained memory along a single recursion path, then the implementation is closer, but the commit message should avoid describing it as a whole-tree cap. |
BPMP firmware debugfs listings can exceed the fixed 16 KiB buffer. After CMD_DEBUG_OPEN reports the listing length, the host-side size check returns -EFBIG and prevents the firmware debugfs trees from being populated. Allocate the listing buffer using the firmware-reported length and populate debugfs in one pass. Validate every read chunk for forward progress, response-payload bounds, and remaining-length bounds before copying or writing it. Bound peak firmware data retained along a recursive in-band population path to 512 KiB, matching the existing MRQ_DEBUGFS whole-tree buffer size. Reject listing lengths that exceed the remaining path budget before allocation. Fixes: 5e37b9c ("firmware: tegra: Add support for in-band debug") Signed-off-by: Koba Ko <kobak@nvidia.com>
221331c to
08eb251
Compare
|
@nvmochs Thanks. I confirmed this with Aniruddha TVS Rao. He found debugfs entries exceeding 16 KiB and will submit the fix as a separate upstream patch. He will share the lore URL once it is posted. I have moved this PR back to draft until that URL is available. I’ll then add I amended the commit message to clarify that the 512 KiB budget bounds peak simultaneously retained listing data along a recursive path. Sibling subtrees are processed sequentially and freed between calls, so this is intentionally not a cumulative whole-tree cap. I also corrected the failure description: |
@kobak2026 - Are you saying that Aniruddha is going to post this patch to LKML? Or a different patch to LKML? |
I think he will post this patch to LKML after finish the internal review. |
Sounds good, thanks for clarifying. |
NVBug: https://nvbugspro.nvidia.com/bug/6463479
BugLink: REQUIRED — create or confirm the Launchpad bug for this EFBIG follow-on
Summary
Fix BPMP firmware debugfs initialization on Vera/NVL72 systems when a
firmware directory listing exceeds the driver's fixed 16 KiB host buffer.
Both ACPI BPMP devices bind to
tegra-bpmp, but debugfs initialization failswith
-EFBIGand neither per-device firmware tree is created. This change:u32listing length withkvmalloc();copying or writing them; and
layout.
Root cause
bpmp_debug_open()reports the total listing length before the host reads thelisting in chunks. The driver previously read the listing into a fixed
DEBUG_READ_MAX_SZbuffer, so valid listings larger than 16 KiB failed beforethe debugfs tree could be populated.
Relationship to the Tegra410 ACPI/MBWT series
The landed V4-derived BOS series and Aniruddha's latest six-patch
upstream-review version leave the existing BPMP debugfs read/population path
unchanged. The latest series adds the Tegra410 client functionality that was
not part of the three-patch V4-derived BOS backport; this patch is an
orthogonal fix for the generic debugfs listing reader and does not add or
depend on the external
dbg_bpmpmodule.Validation
26.04_linux-nvidia-boscommit265816a43353c9ee37a41f7a1342f44b38fdb92f.git diff --check: pass.scripts/checkpatch.pl: 0 errors, 0 warnings.Image modulesbuild: pass; kernel7.0.12-6463479-efbig-r2.10.103.232.135.PASS=11 FAIL=0 SKIP=0; both per-device trees populatedwith 1,797 directories and 21,112 files, and representative
firmware-backed reads succeeded.
PASS=16 FAIL=0 SKIP=1; the sole skip is the DT-onlyregression case on this ACPI DUT.
PASS=2 FAIL=0, exit 0.inst=0 vc=0 bw=0on both devices.-EFBIG, or-27diagnostics.9a3175ccc35ebac31858cbfad372a95b0edfc166d2ddfecd2e5e4ac44a72191f.Caveats
update-initramfsproduced a readable initrd, but itsflash-kernelpost-hook returned nonzero because the manually built kernel has no dpkg
package record. Independent
lsinitramfsvalidation andupdate-grubpassed before the successful boot.