ptp: safely cleanup when unregistering a PTP clock
cover ptp: safely cleanup when unregistering a PTP clock
(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(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
PATCH 1/2 ptp: describe the two disables in ptp_set_pinfunc()
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.
--- drivers/ptp/ptp_chardev.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/ptp/ptp_chardev.c +7 -1
@@ -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;
Signature · 1 line
2.47.3
PATCH 2/2 ptp: rework ptp_clock_unregister() to disable events
In reply to 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.
--- 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(-)
drivers/ptp/ptp_chardev.c +13 -0
@@ -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) {
drivers/ptp/ptp_clock.c +16 -1
@@ -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 ; }
drivers/ptp/ptp_private.h +2 -0
@@ -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);
Signature · 1 line
2.47.3
Quoted 11 lines
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
In reply to Russell King (Oracle)Quoted 1 line
the ordering of
ptp_clock_unregister()is not ideal, as the chardev
^
Nit: Uppercase, 't' -> 'T'
drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 55
Quoted 6 lines
drivers/ptp/ptp_chardev.c +6
-0+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.
drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 57
Quoted 2 lines
drivers/ptp/ptp_chardev.c +2
-0+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.
Quoted 3 lines
drivers/ptp/ptp_chardev.c +3
-0+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()
In reply to Russell King (Oracle)Quoted 6 lines · 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) <[email protected]>---
Re: [PATCH net-next 1/2] ptp: describe the two disables in ptp_set_pinfunc()
In reply to Russell King (Oracle)Quoted 5 lines · 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) <[email protected]>
Re: [PATCH net-next 2/2] ptp: rework ptp_clock_unregister() to disable events
In reply to Russell King (Oracle)Quoted 30 lines · 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) <[email protected]>--- 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(-)drivers/ptp/ptp_chardev.c +3
-0@@ -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;
drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 62
Quoted 10 lines
drivers/ptp/ptp_chardev.c +10
-0+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
In reply to Wei FangQuoted 4 lines · Wei Fang
Quoted 1 line
the ordering of
ptp_clock_unregister()is not ideal, as the chardev^
Nit: Uppercase, 't' -> 'T'
Quoted 9 lines
drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 55
Quoted 6 lines
drivers/ptp/ptp_chardev.c +6
-0+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.chas been converted to use the auto-cleanup API (scoped_cond_guard()), soscoped_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?
Signature · 2 lines
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
Quoted 15 lines · Russell King (Oracle)
Quoted 4 lines · Wei Fang
Quoted 1 line
the ordering of
ptp_clock_unregister()is not ideal, as the chardev^
Nit: Uppercase, 't' -> 'T'Quoted 9 lines
drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 55
Quoted 6 lines
drivers/ptp/ptp_chardev.c +6
-0+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.chas been converted to use the auto-cleanup API (scoped_cond_guard()), soscoped_guard()can be used here.
Quoted 3 lines
... 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.
Quoted 2 lines
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
In reply to Vadim FedorenkoQuoted 45 lines · Vadim Fedorenko
Quoted 29 lines · 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) <[email protected]>--- 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(-)drivers/ptp/ptp_chardev.c +3
-0@@ -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;drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 61
Quoted 10 lines
drivers/ptp/ptp_chardev.c +10
-0+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.
Quoted 3 lines
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.
Signature · 2 lines
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
Quoted 50 lines · Russell King (Oracle)
Quoted 45 lines · Vadim Fedorenko
Quoted 29 lines · 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) <[email protected]>--- 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(-)drivers/ptp/ptp_chardev.c +3
-0@@ -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;drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 61
Quoted 10 lines
drivers/ptp/ptp_chardev.c +10
-0+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.Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
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.
Quoted 4 lines
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
Quoted 81 lines · Vadim Fedorenko
Quoted 50 lines · Russell King (Oracle)
Quoted 45 lines · Vadim Fedorenko
Quoted 29 lines · 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) <[email protected]>--- 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(-)drivers/ptp/ptp_chardev.c +3
-0@@ -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;drivers/ptp/ptp_chardev.c static int ptp_disable_pinfunc(struct ptp_clock_info *ops, · line 61
Quoted 10 lines
drivers/ptp/ptp_chardev.c +10
-0+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.Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Signature · 2 lines
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
Quoted 20 lines · Russell King (Oracle)
Quoted 19 lines · Vadim Fedorenko
Quoted 9 lines · Russell King (Oracle)
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.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 2 lines
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...
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!