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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/11/2026 5:02 PM, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 
Hm. The intended behavior had been that legacy ioctl would remain
supporting the non-strict mode. I guess the way its implemented doesn't
quite work. The strict flag is cleared automatically but only from the
req.extts.flags, and not from supported_extts_flags. I guess that might
be an oversight in the ioctl implementation. Its been a while so I can't
remember the specific reason this version was opted for. We could modify
the core to also allow non-strict mode for legacy ioctls..

I'd prefer not to fix this in the driver as the AI suggests here. Either
we accept that both ioctls will be strict in cases where the requested
mode is not listed, or we need to change the core to work this way.
Having each driver have its own variation of policy was one of the
things I aimed to avoid with my refactor :/

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