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

stm32h7-eth: Properly maintain Tx tail pointer #1495

Merged
merged 1 commit into from
Aug 17, 2023
Merged

stm32h7-eth: Properly maintain Tx tail pointer #1495

merged 1 commit into from
Aug 17, 2023

Conversation

dancrossnyc
Copy link
Contributor

Analagous to #1493, we should maintain the Tx tail pointer properly, as well. Note that since the device won't examine a Tx descriptor until the tail pointer is set appropriately, we can relax stores and rely on the barrier in tx_notify to flush everything to RAM properly before writing the tail pointer and kicking off a DMA.

Copy link
Contributor

@lzrd lzrd left a comment

Choose a reason for hiding this comment

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

This is a very good fix. The previous mental model was that the OWN bit was the key concept driving IO progress and that the tail pointer was just used to tell the HW to look at descriptors again.
In fact, without proper use of the tail pointer, it is probable that the HW is constantly polling the descriptor for a change in the OWN bit, thereby wasting power and memory bandwidth. Dan observes that the HW was never entering the SUSPEND state.

Copy link
Collaborator

@mkeeter mkeeter left a comment

Choose a reason for hiding this comment

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

The code looks good and I tested it for a while locally.

There are a bunch of comments in ring.rs that are now outdated, talking about how we're careful to handle memory ordering with a RELEASE store. I'm going to pre-approve this now, but can you go through and update those comments before merging?

(apologies for not catching this during the Rx PR)

Analagous to #1493, we should maintain the Tx tail pointer
properly, as well.  Note that since the device won't examine a
Tx descriptor until the tail pointer is set appropriately, we
can relax stores and rely on the barrier in `tx_notify` to flush
everything to RAM properly before writing the tail pointer and
kicking off a DMA.
@dancrossnyc
Copy link
Contributor Author

The code looks good and I tested it for a while locally.

There are a bunch of comments in ring.rs that are now outdated, talking about how we're careful to handle memory ordering with a RELEASE store. I'm going to pre-approve this now, but can you go through and update those comments before merging?

I believe I've gotten them all; mind taking another quick look?

(apologies for not catching this during the Rx PR)

No worries!

@mkeeter
Copy link
Collaborator

mkeeter commented Aug 17, 2023

Looks good, thanks!

@dancrossnyc dancrossnyc merged commit f88d569 into master Aug 17, 2023
71 checks passed
@dancrossnyc dancrossnyc deleted the fixtx branch August 17, 2023 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants