netdev - Re: [PATCH RFC net-next 2/5] ptp: marvell: add core support for Marvell PTP v2.1

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: <20250613171913.11a10645@kmaincent-XPS-13-7390>
Date: Fri, 13 Jun 2025 17:19:13 +0200
From: Kory Maincent <kory.maincent@...tlin.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
 Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH RFC net-next 2/5] ptp: marvell: add core support for
 Marvell PTP v2.1

Hello Russell,

Le Wed, 16 Apr 2025 10:22:47 +0100,
"Russell King (Oracle)" <linux@...linux.org.uk> a écrit :

> On Wed, Apr 16, 2025 at 10:48:49AM +0200, Kory Maincent wrote:
> > On Fri, 11 Apr 2025 22:26:37 +0100
> > Russell King <rmk+kernel@...linux.org.uk> wrote:  
> > > Provide core support for the Marvell PTP v2.1 implementations, which
> > > consist of a TAI (time application interface) and timestamping blocks.
> > > This hardware can be found in Marvell 88E151x PHYs, Armada 38x and
> > > Armada 37xx (mvneta), as well as Marvell DSA devices.
> > > 
> > > Support for both arrival timestamps is supported, we use arrival 1 for
> > > PTP peer delay messages, and arrival 0 for all other messages.
> > > 
> > > External event capture is also supported.
> > > 
> > > PPS output and trigger generation is not supported.
> > > 
> > > This core takes inspiration from the existing Marvell 88E6xxx DSA PTP
> > > code and DP83640 drivers. Like the original 88E6xxx DSA code, we
> > > use a delayed work to keep the cycle counter updated, and a separate
> > > delayed work for event capture.
> > > 
> > > We expose the ptp clock aux work to allow users to support single and
> > > multi-port designs - where there is one Marvell TAI instance and a
> > > number of Marvell TS instances.  
> > 
> > ...
> >   
> > > +#define MV_PTP_MSGTYPE_DELAY_RESP	9
> > > +
> > > +/* This defines which incoming or outgoing PTP frames are timestampped */
> > > +#define MV_PTP_MSD_ID_TS_EN	(BIT(PTP_MSGTYPE_SYNC) | \
> > > +				 BIT(PTP_MSGTYPE_DELAY_REQ) | \
> > > +				 BIT(MV_PTP_MSGTYPE_DELAY_RESP))
> > > +/* Direct Sync messages to Arr0 and delay messages to Arr1 */
> > > +#define MV_PTP_TS_ARR_PTR	(BIT(PTP_MSGTYPE_DELAY_REQ) | \
> > > +				 BIT(MV_PTP_MSGTYPE_DELAY_RESP))  
> > 
> > Why did you have chosen to use two queues with two separate behavior?
> > I have tried using only one queue and the PTP as master behaves correctly
> > without all these overrun. It is way better with one queue.
> > Maybe it was not the best approach if you want to use the two queues.    
> 
> First, both queues have the same behaviour.
> 
> Second, because they *aren't* queues as they can only stamp one message.
> The sync messages come from the master on a regular basis. The delay
> response messages come from the master in response to a delay request
> message, the timing of which is determined by the local slave.
> 
> If the local end sends a delay request just at the point that the master
> sends a sync message causing the master to immediately follow the sync
> message with the delay response message, then we could get an overrun
> on a single queue - because we'll stamp the sync message and if we don't
> read the timestamp quickly enough, the stamp registers will be busy
> preventing the timestamp of the delay response being captured.

I just come back on this. I think you are wrong.
We are using different registers to save timestamp for departure or arrival
packets.
If there is a sync message and a delay request message (the delay response is
not timestamped) at the same time, the sync message timestamp will be save in
the departure registers and the delay request timestamp in the arrival registers
(or the contrary if the Marvell PTP is a follower). There is no possibility to
loose the timestamp on that case, therefore using two registers for arrival
packets is useless.

FYI, I just tested the patch series on the Espressobin which have a mv88e6xxx
and the PTP worked well. 

Do you now If you will have time to send a v2 or should I take the lead on
this series? 

> With the overruns that I've seen, they've always been on the second
> "queue" and have always been for a sequence number several in the past
> beyond the point that the overrun has been reported. However, the
> packet which the sequence number matches had already been received -
> and several others have also been received. I've been wondering if it's
> a hardware bug, or maybe it's something other bits of the kernel is
> doing wrong.

I think we should drop this as I don't see anything relevant to use two
"queues" except overrun cases.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com