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@xxxxxxxxxxxxx>
>
> ---
> 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;
>