sam0_eth: implement .confirm_send() to fix fragmented sending#20666
sam0_eth: implement .confirm_send() to fix fragmented sending#20666maribu merged 2 commits intoRIOT-OS:masterfrom
Conversation
github-actions
bot
added
Platform: ARM
|
Looks good from a quick peek. There still is a WIP commit. Do you mind to ping me when this is ready for review? |
benpicco
force-pushed
the
cpu/sam0_common-netdev_new_api
branch
from
5bc1d91 to
a8674d1
Compare
|
oops, forgot to reword that commit |
|
I can give it a try on hardware probably tomorrow evening. |
| return -EIO; | ||
| } | ||
|
|
||
| return 1; |
There was a problem hiding this comment.
Ideally it should return the bytes actually transmitted instead of 1 here for netstats to be correct.
There was a problem hiding this comment.
Makes me wonder: what happens with the return value of .send()?
There was a problem hiding this comment.
The return valur of .send() should be 0 on "no error as of now". This doesn't rule out that confirm_send reports an error later on.
The reasoning is that crazy things that change the size of the frame like bit-stuffing, escaping, padding (or whatever may apply to the netdev) can be done during TX, and is not needed to be accounted for when accepting the frame. After the fact it might be cheaper to provide the actual byte count.
There was a problem hiding this comment.
Ah good catch, I'm still returning the size of the frame as with the old API
|
Regarding the buffer alignment, it was probably me being too zealous. The only implicit alignment that I can see right now, is for RX buffers which use bits 31 to 2 to describe the address of the beginning buffer. Bits 0 & 1 are then reuse for other purpose in the buffer descriptors. |
dylad
left a comment
dylad
left a comment
There was a problem hiding this comment.
Looking good on my side too, just miss some IRQ bits enable (see linline)
|
@benpicco I've tested this on hardware. Looking good. You can also drop the additional commit. |
benpicco
force-pushed
the
cpu/sam0_common-netdev_new_api
branch
from
7429778 to
d6d7d90
Compare
benpicco
added
the
CI: ready for build
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
Thanks a bunch. Interested in running a PTP client on same54-xpro? With this in place, it should be relatively easy to add the missing pieces for that :)
5 participants
Contribution description
This implements the new confirm_send API which conveniently avoids the issue that occurs when sending while a transfer is ongoing (and all TX buffers are in use).
This happens e.g. when a large fragmented packet is sent, but would also happen with 4 small packets sent at once.
Since we don't need so many TX buffers anymore, I reduced
ETH_TX_BUFFER_COUNTto 2 - using just a single buffer still has issues.In theory the new API would also allow to directly use the buffers from the iolist, however those can't be expected to be aligned to
GMAC_BUF_ALIGNMENTso we can't take the zero-copy route. (I didn't try this though, from a quick glance I couldn't discover the section that prescribes that alignment in the data sheet.)Testing procedure
This PR contains a modified
examples/gnrc_networkingto allow for easy testing of large fragmented packets.I will drop when you agree with the driver changes.
Issues/PRs references
fixes issue described in #20534