[net-next,3/3] net: mscc: ocelot: support PTP Sync one-step timestamping

Message ID 20210416123655.42783-4-yangbo.lu@nxp.com
State New
Headers show
Series
  • Support ocelot PTP Sync one-step timestamping
Related show

Commit Message

Y.b. Lu April 16, 2021, 12:36 p.m.
Although HWTSTAMP_TX_ONESTEP_SYNC existed in ioctl for hardware timestamp
configuration, the PTP Sync one-step timestamping had never been supported.

This patch is to truely support it. The hardware timestamp request type is
stored in DSA_SKB_CB_PRIV first byte per skb, so that corresponding
configuration could be done during transmitting. Non-onestep-Sync packet
with one-step timestamp request should fall back to use two-step timestamp.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c     | 57 ++++++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_net.c |  5 +--
 include/soc/mscc/ocelot.h              |  1 +
 net/dsa/tag_ocelot.c                   | 25 ++---------
 net/dsa/tag_ocelot_8021q.c             | 39 +++++-------------
 5 files changed, 72 insertions(+), 55 deletions(-)

Comments

Vladimir Oltean April 18, 2021, 9:46 a.m. | #1
On Fri, Apr 16, 2021 at 08:36:55PM +0800, Yangbo Lu wrote:
> Although HWTSTAMP_TX_ONESTEP_SYNC existed in ioctl for hardware timestamp

> configuration, the PTP Sync one-step timestamping had never been supported.

> 

> This patch is to truely support it.


Actually the ocelot switchdev driver does support one-step timestamping,
just the felix DSA driver does not.

> The hardware timestamp request type is

> stored in DSA_SKB_CB_PRIV first byte per skb, so that corresponding

> configuration could be done during transmitting. Non-onestep-Sync packet

> with one-step timestamp request should fall back to use two-step timestamp.

> 

> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

> ---

>  drivers/net/ethernet/mscc/ocelot.c     | 57 ++++++++++++++++++++++++++

>  drivers/net/ethernet/mscc/ocelot_net.c |  5 +--

>  include/soc/mscc/ocelot.h              |  1 +

>  net/dsa/tag_ocelot.c                   | 25 ++---------

>  net/dsa/tag_ocelot_8021q.c             | 39 +++++-------------

>  5 files changed, 72 insertions(+), 55 deletions(-)

> 

> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c

> index 541d3b4076be..69d36b6241ff 100644

> --- a/drivers/net/ethernet/mscc/ocelot.c

> +++ b/drivers/net/ethernet/mscc/ocelot.c

> @@ -6,6 +6,7 @@

>   */

>  #include <linux/dsa/ocelot.h>

>  #include <linux/if_bridge.h>

> +#include <linux/ptp_classify.h>

>  #include <soc/mscc/ocelot_vcap.h>

>  #include "ocelot.h"

>  #include "ocelot_vcap.h"

> @@ -546,6 +547,50 @@ static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,

>  	spin_unlock(&ocelot_port->ts_id_lock);

>  }

>  

> +bool ocelot_ptp_rew_op(struct sk_buff *skb, struct sk_buff *clone, u32 *rew_op)

> +{

> +	/* For two-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV

> +	 * and timestamp ID in clone->cb[0].

> +	 * For one-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV.

> +	 */

> +	u8 *ptp_cmd = DSA_SKB_CB_PRIV(skb);


This is fine in the sense that it works, but please consider creating
something similar to sja1105:

struct ocelot_skb_cb {
	u8 ptp_cmd; /* For both one-step and two-step timestamping */
	u8 ts_id; /* Only for two-step timestamping */
};

#define OCELOT_SKB_CB(skb) \
	((struct ocelot_skb_cb *)DSA_SKB_CB_PRIV(skb))

And then access as OCELOT_SKB_CB(skb)->ptp_cmd, OCELOT_SKB_CB(clone)->ts_id.

and put a comment to explain that this is done in order to have common
code between Felix DSA and Ocelot switchdev. Basically Ocelot will not
use the first 8 bytes of skb->cb, but there's enough space for this to
not make any difference. The original skb will hold only ptp_cmd, the
clone will only hold ts_id, but it helps to have the same structure in
place.

If you create this ocelot_skb_cb structure, I expect the comment above
to be fairly redundant, you can consider removing it.

> +

> +	if (clone) {

> +		*rew_op = *ptp_cmd;

> +		*rew_op |= clone->cb[0] << 3;

> +	} else if (*ptp_cmd) {

> +		*rew_op = *ptp_cmd;

> +	} else {

> +		return false;

> +	}

> +

> +	return true;


Just make this function return an u32. If the packet isn't PTP, the
rew_op will be 0.

> +}

