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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250814090248.e7o6rzfr7vfcgfsc@DEN-DL-M31836.microchip.com>
Date: Thu, 14 Aug 2025 11:02:48 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
CC: <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <richardcochran@...il.com>, <viro@...iv.linux.org.uk>,
	<atenart@...nel.org>, <quentin.schulz@...tlin.com>, <olteanv@...il.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net v2] phy: mscc: Fix timestamping for vsc8584

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@...rochip.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