diff mbox series

[net,1/2] net: micrel: Fix PTP frame parsing for lan8814

Message ID 20240113131521.1051921-2-horatiu.vultur@microchip.com
State New
Headers
Series net: micrel: Fixes for PHC for lan8814 |

Commit Message

Horatiu Vultur Jan. 13, 2024, 1:15 p.m. UTC 
  The HW has the capability to check each frame if it is a PTP frame,
which domain it is, which ptp frame type it is, different ip address in
the frame. And if one of these checks fail then the frame is not
timestamp. Most of these checks were disabled except checking the field
minorVersionPTP inside the PTP header. Meaning that once a partner sends
a frame compliant to 8021AS which has minorVersionPTP set to 1, then the
frame was not timestamp because the HW expected by default a value of 0
in minorVersionPTP. This is exactly the same issue as on lan8841.
Fix this issue by removing this check so the userspace can decide on this.

Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/micrel.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Maxime Chevallier Jan. 15, 2024, 10:06 a.m. UTC | #1 
Hello Horatiu,

On Sat, 13 Jan 2024 14:15:20 +0100
Horatiu Vultur <horatiu.vultur@microchip.com> wrote:

> The HW has the capability to check each frame if it is a PTP frame,
> which domain it is, which ptp frame type it is, different ip address in
> the frame. And if one of these checks fail then the frame is not
> timestamp. Most of these checks were disabled except checking the field
> minorVersionPTP inside the PTP header. Meaning that once a partner sends
> a frame compliant to 8021AS which has minorVersionPTP set to 1, then the
> frame was not timestamp because the HW expected by default a value of 0
> in minorVersionPTP. This is exactly the same issue as on lan8841.
> Fix this issue by removing this check so the userspace can decide on this.
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/net/phy/micrel.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index bf4053431dcb3..1752eaeadc42e 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -120,6 +120,9 @@
>   */
>  #define LAN8814_1PPM_FORMAT			17179
>  
> +#define PTP_RX_VERSION				0x0248
> +#define PTP_TX_VERSION				0x0288
> +
>  #define PTP_RX_MOD				0x024F
>  #define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
>  #define PTP_RX_TIMESTAMP_EN			0x024D
> @@ -3150,6 +3153,10 @@ static void lan8814_ptp_init(struct phy_device *phydev)
>  	lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_IP_ADDR_EN, 0);
>  	lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_IP_ADDR_EN, 0);
>  
> +	/* Disable checking for minorVersionPTP field */
> +	lanphy_write_page_reg(phydev, 5, PTP_RX_VERSION, 0xff00);
> +	lanphy_write_page_reg(phydev, 5, PTP_TX_VERSION, 0xff00);

Small nit: This looks a bit like magic values, from the datasheet I
understand the upper byte is the max supported version and the lower
byte is the min supported version, maybe this could be wrapped in
macros ?

> +
>  	skb_queue_head_init(&ptp_priv->tx_queue);
>  	skb_queue_head_init(&ptp_priv->rx_queue);
>  	INIT_LIST_HEAD(&ptp_priv->rx_ts_list);

Besides that,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime
Divya Koppera Jan. 17, 2024, 5:38 a.m. UTC | #2 
> The HW has the capability to check each frame if it is a PTP frame, which
> domain it is, which ptp frame type it is, different ip address in the frame. And if
> one of these checks fail then the frame is not timestamp. Most of these checks
> were disabled except checking the field minorVersionPTP inside the PTP
> header. Meaning that once a partner sends a frame compliant to 8021AS
> which has minorVersionPTP set to 1, then the frame was not timestamp
> because the HW expected by default a value of 0 in minorVersionPTP. This is
> exactly the same issue as on lan8841.
> Fix this issue by removing this check so the userspace can decide on this.
> 
> Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Reviewed-by: Divya Koppera <divya.koppera@microchip.com>
Horatiu Vultur Jan. 18, 2024, 7:43 a.m. UTC | #3 
The 01/15/2024 11:06, Maxime Chevallier wrote:
> 
> Hello Horatiu,