> +EXPORT_SYMBOL(ocelot_ptp_rew_op);

> +

> +static bool ocelot_ptp_is_onestep_sync(struct sk_buff *skb)

> +{

> +	struct ptp_header *hdr;

> +	unsigned int ptp_class;

> +	u8 msgtype, twostep;

> +

> +	ptp_class = ptp_classify_raw(skb);

> +	if (ptp_class == PTP_CLASS_NONE)

> +		return false;

> +

> +	hdr = ptp_parse_header(skb, ptp_class);

> +	if (!hdr)

> +		return false;

> +

> +	msgtype = ptp_get_msgtype(hdr, ptp_class);

> +	twostep = hdr->flag_field[0] & 0x2;

> +

> +	if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0)

> +		return true;

> +

> +	return false;

> +}

> +


This is generic, but if you were to move it to net/core/ptp_classifier.c,
I think you would have to pass the output of ptp_classify_raw() as an
"unsigned int type" argument. So I think I would leave it the way it is
for now - inside of ocelot - until somebody else needs something
similar, and we see what is the required prototype.

>  int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,

>  				 struct sk_buff *skb,

>  				 struct sk_buff **clone)

> @@ -553,12 +598,24 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,

>  	struct ocelot_port *ocelot_port = ocelot->ports[port];

>  	u8 ptp_cmd = ocelot_port->ptp_cmd;

>  

> +	/* Store ptp_cmd in first byte of DSA_SKB_CB_PRIV per skb */

> +	if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) {

> +		if (ocelot_ptp_is_onestep_sync(skb)) {

> +			*(u8 *)DSA_SKB_CB_PRIV(skb) = ptp_cmd;

> +			return 0;

> +		}

> +

> +		/* Fall back to two-step timestamping */

> +		ptp_cmd = IFH_REW_OP_TWO_STEP_PTP;

> +	}

> +

>  	if (ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

>  		*clone = skb_clone_sk(skb);

>  		if (!(*clone))

>  			return -ENOMEM;

>  

>  		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);

> +		*(u8 *)DSA_SKB_CB_PRIV(skb) = ptp_cmd;

>  	}

>  

>  	return 0;

> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> index 8293152a6dc1..eb3d525731da 100644

> --- a/drivers/net/ethernet/mscc/ocelot_net.c

> +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> @@ -514,10 +514,7 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)

>  			return NETDEV_TX_OK;

>  		}

>  

> -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> -			rew_op = ocelot_port->ptp_cmd;

> -			rew_op |= clone->cb[0] << 3;

> -		}

> +		ocelot_ptp_rew_op(skb, clone, &rew_op);

>  	}

>  

>  	ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);

> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h

> index 9cdaf1d9199f..19413532db0b 100644

> --- a/include/soc/mscc/ocelot.h

> +++ b/include/soc/mscc/ocelot.h

> @@ -820,6 +820,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,

>  int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid);

>  int ocelot_hwstamp_get(struct ocelot *ocelot, int port, struct ifreq *ifr);

>  int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr);

> +bool ocelot_ptp_rew_op(struct sk_buff *skb, struct sk_buff *clone, u32 *rew_op);

>  int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,

>  				 struct sk_buff *skb,

>  				 struct sk_buff **clone);

> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c

> index f9df9cac81c5..d5c73b36f0c1 100644

> --- a/net/dsa/tag_ocelot.c

> +++ b/net/dsa/tag_ocelot.c

> @@ -5,25 +5,6 @@

>  #include <soc/mscc/ocelot.h>

>  #include "dsa_priv.h"

>  

> -static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,

> -			    struct sk_buff *clone)

> -{

> -	struct ocelot *ocelot = dp->ds->priv;

> -	struct ocelot_port *ocelot_port;

> -	u64 rew_op;

> -

> -	ocelot_port = ocelot->ports[dp->index];

> -	rew_op = ocelot_port->ptp_cmd;

> -

> -	/* Retrieve timestamp ID populated inside skb->cb[0] of the

> -	 * clone by ocelot_port_add_txtstamp_skb

> -	 */

> -	if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> -		rew_op |= clone->cb[0] << 3;

> -

> -	ocelot_ifh_set_rew_op(injection, rew_op);

> -}

> -

>  static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,

>  			       __be32 ifh_prefix, void **ifh)

