dw_upsizer: Pad when the fast path accepts a beat carrying TLAST - #9
Open
dngr2 wants to merge 1 commit into
Open
dw_upsizer: Pad when the fast path accepts a beat carrying TLAST#9dngr2 wants to merge 1 commit into
dngr2 wants to merge 1 commit into
Conversation
AcceptDataIn moves to Pad when a beat arrives with TLAST before the wide
word is full, so the partial word is flushed. The DataOut fast path
("already accept next subtransfer") also consumes a beat and latches its
TLAST, but returned to AcceptDataIn unconditionally.
A frame whose final beat was taken through that path therefore never
padded. Its remainder stayed in the register with the stream idle, and
left later merged into the next frame that happened to fill the word.
Silent truncation of one frame and corruption of the next, with no error
raised at any point.
Only lengths one beat past a word boundary reach the fast path with TLAST
set. At 64 bits out and 8 bits in that is 9, 17, 25 and so on; a length of
1 is unaffected because the first beat after reset is taken by
AcceptDataIn. pulp-ethernet drives exactly this configuration on its
receive path, where an Ethernet frame of 65, 73 or 1513 bytes hits it.
The beat accepted on the fast path is the first of a new word
(counter_d == 1) and DataWidthOut > DataWidthIn is asserted, so the word
cannot already be full: TLAST alone decides, and Pad handles the rest.
The added testbench sweeps every length from one beat to three words
rather than picking one, since a single hand-chosen length very likely
misses this, and holds TVALID high across the frame because a gap before
the final beat hides it. Against the unfixed module three of its checks
fail.
Verified with Verilator 5.006; CI does not run simulation.
dngr2
force-pushed
the
fix/dw-upsizer-pad-after-fastpath-last
branch
from
August 16, 2026 02:47
4edc26e to
95989b0
Compare
Author
|
Note on the force-push you will see on this branch: the commit author email was wrong, and I corrected it. The tree is unchanged — same diff, same tests, only the author metadata differs. I know |
Collaborator
|
Thanks a lot @dngr2. It would also really help if you could write down a cookie recipe. |
Author
|
Ha — I think I'll keep the baking off GitHub. Happy to address anything on the actual upsizer change though; just point me at it and I'll push a commit. |
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
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.
Independent of #8 — different module, no shared lines, either can go first.
The bug
AcceptDataInmoves toPadwhen a beat arrives withTLASTbefore the wide word is full, so the partial word gets flushed. TheDataOutfast path (// already accept next subtransfer) also consumes a beat and latches itsTLAST, but returns toAcceptDataInunconditionally.So a frame whose final beat is taken through that path is never padded. Its remainder sits in the register with the stream idle, and leaves later merged into whichever frame next fills the word.
One frame silently truncated, the next silently corrupted, and nothing reports an error.
Which lengths
Only lengths one beat past a word boundary reach the fast path with
TLASTset. At 64 bits out / 8 bits in that is 9, 17, 25… A length of 1 is unaffected, because the first beat after reset is taken byAcceptDataIn.Sweeping every length from 1 to 24 beats against the unfixed module:
Exactly the ≡1 (mod 8) lengths, as the mechanism predicts.
This matters beyond the abstract case:
pulp-ethernetinstantiates this module in exactly this configuration on its receive path, so an Ethernet frame of 65, 73 or 1513 bytes hits it.The fix
The beat accepted on the fast path is the first of a new word (
counter_d == 1), andDataWidthOut > DataWidthInis asserted at elaboration, so the word cannot already be full.TLASTalone therefore decides, andPadhandles the alignment exactly as it does for theAcceptDataInpath.The testbench
test/tb_axi_stream_dw_upsizer_last.sv, registered inBender.yml.Two deliberate choices, both of which the bug hides behind:
TVALIDhigh across the frame. Insert a gap before the final beat and the fast path is never taken, so the bug does not appear.Against the unfixed module three checks fail; with the fix all pass, including content checks that the 9-beat frame's tail byte,
TKEEP=0x01andTLASTare right rather than merely that two words came out.Verified with Verilator 5.006. As with #8, CI here runs license, commit and verible lint but no simulation, so this is a local result and worth an independent check.