[v1] net: sparx5: Fix invalid timestamps

[PATCH] net: sparx5: Fix invalid timestamps

Posted by Aakash Menon 2 months, 2 weeks ago

Bit 270-271 are occasionally unexpectedly set by the hardware.

This issue was observed with 10G SFPs causing huge time errors (> 30ms) in PTP.

Only 30 bits are needed for the nanosecond part of the timestamp, clear 2 most significant bits before extracting timestamp from the internal frame header.

Signed-off-by: Aakash Menon <aakash.menon@protempis.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_packet.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
index f3f5fb420468..a05263488851 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
@@ -45,8 +45,12 @@ void sparx5_ifh_parse(u32 *ifh, struct frame_info *info)
 	fwd = (fwd >> 5);
 	info->src_port = FIELD_GET(GENMASK(7, 1), fwd);
 
+	/*
+	 * Bit 270-271 are occasionally unexpectedly set by the hardware,
+	 * clear bits before extracting timestamp
+	 */
 	info->timestamp =
-		((u64)xtr_hdr[2] << 24) |
+		((u64)(xtr_hdr[2] & 0x3F) << 24) |
 		((u64)xtr_hdr[3] << 16) |
 		((u64)xtr_hdr[4] <<  8) |
 		((u64)xtr_hdr[5] <<  0);
-- 
2.46.0

Re: [PATCH] net: sparx5: Fix invalid timestamps

Posted by Daniel Machon 2 months, 2 weeks ago

> Bit 270-271 are occasionally unexpectedly set by the hardware.
> 
> This issue was observed with 10G SFPs causing huge time errors (> 30ms) in PTP.
> 
> Only 30 bits are needed for the nanosecond part of the timestamp, clear 2 most significant bits before extracting timestamp from the internal frame header.
> 
> Signed-off-by: Aakash Menon <aakash.menon@protempis.com>
> ---
>  drivers/net/ethernet/microchip/sparx5/sparx5_packet.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> index f3f5fb420468..a05263488851 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> @@ -45,8 +45,12 @@ void sparx5_ifh_parse(u32 *ifh, struct frame_info *info)
>         fwd = (fwd >> 5);
>         info->src_port = FIELD_GET(GENMASK(7, 1), fwd);
> 
> +       /*
> +        * Bit 270-271 are occasionally unexpectedly set by the hardware,
> +        * clear bits before extracting timestamp
> +        */
>         info->timestamp =
> -               ((u64)xtr_hdr[2] << 24) |
> +               ((u64)(xtr_hdr[2] & 0x3F) << 24) |
>                 ((u64)xtr_hdr[3] << 16) |
>                 ((u64)xtr_hdr[4] <<  8) |
>                 ((u64)xtr_hdr[5] <<  0);
> --
> 2.46.0
>

Hi Aakash,

I will (or somebody else) try to reproduce and test this at the
beginning of the next week.

Meanwhile, you can address the issues that Simon mentioned.

Thanks.

/Daniel

[PATCH net] net: sparx5: Fix invalid timestamps

Posted by Aakash Menon 2 months, 2 weeks ago

Bit 270-271 are occasionally unexpectedly set by the hardware. This issue
was observed with 10G SFPs causing huge time errors (> 30ms) in PTP. Only
30 bits are needed for the nanosecond part of the timestamp, clear 2 most
significant bits before extracting timestamp from the internal frame
header.

