Fix memory leak when running one-step timestamping. When running
one-step sync timestamping, the HW is configured to insert the TX time
into the frame, so there is no reason to keep the skb anymore. As in
this case the HW will never generate an interrupt to say that the frame
was timestamped, then the frame will never released.
Fix this by freeing the frame in case of one-step timestamping.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
index ed8fb14a7f215..db8ca1dfd5322 100644
--- a/drivers/net/phy/mscc/mscc_ptp.c
+++ b/drivers/net/phy/mscc/mscc_ptp.c
@@ -1173,6 +1173,13 @@ static void vsc85xx_txtstamp(struct mii_timestamper *mii_ts,
 		return;
 	}
 
+	if (vsc8531->ptp->tx_type == HWTSTAMP_TX_ONESTEP_SYNC) {
+		if (ptp_msg_is_sync(skb, type)) {
+			kfree_skb(skb);
+			return;
+		}
+	}
+
 	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
 	mutex_lock(&vsc8531->ts_lock);
-- 
2.34.1

On Wed, May 21, 2025 at 03:11:14PM +0200, Horatiu Vultur wrote:
> Fix memory leak when running one-step timestamping. When running
> one-step sync timestamping, the HW is configured to insert the TX time
> into the frame, so there is no reason to keep the skb anymore. As in
> this case the HW will never generate an interrupt to say that the frame
> was timestamped, then the frame will never released.
> Fix this by freeing the frame in case of one-step timestamping.
> 
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> index ed8fb14a7f215..db8ca1dfd5322 100644
> --- a/drivers/net/phy/mscc/mscc_ptp.c
> +++ b/drivers/net/phy/mscc/mscc_ptp.c
> @@ -1173,6 +1173,13 @@ static void vsc85xx_txtstamp(struct mii_timestamper *mii_ts,
>  		return;
>  	}
>  
> +	if (vsc8531->ptp->tx_type == HWTSTAMP_TX_ONESTEP_SYNC) {
> +		if (ptp_msg_is_sync(skb, type)) {
> +			kfree_skb(skb);
> +			return;
> +		}
> +	}

I don't remember everything about TS but I think the above is fine. Also
while looking at this I saw this function is doing the following too:

  if (!vsc8531->ptp->configured)
	return;

I guess we should free the skb for all paths not putting it in the tx
queue. As there would be 3 paths freeing the skb + returning, you might
as well use a label for easier maintenance.

Thanks,
Antoine

The 05/21/2025 17:34, Antoine Tenart wrote:

Hi Antoine,

> 
> On Wed, May 21, 2025 at 03:11:14PM +0200, Horatiu Vultur wrote:
> > Fix memory leak when running one-step timestamping. When running
> > one-step sync timestamping, the HW is configured to insert the TX time
> > into the frame, so there is no reason to keep the skb anymore. As in
> > this case the HW will never generate an interrupt to say that the frame
> > was timestamped, then the frame will never released.
> > Fix this by freeing the frame in case of one-step timestamping.
> >
> > 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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c
> > index ed8fb14a7f215..db8ca1dfd5322 100644
> > --- a/drivers/net/phy/mscc/mscc_ptp.c
> > +++ b/drivers/net/phy/mscc/mscc_ptp.c
> > @@ -1173,6 +1173,13 @@ static void vsc85xx_txtstamp(struct mii_timestamper *mii_ts,
> >               return;
> >       }
> >
> > +     if (vsc8531->ptp->tx_type == HWTSTAMP_TX_ONESTEP_SYNC) {
> > +             if (ptp_msg_is_sync(skb, type)) {
> > +                     kfree_skb(skb);
> > +                     return;
> > +             }
> > +     }
> 
> I don't remember everything about TS but I think the above is fine. Also
> while looking at this I saw this function is doing the following too:
> 
>   if (!vsc8531->ptp->configured)
>         return;
> 
> I guess we should free the skb for all paths not putting it in the tx
> queue. As there would be 3 paths freeing the skb + returning, you might
> as well use a label for easier maintenance.

Yes, you are correct we should free the skb also in this case.
I will fix this in the next version.

> 
> Thanks,
> Antoine

-- 
/Horatiu