[PATCH net v2] phy: mscc: Fix timestamping for vsc8584

[PATCH net v2] phy: mscc: Fix timestamping for vsc8584

[Horatiu Vultur]

There was a problem when we received frames and the frames were
timestamped. The driver is configured to store the nanosecond part of
the timestmap in the ptp reserved bits and it would take the second part
by reading the LTC. The problem is that when reading the LTC we are in
atomic context and to read the second part will go over mdio bus which
might sleep, so we get an error.
The fix consists in actually put all the frames in a queue and start the
aux work and in that work to read the LTC and then calculate the full
received time.

Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

---
v1->v2:
- use sk_buff_head instead of a list_head and spinlock_t
- stop allocating vsc8431_skb but put the timestamp in skb->cb
---
 drivers/net/phy/mscc/mscc.h     | 12 ++++++++
 drivers/net/phy/mscc/mscc_ptp.c | 50 +++++++++++++++++++++++++--------
 2 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 6a3d8a754eb8d..58c6d47fbe046 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -362,6 +362,13 @@ struct vsc85xx_hw_stat {
 	u16 mask;
 };
 
+struct vsc8531_skb_cb {
+	u32 ns;
+};
+
+#define VSC8531_SKB_CB(skb) \
+	((struct vsc8531_skb_cb *)((skb)->cb))
+
 struct vsc8531_private {
 	int rate_magic;
 	u16 supp_led_modes;
@@ -410,6 +417,11 @@ struct vsc8531_private {
 	 */
 	struct mutex ts_lock;
 	struct mutex phc_lock;
+
+	/* list of skbs that were received and need timestamp information but it
+	 * didn't received it yet
+	 */
+	struct sk_buff_head rx_skbs_list;
 };
 
 /* Shared structure between the PHYs of the same package.
diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
index 275706de5847c..d368d4fd82e17 100644
--- a/drivers/net/phy/mscc/mscc_ptp.c
+++ b/drivers/net/phy/mscc/mscc_ptp.c
@@ -1194,9 +1194,8 @@ static bool vsc85xx_rxtstamp(struct mii_timestamper *mii_ts,
 {
 	struct vsc8531_private *vsc8531 =
 		container_of(mii_ts, struct vsc8531_private, mii_ts);
-	struct skb_shared_hwtstamps *shhwtstamps = NULL;
+
 	struct vsc85xx_ptphdr *ptphdr;
-	struct timespec64 ts;
 	unsigned long ns;
 
 	if (!vsc8531->ptp->configured)
@@ -1206,27 +1205,52 @@ static bool vsc85xx_rxtstamp(struct mii_timestamper *mii_ts,
 	    type == PTP_CLASS_NONE)
 		return false;
 
-	vsc85xx_gettime(&vsc8531->ptp->caps, &ts);
-
 	ptphdr = get_ptp_header_rx(skb, vsc8531->ptp->rx_filter);
 	if (!ptphdr)
 		return false;
 
-	shhwtstamps = skb_hwtstamps(skb);
-	memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
-
 	ns = ntohl(ptphdr->rsrvd2);
 
-	/* nsec is in reserved field */
-	if (ts.tv_nsec < ns)
-		ts.tv_sec--;
+	VSC8531_SKB_CB(skb)->ns = ns;
+	skb_queue_tail(&vsc8531->rx_skbs_list, skb);
 
-	shhwtstamps->hwtstamp = ktime_set(ts.tv_sec, ns);
-	netif_rx(skb);
+	ptp_schedule_worker(vsc8531->ptp->ptp_clock, 0);
 
 	return true;
 }
 
