Skip to content

[26.04_linux-nvidia-bos] firmware: tegra: bpmp: grow debugfs read buffer - #512

Draft
kobak2026 wants to merge 1 commit into
NVIDIA:26.04_linux-nvidia-bosfrom
kobak2026:bug-6463479/bpmp-debugfs-bos-current
Draft

[26.04_linux-nvidia-bos] firmware: tegra: bpmp: grow debugfs read buffer#512
kobak2026 wants to merge 1 commit into
NVIDIA:26.04_linux-nvidia-bosfrom
kobak2026:bug-6463479/bpmp-debugfs-bos-current

Conversation

@kobak2026

Copy link
Copy Markdown
Collaborator

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 fails
with -EFBIG and neither per-device firmware tree is created. This change:

  • allocates the exact firmware-reported u32 listing length with kvmalloc();
  • rejects zero-length, oversized, or beyond-remaining firmware chunks before
    copying or writing them; and
  • preserves the existing one-pass population and per-device ACPI debugfs
    layout.

Root cause

bpmp_debug_open() reports the total listing length before the host reads the
listing in chunks. The driver previously read the listing into a fixed
DEBUG_READ_MAX_SZ buffer, so valid listings larger than 16 KiB failed before
the 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_bpmp module.

Validation

  • Based on current 26.04_linux-nvidia-bos commit
    265816a43353c9ee37a41f7a1342f44b38fdb92f.
  • git diff --check: pass.
  • strict scripts/checkpatch.pl: 0 errors, 0 warnings.
  • full arm64 Image modules build: pass; kernel
    7.0.12-6463479-efbig-r2.
  • Booted the repaired kernel on DUT 10.103.232.135.
  • BPMP debugfs: PASS=11 FAIL=0 SKIP=0; both per-device trees populated
    with 1,797 directories and 21,112 files, and representative
    firmware-backed reads succeeded.
  • MBWT V4 sysfs: PASS=16 FAIL=0 SKIP=1; the sole skip is the DT-only
    regression case on this ACPI DUT.
  • Complete BPMP suite: PASS=2 FAIL=0, exit 0.
  • Post-test controls restored to inst=0 vc=0 bw=0 on both devices.
  • Zero matching BPMP debugfs initialization, -EFBIG, or -27 diagnostics.
  • Authoritative log SHA-256:
    9a3175ccc35ebac31858cbfad372a95b0edfc166d2ddfecd2e5e4ac44a72191f.

Caveats

  • NVPunish is explicitly pending and was not run.
  • The DT-only regression case is not applicable to the ACPI DUT.
  • update-initramfs produced a readable initrd, but its flash-kernel
    post-hook returned nonzero because the manually built kernel has no dpkg
    package record. Independent lsinitramfs validation and update-grub
    passed before the successful boot.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

PR Validation Report

Patchscan ✅ No Missing Fixes

All cherry-picked commits checked — no missing upstream fixes found.

PR Lint ❌ Errors found

Details
Checking 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

@nvmochs

nvmochs commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

@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:

  • Is Aniruddha going to incorporate this fix in his next revision sent to LKML?
  • The commit message isn't wrapped.

@jamieNguyenNVIDIA

Copy link
Copy Markdown
Collaborator

@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:

  • Is Aniruddha going to incorporate this fix in his next revision sent to LKML?
  • The commit message isn't wrapped.

I agree with these, and I think the commit message could use a "Fixes" tag as well:

Fixes: 5e37b9c137ee ("firmware: tegra: Add support for in-band debug")

It also looks like there's a pending patch that's trying to fix seqbuf_ helpers that might be of interest: https://lore.kernel.org/all/20260715083726.30740-1-pengpeng@iscas.ac.cn/

@kobak2026
kobak2026 force-pushed the bug-6463479/bpmp-debugfs-bos-current branch from 5d9af51 to 2c0e174 Compare July 29, 2026 16:46
@kobak2026

Copy link
Copy Markdown
Collaborator Author

@nvmochs @jamieNguyenNVIDIA, thanks.

I checked directly with Aniruddha. His current view is that, if this fix is
needed upstream, it should be submitted separately rather than folded into his
Tegra410 ACPI/MBWT series. He is waiting for clarification from the BPMP
team/FW owner; the current indication is that a recent BPMP firmware update
caused the debugfs listing to exceed 16 KiB.

Agreed on the commit message. I wrapped the body and added:

Fixes: 5e37b9c137ee ("firmware: tegra: Add support for in-band debug")

The updated commit is 2c0e174090079.

I also reviewed the linked seqbuf patch. It rejects truncated internal
directory records, such as incomplete u32 fields or names without a NUL
terminator. This PR addresses a different path: a complete, valid directory
listing exceeding the fixed 16 KiB buffer and failing with -EFBIG. The
seqbuf change is complementary hardening, but I propose keeping it separate
and handling it through the normal upstream/backport flow.

