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 05:20:19PM +0100, Vadim Fedorenko wrote:
> On 16/09/2025 16:45, Russell King (Oracle) wrote:
> > 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.
> 
> That's part of mlx5 and at least Intel's igc and igb drivers.
> 
> > 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.
> 
> In our use-case we use PPS out as an input to the external monitoring
> and we would like to see the PPS signal to drift in case of any errors.
> 
> > 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.
> 
> The deduction based on the amount of references to ptp device is not
> quite correct. Another option is to introduce another flag and use it
> as a signal to remove the function in case of error/shutdown/etc.
> > 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.
> 
> And again, there is another use-case which actually needs thisunsynchronised
> signal

For my use case (Marvell platforms) we only support EXTTS there, so I'm
happy to restrict this behaviour to EXTTS. If drivers need e.g. timers
or workqueues to support the other pin functions, then this will need
to be revisited so they can safely tear down their software resources.

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