Hi Maxime,

> 
> On Sat, 13 Jan 2024 14:15:20 +0100
> Horatiu Vultur <horatiu.vultur@microchip.com> wrote:
> 
> > The HW has the capability to check each frame if it is a PTP frame,
> > which domain it is, which ptp frame type it is, different ip address in
> > the frame. And if one of these checks fail then the frame is not
> > timestamp. Most of these checks were disabled except checking the field
> > minorVersionPTP inside the PTP header. Meaning that once a partner sends
> > a frame compliant to 8021AS which has minorVersionPTP set to 1, then the
> > frame was not timestamp because the HW expected by default a value of 0
> > in minorVersionPTP. This is exactly the same issue as on lan8841.
> > Fix this issue by removing this check so the userspace can decide on this.
> >
> > Fixes: ece19502834d ("net: phy: micrel: 1588 support for LAN8814 phy")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/phy/micrel.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index bf4053431dcb3..1752eaeadc42e 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -120,6 +120,9 @@
> >   */
> >  #define LAN8814_1PPM_FORMAT                  17179
> >
> > +#define PTP_RX_VERSION                               0x0248
> > +#define PTP_TX_VERSION                               0x0288
> > +
> >  #define PTP_RX_MOD                           0x024F
> >  #define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
> >  #define PTP_RX_TIMESTAMP_EN                  0x024D
> > @@ -3150,6 +3153,10 @@ static void lan8814_ptp_init(struct phy_device *phydev)
> >       lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_IP_ADDR_EN, 0);
> >       lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_IP_ADDR_EN, 0);
> >
> > +     /* Disable checking for minorVersionPTP field */
> > +     lanphy_write_page_reg(phydev, 5, PTP_RX_VERSION, 0xff00);
> > +     lanphy_write_page_reg(phydev, 5, PTP_TX_VERSION, 0xff00);
> 
> Small nit: This looks a bit like magic values, from the datasheet I
> understand the upper byte is the max supported version and the lower
> byte is the min supported version, maybe this could be wrapped in
> macros ?

Your understanding is correct. Yes, I will update this in the next
series.

> 
> > +
> >       skb_queue_head_init(&ptp_priv->tx_queue);
> >       skb_queue_head_init(&ptp_priv->rx_queue);
> >       INIT_LIST_HEAD(&ptp_priv->rx_ts_list);
> 
> Besides that,
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> Thanks,
> 
> Maxime
diff mbox series

Patch

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index bf4053431dcb3..1752eaeadc42e 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -120,6 +120,9 @@ 
  */
 #define LAN8814_1PPM_FORMAT			17179
 
+#define PTP_RX_VERSION				0x0248
+#define PTP_TX_VERSION				0x0288
+
 #define PTP_RX_MOD				0x024F
 #define PTP_RX_MOD_BAD_UDPV4_CHKSUM_FORCE_FCS_DIS_ BIT(3)
 #define PTP_RX_TIMESTAMP_EN			0x024D
@@ -3150,6 +3153,10 @@  static void lan8814_ptp_init(struct phy_device *phydev)
 	lanphy_write_page_reg(phydev, 5, PTP_TX_PARSE_IP_ADDR_EN, 0);
 	lanphy_write_page_reg(phydev, 5, PTP_RX_PARSE_IP_ADDR_EN, 0);
 
+	/* Disable checking for minorVersionPTP field */
+	lanphy_write_page_reg(phydev, 5, PTP_RX_VERSION, 0xff00);
+	lanphy_write_page_reg(phydev, 5, PTP_TX_VERSION, 0xff00);
+
 	skb_queue_head_init(&ptp_priv->tx_queue);
 	skb_queue_head_init(&ptp_priv->rx_queue);
 	INIT_LIST_HEAD(&ptp_priv->rx_ts_list);