I will keep this PR in draft until the required Launchpad metadata is ready,
then mark it ready for review.

@kobak2026
kobak2026 marked this pull request as ready for review July 29, 2026 16:56
@nirmoy nirmoy added help wanted Extra attention is needed pending_review_comment labels Jul 29, 2026
@nirmoy

nirmoy commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

BaseOS Kernel Review

Summary

No 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: 08eb251b260a

This comment is maintained by nv-pr-bot. It is updated when the GitHub watcher publishes a newer review.

@kobak2026
kobak2026 force-pushed the bug-6463479/bpmp-debugfs-bos-current branch from 2c0e174 to 221331c Compare July 30, 2026 06:15
@arighi

arighi commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

@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?):

#define BPMP_DEBUGFS_MAX_DIR_SIZE SZ_1M // ... or any reasonable max value
...

if (len > BPMP_DEBUGFS_MAX_DIR_SIZE) {
    err = -EFBIG;
    goto close;
}
data = kvmalloc(len, GFP_KERNEL | __GFP_NORETRY);
...

What do you think?

@kobak2026

Copy link
Copy Markdown
Collaborator Author

@nirmoy @arighi, thanks for reviewing this. I agree that the
firmware-reported datalen must be bounded before allocation.

I updated the PR in 221331ce7a606 to reuse the existing 512 KiB legacy
MRQ_DEBUGFS whole-tree limit as a shared bound. mrq_debug_read() now rejects
datalen above the caller's remaining budget before kvmalloc(), and
recursive population subtracts each live parent listing from that budget.
This bounds both each allocation and aggregate live listing memory to 512 KiB
while still allowing valid listings larger than the old 16 KiB buffer.

I did not add __GFP_NORETRY. The kvmalloc() API comment in this tree
explicitly says that modifier is unsupported. For allocations larger than
PAGE_SIZE, kmalloc_gfp_adjust() already adds __GFP_NOWARN and clears
__GFP_DIRECT_RECLAIM unless __GFP_RETRY_MAYFAIL is requested, before
falling back to vmalloc(). With the new 512 KiB aggregate bound, I don't
think adding an unsupported modifier is appropriate here.

Strict commit-level checkpatch and the mandatory exact-SHA nvidia-bos arm64
Image modules dtbs build passed. Post-push patchscan and commit lint also
pass; the CI failure is only the separate deferred Launchpad BugLink. Fresh
target-kernel runtime verification remains pending.

The maintained Boro report still references old head 2c0e174; the current
head is 221331ce7a606.

@arighi

arighi commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

@kobak2026 looks good now, thanks!

Acked-by: Andrea Righi <arighi@nvidia.com>

@clsotog

clsotog commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Acked-by: Carol L Soto <csoto@nvidia.com>

@nirmoy nirmoy added has_2_acks and removed help wanted Extra attention is needed has_1_ack labels Jul 30, 2026
@nvmochs

nvmochs commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

@kobak2026

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:

  child_budget = remaining_budget - dsize;

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():

  err = mrq_debug_read(bpmp, ppath, *remaining_budget, &buf, &dsize);
  if (err)
      goto out;

  *remaining_budget -= dsize;

  ...
  err = bpmp_populate_debugfs_inband(bpmp, dentry, pathbuf,
                                     remaining_budget);

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>
@kobak2026
kobak2026 force-pushed the bug-6463479/bpmp-debugfs-bos-current branch from 221331c to 08eb251 Compare July 31, 2026 06:36
@kobak2026
kobak2026 marked this pull request as draft July 31, 2026 06:40
@kobak2026

Copy link
Copy Markdown
Collaborator Author

@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 (backported from <lore URL>), push the final amended commit, and return the PR to Ready for Review.

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: CMD_DEBUG_OPEN reports the listing length successfully, and the subsequent host-side check in mrq_debug_read() returns -EFBIG. No driver code change was needed for these clarifications.

@nvmochs

nvmochs commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@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 (backported from <lore URL>), push the final amended commit, and return the PR to Ready for Review.

@kobak2026 - Are you saying that Aniruddha is going to post this patch to LKML? Or a different patch to LKML?

@kobak2026

kobak2026 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

@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 (backported from <lore URL>), push the final amended commit, and return the PR to Ready for Review.

@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.
he may refine/modify code I will update this PR after he pushes.

@nvmochs

nvmochs commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

@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 (backported from <lore URL>), push the final amended commit, and return the PR to Ready for Review.

@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. he may refine/modify code I will update this PR after he pushes.

Sounds good, thanks for clarifying.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants