Plan to validate supported flags in PTP core (Was: Re: [PATCH net v2 0/2] Fixes for perout configuration in IEP driver)

[Jacob Keller]

On 2/21/2025 2:22 PM, Jacob Keller wrote:
> 
> 
> On 2/20/2025 5:24 PM, Jakub Kicinski wrote:
>> On Wed, 19 Feb 2025 15:37:16 -0800 Jacob Keller wrote:
>>> On 2/18/2025 10:26 PM, Meghana Malladi wrote:
>>>> IEP driver supports both pps and perout signal generation using testptp
>>>> application. Currently the driver is missing to incorporate the perout
>>>> signal configuration. This series introduces fixes in the driver to
>>>> configure perout signal based on the arguments passed by the perout
>>>> request.
>>>>   
>>>
>>> This could be interpreted as a feature implementation rather than a fix.
>>>
>>> Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
>>
>> Agreed, ideally we should get a patch for net which rejects
>> all currently (as in - in Linus's tree) unsupported settings.
>> That would be a fix.
>>
>> Then once that's merged add support for the new settings in net-next.
>>
>> Hope that makes sense?
> 
> +1 on this direction, its important that the driver does not accept
> configuration which is incorrect.
> 
> Reminds me of my backlog to refactor this whole mess into a
> supported_flags field in the PTP ops structure. Maybe it is again time
> to revive that.
> 
I've been looking into this the last week or so and I have what i think
is a workable plan, but I'd like to get some feedback before continuing.

I believe we ought to add .supported_extts_flags and
.supported_perout_flags to the ptp_info struct. These will get checked
by the core, to reject commands which set flags not specified here.

For PTP_EXTTS_REQUEST, the PTP_ENABLE_FEATURE flag is always accepted.

The behavior of PTP_RISING_EDGE and PTP_FALLING_EDGE is somewhat
complicated. I think the best way to handle it is to check the
PTP_STRICT_FLAGS. If the driver sets this, then assume they will
validate the flags properly and only enable strict matching modes. In
that case, check against all flags as.

If the driver does not set PTP_STRICT_FLAGS, (For example, drivers which
don't set anything because they forgot), we only allow ENABLE,
RISING_EDGE, and FALLING_EDGE, but not STRICT. This essentially disables
the v2 ioctl, and prevents users from getting strict behavior, (since
the driver did not say it would handle strict behavior!)

For periodic output, its easy, since all the flags are present only with
the v2 ioctl, so we can do a simple & ~supported_flags check.

The bigger question I have is.. what should we do about the existing
drivers which do not bother to check flags at all?

In my search I found these driver files set the n_ext_ts field to a
non-zero value but do not have any flag checks:

>      • drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
>      • drivers/net/ethernet/freescale/enetc/enetc_ptp.c
>      • drivers/net/ethernet/intel/i40e/i40e_ptp.c
>      • drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
>      • drivers/net/ethernet/renesas/rtsn.c
>      • drivers/net/ethernet/renesas/rtsn.h
>      • drivers/net/ethernet/ti/am65-cpts.c
>      • drivers/net/ethernet/ti/cpts.h
>      • drivers/net/ethernet/ti/icssg/icss_iep.c
>      • drivers/net/ethernet/xscale/ptp_ixp46x.c
>      • drivers/net/phy/bcm-phy-ptp.c
>      • drivers/ptp/ptp_ocp.c
>      • drivers/ptp/ptp_pch.c
>      • drivers/ptp/ptp_qoriq.c

If a user sends the V2 ioctl, they'll happily ignore strict checks and
do who knows what.

With my changes applied, the core would now automatically reject the v2
ioctl.

I believe this is an improvement, but I'm not sure whether we should
just make this change with a net-next feature improvement.

Should I prepare a patch for net which updates the drivers to manually
check and reject requests with flags other than PTP_EXT_TS_V1_FLAGS
first, before sending my changes to the core?

Worse, there are a couple of drivers including one hardware in igb, the
renesas driver, and the lan743x driver which do some checks but which
ultimately don't properly honor the PTP_STRICT_FLAGS.

If I send these all together as separate patches, I end up with more
than 15 patches total, and thats before I've finished investigating the
PTP_PEROUT_REQUEST, which I believe is likely to have a similar number
of issues.

Would a series with individual patches for the 3 special cases + one
patch to handle all the drivers that have no explicit flag check be
acceptable? Or should I do individual patches for each driver and just
break the series up? Or are we ok with just fixing this in next with the
.supported_extts_flags change?

Thanks,
Jake