Skip to content

PlatformRequestHandler: skip system limit update during init - #1402

Open
SammyTourani wants to merge 1 commit into
NVIDIA:mainfrom
SammyTourani:fix/issue-1360
Open

SammyTourani wants to merge 1 commit into
NVIDIA:mainfrom
SammyTourani:fix/issue-1360

Conversation

@SammyTourani

Copy link
Copy Markdown

Fixes #1360

At boot, pfmreqhndlrInitSensors() calls _pfmreqhndlrCallPshareStatus(pPlatformRequestHandler, NV_TRUE) before PSHAREPARAMS has set up the TGPU and PPMD limit counters. If the SBIOS reports UPDATE_LIMIT pending in that PSHARESTATUS reply, the bSystemParamLimitUpdate block calls _pfmreqhndlrUpdateTgpuLimit() and _pfmreqhndlrUpdatePpmdLimit(). Both look up counters that don't exist yet, get NV_ERR_INVALID_DATA, and log the two assertion failures quoted in #1360 on every driver load. #1310 and #1393 quote the same lines. At that point the block can do nothing else: both lookups fail before any _DSM call or GSP control. Init already reads both limits once the counters exist. pfmreqhndlrInitSensors() samples PPMD right after registering it. The TGPU limit is applied by _pfmreqhndlrThermPmuPostInitWorkItem() (GPS 2.x) or by pfmreqhndlrStateLoad() (1.x).

This change adds && !bInit to that block, which the EDPpeak and user-configurable-TGP blocks in the same function already have. A pending system limit update is therefore handled only at runtime. bSystemParamLimitUpdate is rewritten from every PSHARESTATUS reply before it is read, so leaving it set after init has no effect. This is the fix proposed in #1360.

Verification

I had no affected laptop, so nothing here ran on hardware.

  1. I built the unmodified platform_request_handler.c and platform_request_handler_ctrl.c, plus nvassert.c and nvstatus.c, into a userspace test program. It simulates the SBIOS GPS _DSM (SUPPORT, PSHARESTATUS, PSHAREPARAMS, PCONTROL) and the GSP-RM side of the PRH internal controls. The only other external symbols are small fakes: timer, GPU manager, memory helpers and printing. Each scenario is a driver load followed by a runtime limit change:

    • Load: pfmreqhndlrStateInit(), the GSP PFM_REQ_HNDLR_STATE_SYNC callbacks, the queued work items, then pfmreqhndlrStateLoad().
    • Runtime: pfmreqhndlrControl(DATA_INIT_USING_SBIOS_AND_ACK).

    The scenarios are GPS 2.x with UPDATE_LIMIT pending at boot, GPS 2.x with nothing pending, and GPS 1.x with UPDATE_LIMIT pending. The checks are: no assertion failures, the SBIOS TGPU limit reaches GSP-RM at load and after the runtime change, and on 2.x the platform power mode is cached at load and notified after the runtime change.

    On 615.71.09 the harness prints the two assert lines above (one on 1.x), and 2 of 16 checks fail. With this change there are no asserts and 16 of 16 checks pass. Apart from the removed assert lines, the two runs are identical: every _DSM call with its arguments, every GSP-RM control, the PLATFORM_POWER_MODE_CHANGE events, the counter and PPM state, and the TGPU limits applied (87 C at load, 80 C at runtime). The one other difference is bSystemParamLimitUpdate, which stays set after load. The next PSHARESTATUS reply rewrites it before anything reads it.

    Command: ./verify.sh <checkout> 61dcc937 HEAD (builds the harness from git archive of each revision, runs it, diffs the logs)
    Result:

    ===== 61dcc937 (61dcc937): 615.71.09
    == GPS 2.x SBIOS, UPDATE_LIMIT pending at boot (reported: Lenovo LOQ 15IRX11)
    NVRM: GPU0 nvAssertOkFailedNoLog: Assertion failed: Invalid data passed [NV_ERR_INVALID_DATA] (0x00000025) returned from PlatformRequestHandler failed to get target temp from SBIOS @ platform_request_handler_ctrl.c:2175
    NVRM: GPU0 nvAssertOkFailedNoLog: Assertion failed: Invalid data passed [NV_ERR_INVALID_DATA] (0x00000025) returned from PlatformRequestHandler failed to get platform power mode from SBIOS @ platform_request_handler_ctrl.c:2118
      FAIL: no assertion failures during load
    ...
    == GPS 1.x SBIOS, UPDATE_LIMIT pending at boot
    NVRM: GPU0 nvAssertOkFailedNoLog: Assertion failed: Invalid data passed [NV_ERR_INVALID_DATA] (0x00000025) returned from PlatformRequestHandler failed to get target temp from SBIOS @ platform_request_handler_ctrl.c:2175
      FAIL: no assertion failures during load
    ...
    FAILED (2 failed checks)
    exit status: 1
    
    ===== HEAD (8a234aa8): PlatformRequestHandler: skip system limit update during init
    ... (16 x PASS)
    PASSED (0 failed checks)
    exit status: 0
    
  2. I compiled platform_request_handler_ctrl.c with the exact command printed by make -n -C src/nvidia TARGET_OS=Linux TARGET_ARCH=x86_64 CC=clang, run as clang --target=x86_64-linux-gnu.

    Command: clang --target=x86_64-linux-gnu <flags from make -n> -c platform_request_handler_ctrl.c, run on the file before and after the change.
    Result: exit 0 with 0 warnings and 0 errors in both cases. Only _pfmreqhndlrCallPshareStatus changes in the object. It gains one test of bInit, and the other 40 functions are identical.

pfmreqhndlrInitSensors() calls _pfmreqhndlrCallPshareStatus() with
bInit set before PSHAREPARAMS has set up the TGPU and PPMD limit
counters. When the SBIOS reports UPDATE_LIMIT pending at that point,
the bSystemParamLimitUpdate block looks up both counters, gets
NV_ERR_INVALID_DATA and logs two assertion failures on every driver
load. Both limits are read later in init, once the counters exist.

Handle a pending system limit update only at runtime, as the EDPpeak
and user configurable TGP blocks in the same function already do.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

PlatformRequestHandler: missing !bInit guard on bSystemParamLimitUpdate causes NV_ERR_INVALID_DATA at boot

2 participants