Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sam0_eth: implement .confirm_send() to fix fragmented sending #20666

Merged
merged 2 commits into from
May 24, 2024

Conversation

benpicco
Copy link
Contributor

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_COUNT to 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_ALIGNMENT so 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_networking to 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

@benpicco benpicco requested review from dylad and keestux as code owners May 13, 2024 17:16
@benpicco benpicco requested a review from maribu May 13, 2024 17:16
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications labels May 13, 2024
@maribu
Copy link
Member

maribu commented May 13, 2024

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 benpicco force-pushed the cpu/sam0_common-netdev_new_api branch from 5bc1d91 to a8674d1 Compare May 13, 2024 17:49
@benpicco
Copy link
Contributor Author

oops, forgot to reword that commit

@dylad
Copy link
Member

dylad commented May 13, 2024

I can give it a try on hardware probably tomorrow evening.

return -EIO;
}

return 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally it should return the bytes actually transmitted instead of 1 here for netstats to be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder: what happens with the return value of .send()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://doc.riot-os.org/structnetdev__driver.html#a39585137953a92c1c58b5e0ad3262096

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch, I'm still returning the size of the frame as with the old API

@dylad
Copy link
Member

dylad commented May 13, 2024

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.

Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good on my side too, just miss some IRQ bits enable (see linline)

cpu/sam0_common/sam0_eth/eth-netdev.c Outdated Show resolved Hide resolved
@dylad
Copy link
Member

dylad commented May 15, 2024

@benpicco I've tested this on hardware. Looking good. You can also drop the additional commit.

@benpicco benpicco force-pushed the cpu/sam0_common-netdev_new_api branch from 7429778 to d6d7d90 Compare May 23, 2024 15:19
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 23, 2024
@github-actions github-actions bot removed the Area: examples Area: Example Applications label May 23, 2024
@riot-ci
Copy link

riot-ci commented May 23, 2024

Murdock results

✔️ PASSED

d6d7d90 cpu/sam0_common: eth: reduce TX buffer count

Success Failures Total Runtime
10104 0 10105 14m:13s

Artifacts

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@maribu maribu enabled auto-merge May 24, 2024 15:36
@maribu maribu added this pull request to the merge queue May 24, 2024
Merged via the queue into RIOT-OS:master with commit b55d79f May 24, 2024
27 checks passed
@benpicco benpicco deleted the cpu/sam0_common-netdev_new_api branch May 24, 2024 20:26
@mguetschow mguetschow added this to the Release 2024.07 milestone Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants