[PATCH net-next 0/2] ptp: safely cleanup when unregistering a PTP clock

[PATCH net-next 0/2] ptp: safely cleanup when unregistering a PTP clock

[Russell King (Oracle)]

(I'm assuming drivers/ptp/ patches go via net-next as there is no
git tree against the "PTP HARDWARE CLOCK SUPPORT" maintainers entry.)

The standard rule in the kernel for unregistering user visible devices
is to unpublish the userspace API before doing any shutdown of the
resources necessary for the operation of the device.

PTP has several issues in this area:

1. ptp_clock_unregister() cancells and destroys work while the PTP
   chardev is still published, which gives the opportunity for a
   precisely timed user API call to cause a driver to attempt to
   queue the aux work.

2. PTP pins are not cleaned up - if userspace has enabled PTP pins,
   e.g. for extts, drivers are forced to do cleanup before calling
   ptp_clock_unregister() to stop events being forwarded into the
   PTP layer. E.g mv88e6xxx cancells its internal tai_event_work
   to avoid calling into the PTP clock code with a stale ptp_clock
   pointer, but a badly timed userspace EXTTS enable will re-schedule
   the tai_event_work.

Simplify the process by ensuring that:

1. we take a referene on the PTP struct device to stop the
   ptp_clock structure going away underneath us when we call
   posix_clock_unregister().

2. call posix_clock_unregister() to remove the /dev/ptp* device.

3. add additional functionality to disable any PTP pins that have
   been configured on this device. This should shutdown all events
   coming from PTP clock drivers.

4. cancel the delayed aux_work and destroy the kthread.

5. remove the PPS source.

6. drop the reference on the PTP struct device to allow the
   ptp_clock structure to be released.

This is difficult for me to test beyond build testing - on the
Clearfog platform with Marvell PHY PTP, the ethernet PHY is the
primary connectivity, so removing the PHY driver for an in-use
network interface isn't possible.

On the ZII rev B platform, where the DSA switches have the TAI
hardware and where root NFS is used, removal of the DSA switch
module somehow forces the FEC interface _not_ connected to the DSA
switch to lose link, causing the machine to become unresponsive
as its root filesystem vanishes.

 drivers/ptp/ptp_chardev.c | 21 ++++++++++++++++++++-
 drivers/ptp/ptp_clock.c   | 17 ++++++++++++++++-
 drivers/ptp/ptp_private.h |  2 ++
 3 files changed, 38 insertions(+), 2 deletions(-)

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

[PATCH net-next 1/2] ptp: describe the two disables in ptp_set_pinfunc()

[Russell King (Oracle)]

Accurately describe what each call to ptp_disable_pinfunc() is doing,
rather than the misleading comment above the first disable. This helps
to make the code more readable.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/ptp/ptp_chardev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index e9719f365aab..eb4f6d1b1460 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -91,12 +91,18 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		return -EOPNOTSUPP;
 	}
 
-	/* Disable whatever function was previously assigned. */
+	/* Disable whichever pin was previously assigned to this function and
+	 * channel.
+	 */
 	if (pin1) {
 		ptp_disable_pinfunc(info, func, chan);
 		pin1->func = PTP_PF_NONE;
 		pin1->chan = 0;
 	}
+
+	/* Disable whatever function was previously assigned to the requested
+	 * pin.
+	 */
 	ptp_disable_pinfunc(info, pin2->func, pin2->chan);
 	pin2->func = func;
 	pin2->chan = chan;
-- 
2.47.3

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

[Russell King (Oracle)]

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);
+}
+
 int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		    enum ptp_pin_function func, unsigned int chan)
 {
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 1d920f8e20a8..d2eb77081071 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -498,9 +498,23 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
 	}
 
+	/* Get the device to stop posix_clock_unregister() doing the last put
+	 * and freeing the structure(s)
+	 */
+	get_device(&ptp->dev);
+
+	/* Wake up any userspace waiting for an event. */
 	ptp->defunct = 1;
 	wake_up_interruptible(&ptp->tsev_wq);
 
+	/* Tear down the POSIX clock, which removes the user interface. */
+	posix_clock_unregister(&ptp->clock);
+
+	/* Disable any pin functions that the user may have setup, quiescing
+	 * all incoming events.
+	 */
+	ptp_disable_all_pins(ptp);
+
 	if (ptp->kworker) {
 		kthread_cancel_delayed_work_sync(&ptp->aux_work);
 		kthread_destroy_worker(ptp->kworker);
@@ -510,7 +524,8 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 	if (ptp->pps_source)
 		pps_unregister_source(ptp->pps_source);
 
-	posix_clock_unregister(&ptp->clock);
+	/* The final put, normally here, will invoke ptp_clock_release(). */
+	put_device(&ptp->dev);
 
 	return 0;
 }
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index b352df4cd3f9..9d90aace7e64 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -141,6 +141,8 @@ extern const struct class ptp_class;
  * see ptp_chardev.c
  */
 
+void ptp_disable_all_pins(struct ptp_clock *ptp);
+
 /* caller must hold pincfg_mux */
 int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		    enum ptp_pin_function func, unsigned int chan);
-- 
2.47.3

RE: [PATCH net-next 0/2] ptp: safely cleanup when unregistering a PTP clock

[Wei Fang]

