Re: [PATCH net v2 3/3] ptp: qoriq: 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] The commit message says that making the legacy
  PTP_EXTTS_REQUEST ioctl strict "cannot be avoided", because the core…

--

ptp: qoriq: fix support for PTP_EXTTS_REQUEST2 ioctl

This patch sets .supported_extts_flags in ptp_qoriq_caps to
PTP_RISING_EDGE | PTP_STRICT_FLAGS, so that the PTP core stops rejecting
every PTP_EXTTS_REQUEST2 request. Because of this, the legacy
PTP_EXTTS_REQUEST ioctl now also rejects falling-edge and both-edge
requests.

> This also makes the legacy PTP_EXTTS_REQUEST ioctl strict: a request for
> the falling edge, or for both edges, now fails with -EOPNOTSUPP instead
> of being timestamped on the rising edge. That cannot be avoided: the core
> adds PTP_STRICT_FLAGS to every PTP_EXTTS_REQUEST2 request, so a driver
> has to declare it to accept them, and once declared it applies to both
> ioctls.

> 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: Medium]
Is the "cannot be avoided" part of the commit message accurate?

In ptp_extts_request() in drivers/ptp/ptp_chardev.c, the strict bit is
only added for PTP_EXTTS_REQUEST2. For the legacy ioctl it is always
removed by the mask:

drivers/ptp/ptp_chardev.c:ptp_extts_request() {
    ...
	if (cmd == PTP_EXTTS_REQUEST2) {
		/* Tell the drivers to check the flags carefully. */
		req.extts.flags |= PTP_STRICT_FLAGS;
    ...
	} else {
		req.extts.flags &= PTP_EXTTS_V1_VALID_FLAGS;
    ...
}

PTP_EXTTS_V1_VALID_FLAGS contains only ENABLE, RISING and FALLING. The
.enable callback can therefore tell the two ioctls apart by checking
PTP_STRICT_FLAGS in rq->extts.flags.

Drivers in the tree already do this. mv88e6xxx declares
RISING | FALLING | STRICT and rejects the unsupported edge combination
only for strict requests:

drivers/net/dsa/mv88e6xxx/ptp.c:mv88e6352_ptp_enable_extts() {
    ...
	if ((rq->extts.flags & PTP_STRICT_FLAGS) &&
	    (rq->extts.flags & PTP_ENABLE_FEATURE) &&
	    (rq->extts.flags & PTP_EXTTS_EDGES) == PTP_EXTTS_EDGES)
		return -EOPNOTSUPP;
    ...
}

sja1105 does something similar in sja1105_ptp.c.

Could qoriq do the same? It would declare PTP_RISING_EDGE |
PTP_FALLING_EDGE | PTP_STRICT_FLAGS in ptp_qoriq_caps. Then
ptp_qoriq_enable() would return -EOPNOTSUPP when STRICT, ENABLE and
FALLING are all set. That fixes PTP_EXTTS_REQUEST2 and leaves the legacy
ioctl lenient. enetc_ptp.c also uses ptp_qoriq_enable(), so the same
check would cover enetc.

As written, with the Fixes: tag, this is going to stable. Legacy
PTP_EXTTS_REQUEST calls on QorIQ timers that ask for the falling edge or
for both edges now fail with -EOPNOTSUPP. Before this patch they
succeeded and were timestamped on the rising edge. Commit 7c571ac57d9d
calls the legacy edge flags hints that are not expected to be enforced.

testptp hits this path. Running it with -e and -E 2 or -E 3 sends
PTP_EXTTS_REQUEST with PTP_FALLING_EDGE set.

If the stricter legacy behaviour is intended, could the commit message
say that a lenient alternative exists, rather than saying the change
cannot be avoided?

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

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