Re: [PATCH net-next v4 01/11] net: dsa: microchip: fully save the periodic output request

[Bastien Curutchet]

Hi all,

On 9/29/26 2:27 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 (1) · Medium: 1 (1) · 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] The patch keeps one saved request (ptp_data->perout_request) and
>    one device-wide tou_mode, but the driver allows several trigger output…
> - [Medium] This patch fixes a user-visible bug that already exists on the
>    switches supported today (KSZ9477 family), but the commit message does…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it is serious.
> - [Medium] This problem wasn't introduced by this patch, but the restart
>    path calls ptp_find_pin() without holding ptp_clock::pincfg_mux, which…
> - [Low] ksz_ptp_enable_perout() selects the TOU and resets it with
>    ksz_ptp_tou_reset(), which clears TRIG_ENABLE, before validation,…
>
> --
>
> net: dsa: microchip: fully save the periodic output request
>
> This commit replaces the saved start time and period in struct
> ksz_ptp_data with a full copy of the last successful struct
> ptp_perout_request. After settime/adjtime, ksz_ptp_restart_perout() now
> reuses that saved request, including its index and flags, instead of
> forcing both to zero.
>
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> > index 39cc70d65900b..47cf397481d5e 100644
> > --- a/drivers/net/dsa/microchip/ksz_ptp.c
> > +++ b/drivers/net/dsa/microchip/ksz_ptp.c
>
> [ ... ]
>
> > @@ -263,6 +261,8 @@ static int ksz_ptp_enable_perout(struct ksz_device *dev,
> >   	if (ret)
> >   		return ret;
> >   
> > +	memcpy(&ptp_data->perout_request, request,
> > +	       sizeof(struct ptp_perout_request));
> >   	ptp_data->tou_mode = KSZ_PTP_TOU_PEROUT;
> >   
> >   	return 0;
>
> [Severity: High]
> struct ksz_ptp_data has only one perout_request and one tou_mode. What
> happens when more than one periodic output is active?

Indeed, this patch series supports only one periodic output at a time, 
as is already the case for the others switches (which also have several 
output pins). This is sufficient for my use case, and it doesn't prevent 
more advanced support from being added later.
[...]

> [Severity: Low]
> This isn't a bug introduced by this patch,

[...]
> [Severity: High]
> This isn't a bug introduced by this patch,

[...]
> [Severity: Medium]
> The commit message says:
>
>    I don't know if other switches use non-zero indexes but the KSZ8463 has
>    twelve periodic outputs, so this index has to also be saved from now on.
>
> Doesn't the removed request.index = 0 already break restart on the
> KSZ9477-family switches supported today? ksz_ptp_clock_register()
> advertises:
>
> 	ptp_data->caps.n_per_out	= 3;
>
> ptp_perout_request() in drivers/ptp/ptp_chardev.c only rejects
> perout->index >= n_per_out, so indexes 1 and 2 reach
> ksz_ptp_enable_perout().
>
> With the old code, a clock step through ksz_ptp_settime() or
> ksz_ptp_adjtime() while channel 1 or 2 is running goes one of two ways:
>
> - If no pin is assigned to PEROUT channel 0, ptp_find_pin() fails and
>    ksz_ptp_enable_perout() returns -EINVAL. settime/adjtime then returns
>    an error and the real channel is never re-armed.
>
> - If a pin is assigned to channel 0, TOU0 is re-armed on that pin with
>    the other channel's period.
>
> Could the commit message describe this? Should it also carry a Fixes: tag
> for 1f12ae5b67608f ("net: dsa: microchip: ptp: add periodic output
> signal") so it can be considered for stable?

I had sent a fix for this but we agreed to handle this directly in 
net-next (cf 
https://lore.kernel.org/all/ee69ae01-97ab-470c-ba1d-658f1700463c@xxxxxxxxxxx/)

> >   
> >   	return ksz_ptp_enable_perout(dev, &request, 1);
>
> [Severity: Medium]
> This is a pre-existing issue,

[...]


Best regards,
Bastien