>  {

> @@ -32,6 +13,7 @@ static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,

>  	struct dsa_switch *ds = dp->ds;

>  	void *injection;

>  	__be32 *prefix;

> +	u32 rew_op = 0;

>  

>  	injection = skb_push(skb, OCELOT_TAG_LEN);

>  	prefix = skb_push(skb, OCELOT_SHORT_PREFIX_LEN);

> @@ -42,9 +24,8 @@ static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,

>  	ocelot_ifh_set_src(injection, ds->num_ports);

>  	ocelot_ifh_set_qos_class(injection, skb->priority);

>  

> -	/* TX timestamping was requested */

> -	if (clone)

> -		ocelot_xmit_ptp(dp, injection, clone);

> +	if (ocelot_ptp_rew_op(skb, clone, &rew_op))

> +		ocelot_ifh_set_rew_op(injection, rew_op);

>  

>  	*ifh = injection;

>  }

> diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c

> index 5f3e8e124a82..bf32649a5a7b 100644

> --- a/net/dsa/tag_ocelot_8021q.c

> +++ b/net/dsa/tag_ocelot_8021q.c

> @@ -13,32 +13,6 @@

>  #include <soc/mscc/ocelot_ptp.h>

>  #include "dsa_priv.h"

>  

> -static struct sk_buff *ocelot_xmit_ptp(struct dsa_port *dp,

> -				       struct sk_buff *skb,

> -				       struct sk_buff *clone)

> -{

> -	struct ocelot *ocelot = dp->ds->priv;

> -	struct ocelot_port *ocelot_port;

> -	int port = dp->index;

> -	u32 rew_op;

> -

> -	if (!ocelot_can_inject(ocelot, 0))

> -		return NULL;

> -

> -	ocelot_port = ocelot->ports[port];

> -	rew_op = ocelot_port->ptp_cmd;

> -

> -	/* Retrieve timestamp ID populated inside skb->cb[0] of the

> -	 * clone by ocelot_port_add_txtstamp_skb

> -	 */

> -	if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> -		rew_op |= clone->cb[0] << 3;

> -

> -	ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);

> -

> -	return NULL;

> -}

> -

>  static struct sk_buff *ocelot_xmit(struct sk_buff *skb,

>  				   struct net_device *netdev)

>  {

> @@ -47,10 +21,17 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,

>  	u16 queue_mapping = skb_get_queue_mapping(skb);

>  	u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);

>  	struct sk_buff *clone = DSA_SKB_CB(skb)->clone;

> +	struct ocelot *ocelot = dp->ds->priv;

> +	int port = dp->index;

> +	u32 rew_op = 0;

> +

> +	if (ocelot_ptp_rew_op(skb, clone, &rew_op)) {

> +		if (!ocelot_can_inject(ocelot, 0))

> +			return NULL;

>  

> -	/* TX timestamping was requested, so inject through MMIO */

> -	if (clone)

> -		return ocelot_xmit_ptp(dp, skb, clone);

> +		ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);

> +		return NULL;

> +	}

>  

>  	return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,

>  			      ((pcp << VLAN_PRIO_SHIFT) | tx_vid));

> -- 

> 2.25.1

>
Y.b. Lu April 20, 2021, 7:33 a.m. | #2
Hi Vladimir,

> -----Original Message-----

> From: Vladimir Oltean <olteanv@gmail.com>

> Sent: 2021年4月18日 17:46

> To: Y.b. Lu <yangbo.lu@nxp.com>

> Cc: netdev@vger.kernel.org; Richard Cochran <richardcochran@gmail.com>;

> Vladimir Oltean <vladimir.oltean@nxp.com>; David S . Miller

> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Jonathan Corbet

> <corbet@lwn.net>; Kurt Kanzenbach <kurt@linutronix.de>; Andrew Lunn

> <andrew@lunn.ch>; Vivien Didelot <vivien.didelot@gmail.com>; Florian

> Fainelli <f.fainelli@gmail.com>; Claudiu Manoil <claudiu.manoil@nxp.com>;

> Alexandre Belloni <alexandre.belloni@bootlin.com>;

> UNGLinuxDriver@microchip.com; linux-doc@vger.kernel.org;

> linux-kernel@vger.kernel.org

> Subject: Re: [net-next 3/3] net: mscc: ocelot: support PTP Sync one-step

> timestamping

> 

> On Fri, Apr 16, 2021 at 08:36:55PM +0800, Yangbo Lu wrote:

> > Although HWTSTAMP_TX_ONESTEP_SYNC existed in ioctl for hardware

> > timestamp configuration, the PTP Sync one-step timestamping had never

> been supported.

> >

> > This patch is to truely support it.

> 

> Actually the ocelot switchdev driver does support one-step timestamping, just

