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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi Paolo,

On 9/22/26 12:01 PM, Paolo Abeni wrote:
> 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.
> 

I agree it is bad but it's not new IMO. In the current behavior, the 
period and target time are already stored before the request being 
validated. This patch only stores the rest of the request with them.

I'll still send a v3 storing the request at the end of the 
perout_enable() so both issues get fixed at once.

> 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.
> 
The issue isn't fixed in the vanilla tree, it's only hidden by the fact 
that the driver doesn't register the flags it supports in 
.supported_perout_flags.
And only the flag issue is hidden, the index still gets wrongly set to 0 
on restart.


Best regards,
Bastien