[PATCH net-next v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[PATCH net-next v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[Russell King]

The hardware timestamps for packets contain a truncated seconds field,
only containing two bits of seconds. In order to provide the full
number of seconds, we need to keep track of the full hardware clock by
reading it every two seconds.

However, if we fail to read the clock, we silently ignore the error.
Print a warning indicating that the PP2 TAI clock timestamps have
become unreliable.

Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
--
v2: correct dev_warn_once() indentation
v3: add '\n'
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
index 95862aff49f1..d4e8708a9c76 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_tai.c
@@ -54,6 +54,7 @@
 #define TCSR_CAPTURE_0_VALID		BIT(0)
 
 struct mvpp2_tai {
+	struct device *dev;
 	struct ptp_clock_info caps;
 	struct ptp_clock *ptp_clock;
 	void __iomem *base;
@@ -303,7 +304,8 @@ static long mvpp22_tai_aux_work(struct ptp_clock_info *ptp)
 {
 	struct mvpp2_tai *tai = ptp_to_tai(ptp);
 
-	mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL);
+	if (mvpp22_tai_gettimex64(ptp, &tai->stamp, NULL) < 0)
+		dev_warn_once(tai->dev, "PTP timestamps are unreliable\n");
 
 	return msecs_to_jiffies(2000);
 }
@@ -401,6 +403,7 @@ int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv)
 
 	spin_lock_init(&tai->lock);
 
+	tai->dev = dev;
 	tai->base = priv->iface_base;
 
 	/* The step size consists of three registers - a 16-bit nanosecond step
-- 
2.30.2

Re: [PATCH net-next v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[Richard Cochran]

On Fri, Dec 13, 2024 at 04:34:06PM +0000, Russell King wrote:
> The hardware timestamps for packets contain a truncated seconds field,
> only containing two bits of seconds. In order to provide the full
> number of seconds, we need to keep track of the full hardware clock by
> reading it every two seconds.
> 
> However, if we fail to read the clock, we silently ignore the error.
> Print a warning indicating that the PP2 TAI clock timestamps have
> become unreliable.

Rather than printing a warning that user space might not read, why not
set a flag and stop delivering time stamps until the upper bits are
available once again?

Thanks,
Richard

Re: [PATCH net-next v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[Russell King (Oracle)]

On Fri, Dec 13, 2024 at 09:14:02PM -0800, Richard Cochran wrote:
> On Fri, Dec 13, 2024 at 04:34:06PM +0000, Russell King wrote:
> > The hardware timestamps for packets contain a truncated seconds field,
> > only containing two bits of seconds. In order to provide the full
> > number of seconds, we need to keep track of the full hardware clock by
> > reading it every two seconds.
> > 
> > However, if we fail to read the clock, we silently ignore the error.
> > Print a warning indicating that the PP2 TAI clock timestamps have
> > become unreliable.
> 
> Rather than printing a warning that user space might not read, why not
> set a flag and stop delivering time stamps until the upper bits are
> available once again?

If we fail to read the clock, that will be because the hardware didn't
respond to our request to read it, which means the hardware broke in
some way. We could make mvpp22_tai_tstamp() fail and not provide
timestamps until we have successfully read the HW clock, but we would
still want to print a warning to explain why HW timestamps vanish.

However, if this happens, then it also means that the gettimex64 PTP
clock ioctl will also fail, so I would suggest that the user would
find out about it anyway.

So, I don't think the extra complexity is worth doing.

This is to catch a spurious failure that may only affects an occasoinal
attempt to read the HW PTP time. Currently, we would never know,
because the kernel is currently completely silent if that were to ever
happen.

-- 
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 v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[Richard Cochran]

On Thu, Jan 02, 2025 at 04:26:04PM +0000, Russell King (Oracle) wrote:

> If we fail to read the clock, that will be because the hardware didn't
> respond to our request to read it, which means the hardware broke in
> some way. We could make mvpp22_tai_tstamp() fail and not provide
> timestamps until we have successfully read the HW clock, but we would
> still want to print a warning to explain why HW timestamps vanish.

Sure, keep the warning, but also block time stamp delivery.

> This is to catch a spurious failure that may only affects an occasoinal
> attempt to read the HW PTP time. Currently, we would never know,
> because the kernel is currently completely silent if that were to ever
> happen.

Is the failure spurious, or is the hardware broken and won't recover?

Thanks,
Richard

Re: [PATCH net-next v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[Russell King (Oracle)]

On Fri, Jan 03, 2025 at 12:43:56AM -0800, Richard Cochran wrote:
> On Thu, Jan 02, 2025 at 04:26:04PM +0000, Russell King (Oracle) wrote:
> 
> > If we fail to read the clock, that will be because the hardware didn't
> > respond to our request to read it, which means the hardware broke in
> > some way. We could make mvpp22_tai_tstamp() fail and not provide
> > timestamps until we have successfully read the HW clock, but we would
> > still want to print a warning to explain why HW timestamps vanish.
> 
> Sure, keep the warning, but also block time stamp delivery.
> 
> > This is to catch a spurious failure that may only affects an occasoinal
> > attempt to read the HW PTP time. Currently, we would never know,
> > because the kernel is currently completely silent if that were to ever
> > happen.
> 
> Is the failure spurious, or is the hardware broken and won't recover?

I have absolutely no idea. I've never seen it happen.

That's why I think going further than I have (and as you are suggesting)
is totally overkill.

-- 
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 v3] net: mvpp2: tai: warn once if we fail to update our timestamp

[Russell King (Oracle)]

On Fri, Jan 03, 2025 at 09:08:22AM +0000, Russell King (Oracle) wrote:
> On Fri, Jan 03, 2025 at 12:43:56AM -0800, Richard Cochran wrote:
> > On Thu, Jan 02, 2025 at 04:26:04PM +0000, Russell King (Oracle) wrote:
> > 
> > > If we fail to read the clock, that will be because the hardware didn't
> > > respond to our request to read it, which means the hardware broke in
> > > some way. We could make mvpp22_tai_tstamp() fail and not provide
> > > timestamps until we have successfully read the HW clock, but we would
> > > still want to print a warning to explain why HW timestamps vanish.
> > 
> > Sure, keep the warning, but also block time stamp delivery.
> > 
> > > This is to catch a spurious failure that may only affects an occasoinal
> > > attempt to read the HW PTP time. Currently, we would never know,
> > > because the kernel is currently completely silent if that were to ever
> > > happen.
> > 
> > Is the failure spurious, or is the hardware broken and won't recover?
> 
> I have absolutely no idea. I've never seen it happen.
> 
> That's why I think going further than I have (and as you are suggesting)
> is totally overkill.

However, I should point out that I don't use PTP as a general rule for
multiple reasons:

1. PTP is not as easy to deploy as NTP
2. Not everything has PTP support, whereas anything can support NTP.
3. PHY-based PTP support (where we need to read system time, read the
   PHY's PTP clock, re-read the system time and interpolate) seems to
   lead to a lot of noise making the stability of PHY based PTP
   inferior to NTP.

Given this, I don't run PTP except for testing.

Therefore, I think I'll drop this patch - it's clearly an approach that
isn't wanted, and we'll just have silent *and* *wrong* hardware
timestamps if this occurs. It seems it's better that users live in
ignorance that their system has a problem.

Thanks.

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