> This is difficult for me to test beyond build testing - on the
> Clearfog platform with Marvell PHY PTP, the ethernet PHY is the
> primary connectivity, so removing the PHY driver for an in-use
> network interface isn't possible.
> 
> On the ZII rev B platform, where the DSA switches have the TAI
> hardware and where root NFS is used, removal of the DSA switch
> module somehow forces the FEC interface _not_ connected to the DSA
> switch to lose link, causing the machine to become unresponsive
> as its root filesystem vanishes.
> 

After applying the patch, I tested the unbinding and rebinding of
the ptp_netc driver on the i.MX95 platform, I did not see any
issues, and the NETC Timer works properly after rebinding.
I cannot test the pinfunc, because ptp_netc driver does not
support it. Do you need my Tested-by tag for this patch set?

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

[Wei Fang]

> the ordering of ptp_clock_unregister() is not ideal, as the chardev
 ^
Nit: Uppercase, 't' -> 'T'

> +void ptp_disable_all_pins(struct ptp_clock *ptp)
> +{
> +	struct ptp_clock_info *info = ptp->info;
> +	unsigned int i;
> +
> +	mutex_lock(&ptp->pincfg_mux);

Currently ptp_chardev.c has been converted to use the auto-cleanup
API (scoped_cond_guard()), so scoped_guard() can be used here.

> +	for (i = 0; i < info->n_pins; i++)
> +		if (info->pin_config[i].func != PTP_PF_NONE)

It seems unnecessary to check this, since ptp_disable_pinfunc() does
nothing for PTP_PF_NONE, but of course the check can avoid meaningless
function calls. It is up to you to keep it or not.

> +			ptp_disable_pinfunc(info, info->pin_config[i].func,
> +					    info->pin_config[i].chan);
> +	mutex_unlock(&ptp->pincfg_mux);

Re: [PATCH net-next 1/2] ptp: describe the two disables in ptp_set_pinfunc()

[Vladimir Oltean]

On Mon, Sep 15, 2025 at 03:42:00PM +0100, Russell King (Oracle) wrote:
> Accurately describe what each call to ptp_disable_pinfunc() is doing,
> rather than the misleading comment above the first disable. This helps
> to make the code more readable.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next 1/2] ptp: describe the two disables in ptp_set_pinfunc()

[Vadim Fedorenko]

On 15/09/2025 15:42, Russell King (Oracle) wrote:
> Accurately describe what each call to ptp_disable_pinfunc() is doing,
> rather than the misleading comment above the first disable. This helps
> to make the code more readable.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

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

[Vadim Fedorenko]

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.

Why do you need to clear up configured PEROUT function?

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

[Russell King (Oracle)]

On Tue, Sep 16, 2025 at 09:03:17AM +0000, Wei Fang wrote:
> > the ordering of ptp_clock_unregister() is not ideal, as the chardev
>  ^
> Nit: Uppercase, 't' -> 'T'
> 
> > +void ptp_disable_all_pins(struct ptp_clock *ptp)
> > +{
> > +	struct ptp_clock_info *info = ptp->info;
> > +	unsigned int i;
> > +
> > +	mutex_lock(&ptp->pincfg_mux);
> 
> Currently ptp_chardev.c has been converted to use the auto-cleanup
> API (scoped_cond_guard()), so scoped_guard() can be used here.

... which are very non-C like, non-obvious, and I currently have no
idea at the moment how to use it. In my opinion, it makes code more
difficult to understand.

Maybe someone else can convert this for me to this non-C like
structure?

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

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

[Andrew Lunn]

On Tue, Sep 16, 2025 at 02:38:23PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 16, 2025 at 09:03:17AM +0000, Wei Fang wrote:
> > > the ordering of ptp_clock_unregister() is not ideal, as the chardev
> >  ^
> > Nit: Uppercase, 't' -> 'T'
> > 
> > > +void ptp_disable_all_pins(struct ptp_clock *ptp)
> > > +{
> > > +	struct ptp_clock_info *info = ptp->info;
> > > +	unsigned int i;
> > > +
> > > +	mutex_lock(&ptp->pincfg_mux);
> > 
> > Currently ptp_chardev.c has been converted to use the auto-cleanup
> > API (scoped_cond_guard()), so scoped_guard() can be used here.
> 
> ... which are very non-C like, non-obvious, and I currently have no
> idea at the moment how to use it. In my opinion, it makes code more
> difficult to understand.

+1

Plain scoped_guard() is not too magical and reasonably
understandable. But ptp_chardev.c is using scoped_conf_guard() which
is much more magical and i have no idea what it does.

> Maybe someone else can convert this for me to this non-C like
> structure?

Yes, if you want to write plain simple code, I submit it so. Somebody
else can follow up with an obfuscation patch.

     Andrew

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

[Russell King (Oracle)]

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!

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

[Vadim Fedorenko]

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

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

[Russell King (Oracle)]

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!

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

[Russell King (Oracle)]

On Tue, Sep 16, 2025 at 08:46:28PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 16, 2025 at 05:20:19PM +0100, Vadim Fedorenko wrote:
> > On 16/09/2025 16:45, Russell King (Oracle) wrote:
> > > 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.

I think I've missed another source of trouble here: PPS input, enabled
via PTP_CLK_REQ_PPS, which seems to be a purely software construct
where the hardware triggers e.g. an interrupt to forward a PPS event.

As this is not a pin, this doesn't get disabled. This is subject to
all the same race conditions that EXTTS is subject to, so I think
ptp_unregister_clock() needs to deal with this too.

New patches on their way shortly...

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