voltage: fix status reporting and linear range level packing - #116
Open
yeongjoshua wants to merge 4 commits into
Open
yeongjoshua wants to merge 4 commits into
yeongjoshua wants to merge 4 commits into
Conversation
saini-ranbirs
left a comment
saini-ranbirs
left a comment
Collaborator
There was a problem hiding this comment.
Commit message
hw/misc/librpmi => No such tree structure
voltage: ... should be good enough
slingappa
reviewed
Aug 31, 2026
The file mixes tabs and eight space indentation, VOLT_GET_SUPPORTED_LEVELS being space indented throughout while the services around it use tabs, and spaces the statements of otherwise identical service bodies differently. Indent with tabs, drop the blank lines that separate statements belonging to one step and add one before the return that closes each service, so the services read the same way. No functional change. Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
Every service in the group repeated the same rpmi_to_xe32() store of STATUS on each of its exit paths, up to three times per function, and VOLT_GET_ATTRIBUTES missed the successful one entirely. Its reply kept whatever the previous response had left in the buffer, so a valid query could report an error while carrying correct attributes. Each path now leaves its status in ret and the single store at the done label serves all of them, the successful path included, where ret is already RPMI_SUCCESS from the call that reached it. Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
The specification types the levels of a VOLT_GET_SUPPORTED_LEVELS reply as "uint32 or uint32[3]", a discrete level being a single voltage and a linear range level being a (min, max, step) tuple, and states that NUM_LEVELS, RETURNED and REMAINING count levels, each linear range counting as one. The reply was packed as one word per level regardless of the format, so a linear domain returned only its voltage_min and sized the response for a single word. A client reading the tuple it was promised got uninitialised message data as voltage_max and voltage_step. The number of levels offered to the platform was miscounted the same way, being derived from the words a message can hold. Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
The vdd_mem domain declares the linear range format but describes its levels the way a discrete domain would, as a two word list of voltages with NUM_LEVELS counting the words. The specification counts each linear range as one level and carries it as a (min, max, step) tuple, so a reply sized at three words per level ran off the end of that array. The domain now publishes an array of tuples, ARRAY_SIZE() counts its levels the way it already counts discrete ones, and the platform hooks report and index levels rather than words. Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
Collaborator
Author
|
Thanks @saini-ranbirs for review comments |
| done: | ||
| *response_datalen = resp_dlen; | ||
| resp[0] = rpmi_to_xe32(trans->is_be, (rpmi_uint32_t)ret); | ||
| *response_datalen = sizeof(*resp); |
Collaborator
There was a problem hiding this comment.
Just like common handling wrt response data length has been done rpmi_volt_set_config(), see if it is good to extend the same approach elsewhere in the file.
If the success and failure paths have different response data length, then set length appropriately as in -
*response_datalen = (ret) ? failure path value; success path value;
or if (ret) ... else ... approach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes two conformance bugs in the VOLTAGE service group (SERVICEGROUP_ID 0x0007) as specified in the RPMI specification, where the replies built by the library did not match the encoding the specification defines.
Overview
The VOLTAGE service group lets a supervisor enumerate the voltage domains of a platform and read or change their levels. Two of its services built replies that a conforming client cannot use: a successful attributes query did not report its status, and the supported levels of a linear range domain were truncated to a third of their width. Both are in the reply framing, which the library owns, so a platform implementation cannot work around either of them.
Changes
VOLT_GET_ATTRIBUTES (0x03) — rpmi_volt_get_attributes() fills in the capability, level count, transition latency and domain name of a successful reply, but writes resp[0] only on its error paths, unlike every other service in the group. On success the status word keeps whatever the previous response left in the buffer, so a valid query can report an error while carrying correct attributes. To reproduce: query an invalid domain, then a valid one — the second reply carries the RPMI_ERR_INVALID_PARAM of the first.
VOLT_GET_SUPPORTED_LEVELS (0x04) — the spec types VOLTAGE_LEVEL[] as uint32 or uint32[3], and says NUM_LEVELS/RETURNED/REMAINING count levels, each linear range counting as one. The reply was packed as one word per level regardless of format, so a linear domain returned only its voltage_min. Fixed by deriving the width of a level from the advertised format and applying it to the copy loop, response_datalen, and the message capacity (which counted words rather than levels).
Testing
make — builds successfully without warnings
make LIBRPMI_TEST=y — builds with tests successfully
make LIBRPMI_TEST=y check — all tests pass, 134 total, 17 in the voltage service group
Each fix is covered by a test that fails without it.
Compliance — brings VOLT_GET_ATTRIBUTES and VOLT_GET_SUPPORTED_LEVELS into line with the VOLTAGE service group as specified in the RISC-V RPMI specification.