Give SigmaStar and Ingenic the layout their bootloader has - #131
Merged
Conversation
`FlashLayout::EIGHT_MEG_LAYOUT_VENDORS = %w[SigmaStar Ingenic]` was removed in #78 on the grounds that their bootloaders define mtdpartsnor16m and the uknor16m/urnor16m pair "down to the byte", the same as the HiSilicon and Goke ones. The repositories that carry those definitions -- u-boot-msc313e, u-boot-t20, u-boot-t40 -- are real, and no released binary is built from any of them, so no camera runs them. OpenIPC/firmware's .github/workflows/uboot.yml is what builds the bootloaders this site links. It clones openipc/u-boot-sigmastar and openipc/u-boot-ingenic and runs their build.sh; the SigmaStar one's spinor loop is `ssc377 ssc377d ssc377de ssc377qe ssc378de ssc378qe` against include/configs/infinity6c.h, which includes configs/sstar-common.h. That header is the whole environment every SSC3xx NOR camera boots with: kernaddr=0x50000 kernsize=0x200000 rootaddr=0x250000 rootsize=0x500000 rootmtd=5120k uknor / urnor / ubnor CONFIG_BOOTARGS "... mtdparts=NOR_FLASH:256k(boot),64k(env),2048k(kernel), ${rootmtd}(rootfs),-(rootfs_data) ..." A repo-wide grep for uknor8m|uknor16m|urnor8m|urnor16m|setnor8m|setnor16m| mtdpartsnor returns nothing in either repo. u-boot-ingenic's isvp_common.h carries the same unsuffixed uknor/urnor. **The 16MB image cannot boot.** One mtdparts string, a 2048k kernel inside it, and only ${rootmtd} varying: the rootfs starts at 0x250000 at every chip size. Handing these two NOR[16] put it at 0x350000. The image openipc.org serves for ssc377qe today has "hsqs" at 0x350000 and 0xff at 0x250000, and its env region at 0x40000 is blank -- a full-image flash erases the env, so the camera comes up on the compiled-in bootargs, looks for the rootfs at 0x250000, finds erased flash and panics on root mount. The 8MB-layout image of the same page has it at 0x250000 and boots. Since #130 the 16MB layout is what a 16MB chip selects by default, so this is what a SigmaStar visitor gets by pressing the button. **None of the macros exist.** `run setnor16m`, `run uknor16m; run urnor16m` and the printenv hint under them name variables no SigmaStar or Ingenic camera has. U-Boot answers `## Error: "uknor16m" not defined` and flashes nothing, with no failure the reader can see. This predates #78 -- the expert section has emitted suffixed macros for these vendors since it existed. So the pin was right for these two, even though its own comment gave a wrong reason for it. What replaces it is the same fact stated properly: they have one mtdparts, so 16MB means rootmtd=10240k at the same offset rather than a rootfs 1MB further up, and their macros are `uknor`/`urnor` with nothing after them. That keeps Ultimate reachable, which a flat pin to the 8MB numbers would not: its rootfs is 7832KB on ssc338q, 7252KB on ssc30kq and 6772KB on t31 against the 5120KB rootmtd defaults to. rootmtd is a plain variable, saved unexpanded and expanded at boot by `cmdnor`, so the step that a macro does elsewhere is a setenv here: setenv rootmtd 10240k; setenv rootsize 0xA00000 saveenv reset and where the layout is already the bootloader's own, that step is dropped rather than rendered as a `run setnor8m` under a heading calling it required. @flash_type_command is gone with it. It was the layout name, which is the macro suffix only for the bootloaders that name their macros after the layout, and the view read it in four places -- so the offsets and the commands could disagree. Camera answers both now, off the same suffix.
The test that pinned every vendor to 0x350000 was written when every vendor had the same answer. It still checks the thing it was for -- that the image and the page cannot describe different layouts -- but reads the offset each bootloader is actually on. A model of its own per vendor with it: the cache filename carries the model and not the vendor, so one model built under three vendors has the second and third read back the first's image. Nothing in production can hit that, a model belongs to one vendor, but a test that did would pass on a stale file.
PR Summary by QodoAlign SigmaStar and Ingenic NOR layouts with shipped bootloaders
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1. Stale images survive layout change
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| # bootloader cannot boot. | ||
| def nor_layout | ||
| @nor_layout ||= FlashLayout.nor(@layout) | ||
| @nor_layout ||= FlashLayout.nor(@layout, @soc.vendor.name) |
There was a problem hiding this comment.
1. Stale images survive layout change 🐞 Bug ≡ Correctness
The new vendor-specific layout is only applied when an image is assembled, while natural 16MB images retain their old cache filename; a fresh, same-size pre-PR SigmaStar/Ingenic image therefore bypasses assembly and is served with its rootfs still at 0x350000. This leaves the primary boot failure unfixed until the source archive becomes newer or the age-based cache purge removes the artifact.
Agent Prompt
## Issue description
The PR changes the assembled layout of natural 16MB SigmaStar and Ingenic NOR images, but those images keep the same cache filename. Existing same-size cached images pass the freshness checks and can continue to be served with the old, unbootable rootfs offset.
## Issue Context
Firmware cache validity currently depends on file existence, permissions, size, and source mtimes; it does not include a layout implementation version or inspect partition offsets. Ensure deployment or cache-key behavior forces all affected pre-change images to be rebuilt, while preserving safe concurrent generation.
## Fix Focus Areas
- app/models/firmware.rb[74-92]
- app/models/firmware.rb[136-183]
- app/models/firmware.rb[197-212]
- app/models/firmware.rb[448-456]
- test/models/firmware_test.rb[601-614]
- deploy/purge-firmware-cache.sh[50-68]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 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.
Found while installing OpenIPC on a 16MB SSC377QE camera and then checking the page against what actually went on the flash.
The image a 16MB SigmaStar chip is offered cannot boot
FlashLayout::EIGHT_MEG_LAYOUT_VENDORS = %w[SigmaStar Ingenic]was removed in #78, on the grounds that those vendors' bootloaders definemtdpartsnor16mand theuknor16m/urnor16mpair the same as the HiSilicon and Goke ones.u-boot-msc313e,u-boot-t20andu-boot-t40do define them — and no released binary is built from any of them.OpenIPC/firmware/.github/workflows/uboot.ymlis what builds the bootloaders this site links. It clonesopenipc/u-boot-sigmastarandopenipc/u-boot-ingenicand runs theirbuild.sh; the SigmaStar one'sspinorloop is literallyssc377 ssc377d ssc377de ssc377qe ssc378de ssc378qeagainstinclude/configs/infinity6c.h, which includesconfigs/sstar-common.h. That header is the whole environment every SSC3xx NOR camera boots with:A repo-wide grep for
uknor8m|uknor16m|urnor8m|urnor16m|setnor8m|setnor16m|mtdpartsnorreturns nothing in either repo;u-boot-ingenic'sisvp_common.hhas the same unsuffixeduknor/urnor.One mtdparts string, a 2048k kernel inside it, and only
${rootmtd}varying — so the rootfs starts at 0x250000 at every chip size. Handing these twoNOR[16]puts it at 0x350000. Probed on the images the site serves today for ssc377qe (flash_size=16&fw_release=lite), by range request:layout=8layout=16ff ff ff ff27051956uImage27051956uImage68737173"hsqs"ff ff ff ff68737173"hsqs"The env region is blank because the full-image flash erases it, so the camera comes up on the compiled-in bootargs, looks for the rootfs at 0x250000, finds erased flash and panics on root mount. Since #130 the 16MB layout is what a 16MB chip selects by default, so that is what a SigmaStar visitor gets by pressing the button. The 8MB-layout image of the same page is correct and boots.
None of the macros the expert section names exist
run setnor16m,run uknor16m; run urnor16mand theprintenvhint under them name variables no SigmaStar or Ingenic camera has. U-Boot answers## Error: "uknor16m" not definedand flashes nothing, with no failure the reader can see. This one predates #78 — the expert section has emitted suffixed macros for these vendors since it existed.What this does
The pin was right for these two; its stated reason was not. This replaces it with the fact itself:
FlashLayoutgets a second table for bootloaders with one mtdparts string. 16MB there meansrootmtd=10240kat the same 0x250000, not a rootfs 1MB further up; the kernel partition stays 2048k because the bootargs say so.Camera#bootloader_macro_suffixrather than the layout name, so these cameras are toldrun uknor; run urnorand the hint namesuknor, urnorwith noset…they could not find in their own printenv.Camera#layout_commands: a macro where there is one, and thesetenvthat macro would have done where there is not —setenv rootmtd 10240k; setenv rootsize 0xA00000,saveenv,reset. Empty where the layout is already the bootloader's default, and the heading above it is skipped rather than printed over a command that does nothing.@flash_type_commandis gone. It was read in four places in the view, so the offsets and the commands could disagree.Keeping both layouts is deliberate: an Ultimate rootfs is 7832KB on ssc338q, 7252KB on ssc30kq and 6772KB on t31, against the 5120KB
rootmtddefaults to, so a flat pin to the 8MB numbers would take Ultimate off eleven SoCs that publish it.rootmtdis a plain variable, stored unexpanded and expanded at boot bycmdnor, so raising it is all that is needed and nothing has to move.What I have and have not verified
Verified on hardware and against the served artifacts:
mtdparts=nor0:— the vendor spelling — is ignored by the OpenIPC kernel, which names the deviceNOR_FLASH, and that the mismatch is exactly this root-mount panic;Not verified on hardware: the
rootmtd=10240kvariant. It follows fromsstar-common.hand from${rootmtd}being expanded at boot, but nobody has flashed an Ultimate image to a SigmaStar camera this way. Worth a bench run before this leaves draft — I have the hardware for the Lite half, not for an Ultimate one.The alternative fix is upstream rather than here: add
mtdpartsnor8m/mtdpartsnor16mand theuknor*/urnor*/setnor*sets tou-boot-sigmastarandu-boot-ingenicso they matchu-boot-hi3516ev200/include/configs/hi-common.h, and then the page's current behaviour becomes correct as written. That is a better end state and a much longer path; this change makes the page describe the bootloaders that exist today.