Re: [PATCH net] net: stmmac: ptp: fix support for PTP_EXTTS_REQUEST2 ioctl

[netdev-bot+sashiko@kernel.org]

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] This patch adds PTP_STRICT_FLAGS to .supported_extts_flags in
  stmmac_ptp_clock_ops and dwmac1000_ptp_clock_ops…

--

net: stmmac: ptp: fix support for PTP_EXTTS_REQUEST2 ioctl

This patch sets .supported_extts_flags to PTP_RISING_EDGE | PTP_STRICT_FLAGS
in stmmac_ptp_clock_ops and dwmac1000_ptp_clock_ops. The goal is to make
PTP_EXTTS_REQUEST2 work again for the rising edge, after the core started
validating extts flags.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index a37405534bebf..31a66e855d6b1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -277,6 +277,7 @@ const struct ptp_clock_info stmmac_ptp_clock_ops = {
>  	.n_per_out = 0, /* will be overwritten in stmmac_ptp_register */
>  	.n_pins = 0,
>  	.pps = 0,
> +	.supported_extts_flags = PTP_RISING_EDGE | PTP_STRICT_FLAGS,

[Severity: Medium]
Does setting PTP_STRICT_FLAGS here also change how the legacy
PTP_EXTTS_REQUEST (v1) ioctl behaves on stmmac? The same applies to the
identical line added to dwmac1000_ptp_clock_ops.

For v1, ptp_extts_request() in drivers/ptp/ptp_chardev.c masks the flags
to PTP_EXTTS_V1_VALID_FLAGS, which still includes PTP_FALLING_EDGE. It
only treats the edge bits as hints when the driver does not set
PTP_STRICT_FLAGS:

drivers/ptp/ptp_chardev.c:ptp_extts_request() {
    ...
	} 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. Every v1 edge combination
therefore reached stmmac_enable() or dwmac1000_ptp_enable(), and both of
them ignore the edge bits.

After this patch, a v1 request with PTP_FALLING_EDGE set returns
-EOPNOTSUPP. That covers falling only and rising plus falling. It also
covers a v1 disable request that carries edge bits, because the check
runs whether or not PTP_ENABLE_FEATURE is set.

The in-tree tools/testing/selftests/ptp/testptp.c sends exactly these
requests for -E 2 and -E 3:

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

Those calls appear to succeed today on stmmac and fail after this patch.

The commit message says:

    Only the PTP_EXTTS_REQUEST ioctl still works.

That suggests v1 behaviour is unchanged. Could the commit message
describe the v1 change explicitly, since the patch is tagged for stable?

This does seem to be an unavoidable side effect of making
PTP_EXTTS_REQUEST2 work. The kernel-doc for supported_extts_flags in
include/linux/ptp_clock_kernel.h requires PTP_STRICT_FLAGS for
PTP_EXTTS_REQUEST2. Several other drivers already use PTP_RISING_EDGE |
PTP_STRICT_FLAGS, including ptp_qoriq, enetc, dpaa2, bnxt and am65-cpts.

>  	.adjfine = stmmac_adjust_freq,
>  	.adjtime = stmmac_adjust_time,
>  	.gettimex64 = stmmac_get_timex,

[ ... ]

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