Re: [PATCH net v2] net: dsa: microchip: save the periodic output request

[Paolo Abeni <pabeni@redhat.com>]

On 9/22/26 08:32, Bastien Curutchet wrote:
> On 9/21/26 10:52 AM, netdev-bot+sashiko@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential
>> issue(s) to consider.
>>
>> Critical: 0 · High: 1 (2) · Medium: 1 (2) · Low: 0 (1)
>>
>> 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).
>>
>> - [High] ksz_ptp_enable_perout()
>>    (drivers/net/dsa/microchip/ksz_ptp.c:226) commits the caller-supplied…
>> - [Medium] The commit message states that 'if a flag was set in the
>>    request that triggered the periodic output, it is lost when the output…
>>
>> Pre-existing issues:
>> - [High] A non-canonical but ABI-accepted period whose timespec64
>>    conversion is zero (e.g. period.sec = -1, period.nsec = 1000000000;…
>> - [High] In ksz_ptp_restart_perout()
>>    (drivers/net/dsa/microchip/ksz_ptp.c) the elapsed-period quotient is…
>> - [Medium] The device exposes three trigger output units (caps.n_per_out
>>    = 3 at drivers/net/dsa/microchip/ksz_ptp.c:1056, PTP_TOU_INDEX =…
>> - [Medium] ksz_ptp_enable_perout() resolves the output pin with
>>    ptp_find_pin() (drivers/net/dsa/microchip/ksz_ptp.c:206).
>> - [Low] ksz_ptp_tou_reset() (drivers/net/dsa/microchip/ksz_ptp.c:66-86)
>>    assigns the result of the TRIG_RESET read-modify-write to 'ret' at…
>>
>> -- 
>>
>> net: dsa: microchip: save the periodic output request
>>
>> The patch replaces the two stored timespec64 values (perout_target_time_first
>> and perout_period) in struct ksz_ptp_data with a copy of the full
>> ptp_perout_request, so that the index and flags survive a perout restart.
>> ksz_ptp_restart_perout() now replays the saved request with only the start
>> time recomputed.
>>
>> A few questions below.
>>
>>>      When a periodic output is initialized, only the start and the period are
>>>      stored, not the flags nor the pin index. So when the periodic output is
>>>      restarted, the request flags and index are always set to 0. So if a pin
>>>      other than the first one was used, or if a flag was set in the request
>>>      that triggered the periodic output, it is lost when the output is
>>>      restarted.
>>
>> [Severity: Medium]
>> Can request->flags ever be non-zero in this driver?
>>
>> ksz_ptp_clock_register() sets caps.n_pins, caps.n_per_out and the callbacks,
>> but never sets caps.supported_perout_flags, and caps lives in the zeroed
>> struct ksz_device, so the mask stays 0. The core rejects any flag that is not
>> advertised before it calls the driver:
>>
>> drivers/ptp/ptp_chardev.c:ptp_perout_request() {
>>     ...
>>     if (perout->flags & ~ops->supported_perout_flags)
>>         return -EOPNOTSUPP;
>>     ...
>>     return ops->enable(ops, &req, perout->period.sec || perout->period.nsec);
>> }
>>
> 
> This check is 'fairly' recent, it was added by d9f3e9ecc456 ("net: ptp: introduce .supported_perout_flags to ptp_clock_info") (v6.15). I experienced the reset issue on a v6.12 kernel.
> 
> Even if it can't happen right now, as soon as this driver registers any new flag, the bug will come back.
> 
> 
> 
> All others comments are already existing issues that can be fixed independently from this patch so I don't plan to address them right now.
This one:

"""
Should the request be committed to ptp_data->perout_request only after it has
been validated and the hardware has actually been programmed?
[...]

There is also a period that this check rejects but the memcpy has already
stored: period.sec = 4, period.nsec = 294967296 gives 0x100000000 ns, and
TRIG_CYCLE_WIDTH_M is GENMASK(31, 0), so the request returns -EINVAL. On the
next clock step, ksz_ptp_restart_perout() feeds that value to
div_u64(now_ns - first_ns, period_ns), whose divisor parameter is u32, so the
divisor truncates to 0. Is that a divide error in process context with
ptp_data->lock held?
"""

Looks new to me, and bad.

Also if the issue you are observing on an older kernel is already fixed in
the vanilla tree, I suggest instead sending to stable the relevant change.

Thanks,

Paolo