> the felix DSA driver does not.


Actually I don’t think ocelot support one-step timestamping properly.
Without one-step sync packet identification, the one-step timestamping is applying to all packets requiring timestamp.
The user space PTP stack won't work.

> 

> > The hardware timestamp request type is stored in DSA_SKB_CB_PRIV first

> > byte per skb, so that corresponding configuration could be done during

> > transmitting. Non-onestep-Sync packet with one-step timestamp request

> > should fall back to use two-step timestamp.

> >

> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

> > ---

> >  drivers/net/ethernet/mscc/ocelot.c     | 57

> ++++++++++++++++++++++++++

> >  drivers/net/ethernet/mscc/ocelot_net.c |  5 +--

> >  include/soc/mscc/ocelot.h              |  1 +

> >  net/dsa/tag_ocelot.c                   | 25 ++---------

> >  net/dsa/tag_ocelot_8021q.c             | 39 +++++-------------

> >  5 files changed, 72 insertions(+), 55 deletions(-)

> >

> > diff --git a/drivers/net/ethernet/mscc/ocelot.c

> > b/drivers/net/ethernet/mscc/ocelot.c

> > index 541d3b4076be..69d36b6241ff 100644

> > --- a/drivers/net/ethernet/mscc/ocelot.c

> > +++ b/drivers/net/ethernet/mscc/ocelot.c

> > @@ -6,6 +6,7 @@

> >   */

> >  #include <linux/dsa/ocelot.h>

> >  #include <linux/if_bridge.h>

> > +#include <linux/ptp_classify.h>

> >  #include <soc/mscc/ocelot_vcap.h>

> >  #include "ocelot.h"

> >  #include "ocelot_vcap.h"

> > @@ -546,6 +547,50 @@ static void ocelot_port_add_txtstamp_skb(struct

> ocelot *ocelot, int port,

> >  	spin_unlock(&ocelot_port->ts_id_lock);

> >  }

> >

> > +bool ocelot_ptp_rew_op(struct sk_buff *skb, struct sk_buff *clone,

> > +u32 *rew_op) {

> > +	/* For two-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV

> > +	 * and timestamp ID in clone->cb[0].

> > +	 * For one-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV.

> > +	 */

> > +	u8 *ptp_cmd = DSA_SKB_CB_PRIV(skb);

> 

> This is fine in the sense that it works, but please consider creating something

> similar to sja1105:

> 

> struct ocelot_skb_cb {

> 	u8 ptp_cmd; /* For both one-step and two-step timestamping */

> 	u8 ts_id; /* Only for two-step timestamping */ };

> 

> #define OCELOT_SKB_CB(skb) \

> 	((struct ocelot_skb_cb *)DSA_SKB_CB_PRIV(skb))

> 

> And then access as OCELOT_SKB_CB(skb)->ptp_cmd,

> OCELOT_SKB_CB(clone)->ts_id.

> 

> and put a comment to explain that this is done in order to have common code

> between Felix DSA and Ocelot switchdev. Basically Ocelot will not use the first

> 8 bytes of skb->cb, but there's enough space for this to not make any

> difference. The original skb will hold only ptp_cmd, the clone will only hold

> ts_id, but it helps to have the same structure in place.

> 

> If you create this ocelot_skb_cb structure, I expect the comment above to be

> fairly redundant, you can consider removing it.

> 


You're right to define the structure.
Considering patch #1, move skb cloning to drivers, and populate DSA_SKB_CB(skb)->clone if needs to do so (per suggestion).
Can we totally drop dsa_skb_cb in dsa core? The only usage of it is holding a skb clone pointer, for only felix and sja1105.
Actually we can move such pointer in <device>_skb_cb, instead of reserving the space of skb for any drivers.

Do you think so?

> > +

> > +	if (clone) {

> > +		*rew_op = *ptp_cmd;

> > +		*rew_op |= clone->cb[0] << 3;

> > +	} else if (*ptp_cmd) {

> > +		*rew_op = *ptp_cmd;

> > +	} else {

> > +		return false;

> > +	}

> > +

> > +	return true;

> 

> Just make this function return an u32. If the packet isn't PTP, the rew_op will

> be 0.


Ok. Thanks.

> 

> > +}

> > +EXPORT_SYMBOL(ocelot_ptp_rew_op);

> > +

