Re: [PATCH net-next 2/2] ptp: rework ptp_clock_unregister() to disable events

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Sep 16, 2025 at 02:02:56PM +0100, Vadim Fedorenko wrote:
> On 15/09/2025 15:42, Russell King (Oracle) wrote:
> > the ordering of ptp_clock_unregister() is not ideal, as the chardev
> > remains published while state is being torn down. There is also no
> > cleanup of enabled pin settings, which means enabled events can
> > still forward into the core.
> > 
> > Rework the ordering of cleanup in ptp_clock_unregister() so that we
> > unpublish the posix clock (and user chardev), disable any pins that
> > have events enabled, and then clean up the aux work and PPS source.
> > 
> > This avoids potential use-after-free and races in PTP clock driver
> > teardown.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >   drivers/ptp/ptp_chardev.c | 13 +++++++++++++
> >   drivers/ptp/ptp_clock.c   | 17 ++++++++++++++++-
> >   drivers/ptp/ptp_private.h |  2 ++
> >   3 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
> > index eb4f6d1b1460..640a98f17739 100644
> > --- a/drivers/ptp/ptp_chardev.c
> > +++ b/drivers/ptp/ptp_chardev.c
> > @@ -47,6 +47,19 @@ static int ptp_disable_pinfunc(struct ptp_clock_info *ops,
> >   	return err;
> >   }
> > +void ptp_disable_all_pins(struct ptp_clock *ptp)
> > +{
> > +	struct ptp_clock_info *info = ptp->info;
> > +	unsigned int i;
> > +
> > +	mutex_lock(&ptp->pincfg_mux);
> > +	for (i = 0; i < info->n_pins; i++)
> > +		if (info->pin_config[i].func != PTP_PF_NONE)
> > +			ptp_disable_pinfunc(info, info->pin_config[i].func,
> > +					    info->pin_config[i].chan);
> > +	mutex_unlock(&ptp->pincfg_mux);
> > +}
> > +
> 
> This part is questionable. We do have devices which have PPS out enabled
> by default. The driver reads pins configuration from the HW during PTP
> init phase and sets up proper function for pin in ptp_info::pin_config.
> 
> With this patch applied these pins have PEROUT function disabled on
> shutdown and in case of kexec'ing into a new kernel the PPS out feature
> needs to be manually enabled, and it breaks expected behavior.

Does kexec go to the trouble of unregistering PTP clocks? I don't see
any driver in drivers/ptp/ that has the .shutdown method populated.

That doesn't mean there aren't - it isn't obvious where they are or
if it does happen.

The question about whether one wants to leave the other features in
place when the driver is removed is questionable - without the driver
(or indeed without something discplining the clock) it's going to
drift, so the accuracy of e.g. the PPS signal is going to be as good
as the clock source clocking the TAI.

Having used NTP with a PPS sourced from a GPS, I'd personally want
the PPS to stop if the GPS is no longer synchronised, so NTP knows
that a fault has occurred, rather than PPS continuing but being
undiscplined and thus of unknown accuracy.

I'd suggest that whether PPS continues to be generated should be a
matter of user policy. I would suggest that policy should include
whether or not userspace is discplining the clock - in other words,
whether the /dev/ptp* device is open or not.

Consider the case where the userspace daemons get OOM-killed and
that isn't realised. The PPS signal continues to be generated but
is now unsynchronised and drifts. Yet PPS users continue to
believe it's accurate.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!