[PATCH net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames

[PATCH net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames

[Horatiu Vultur]

We have noticed that when PHY timestamping is enabled, L2 frames seems
to be modified by changing two 2 bytes with a value of 0. The place were
these 2 bytes seems to be random(or I couldn't find a pattern).  In most
of the cases the userspace can ignore these frames but if for example
those 2 bytes are in the correction field there is nothing to do.  This
seems to happen when configuring the HW for IPv4 even that the flow is
not enabled.
These 2 bytes correspond to the UDPv4 checksum and once we don't enable
clearing the checksum when using L2 frames then the frame doesn't seem
to be changed anymore.

Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/mscc/mscc_ptp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
index 6f96f2679f0bf..6b800081eed52 100644
--- a/drivers/net/phy/mscc/mscc_ptp.c
+++ b/drivers/net/phy/mscc/mscc_ptp.c
@@ -946,7 +946,9 @@ static int vsc85xx_ip1_conf(struct phy_device *phydev, enum ts_blk blk,
 	/* UDP checksum offset in IPv4 packet
 	 * according to: https://tools.ietf.org/html/rfc768
 	 */
-	val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26) | IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
+	val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26);
+	if (enable)
+		val |= IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
 	vsc85xx_ts_write_csr(phydev, blk, MSCC_ANA_IP1_NXT_PROT_UDP_CHKSUM,
 			     val);
 
-- 
2.34.1

Re: [PATCH net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames

[Andrew Lunn]

On Fri, May 23, 2025 at 10:27:16AM +0200, Horatiu Vultur wrote:
> We have noticed that when PHY timestamping is enabled, L2 frames seems
> to be modified by changing two 2 bytes with a value of 0. The place were
> these 2 bytes seems to be random(or I couldn't find a pattern).  In most
> of the cases the userspace can ignore these frames but if for example
> those 2 bytes are in the correction field there is nothing to do.  This
> seems to happen when configuring the HW for IPv4 even that the flow is
> not enabled.
> These 2 bytes correspond to the UDPv4 checksum and once we don't enable
> clearing the checksum when using L2 frames then the frame doesn't seem
> to be changed anymore.
> 
> Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/net/phy/mscc/mscc_ptp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> index 6f96f2679f0bf..6b800081eed52 100644
> --- a/drivers/net/phy/mscc/mscc_ptp.c
> +++ b/drivers/net/phy/mscc/mscc_ptp.c
> @@ -946,7 +946,9 @@ static int vsc85xx_ip1_conf(struct phy_device *phydev, enum ts_blk blk,
>  	/* UDP checksum offset in IPv4 packet
>  	 * according to: https://tools.ietf.org/html/rfc768
>  	 */
> -	val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26) | IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
> +	val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26);
> +	if (enable)
> +		val |= IP1_NXT_PROT_UDP_CHKSUM_CLEAR;

Is this towards the media, or received from the media?  Have you tried
sending packets with deliberately broken UDPv4 checksum? Does the PHYs
PTP engine correctly ignore such packets?

I suppose the opposite could also be true. Do you see it ignoring
frames which are actually O.K? It could be looking in the wrong place
for the checksum, so the checksum fails.

	Andrew

Re: [PATCH net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames

[Horatiu Vultur]

The 05/23/2025 14:59, Andrew Lunn wrote:

Hi Andrew,

> 
> On Fri, May 23, 2025 at 10:27:16AM +0200, Horatiu Vultur wrote:
> > We have noticed that when PHY timestamping is enabled, L2 frames seems
> > to be modified by changing two 2 bytes with a value of 0. The place were
> > these 2 bytes seems to be random(or I couldn't find a pattern).  In most
> > of the cases the userspace can ignore these frames but if for example
> > those 2 bytes are in the correction field there is nothing to do.  This
> > seems to happen when configuring the HW for IPv4 even that the flow is
> > not enabled.
> > These 2 bytes correspond to the UDPv4 checksum and once we don't enable
> > clearing the checksum when using L2 frames then the frame doesn't seem
> > to be changed anymore.
> >
> > Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/phy/mscc/mscc_ptp.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> > index 6f96f2679f0bf..6b800081eed52 100644
> > --- a/drivers/net/phy/mscc/mscc_ptp.c
> > +++ b/drivers/net/phy/mscc/mscc_ptp.c
> > @@ -946,7 +946,9 @@ static int vsc85xx_ip1_conf(struct phy_device *phydev, enum ts_blk blk,
> >       /* UDP checksum offset in IPv4 packet
> >        * according to: https://tools.ietf.org/html/rfc768
> >        */
> > -     val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26) | IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
> > +     val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26);
> > +     if (enable)
> > +             val |= IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
> 
> Is this towards the media, or received from the media?

It is when the vsc85xx PHY receives frames from the link partner.

>Have you tried sending packets with deliberately broken UDPv4 checksum?
>Does the PHYs PTP engine correctly ignore such packets?

No, I have not done that. What I don't understand is why should I send
UDPv4 frames when we enable to timestamp only L2 frames.

> 
> I suppose the opposite could also be true. Do you see it ignoring
> frames which are actually O.K? It could be looking in the wrong place
> for the checksum, so the checksum fails.

I have not seen any frames being ignored by HW. The HW is configured to
set the nanosecond part of the RX timestamp into the frame. And it is
always doing that, the problem is that sometimes on top of this change
it also replaces 2 bytes in the frame with 0. And it is the userspace
(ptp4l) who ignores those frames because the are corrupted.

> 
>         Andrew

-- 
/Horatiu

Re: [PATCH net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames

[Andrew Lunn]

On Mon, May 26, 2025 at 08:54:45AM +0200, Horatiu Vultur wrote:
> The 05/23/2025 14:59, Andrew Lunn wrote:
> 
> Hi Andrew,
> 
> > 
> > On Fri, May 23, 2025 at 10:27:16AM +0200, Horatiu Vultur wrote:
> > > We have noticed that when PHY timestamping is enabled, L2 frames seems
> > > to be modified by changing two 2 bytes with a value of 0. The place were
> > > these 2 bytes seems to be random(or I couldn't find a pattern).  In most
> > > of the cases the userspace can ignore these frames but if for example
> > > those 2 bytes are in the correction field there is nothing to do.  This
> > > seems to happen when configuring the HW for IPv4 even that the flow is
> > > not enabled.
> > > These 2 bytes correspond to the UDPv4 checksum and once we don't enable
> > > clearing the checksum when using L2 frames then the frame doesn't seem
> > > to be changed anymore.
> > >
> > > Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
> > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > > ---
> > >  drivers/net/phy/mscc/mscc_ptp.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> > > index 6f96f2679f0bf..6b800081eed52 100644
> > > --- a/drivers/net/phy/mscc/mscc_ptp.c
> > > +++ b/drivers/net/phy/mscc/mscc_ptp.c
> > > @@ -946,7 +946,9 @@ static int vsc85xx_ip1_conf(struct phy_device *phydev, enum ts_blk blk,
> > >       /* UDP checksum offset in IPv4 packet
> > >        * according to: https://tools.ietf.org/html/rfc768
> > >        */
> > > -     val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26) | IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
> > > +     val |= IP1_NXT_PROT_UDP_CHKSUM_OFF(26);
> > > +     if (enable)
> > > +             val |= IP1_NXT_PROT_UDP_CHKSUM_CLEAR;
> > 
> > Is this towards the media, or received from the media?
> 
> It is when the vsc85xx PHY receives frames from the link partner.
> 
> >Have you tried sending packets with deliberately broken UDPv4 checksum?
> >Does the PHYs PTP engine correctly ignore such packets?
> 
> No, I have not done that. What I don't understand is why should I send
> UDPv4 frames when we enable to timestamp only L2 frames.

Ah, O.K. I was just wondering if the UDP checksumming in the hardware
is broken in general. If so, you might want to disable it, and do the
checks in software.

       Andrew

Re: [PATCH net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 23 May 2025 10:27:16 +0200 you wrote:
> We have noticed that when PHY timestamping is enabled, L2 frames seems
> to be modified by changing two 2 bytes with a value of 0. The place were
> these 2 bytes seems to be random(or I couldn't find a pattern).  In most
> of the cases the userspace can ignore these frames but if for example
> those 2 bytes are in the correction field there is nothing to do.  This
> seems to happen when configuring the HW for IPv4 even that the flow is
> not enabled.
> These 2 bytes correspond to the UDPv4 checksum and once we don't enable
> clearing the checksum when using L2 frames then the frame doesn't seem
> to be changed anymore.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: mscc: Stop clearing the the UDPv4 checksum for L2 frames
    https://git.kernel.org/netdev/net/c/57a92d14659d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html