> > +static bool ocelot_ptp_is_onestep_sync(struct sk_buff *skb) {

> > +	struct ptp_header *hdr;

> > +	unsigned int ptp_class;

> > +	u8 msgtype, twostep;

> > +

> > +	ptp_class = ptp_classify_raw(skb);

> > +	if (ptp_class == PTP_CLASS_NONE)

> > +		return false;

> > +

> > +	hdr = ptp_parse_header(skb, ptp_class);

> > +	if (!hdr)

> > +		return false;

> > +

> > +	msgtype = ptp_get_msgtype(hdr, ptp_class);

> > +	twostep = hdr->flag_field[0] & 0x2;

> > +

> > +	if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0)

> > +		return true;

> > +

> > +	return false;

> > +}

> > +

> 

> This is generic, but if you were to move it to net/core/ptp_classifier.c, I think

> you would have to pass the output of ptp_classify_raw() as an "unsigned int

> type" argument. So I think I would leave it the way it is for now - inside of

> ocelot - until somebody else needs something similar, and we see what is the

> required prototype.


Yes. This is indeed generic, and useful. I'd like to leave it the way it is for now.
I may make another patch-set for this in future.

> 

> >  int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,

> >  				 struct sk_buff *skb,

> >  				 struct sk_buff **clone)

> > @@ -553,12 +598,24 @@ int ocelot_port_txtstamp_request(struct ocelot

> *ocelot, int port,

> >  	struct ocelot_port *ocelot_port = ocelot->ports[port];

> >  	u8 ptp_cmd = ocelot_port->ptp_cmd;

> >

> > +	/* Store ptp_cmd in first byte of DSA_SKB_CB_PRIV per skb */

> > +	if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) {

> > +		if (ocelot_ptp_is_onestep_sync(skb)) {

> > +			*(u8 *)DSA_SKB_CB_PRIV(skb) = ptp_cmd;

> > +			return 0;

> > +		}

> > +

> > +		/* Fall back to two-step timestamping */

> > +		ptp_cmd = IFH_REW_OP_TWO_STEP_PTP;

> > +	}

> > +

> >  	if (ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> >  		*clone = skb_clone_sk(skb);

> >  		if (!(*clone))

> >  			return -ENOMEM;

> >

> >  		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);

> > +		*(u8 *)DSA_SKB_CB_PRIV(skb) = ptp_cmd;

> >  	}

> >

> >  	return 0;

> > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c

> > b/drivers/net/ethernet/mscc/ocelot_net.c

> > index 8293152a6dc1..eb3d525731da 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_net.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> > @@ -514,10 +514,7 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff

> *skb, struct net_device *dev)

> >  			return NETDEV_TX_OK;

> >  		}

> >

> > -		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {

> > -			rew_op = ocelot_port->ptp_cmd;

> > -			rew_op |= clone->cb[0] << 3;

> > -		}

> > +		ocelot_ptp_rew_op(skb, clone, &rew_op);

> >  	}

> >

> >  	ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb); diff --git

> > a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index

> > 9cdaf1d9199f..19413532db0b 100644

> > --- a/include/soc/mscc/ocelot.h

> > +++ b/include/soc/mscc/ocelot.h

> > @@ -820,6 +820,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int

> > port, u16 vid, bool pvid,  int ocelot_vlan_del(struct ocelot *ocelot,

> > int port, u16 vid);  int ocelot_hwstamp_get(struct ocelot *ocelot, int

> > port, struct ifreq *ifr);  int ocelot_hwstamp_set(struct ocelot

> > *ocelot, int port, struct ifreq *ifr);

> > +bool ocelot_ptp_rew_op(struct sk_buff *skb, struct sk_buff *clone,

> > +u32 *rew_op);

> >  int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,

> >  				 struct sk_buff *skb,

> >  				 struct sk_buff **clone);

> > diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c index

> > f9df9cac81c5..d5c73b36f0c1 100644

> > --- a/net/dsa/tag_ocelot.c

> > +++ b/net/dsa/tag_ocelot.c

> > @@ -5,25 +5,6 @@

> >  #include <soc/mscc/ocelot.h>

> >  #include "dsa_priv.h"

> >

> > -static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,

> > -			    struct sk_buff *clone)

> > -{

> > -	struct ocelot *ocelot = dp->ds->priv;

> > -	struct ocelot_port *ocelot_port;

> > -	u64 rew_op;

> > -

> > -	ocelot_port = ocelot->ports[dp->index];

> > -	rew_op = ocelot_port->ptp_cmd;

> > -

> > -	/* Retrieve timestamp ID populated inside skb->cb[0] of the

> > -	 * clone by ocelot_port_add_txtstamp_skb

> > -	 */

> > -	if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> > -		rew_op |= clone->cb[0] << 3;

> > -

> > -	ocelot_ifh_set_rew_op(injection, rew_op);

> > -}

