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

[Bastien Curutchet]

Hi all,

On 9/21/26 10:52 AM, netdev-bot+sashiko@xxxxxxxxxx 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.


Best regards,
Bastien