Re: [PATCH net-next 3/3] net: ethernet: ti: am65-cpts: implement PTP_SYS_OFFSET_PRECISE

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The optional ti,gtc-push property fails in two different ways
  in am65_cpts_of_parse() (drivers/net/ethernet/ti/am65-cpts.c).
- [Medium] am65_cpts_gtc_push_select() in
  drivers/net/ethernet/ti/am65-cpts.c writes the EXPBIT_SEL field of…
- [Medium] am65_cpts_gtc_push_select() in
  drivers/net/ethernet/ti/am65-cpts.c reads GTC_CFG0_PUSHEVT back only…

--

net: ethernet: ti: am65-cpts: implement PTP_SYS_OFFSET_PRECISE

This commit adds getcrosststamp support to the TI K3 CPTS driver. It uses
the Global Timebase Counter push event output, routed to a CPTS hardware
timestamp input. For each request the driver enables the push input,
waits for an event and pairs the PHC timestamp with the computed GTC
edge, retrying up to three times. The feature is enabled when the CPTS
node has a ti,gtc-push property naming the GTC CFG0 syscon and the push
input.

> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index 0d71147f3c291..70b0d43bfe788 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c

[ ... ]

> @@ -797,6 +839,226 @@ static int am65_cpts_ptp_enable(struct ptp_clock_info *ptp,

[ ... ]

> +/* select the counter bit the GTC exports on its push event output */
> +static int am65_cpts_gtc_push_select(struct am65_cpts *cpts)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_update_bits(cpts->gtc, AM65_CPTS_GTC_PUSHEVT,
> +				 AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL,
> +				 FIELD_PREP(AM65_CPTS_GTC_PUSHEVT_EXPBIT_SEL,
> +					    AM65_CPTS_GTC_PUSH_BIT));

[Severity: Medium]
Is it safe for this driver to own the EXPBIT_SEL field of GTC_CFG0_PUSHEVT?

The register belongs to the SoC-wide GTC, not to the CPTS.
am65_cpts_ptp_getcrosststamp() rewrites it on every PTP_SYS_OFFSET_PRECISE
request. The previous value is never saved or restored, on success or on
error.

The GTC push event output goes into the Time Sync Router, which can send
it to other timesync capture inputs, SYNC_OUT pins, or software on other
cores. Suppose one of those consumers expects a different exported bit.
Won't its signal frequency change after the first request from Linux, and
then be forced back to bit 15 on every later request?

The ti,gtc-push binding added in this series describes the GTC as the
SoC's system counter. It doesn't give this CPTS exclusive ownership of the
push output.

> +	if (ret)
> +		return ret;
> +	/* flush the posted write before the input is enabled */
> +	return regmap_read(cpts->gtc, AM65_CPTS_GTC_PUSHEVT, &val);

[Severity: Medium]
Should val be checked here, to confirm that EXPBIT_SEL now reads back as
AM65_CPTS_GTC_PUSH_BIT?

am65_cpts_gtc_push_next_edge() and the window check in
am65_cpts_gtc_push_wait() both assume rising edges at k * 2^16 + 2^15:

	if ((s64)(popped - edge) < 0 ||
	    (s64)(popped - edge) >= (s64)AM65_CPTS_GTC_PUSH_PERIOD) {
		...
		return -EAGAIN;
	}

	push->cycles = edge;

The write might not take effect. For example, the CFG0 region could be
firewalled or read-only in some security configuration, or another agent
could rewrite the field. Then the exported bit b' is not 15.

With b' < 15, the first event comes from an edge before the expected
bit-15 edge. If IRQ thread latency puts popped past edge, the attempt is
accepted with push->cycles = edge. The result is off by up to that
latency.

With b' = 16, every bit-16 edge falls 2^15 cycles after a bit-15 edge,
which is inside the window. About half of attempts would pass while being
about 164 us off at 200 MHz.

In both cases, wouldn't PTP_SYS_OFFSET_PRECISE return a wrong
cross-timestamp to userspace with no error? The syscon regmap is uncached,
so a FIELD_GET() on val would catch the mismatch.

[ ... ]

> @@ -1160,6 +1423,24 @@ static int am65_cpts_of_parse(struct am65_cpts *cpts, struct device_node *node)
>  		}
>  	}
>  
> +	if (of_property_present(node, "ti,gtc-push")) {
> +		gtc = syscon_regmap_lookup_by_phandle_args(node, "ti,gtc-push",
> +							   1, &prop[0]);
> +		if (IS_ERR(gtc))
> +			return dev_err_probe(cpts->dev, PTR_ERR(gtc),
> +					     "Failed to get GTC syscon\n");

[Severity: Medium]
Should a failed syscon lookup be fatal here? A bad index just below only
logs an error and leaves the feature disabled.

Returning here makes am65_cpts_create() return ERR_PTR(ret).
am65_cpsw_init_cpts() in am65-cpsw-nuss.c then fails the whole CPSW probe:

	if (IS_ERR(cpts)) {
		int ret = PTR_ERR(cpts);

		of_node_put(node);
		dev_err(dev, "cpts create err %d\n", ret);
		return ret;
	}

So a problem with the optional cross-timestamp property takes down the
Ethernet interface.

A correct DT can also hit this. TI_K3_AM65_CPTS does not select or depend
on MFD_SYSCON, and ARCH_K3 does not select it either:

config TI_K3_AM65_CPTS
	tristate "TI K3 AM65x CPTS"
	depends on ARCH_K3 && OF
	depends on PTP_1588_CLOCK

With CONFIG_MFD_SYSCON=n, the syscon stub returns ERR_PTR(-ENOTSUPP). Any
DT with ti,gtc-push would then stop Ethernet from probing. The same
happens if the phandle points at a node without the "syscon" compatible.

The existing ti,pps handling and the new index check both treat errors as
non-fatal. Could this path do the same, or should a Kconfig dependency on
MFD_SYSCON be added?

> +
> +		if (prop[0] > 7 ||
> +		    (cpts->pps_present && prop[0] == cpts->pps_hw_ts_idx)) {
> +			dev_err(cpts->dev,
> +				"invalid GTC push HWx_TS_PUSH index: %u\n",
> +				prop[0]);
> +		} else {
> +			cpts->gtc = gtc;
> +			cpts->gtc_push_hw_idx = prop[0];
> +		}
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794284248.git.jjc%40jclark.com