padmux: a row names its functions by offset, not by pointer - #214
Merged
Merged
Conversation
A muxctrl row held its function names as `const char *`. In a
position-independent consumer every one of those is a R_ARM_RELATIVE
relocation -- eight bytes of .rel.dyn on top of the four-byte slot -- so
pointing at a pad name cost four times what storing it did, and the names
are shared five ways.
Measured in a majestic build for hi3516ev200, which carries the V4 tables
because one SDK code covers ev200, ev300, 3518ev300 and dv200: 8,096 bytes
of .data.rel.ro for the 300 tables, 11,392 bytes of relocations into them,
and 2,862 bytes for all 269 distinct names those 1,424 pointers reached.
Relocations were 44% of the pad tables and nobody had ever looked.
So the rows hold uint16_t offsets into one blob of names, and
src/padmux_names.{h,c} is that blob: a struct of char arrays, one per
distinct name, with offsetof() giving an integer constant expression a
static initialiser can use. A member is compiled only when some table that
uses it is, so a consumer carrying one family pays for that family's names
alone -- and keeps the dedup, which storing the names inline would have
lost. Offset 0 is a one-byte empty string, which is what keeps 0 usable as
the end of a row's list.
Same ARM PIE objects, V4 only, before and after:
.rel.dyn 13,936 -> 2,552
.data.rel.ro 8,120 -> 24
.rodata 3,308 -> 8,316
.text 2,052 -> 2,156
total 27,416 -> 13,048
Every family shrinks: V5 -9,080, V4A -7,076, V2 -4,520, V1 -2,060, and
-48,852 with all of them in. The static ipctool binary, which has no
relocations at all, still loses 12 KB on arm32 and 48 KB on arm64 -- that
part is just a 2-byte offset against a 4- or 8-byte pointer.
The names are identifiers now rather than string literals, which is what
lets them intern, and a typo is a compile error instead of a pad function
nothing will ever select. Sixteen names spell two peripherals reached
through one selector value -- "SDIO0_CCLK_OUT/EMMC_CLK_OUT" -- and the
slash becomes a double underscore; the member's initialiser carries the
true spelling, so no caller sees the mangling. No name of its own contains
a double underscore, and the generator's --selftest is what holds that.
Nothing at build time regenerates the blob: a cross build must not need
python3 in the toolchain container. `--check` in CI is what catches a stale
one -- a row naming a function no longer in the blob already fails to
compile, and this is for the quiet half, a name left behind after its last
row went.
Verified by dumping every pad row of every family -- address, selector,
function name, GPIO alternative and flags -- through ipchw_padmux_by_prefix
before and after: 7,821 rows, byte-identical. reginfo_test passes for every
IPCHW_PADMUX selection, which the workflow now sweeps; it only built v1
before, and each family's names sit behind that family's #if, so a guard
naming the wrong family would build everywhere except where it matters.
tools/check_hisi_padmux.py reads the new spelling and still reads the old
one, for a fork or a branch that has not been converted. That needed doing
deliberately: it would not have failed on the new tables, it would have
parsed every row as having no functions at all, called every register "in
the document but not in the table", and offered to --fix the lot.
PR Summary by QodoIntern padmux function names as 16-bit offsets
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Mixed pad rows fail validation
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+338
to
+341
| quoted = re.findall(r'"([^"]*)"', args_src) | ||
| if quoted: | ||
| return quoted | ||
| return [unmangle(i) for i in PMXREF.findall(args_src)] |
There was a problem hiding this comment.
1. Mixed pad rows fail validation 🐞 Bug ≡ Correctness
row_funcs() returns quoted names immediately when it finds any, discarding all interned-name references from the same row. A partially converted or conflict-resolved row containing both spellings is therefore compared against the data sheet with missing selector entries, causing false mismatch reports and blocking otherwise valid repairs.
Agent Prompt
## Issue description
`row_funcs()` treats quoted names and `PMX_` references as mutually exclusive, so mixed rows silently lose every macro-based function and fail validation.
## Fix Focus Areas
- tools/check_hisi_padmux.py[337-341]
## Recommended Fix
Parse quoted strings and `PMX_` references with one ordered token matcher, converting each token according to its spelling while preserving its original selector position. Add a self-test containing both forms in one row.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+163
to
+164
| """The `#if` a member needs: any one of the tables that use it.""" | ||
| parts = sorted(guards) |
There was a problem hiding this comment.
2. Double-underscore names are corrupted 🐞 Bug ≡ Correctness
scan() unconditionally applies demangle() to every PMX_ identifier, so an original function name such as FOO__BAR becomes FOO/BAR after --rewrite has replaced its quoted spelling with PMX_FOO__BAR. The only assertion excluding that input is behind the separate --selftest mode, while CI invokes only --check, so adding such a name can generate a blob whose string no longer matches the pad table's intended function name.
Agent Prompt
Issue description
`PMX_` identifiers encode `/` as `__`, so `demangle()` cannot distinguish an intentional slash encoding from an original pad-function name containing double underscores. The generator currently checks that constraint only in optional `--selftest`; normal `--rewrite` can therefore rewrite `FOO__BAR` to `PMX_FOO__BAR` and subsequently generate the spelling `FOO/BAR`.
Fix Focus Areas
- tools/gen_padmux_names.py[150-158]
- tools/gen_padmux_names.py[278-286]
Recommended Fix
Validate the no-double-underscore naming invariant as part of the normal generation/rewrite path, before quoted names are converted to `PMX_` identifiers. Fail with a clear error identifying the offending name, and keep the validation active for normal `--check`/generation rather than relying on the optional self-test mode.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
widgetii
added a commit
that referenced
this pull request
Sep 16, 2026
) #214 did this for the HiSilicon rows and left the two vendor tables alone on a claim that turned out to be arithmetic I got wrong: I compared SigmaStar's pointer cost against its offset cost PLUS the blob, double-counting the names, and concluded it was worth about a kilobyte. The strings exist either way. Measured, it is 7,348 bytes, and Ingenic is 17,616. Both tables held their names as `const char *`, which in a position-independent consumer is a R_ARM_RELATIVE relocation each: ingenic_pad_t five per pad -- the pad's own name and FUNCTION0..3 -- over 1,706 uses of 325 distinct names. sstar_mode_t one per mode, sstar_pad_t one per pad, 882 between them. Same ARM PIE objects, before and after: ingenic sstar .rel.dyn 13,776 -> 144 7,248 -> 232 .data.rel.ro 8,232 -> 48 11,876 -> 100 .rodata 2,836 -> 6,940 14,260 -> 25,624 .text 1,256 -> 1,352 1,496 -> 1,576 total 26,100 -> 8,484 34,880 -> 27,532 The .rodata rise is the tables themselves moving: pointer-free, they leave .data.rel.ro entirely and land in .rodata, while their names leave .rodata.str1.1 for the blob. SigmaStar keeps a bigger blob than HiSilicon because its names are nearly all distinct -- 560 for 882 uses, against 269 for 1,424 -- so it gains from the relocations rather than from the interning, and gains anyway. The static ipctool binary, which has no relocations at all, still loses 8 KB on arm32 and 24 KB on arm64 over #214. On mips32, where ipctool carries every table and the Ingenic one is native, the two changes together take it from 472,104 to 406,572. Both generated tables were converted in place by gen_padmux_names.py --rewrite, and both vendor generators now emit the PMX_ spelling so a regeneration from a vendor kernel or a GPIO spec matches. The three manglings have to agree; gen_padmux_names.py --selftest pins the rule, and a disagreement is an undeclared identifier in the generated header rather than a wrong name on a pad. --rewrite grew a comment-aware substitution to do it, and needed one: half the SigmaStar pad rows end in a `/* 42 */` pad-id comment, and the first attempt spliced the line back together on the assumption that the code part was a prefix of it. Every such row was silently left as a pointer, and the build caught it only because the struct had already changed type. The banner comments of both files also quote example pad names, which must not reach the blob. map_code() applies the substitution to runs of code and leaves comments byte for byte; with the identity function it rebuilds any line unchanged. Verified the same way as #214: every pad row of every family -- address, selector, function name, GPIO alternative and flags -- dumped through ipchw_padmux_by_prefix before and after and compared. 7,821 rows, byte-identical, which for these two tables is the whole check, since the names ARE the data. Both generated headers also round-trip: substituting PMX_X back to "X" reproduces the previous file exactly. reginfo_test passes for all fifteen IPCHW_PADMUX selections, including sstar, ingenic and "v4;sstar". arm32, arm64 and mips32 cross-build.
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
1 participant
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.
A muxctrl row held its function names as
const char *. In aposition-independent consumer every one of those is a
R_ARM_RELATIVErelocation — eight bytes of
.rel.dynon top of the four-byte slot — sopointing at a pad name cost four times what storing it did, and the names are
shared five ways.
Measured in a majestic build for hi3516ev200, which carries the V4 tables
because one SDK code covers ev200, ev300, 3518ev300 and dv200: 8,096 bytes of
.data.rel.rofor the 300 tables, 11,392 bytes of relocations into them, and2,862 bytes for all 269 distinct names those 1,424 pointers reached.
Relocations were 44% of the pad tables and nobody had ever looked.
What changed
Rows hold
uint16_toffsets into one blob of names.src/padmux_names.{h,c}is that blob, generated by the new
tools/gen_padmux_names.py: a struct ofchararrays, one per distinct name, whereoffsetof()gives the integerconstant expression a static initialiser needs.
A member is compiled only when some table that uses it is, so a consumer
carrying one family pays for that family's names alone — and keeps the dedup,
which storing the names inline would have lost. Offset 0 is a one-byte empty
string, which is what keeps 0 usable as the end of a row's list.
The names are identifiers rather than string literals now, which is what lets
them intern, and a typo is a compile error instead of a pad function nothing
will ever select. Sixteen names spell two peripherals reached through one
selector value —
SDIO0_CCLK_OUT/EMMC_CLK_OUT— and the slash becomes a doubleunderscore; the member's initialiser carries the true spelling, so no caller
sees the mangling. No name of its own contains a double underscore, and the
generator's
--selftestis what holds that.Nothing at build time regenerates the blob: a cross build must not need python3
in the toolchain container.
--checkin CI is what catches a stale one — a rownaming a function no longer in the blob already fails to compile, and the check
is for the quiet half, a name left behind after its last row went.
Size
Same ARM PIE objects, V4 only:
.rel.dyn.data.rel.ro.rodata.textEvery family shrinks: V5 −9,080, V4A −7,076, V2 −4,520, V3A −2,928, V1 −2,060,
and −48,852 with all of them in. The static
ipctoolbinary, which has norelocations at all, still loses 12 KB on arm32 and 48 KB on arm64 — that
part is just a 2-byte offset against a 4- or 8-byte pointer.
In a real majestic
hi3516ev200 litebuild, only this submodule moving: thebinary goes 1,247,824 → 1,235,536, and the
R_ARM_RELATIVEcount goes2,962 → 1,538, which is exactly the 1,424 funcs pointers.
Verification
Every pad row of every family — address, selector, function name, GPIO
alternative and flags — dumped through
ipchw_padmux_by_prefixfromlibipchwbuilt before and after: 7,821 rows, byte-identical.reginfo_testpasses for everyIPCHW_PADMUXselection, which the workflow nowsweeps. It only built
v1before, and each family's names sit behind thatfamily's
#if, so a guard naming the wrong family would build everywhereexcept where it matters.
tools/check_hisi_padmux.pyreads the new spelling and still reads the old one,for a fork or a branch that has not been converted. That needed doing
deliberately: it would not have failed on the new tables, it would have parsed
every row as having no functions at all, called every register "in the document
but not in the table", and offered to
--fixthe lot.Left on the table
Within V4, 168 of 300 rows are exact duplicates — EV300 is a strict subset of
DV200, and 49 of 52 EV200 rows are in _8EV300. Sharing the row objects is
another ~2.8 KB, but it would make datasheet-derived tables reference another
chip's rows, which seems a bad trade for hand-maintained data.