Fixed PTP software timestamping. #55
Open
Conversation
This file contains 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
The TX path ignored whether the SKB wanted a SW or HW stamp, and once HW timestamping was enabled for the whole NIC, it always returned HW stamps. With this fix, if the TX packet requests SW stamp, it correctly only gets SW stamps. On the RX path, the driver has started in an inconsistent state. By default, no timestamping should be done for RX packets. However, the driver automatically enabled RX timestamping on initialization. This fix correctly initializes the NIC with no timestamping and it needs to be set up via SIOCSHWTSTAMP. There was also a problem with the RX path on init. The NIC did not put PTP packets on the PTP ring, but it did append timestamps to them. That resulted in wrong RX packets which were 12/14 bytes longer, because the timestamp length is only subtracted from PTP ring packets. This was worked around by adding a "flipping" enable_ptp() call that first sets the opposite value and then the actually wanted one. This does not seem to be needed for ATL2 chips. Signed-off-by: Martin Pecka <[email protected]>
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.
The TX path ignored whether the SKB wanted a SW or HW stamp, and once HW timestamping was enabled for the whole NIC, it always returned HW stamps. With this fix, if the TX packet requests SW stamp, it skips the whole PTP path (unless both HW and SW stamps are requested).
On the RX path, the driver has started in an inconsistent state. By default, no timestamping should be done for RX packets. However, the driver automatically enabled RX timestamping on initialization. This fix correctly initializes the NIC with no timestamping and it needs to be set up via SIOCSHWTSTAMP.
There was also a problem with the RX path on init. The NIC did not put PTP packets on the PTP ring, but it did append timestamps to them. That resulted in wrong RX packets which were 12/14 bytes longer, because the timestamp length is only subtracted from PTP ring packets. This was worked around by adding a "flipping" enable_ptp() call that first sets the opposite value and then the actually wanted one.
I tested this on QNAP QXG-10G2T with two AQC107S and firmware 3.1.121 on an NVidia Jetson AGX Xavier with kernel 4.9.337.
The problem manifested when using with linuxptp and passing the
-Sflag toptp4l. As a master, it told that there was no tx timestamp found for the sent SYNC packet (because it received a HW stamp which overrides the requested SW stamp). As a slave, it failed to read incoming SYNC messages withbad messageerror which was caused by the incorrect length of the RX packet.I tested this fix both at cold boot (which brings the weird RX config mismatch) and on-the-fly switching between timestamping on and off (
sudo hwstamp_ctl -i eth2 -r 0 -t 0andsudo hwstamp_ctl -i eth2 -r 12 -t 1), link losses and reacquisitions etc. Everything works with this PR.I also tested this with FW 3.0.33, which does not have PTP support. The driver does, however, provide full TX and RX software timestamping capabilities, and these also work with this PR.
I also tested that HW timestamping still works correctly with this PR (on 3.1.121 FW).
I have a few Thunderbolt cards with AQC107S, too, so I'll try to test with these on a normal notebook with newer kernel. I also have one card with (probably) AQC113C, so I'll also test with that one.
Update:
I've added the NEEDS_PTP_FLIP quirk to all cards. I don't really know if it's needed for all of them. But I assume it can't hurt anything.
Also, I needed to remove check for
!aq_ptp->a1_ptpfromaq_ptp_clock_init()so that the initial weird RX state is fixed. I don't really know why this check was there.