Re: [PATCH net 1/3] dpaa2-ptp: fix support for PTP_EXTTS_REQUEST2 ioctl

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] Declaring PTP_STRICT_FLAGS in dpaa2_ptp_caps also makes the
  legacy PTP_EXTTS_REQUEST (v1) ioctl strict on DPAA2, and the commit…

--

dpaa2-ptp: fix support for PTP_EXTTS_REQUEST2 ioctl

This patch sets .supported_extts_flags on dpaa2_ptp_caps to
PTP_RISING_EDGE | PTP_STRICT_FLAGS. With that set, the PTP core accepts
PTP_EXTTS_REQUEST2 on DPAA2 and rejects requests for the falling edge,
which the timer does not support.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> index 4497e3c0456d1..ac2b873dcf8e6 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
> @@ -78,6 +78,7 @@ static const struct ptp_clock_info dpaa2_ptp_caps = {
>  	.n_per_out	= 3,
>  	.n_pins		= 0,
>  	.pps		= 1,
> +	.supported_extts_flags = PTP_RISING_EDGE | PTP_STRICT_FLAGS,

[Severity: Medium]
Does declaring PTP_STRICT_FLAGS here also make the legacy
PTP_EXTTS_REQUEST ioctl strict on DPAA2?

In ptp_extts_request() in drivers/ptp/ptp_chardev.c, v1 requests are
only masked to PTP_EXTTS_V1_VALID_FLAGS. They then go through the same
supported-flags check as v2:

	} else {
		req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
		memset(req.extts.rsv, 0, sizeof(req.extts.rsv));
	}
	[ ... ]
	if (!(supported_extts_flags & PTP_STRICT_FLAGS))
		supported_extts_flags |= PTP_EXTTS_EDGES;
	/* Reject unsupported flags */
	if (req.extts.flags & ~supported_extts_flags)
		return -EOPNOTSUPP;

Before this patch, supported_extts_flags was 0. The effective mask was
therefore ENABLE | RISING | FALLING, and v1 accepted any edge
combination as a hint.

After this patch the mask is ENABLE | RISING | STRICT. A v1 request
with PTP_ENABLE_FEATURE | PTP_FALLING_EDGE, or with both edge bits set,
now returns -EOPNOTSUPP before ops->enable() is called.

One in-tree caller hits this. tools/testing/selftests/ptp/testptp.c
run with -e N -E 2 or -E 3 sends the edge through the legacy ioctl:

	extts_request.flags = PTP_ENABLE_FEATURE | edge;
	if (ioctl(fd, PTP_EXTTS_REQUEST, &extts_request)) {
		perror("PTP_EXTTS_REQUEST");
		extts = 0;

That used to succeed on DPAA2. Now it fails and skips timestamp
collection.

Commit 7c571ac57d9d ("net: ptp: introduce .supported_extts_flags to
ptp_clock_info") said that the edge flags are "merely 'hints' when
using the old PTP_EXTTS_REQUEST ioctl, and are not expected to be
enforced".

The commit message says falling-edge requests should be rejected. It
doesn't say that legacy v1 users are affected too, including the
both-edges hint case.

Should the commit message mention this change for PTP_EXTTS_REQUEST
users? The later patches in the series apply the same change to ENETC
and generic QorIQ PTP, so it would reach those devices as well.

>  	.adjfine	= ptp_qoriq_adjfine,
>  	.adjtime	= ptp_qoriq_adjtime,
>  	.gettime64	= ptp_qoriq_gettime,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-for-upstream-qoriq-ptp-extts-flags-v1-0-b4a161b92832%40free.fr