> > -

> >  static void ocelot_xmit_common(struct sk_buff *skb, struct net_device

> *netdev,

> >  			       __be32 ifh_prefix, void **ifh)  { @@ -32,6 +13,7 @@

> static

> > void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,

> >  	struct dsa_switch *ds = dp->ds;

> >  	void *injection;

> >  	__be32 *prefix;

> > +	u32 rew_op = 0;

> >

> >  	injection = skb_push(skb, OCELOT_TAG_LEN);

> >  	prefix = skb_push(skb, OCELOT_SHORT_PREFIX_LEN); @@ -42,9 +24,8

> @@

> > static void ocelot_xmit_common(struct sk_buff *skb, struct net_device

> *netdev,

> >  	ocelot_ifh_set_src(injection, ds->num_ports);

> >  	ocelot_ifh_set_qos_class(injection, skb->priority);

> >

> > -	/* TX timestamping was requested */

> > -	if (clone)

> > -		ocelot_xmit_ptp(dp, injection, clone);

> > +	if (ocelot_ptp_rew_op(skb, clone, &rew_op))

> > +		ocelot_ifh_set_rew_op(injection, rew_op);

> >

> >  	*ifh = injection;

> >  }

> > diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c

> > index 5f3e8e124a82..bf32649a5a7b 100644

> > --- a/net/dsa/tag_ocelot_8021q.c

> > +++ b/net/dsa/tag_ocelot_8021q.c

> > @@ -13,32 +13,6 @@

> >  #include <soc/mscc/ocelot_ptp.h>

> >  #include "dsa_priv.h"

> >

> > -static struct sk_buff *ocelot_xmit_ptp(struct dsa_port *dp,

> > -				       struct sk_buff *skb,

> > -				       struct sk_buff *clone)

> > -{

> > -	struct ocelot *ocelot = dp->ds->priv;

> > -	struct ocelot_port *ocelot_port;

> > -	int port = dp->index;

> > -	u32 rew_op;

> > -

> > -	if (!ocelot_can_inject(ocelot, 0))

> > -		return NULL;

> > -

> > -	ocelot_port = ocelot->ports[port];

> > -	rew_op = ocelot_port->ptp_cmd;

> > -

> > -	/* Retrieve timestamp ID populated inside skb->cb[0] of the

> > -	 * clone by ocelot_port_add_txtstamp_skb

> > -	 */

> > -	if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)

> > -		rew_op |= clone->cb[0] << 3;

> > -

> > -	ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);

> > -

> > -	return NULL;

> > -}

> > -

> >  static struct sk_buff *ocelot_xmit(struct sk_buff *skb,

> >  				   struct net_device *netdev)

> >  {

> > @@ -47,10 +21,17 @@ static struct sk_buff *ocelot_xmit(struct sk_buff

> *skb,

> >  	u16 queue_mapping = skb_get_queue_mapping(skb);

> >  	u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);

> >  	struct sk_buff *clone = DSA_SKB_CB(skb)->clone;

> > +	struct ocelot *ocelot = dp->ds->priv;

> > +	int port = dp->index;

> > +	u32 rew_op = 0;

> > +

> > +	if (ocelot_ptp_rew_op(skb, clone, &rew_op)) {

> > +		if (!ocelot_can_inject(ocelot, 0))

> > +			return NULL;

> >

> > -	/* TX timestamping was requested, so inject through MMIO */

> > -	if (clone)

> > -		return ocelot_xmit_ptp(dp, skb, clone);

> > +		ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);

> > +		return NULL;

> > +	}

> >

> >  	return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,

> >  			      ((pcp << VLAN_PRIO_SHIFT) | tx_vid));

> > --

> > 2.25.1

> >
Vladimir Oltean April 20, 2021, 8:20 a.m. | #3
On Tue, Apr 20, 2021 at 07:33:39AM +0000, Y.b. Lu wrote:
> > > +	/* For two-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV

> > > +	 * and timestamp ID in clone->cb[0].

> > > +	 * For one-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV.

> > > +	 */

> > > +	u8 *ptp_cmd = DSA_SKB_CB_PRIV(skb);

> >

> > This is fine in the sense that it works, but please consider creating something

> > similar to sja1105:

> >

> > struct ocelot_skb_cb {

> > 	u8 ptp_cmd; /* For both one-step and two-step timestamping */

> > 	u8 ts_id; /* Only for two-step timestamping */ };

> >

> > #define OCELOT_SKB_CB(skb) \

> > 	((struct ocelot_skb_cb *)DSA_SKB_CB_PRIV(skb))