+static long vsc85xx_do_aux_work(struct ptp_clock_info *info)
+{
+	struct vsc85xx_ptp *ptp = container_of(info, struct vsc85xx_ptp, caps);
+	struct skb_shared_hwtstamps *shhwtstamps = NULL;
+	struct phy_device *phydev = ptp->phydev;
+	struct vsc8531_private *priv = phydev->priv;
+	struct sk_buff_head received;
+	struct sk_buff *rx_skb;
+	struct timespec64 ts;
+	unsigned long flags;
+
+	__skb_queue_head_init(&received);
+	spin_lock_irqsave(&priv->rx_skbs_list.lock, flags);
+	skb_queue_splice_tail_init(&priv->rx_skbs_list, &received);
+	spin_unlock_irqrestore(&priv->rx_skbs_list.lock, flags);
+
+	vsc85xx_gettime(info, &ts);
+	while ((rx_skb = __skb_dequeue(&received)) != NULL) {
+		shhwtstamps = skb_hwtstamps(rx_skb);
+		memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
+
+		if (ts.tv_nsec < VSC8531_SKB_CB(rx_skb)->ns)
+			ts.tv_sec--;
+
+		shhwtstamps->hwtstamp = ktime_set(ts.tv_sec,
+						  VSC8531_SKB_CB(rx_skb)->ns);
+		netif_rx(rx_skb);
+	}
+
+	return -1;
+}
+
 static const struct ptp_clock_info vsc85xx_clk_caps = {
 	.owner		= THIS_MODULE,
 	.name		= "VSC85xx timer",
@@ -1240,6 +1264,7 @@ static const struct ptp_clock_info vsc85xx_clk_caps = {
 	.adjfine	= &vsc85xx_adjfine,
 	.gettime64	= &vsc85xx_gettime,
 	.settime64	= &vsc85xx_settime,
+	.do_aux_work	= &vsc85xx_do_aux_work,
 };
 
 static struct vsc8531_private *vsc8584_base_priv(struct phy_device *phydev)
@@ -1567,6 +1592,7 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
 
 	mutex_init(&vsc8531->phc_lock);
 	mutex_init(&vsc8531->ts_lock);
+	skb_queue_head_init(&vsc8531->rx_skbs_list);
 
 	/* Retrieve the shared load/save GPIO. Request it as non exclusive as
 	 * the same GPIO can be requested by all the PHYs of the same package.
-- 
2.34.1

Re: [PATCH net v2] phy: mscc: Fix timestamping for vsc8584

[Paolo Abeni]

On 8/6/25 7:46 AM, Horatiu Vultur wrote:
> @@ -1567,6 +1592,7 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
>  
>  	mutex_init(&vsc8531->phc_lock);
>  	mutex_init(&vsc8531->ts_lock);
> +	skb_queue_head_init(&vsc8531->rx_skbs_list);

The aux work is cancelled by ptp_clock_unregister(), meaning the
`rx_skbs_list` could be left untouched if the unreg happens while the
work is scheduled but not running yet, casing memory leaks.

It's not obvious to me where/how ptp_clock_unregister() is called, as
AFAICS the vsc85xxx phy driver lacks the 'remove' op. In any case I
think you need to explicitly flushing the rx_skbs_list list on removal.

Thanks,

Paolo

Re: [PATCH net v2] phy: mscc: Fix timestamping for vsc8584

[Horatiu Vultur]

The 08/12/2025 11:55, Paolo Abeni wrote:

Hi Paolo,

> 
> On 8/6/25 7:46 AM, Horatiu Vultur wrote:
> > @@ -1567,6 +1592,7 @@ int vsc8584_ptp_probe(struct phy_device *phydev)
> >
> >       mutex_init(&vsc8531->phc_lock);
> >       mutex_init(&vsc8531->ts_lock);
> > +     skb_queue_head_init(&vsc8531->rx_skbs_list);
> 
> The aux work is cancelled by ptp_clock_unregister(), meaning the
> `rx_skbs_list` could be left untouched if the unreg happens while the
> work is scheduled but not running yet, casing memory leaks.
> 
> It's not obvious to me where/how ptp_clock_unregister() is called, as
> AFAICS the vsc85xxx phy driver lacks the 'remove' op. In any case I
> think you need to explicitly flushing the rx_skbs_list list on removal.

Yes, I will flush the rx_skbs_list on removal.
I will look also to see where/how ptp_clock_unregister() is called, but
this will be in a different patch if it is an issue.

> 
> Thanks,
> 
> Paolo
> 

-- 
/Horatiu

Re: [PATCH net v2] phy: mscc: Fix timestamping for vsc8584

[Vadim Fedorenko]

On 06/08/2025 06:46, Horatiu Vultur wrote:
> There was a problem when we received frames and the frames were
> timestamped. The driver is configured to store the nanosecond part of
> the timestmap in the ptp reserved bits and it would take the second part
> by reading the LTC. The problem is that when reading the LTC we are in
> atomic context and to read the second part will go over mdio bus which
> might sleep, so we get an error.
> The fix consists in actually put all the frames in a queue and start the
> aux work and in that work to read the LTC and then calculate the full
> received time.

The expectation here is that aux worker will kick in immediately and the
processing will happen within 1 second of the first stamped skb in the
list. Why cannot you keep cached value of PHC, which is updated roughly 
every 500ms and use it to extend timestamp? Your aux worker will be much 
simpler, and packet processing will be faster...

> 
> Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> 
> ---
> v1->v2:
> - use sk_buff_head instead of a list_head and spinlock_t
> - stop allocating vsc8431_skb but put the timestamp in skb->cb
> ---
>   drivers/net/phy/mscc/mscc.h     | 12 ++++++++
>   drivers/net/phy/mscc/mscc_ptp.c | 50 +++++++++++++++++++++++++--------
>   2 files changed, 50 insertions(+), 12 deletions(-)
> 


[...]

>   /* Shared structure between the PHYs of the same package.
> diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> index 275706de5847c..d368d4fd82e17 100644
> --- a/drivers/net/phy/mscc/mscc_ptp.c
> +++ b/drivers/net/phy/mscc/mscc_ptp.c
> @@ -1194,9 +1194,8 @@ static bool vsc85xx_rxtstamp(struct mii_timestamper *mii_ts,
>   {
>   	struct vsc8531_private *vsc8531 =
>   		container_of(mii_ts, struct vsc8531_private, mii_ts);
> -	struct skb_shared_hwtstamps *shhwtstamps = NULL;
> +
>   	struct vsc85xx_ptphdr *ptphdr;

No empty line needed.

> -	struct timespec64 ts;
>   	unsigned long ns;
>   

Re: [PATCH net v2] phy: mscc: Fix timestamping for vsc8584

[Horatiu Vultur]

The 08/13/2025 22:51, Vadim Fedorenko wrote:

Hi Vadim,

> 
> On 06/08/2025 06:46, Horatiu Vultur wrote:
> > There was a problem when we received frames and the frames were
> > timestamped. The driver is configured to store the nanosecond part of
> > the timestmap in the ptp reserved bits and it would take the second part
> > by reading the LTC. The problem is that when reading the LTC we are in
> > atomic context and to read the second part will go over mdio bus which
> > might sleep, so we get an error.
> > The fix consists in actually put all the frames in a queue and start the
> > aux work and in that work to read the LTC and then calculate the full
> > received time.
> 
> The expectation here is that aux worker will kick in immediately and the
> processing will happen within 1 second of the first stamped skb in the
> list. Why cannot you keep cached value of PHC, which is updated roughly
> every 500ms and use it to extend timestamp? Your aux worker will be much
> simpler, and packet processing will be faster...

Thanks for the suggestion but I don't think it would work in this case.
(if I understood correctly).
The problem is that I don't know if the cache value happened after or
before the timestamp. Let me give you an example: If the ns part in the
received frame is 900ms and the cached value is 2 sec and 400ms. Now I
don't know if the final timestamp should be 1 sec and 400ms or should be
2 sec and 900ms.
I am doing something similar for lan8841 in micrel.c but in that case in
the PTP header I get also the 2 LS bits of the second and then it is
easier to see if the timestamp happen before or after the cached was
updated.

> 
> > 
> > Fixes: 7d272e63e0979d ("net: phy: mscc: timestamping and PHC support")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > 
> > ---
> > v1->v2:
> > - use sk_buff_head instead of a list_head and spinlock_t
> > - stop allocating vsc8431_skb but put the timestamp in skb->cb
> > ---
> >   drivers/net/phy/mscc/mscc.h     | 12 ++++++++
> >   drivers/net/phy/mscc/mscc_ptp.c | 50 +++++++++++++++++++++++++--------
> >   2 files changed, 50 insertions(+), 12 deletions(-)
> > 
> 
> 
> [...]
> 
> >   /* Shared structure between the PHYs of the same package.
> > diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> > index 275706de5847c..d368d4fd82e17 100644
> > --- a/drivers/net/phy/mscc/mscc_ptp.c
> > +++ b/drivers/net/phy/mscc/mscc_ptp.c
> > @@ -1194,9 +1194,8 @@ static bool vsc85xx_rxtstamp(struct mii_timestamper *mii_ts,
> >   {
> >       struct vsc8531_private *vsc8531 =
> >               container_of(mii_ts, struct vsc8531_private, mii_ts);
> > -     struct skb_shared_hwtstamps *shhwtstamps = NULL;
> > +
> >       struct vsc85xx_ptphdr *ptphdr;
> 
> No empty line needed.

Good catch, I will update this in the next version.

> 
> > -     struct timespec64 ts;
> >       unsigned long ns;
> > 

-- 
/Horatiu