Fixes: 70dfe25cd866 ("net: sparx5: Update extraction/injection for
timestamping")
Signed-off-by: Aakash Menon <aakash.menon@protempis.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_packet.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
index f3f5fb420468..a05263488851 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
@@ -45,8 +45,12 @@ void sparx5_ifh_parse(u32 *ifh, struct frame_info *info)
 	fwd = (fwd >> 5);
 	info->src_port = FIELD_GET(GENMASK(7, 1), fwd);
 
+	/*
+	 * Bit 270-271 are occasionally unexpectedly set by the hardware,
+	 * clear bits before extracting timestamp
+	 */
 	info->timestamp =
-		((u64)xtr_hdr[2] << 24) |
+		((u64)(xtr_hdr[2] & 0x3F) << 24) |
 		((u64)xtr_hdr[3] << 16) |
 		((u64)xtr_hdr[4] <<  8) |
 		((u64)xtr_hdr[5] <<  0);
-- 
2.46.0

Re: [PATCH net] net: sparx5: Fix invalid timestamps

Posted by Horatiu Vultur 2 months, 2 weeks ago

The 09/15/2024 22:18, Aakash Menon wrote:
> [Some people who received this message don't often get email from aakash.r.menon@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Bit 270-271 are occasionally unexpectedly set by the hardware. This issue
> was observed with 10G SFPs causing huge time errors (> 30ms) in PTP. Only
> 30 bits are needed for the nanosecond part of the timestamp, clear 2 most
> significant bits before extracting timestamp from the internal frame
> header.
> 
> Fixes: 70dfe25cd866 ("net: sparx5: Update extraction/injection for
> timestamping")
> Signed-off-by: Aakash Menon <aakash.menon@protempis.com>

There are some small issues/comments with this patch:
1. The Fixes tag should still be on one line even if it is longer than
75 columns, so the robots can find it.
2. Please use GENMASK(5, 0) instead of 0x3f.

> ---
>  drivers/net/ethernet/microchip/sparx5/sparx5_packet.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> index f3f5fb420468..a05263488851 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> @@ -45,8 +45,12 @@ void sparx5_ifh_parse(u32 *ifh, struct frame_info *info)
>         fwd = (fwd >> 5);
>         info->src_port = FIELD_GET(GENMASK(7, 1), fwd);
> 
> +       /*
> +        * Bit 270-271 are occasionally unexpectedly set by the hardware,
> +        * clear bits before extracting timestamp
> +        */
>         info->timestamp =
> -               ((u64)xtr_hdr[2] << 24) |
> +               ((u64)(xtr_hdr[2] & 0x3F) << 24) |
>                 ((u64)xtr_hdr[3] << 16) |
>                 ((u64)xtr_hdr[4] <<  8) |
>                 ((u64)xtr_hdr[5] <<  0);
> --
> 2.46.0
> 

-- 
/Horatiu

Re: [PATCH net] net: sparx5: Fix invalid timestamps

Posted by Aakash Menon 2 months, 1 week ago

> > Bit 270-271 are occasionally unexpectedly set by the hardware. This issue
> > was observed with 10G SFPs causing huge time errors (> 30ms) in PTP. Only
> > 30 bits are needed for the nanosecond part of the timestamp, clear 2 most
> > significant bits before extracting timestamp from the internal frame
> > header.
> >
> > Fixes: 70dfe25cd866 ("net: sparx5: Update extraction/injection for
> > timestamping")
> > Signed-off-by: Aakash Menon <aakash.menon@protempis.com>
>
> There are some small issues/comments with this patch:
> 1. The Fixes tag should still be on one line even if it is longer than
> 75 columns, so the robots can find it.
> 2. Please use GENMASK(5, 0) instead of 0x3f.

Hi Horatiu,
Thanks for your feedback. I will make the suggested changes and send a
v2 patch in a new email thread.

- Aakash

Re: [PATCH] net: sparx5: Fix invalid timestamps

Posted by Simon Horman 2 months, 2 weeks ago

On Fri, Sep 13, 2024 at 12:33:57PM -0700, Aakash Menon wrote:
> Bit 270-271 are occasionally unexpectedly set by the hardware.
> 
> This issue was observed with 10G SFPs causing huge time errors (> 30ms) in PTP.
> 
> Only 30 bits are needed for the nanosecond part of the timestamp, clear 2 most significant bits before extracting timestamp from the internal frame header.

Hi Aakash,

Thanks for your patch.

I'll leave the review of the code change itself to others,
but here is some feedback on process.

Please line-wrap patch descriptions at 75 columns wide.

Link: https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format

Assuming this is a bug fix, a Fixes tag should be present.
It should go just before the signed-off-by line (or other tags),
with no blank lines in between.

I'm wondering if this Fixes tag is appropriate:

Fixes: 70dfe25cd866 ("net: sparx5: Update extraction/injection for timestamping")
> Signed-off-by: Aakash Menon <aakash.menon@protempis.com>

Also, for reference, fixes for Networking should, in general,
be targeted at the net tree.

	Subject: [PATCH net] ...

And lastly, if you do post a new patch, be sure to wait 24h since
the original patch posting before doing so.

Link: https://docs.kernel.org/process/maintainer-netdev.html

...