> >

> > And then access as OCELOT_SKB_CB(skb)->ptp_cmd,

> > OCELOT_SKB_CB(clone)->ts_id.

> >

> > and put a comment to explain that this is done in order to have common code

> > between Felix DSA and Ocelot switchdev. Basically Ocelot will not use the first

> > 8 bytes of skb->cb, but there's enough space for this to not make any

> > difference. The original skb will hold only ptp_cmd, the clone will only hold

> > ts_id, but it helps to have the same structure in place.

> >

> > If you create this ocelot_skb_cb structure, I expect the comment above to be

> > fairly redundant, you can consider removing it.

> >

>

> You're right to define the structure.

> Considering patch #1, move skb cloning to drivers, and populate DSA_SKB_CB(skb)->clone if needs to do so (per suggestion).

> Can we totally drop dsa_skb_cb in dsa core? The only usage of it is holding a skb clone pointer, for only felix and sja1105.

> Actually we can move such pointer in <device>_skb_cb, instead of reserving the space of skb for any drivers.

>

> Do you think so?


The trouble with skb->cb is that it isn't zero-initialized. But somebody
needs to initialize the clone pointer to NULL, otherwise you don't know
if this is a valid pointer or not. Because dsa_skb_tx_timestamp() is
called before p->xmit(), the driver has no way to initialize the clone
pointer by itself. So this was done directly from dsa_slave_xmit(), and
not from any driver-specific hook. So this is why there is a
DSA_SKB_CB(skb)->clone and not SJA1105_SKB_CB(skb)->clone. The
alternative would be to memset(skb->cb, 0, 48) which is a bit
sub-optimal because it initializes more than it needs. Alternatively, it
might be possible to introduce a new property in struct dsa_device_ops
which holds sizeof(struct sja1105_skb_cb), and the generic code will
only zero-initialize this number of bytes.
I don't know, if you can get it to work in a way that does not incur a
noticeable performance penalty, I'm okay with whatever you come up with.

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 541d3b4076be..69d36b6241ff 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -6,6 +6,7 @@ 
  */
 #include <linux/dsa/ocelot.h>
 #include <linux/if_bridge.h>
+#include <linux/ptp_classify.h>
 #include <soc/mscc/ocelot_vcap.h>
 #include "ocelot.h"
 #include "ocelot_vcap.h"
@@ -546,6 +547,50 @@  static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
 	spin_unlock(&ocelot_port->ts_id_lock);
 }
 
+bool ocelot_ptp_rew_op(struct sk_buff *skb, struct sk_buff *clone, u32 *rew_op)
+{
+	/* For two-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV
+	 * and timestamp ID in clone->cb[0].
+	 * For one-step timestamp, retrieve ptp_cmd in DSA_SKB_CB_PRIV.
+	 */
+	u8 *ptp_cmd = DSA_SKB_CB_PRIV(skb);
+
+	if (clone) {
+		*rew_op = *ptp_cmd;
+		*rew_op |= clone->cb[0] << 3;
+	} else if (*ptp_cmd) {
+		*rew_op = *ptp_cmd;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(ocelot_ptp_rew_op);
+
+static bool ocelot_ptp_is_onestep_sync(struct sk_buff *skb)
+{
+	struct ptp_header *hdr;
+	unsigned int ptp_class;
+	u8 msgtype, twostep;
+
+	ptp_class = ptp_classify_raw(skb);
+	if (ptp_class == PTP_CLASS_NONE)
+		return false;
+
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
+		return false;
+
+	msgtype = ptp_get_msgtype(hdr, ptp_class);
+	twostep = hdr->flag_field[0] & 0x2;
+
+	if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0)
+		return true;
+
+	return false;
+}
+
 int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
 				 struct sk_buff *skb,
 				 struct sk_buff **clone)
@@ -553,12 +598,24 @@  int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u8 ptp_cmd = ocelot_port->ptp_cmd;
 
+	/* Store ptp_cmd in first byte of DSA_SKB_CB_PRIV per skb */
+	if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) {
+		if (ocelot_ptp_is_onestep_sync(skb)) {
+			*(u8 *)DSA_SKB_CB_PRIV(skb) = ptp_cmd;
+			return 0;
+		}
+
+		/* Fall back to two-step timestamping */
+		ptp_cmd = IFH_REW_OP_TWO_STEP_PTP;
+	}
+
 	if (ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
 		*clone = skb_clone_sk(skb);
 		if (!(*clone))
 			return -ENOMEM;
 
 		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
+		*(u8 *)DSA_SKB_CB_PRIV(skb) = ptp_cmd;
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 8293152a6dc1..eb3d525731da 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -514,10 +514,7 @@  static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
 			return NETDEV_TX_OK;
 		}
 
