|WARNING| examples/ptpclient: compensate for clock frequency drift

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Wed Oct 14 01:41:59 CEST 2026


Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/171207

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-13

I have reviewed this patch against the DPDK guidelines. Below are my findings.

---

## Errors

None.

---

## Warnings

**1. Missing release notes update**

The patch adds new functionality (PI clock servo with frequency adjustment) to an example application. This is a significant change in behavior that should be documented.

**Suggested fix:** Add an entry to `doc/guides/rel_notes/release_<version>.rst` documenting the clock servo enhancement.

**2. Global array initialization**

```c
static struct ptp_clock_servo ptp_servo[RTE_MAX_ETHPORTS];
```

At line 680, the code initializes `freq_adj_supported` to -1 for all ports in a loop. This is correct, but a designated initializer at declaration would be clearer:

```c
static struct ptp_clock_servo ptp_servo[RTE_MAX_ETHPORTS] = {
	[0 ... RTE_MAX_ETHPORTS - 1] = { .freq_adj_supported = -1 }
};
```

This would eliminate the need for the runtime loop.

---

## Info

**1. Print format suggestion**

At line 250:
```c
printf("\ntime transmitter clock id: %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x",
```

The leading newline makes the output context-dependent. Consider using a blank `printf("\n");` call before this if vertical spacing is needed, rather than embedding it in the formatted output.

**2. Double-precision arithmetic in fast path**

The clock servo performs floating-point arithmetic on every DELAY_RESP packet. This is acceptable for a control-path operation (PTP synchronization rate is typically 1-8 Hz), but the function could document that it is not intended for high-frequency calls.

**3. Hardcoded array bounds**

Line 683:
```c
for (portid = 0; portid < RTE_MAX_ETHPORTS; portid++)
```

This initializes the entire `ptp_servo` array, but only `ptp_enabled_port_nb` ports are actually in use. Consider iterating only over enabled ports for efficiency, though the current approach is safe.

**4. Servo parameter tuning**

The PI gains (`PTP_SERVO_KP = 0.7`, `PTP_SERVO_KI = 0.3`) and step threshold (`PTP_STEP_THRESHOLD_NS = 100000`) are taken from linuxptp. These are reasonable defaults, but different deployments may benefit from tunability. Consider making these configurable via command-line arguments in future work (not required for this patch).

---

Review-Result: WARNING


More information about the test-report mailing list