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.

v1: https://lore.kernel.org/all/20250211103527.923849-1-m-malladi@ti.com/

Meghana Malladi (2):
  net: ti: icss-iep: Fix pwidth configuration for perout signal
  net: ti: icss-iep: Fix phase offset configuration for perout signal

 drivers/net/ethernet/ti/icssg/icss_iep.c | 44 ++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 3 deletions(-)


base-commit: 071ed42cff4fcdd89025d966d48eabef59913bf2
-- 
2.43.0

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@intel.com>

> v1: https://lore.kernel.org/all/20250211103527.923849-1-m-malladi@ti.com/
> 
> Meghana Malladi (2):
>   net: ti: icss-iep: Fix pwidth configuration for perout signal
>   net: ti: icss-iep: Fix phase offset configuration for perout signal
> 
>  drivers/net/ethernet/ti/icssg/icss_iep.c | 44 ++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> 
> base-commit: 071ed42cff4fcdd89025d966d48eabef59913bf2

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@intel.com>

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?
-- 
pw-bot: cr

On 2/21/2025 6:54 AM, 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@intel.com>
> 
> 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?

I do agree that this can be interpreted as a feature implementation (as 
the bug here is: missing perout driver implementation for which the 
driver claims it supports). I will post the updated patch series as 
suggested by Jakub.

Thanks,
Meghana Malladi.

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@intel.com>
> 
> 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.

Thanks,
Jake

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@intel.com>
>>
>> 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

On Fri, 7 Mar 2025 15:48:27 -0800 Jacob Keller wrote:
> 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?

A mass rejection of unsupported settings feels like a net-next material
in general. Handling the more complex cases individually and the rest in
a big patch makes sense.

On 3/10/2025 9:25 AM, Jakub Kicinski wrote:
> On Fri, 7 Mar 2025 15:48:27 -0800 Jacob Keller wrote:
>> 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?
> 
> A mass rejection of unsupported settings feels like a net-next material
> in general. Handling the more complex cases individually and the rest in
> a big patch makes sense.

Alright. I think the three cases with drivers that check flags but do so
incorrectly are worth going to net. I'll send that series shortly, and
will follow up with the next series that introduces the supported flags
knobs afterwards.

Thanks,
Jake