-		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
-			rew_op = ocelot_port->ptp_cmd;
-			rew_op |= clone->cb[0] << 3;
-		}
+		ocelot_ptp_rew_op(skb, clone, &rew_op);
 	}
 
 	ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 9cdaf1d9199f..19413532db0b 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -820,6 +820,7 @@  int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid);
 int ocelot_hwstamp_get(struct ocelot *ocelot, int port, struct ifreq *ifr);
 int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr);
+bool ocelot_ptp_rew_op(struct sk_buff *skb, struct sk_buff *clone, u32 *rew_op);
 int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
 				 struct sk_buff *skb,
 				 struct sk_buff **clone);
diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
index f9df9cac81c5..d5c73b36f0c1 100644
--- a/net/dsa/tag_ocelot.c
+++ b/net/dsa/tag_ocelot.c
@@ -5,25 +5,6 @@ 
 #include <soc/mscc/ocelot.h>
 #include "dsa_priv.h"
 
-static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection,
-			    struct sk_buff *clone)
-{
-	struct ocelot *ocelot = dp->ds->priv;
-	struct ocelot_port *ocelot_port;
-	u64 rew_op;
-
-	ocelot_port = ocelot->ports[dp->index];
-	rew_op = ocelot_port->ptp_cmd;
-
-	/* Retrieve timestamp ID populated inside skb->cb[0] of the
-	 * clone by ocelot_port_add_txtstamp_skb
-	 */
-	if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
-		rew_op |= clone->cb[0] << 3;
-
-	ocelot_ifh_set_rew_op(injection, rew_op);
-}
-
 static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
 			       __be32 ifh_prefix, void **ifh)
 {
@@ -32,6 +13,7 @@  static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
 	struct dsa_switch *ds = dp->ds;
 	void *injection;
 	__be32 *prefix;
+	u32 rew_op = 0;
 
 	injection = skb_push(skb, OCELOT_TAG_LEN);
 	prefix = skb_push(skb, OCELOT_SHORT_PREFIX_LEN);
@@ -42,9 +24,8 @@  static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev,
 	ocelot_ifh_set_src(injection, ds->num_ports);
 	ocelot_ifh_set_qos_class(injection, skb->priority);
 
-	/* TX timestamping was requested */
-	if (clone)
-		ocelot_xmit_ptp(dp, injection, clone);
+	if (ocelot_ptp_rew_op(skb, clone, &rew_op))
+		ocelot_ifh_set_rew_op(injection, rew_op);
 
 	*ifh = injection;
 }
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index 5f3e8e124a82..bf32649a5a7b 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -13,32 +13,6 @@ 
 #include <soc/mscc/ocelot_ptp.h>
 #include "dsa_priv.h"
 
-static struct sk_buff *ocelot_xmit_ptp(struct dsa_port *dp,
-				       struct sk_buff *skb,
-				       struct sk_buff *clone)
-{
-	struct ocelot *ocelot = dp->ds->priv;
-	struct ocelot_port *ocelot_port;
-	int port = dp->index;
-	u32 rew_op;
-
-	if (!ocelot_can_inject(ocelot, 0))
-		return NULL;
-
-	ocelot_port = ocelot->ports[port];
-	rew_op = ocelot_port->ptp_cmd;
-
-	/* Retrieve timestamp ID populated inside skb->cb[0] of the
-	 * clone by ocelot_port_add_txtstamp_skb
-	 */
-	if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP)
-		rew_op |= clone->cb[0] << 3;
-
-	ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
-
-	return NULL;
-}
-
 static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
 				   struct net_device *netdev)
 {
@@ -47,10 +21,17 @@  static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
 	u16 queue_mapping = skb_get_queue_mapping(skb);
 	u8 pcp = netdev_txq_to_tc(netdev, queue_mapping);
 	struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+	struct ocelot *ocelot = dp->ds->priv;
+	int port = dp->index;
+	u32 rew_op = 0;
+
+	if (ocelot_ptp_rew_op(skb, clone, &rew_op)) {
+		if (!ocelot_can_inject(ocelot, 0))
+			return NULL;
 
-	/* TX timestamping was requested, so inject through MMIO */
-	if (clone)
-		return ocelot_xmit_ptp(dp, skb, clone);
+		ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb);
+		return NULL;
+	}
 
 	return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q,
 			      ((pcp << VLAN_PRIO_SHIFT) | tx_vid));