Re: [PATCH net 3/3] ptp: qoriq: fix support for PTP_EXTTS_REQUEST2 ioctl

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch sets `.supported_extts_flags = PTP_RISING_EDGE |
  PTP_STRICT_FLAGS` in `ptp_qoriq_caps` (drivers/ptp/ptp_qoriq.c).

--

ptp: qoriq: fix support for PTP_EXTTS_REQUEST2 ioctl

The patch sets .supported_extts_flags in ptp_qoriq_caps to
PTP_RISING_EDGE | PTP_STRICT_FLAGS. As a result, PTP_EXTTS_REQUEST2 requests
are no longer rejected with -EOPNOTSUPP by the PTP core. It also makes the
core reject falling edge requests, which the hardware does not support.

> diff --git a/drivers/ptp/ptp_qoriq.c b/drivers/ptp/ptp_qoriq.c
> index 76272e5ac5b85..bcd29104effe7 100644
> --- a/drivers/ptp/ptp_qoriq.c
> +++ b/drivers/ptp/ptp_qoriq.c
> @@ -355,6 +355,7 @@ static const struct ptp_clock_info ptp_qoriq_caps = {
>  	.n_per_out	= 0,
>  	.n_pins		= 0,
>  	.pps		= 1,
> +	.supported_extts_flags = PTP_RISING_EDGE | PTP_STRICT_FLAGS,

[Severity: Low]
Does this also change the behaviour of the legacy PTP_EXTTS_REQUEST ioctl
on qoriq clocks? The commit message doesn't mention it.

For the legacy ioctl, ptp_extts_request() in drivers/ptp/ptp_chardev.c
masks the flags to the V1 set, which still includes PTP_FALLING_EDGE. It
never adds PTP_STRICT_FLAGS:

	} else {
		req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
		memset(req.extts.rsv, 0, sizeof(req.extts.rsv));
	}

Whether the edge bits are treated as hints depends only on what the driver
declares, not on which ioctl was used:

	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. Legacy requests with
PTP_FALLING_EDGE, or with PTP_RISING_EDGE | PTP_FALLING_EDGE to mean either
edge, were accepted and timestamped on the rising edge.

With this patch, those same legacy requests return -EOPNOTSUPP. For
example, testptp -e N -E 2 or -E 3 in tools/testing/selftests/ptp issues
PTP_EXTTS_REQUEST with PTP_FALLING_EDGE set. That worked on qoriq before
this patch and fails after it.

The rejection itself looks consistent with how the core treats strict
drivers. It also matches dpaa2-ptp and enetc_ptp, which use the same flag
set.

However, the subject and first paragraph describe a PTP_EXTTS_REQUEST2-only
fix, and present the legacy ioctl as the path that already works:

    A driver that does not set
    .supported_extts_flags still accepts the legacy PTP_EXTTS_REQUEST ioctl,
    but every PTP_EXTTS_REQUEST2 request is rejected with -EOPNOTSUPP

The last paragraph says "a falling edge request is rejected", but it doesn't
say this also applies to legacy PTP_EXTTS_REQUEST users. It also doesn't
mention the rising plus falling "either edge" case.

The Fixes: tag makes this a likely stable backport. Could the commit message
say explicitly that legacy PTP_EXTTS_REQUEST callers asking for a falling
edge or both edges will now get -EOPNOTSUPP?

>  	.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