Re: [PATCH net v2 0/5] net: ptp: fix egregious supported flag checks

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

On 3/19/2025 2:21 PM, Paolo Abeni wrote:
> On 3/12/25 11:15 PM, Jacob Keller wrote:
>> In preparation for adding .supported_extts_flags and
>> .supported_perout_flags to the ptp_clock_info structure, fix a couple of
>> places where drivers get existing flag gets grossly incorrect.
>>
>> The igb driver claims 82580 supports strictly validating PTP_RISING_EDGE
>> and PTP_FALLING_EDGE, but doesn't actually check the flags. Fix the driver
>> to require that the request match both edges, as this is implied by the
>> datasheet description.
>>
>> The renesas driver also claims to support strict flag checking, but does
>> not actually check the flags either. I do not have the data sheet for this
>> device, so I do not know what edge it timestamps. For simplicity, just
>> reject all requests with PTP_STRICT_FLAGS. This essentially prevents the
>> PTP_EXTTS_REQUEST2 ioctl from working. Updating to correctly validate the
>> flags will require someone who has the hardware to confirm the behavior.
>>
>> The lan743x driver supports (and strictly validates) that the request is
>> either PTP_RISING_EDGE or PTP_FALLING_EDGE but not both. However, it does
>> not check the flags are one of the known valid flags. Thus, requests for
>> PTP_EXT_OFF (and any future flag) will be accepted and misinterpreted. Add
>> the appropriate check to reject unsupported PTP_EXT_OFF requests and future
>> proof against new flags.
>>
>> The broadcom PHY driver checks that PTP_PEROUT_PHASE is not set. This
>> appears to be an attempt at rejecting unsupported flags. It is not robust
>> against flag additions such as the PTP_PEROUT_ONE_SHOT, or anything added
>> in the future. Fix this by instead checking against the negation of the
>> supported PTP_PEROUT_DUTY_CYCLE instead.
>>
>> The ptp_ocp driver supports PTP_PEROUT_PHASE and PTP_PEROUT_DUTY_CYCLE, but
>> does not check unsupported flags. Add the appropriate check to ensure
>> PTP_PEROUT_ONE_SHOT and any future flags are rejected as unsupported.
>>
>> These are changes compile-tested, but I do not have hardware to validate the
>> behavior.
>>
>> There are a number of other drivers which enable periodic output or
>> external timestamp requests, but which do not check flags at all. We could
>> go through each of these drivers one-by-one and meticulously add a flag
>> check. Instead, these drivers will be covered only by the upcoming
>> .supported_extts_flags and .supported_perout_flags checks in a net-next
>> series.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> I admit I'm the most significant source of latency, but this series is
> IMHO a bit risky to land this late in the cycle in the net tree,
> especially considering the lack of H/W testing for the BCM phy.
> 
> What about applying this to net-next instead?
> 
> Thanks,
> 
> Paolo
> 


I am fine with that. I only sent to net originally because I thought
some folks might consider these worthy of backports due to the potential
for user-error. However, given the minimal testing it may make sense to
go ahead with next. Obviously no real users have complained about the
situation, and this is more in preparation to make the kernel more
resilient to such mistakes in the future with the .supported_*_flags
modifications coming next cycle.

Thanks,
Jake