[net-next 0/3] Support ocelot PTP Sync one-step timestamping

[net-next 0/3] Support ocelot PTP Sync one-step timestamping

[Yangbo Lu]

This patch-set is to support ocelot PTP Sync one-step timestamping.
Actually before that, this patch-set cleans up and optimizes the
DSA slave tx timestamp request handling process.

Yangbo Lu (3):
  net: dsa: optimize tx timestamp request handling
  net: mscc: ocelot: convert to ocelot_port_txtstamp_request()
  net: mscc: ocelot: support PTP Sync one-step timestamping

 Documentation/networking/timestamping.rst     |  7 +-
 .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 20 +++--
 .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  2 +-
 drivers/net/dsa/mv88e6xxx/hwtstamp.c          | 21 +++--
 drivers/net/dsa/mv88e6xxx/hwtstamp.h          |  6 +-
 drivers/net/dsa/ocelot/felix.c                | 15 ++--
 drivers/net/dsa/sja1105/sja1105_ptp.c         |  6 +-
 drivers/net/dsa/sja1105/sja1105_ptp.h         |  2 +-
 drivers/net/ethernet/mscc/ocelot.c            | 81 ++++++++++++++++++-
 drivers/net/ethernet/mscc/ocelot_net.c        | 19 ++---
 include/net/dsa.h                             |  2 +-
 include/soc/mscc/ocelot.h                     |  6 +-
 net/dsa/slave.c                               | 20 ++---
 net/dsa/tag_ocelot.c                          | 25 +-----
 net/dsa/tag_ocelot_8021q.c                    | 39 +++------
 15 files changed, 158 insertions(+), 113 deletions(-)


base-commit: 392c36e5be1dee19ffce8c8ba8f07f90f5aa3f7c
-- 
2.25.1

[net-next 1/3] net: dsa: optimize tx timestamp request handling

[Yangbo Lu]

Optimization could be done on dsa_skb_tx_timestamp(), and dsa device
drivers should adapt to it.

- Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in
  port_txtstamp, so that most skbs not requiring tx timestamp just return.

- No longer to identify PTP packets, and limit tx timestamping only for PTP
  packets. If device driver likes, let device driver do.

- It is a waste to clone skb directly in dsa_skb_tx_timestamp().
  For one-step timestamping, a clone is not needed. For any failure of
  port_txtstamp (this may usually happen), the skb clone has to be freed.
  So put skb cloning into port_txtstamp where it really needs.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
 Documentation/networking/timestamping.rst     |  7 +++++--
 .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 20 ++++++++++++------
 .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  2 +-
 drivers/net/dsa/mv88e6xxx/hwtstamp.c          | 21 +++++++++++++------
 drivers/net/dsa/mv88e6xxx/hwtstamp.h          |  6 +++---
 drivers/net/dsa/ocelot/felix.c                | 11 ++++++----
 drivers/net/dsa/sja1105/sja1105_ptp.c         |  6 +++++-
 drivers/net/dsa/sja1105/sja1105_ptp.h         |  2 +-
 include/net/dsa.h                             |  2 +-
 net/dsa/slave.c                               | 20 +++++-------------
 10 files changed, 57 insertions(+), 40 deletions(-)

diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
index f682e88fa87e..7f04a699a5d1 100644
--- a/Documentation/networking/timestamping.rst
+++ b/Documentation/networking/timestamping.rst
@@ -635,8 +635,8 @@ in generic code: a BPF classifier (``ptp_classify_raw``) is used to identify
 PTP event messages (any other packets, including PTP general messages, are not
 timestamped), and provides two hooks to drivers:
 
-- ``.port_txtstamp()``: The driver is passed a clone of the timestampable skb
-  to be transmitted, before actually transmitting it. Typically, a switch will
+- ``.port_txtstamp()``: A clone of the timestampable skb to be transmitted
+  is needed, before actually transmitting it. Typically, a switch will
   have a PTP TX timestamp register (or sometimes a FIFO) where the timestamp
   becomes available. There may be an IRQ that is raised upon this timestamp's
   availability, or the driver might have to poll after invoking
@@ -645,6 +645,9 @@ timestamped), and provides two hooks to drivers:
   later use (when the timestamp becomes available). Each skb is annotated with
   a pointer to its clone, in ``DSA_SKB_CB(skb)->clone``, to ease the driver's
   job of keeping track of which clone belongs to which skb.
+  But one-step timestamping request is handled differently with above two-step
+  timestamping. The skb clone is no longer needed since hardware will insert
+  TX time information on packet during egress.
 
 - ``.port_rxtstamp()``: The original (and only) timestampable skb is provided
   to the driver, for it to annotate it with a timestamp, if that is immediately
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
index 69dd9a2e8bb6..2ff4b7c08b72 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
@@ -374,31 +374,39 @@ long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
 }
 
 bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type)
+			     struct sk_buff *skb, struct sk_buff **clone)
 {
 	struct hellcreek *hellcreek = ds->priv;
 	struct hellcreek_port_hwtstamp *ps;
 	struct ptp_header *hdr;
+	unsigned int type;
 
 	ps = &hellcreek->ports[port].port_hwtstamp;
 
-	/* Check if the driver is expected to do HW timestamping */
-	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
+	type = ptp_classify_raw(skb);
+	if (type == PTP_CLASS_NONE)
 		return false;
 
 	/* Make sure the message is a PTP message that needs to be timestamped
 	 * and the interaction with the HW timestamping is enabled. If not, stop
 	 * here
 	 */
-	hdr = hellcreek_should_tstamp(hellcreek, port, clone, type);
+	hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
 	if (!hdr)
 		return false;
 
+	*clone = skb_clone_sk(skb);
+	if (!(*clone))
+		return false;
+
 	if (test_and_set_bit_lock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
-				  &ps->state))
+				  &ps->state)) {
+		kfree_skb(*clone);
+		*clone = NULL;
 		return false;
+	}
 
-	ps->tx_skb = clone;
+	ps->tx_skb = *clone;
 
 	/* store the number of ticks occurred since system start-up till this
 	 * moment
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
index c0745ffa1ebb..58cc96642076 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
@@ -45,7 +45,7 @@ int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
 bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
 			     struct sk_buff *clone, unsigned int type);
 bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type);
+			     struct sk_buff *skb, struct sk_buff **clone);
 
 int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
 			  struct ethtool_ts_info *info);
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 094d17a1d037..280a95962861 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -469,24 +469,33 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
 }
 
 bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type)
+			     struct sk_buff *skb, struct sk_buff **clone)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
 	struct ptp_header *hdr;
+	unsigned int type;
 
-	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
-		return false;
+	type = ptp_classify_raw(skb);
+	if (type == PTP_CLASS_NONE)
+		return false
 
-	hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
+	hdr = mv88e6xxx_should_tstamp(chip, port, skb, type);
 	if (!hdr)
 		return false;
 
+	*clone = skb_clone_sk(skb);
+	if (!(*clone))
+		return false;
+
 	if (test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
-				  &ps->state))
+				  &ps->state)) {
+		kfree_skb(*clone);
+		*clone = NULL;
 		return false;
+	}
 
-	ps->tx_skb = clone;
+	ps->tx_skb = *clone;
 	ps->tx_tstamp_start = jiffies;
 	ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
 
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index 9da9f197ba02..da2b253334d0 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -118,7 +118,7 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
 bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 			     struct sk_buff *clone, unsigned int type);
 bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
-			     struct sk_buff *clone, unsigned int type);
+			     struct sk_buff *skb, struct sk_buff **clone);
 
 int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
 			  struct ethtool_ts_info *info);
@@ -152,8 +152,8 @@ static inline bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 }
 
 static inline bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
-					   struct sk_buff *clone,
-					   unsigned int type)
+					   struct sk_buff *skb,
+					   struct sk_buff **clone)
 {
 	return false;
 }
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 6b5442be0230..cdec2f5e271c 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1396,14 +1396,17 @@ static bool felix_rxtstamp(struct dsa_switch *ds, int port,
 }
 
 static bool felix_txtstamp(struct dsa_switch *ds, int port,
-			   struct sk_buff *clone, unsigned int type)
+			   struct sk_buff *skb, struct sk_buff **clone)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 
-	if (ocelot->ptp && (skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
-	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
-		ocelot_port_add_txtstamp_skb(ocelot, port, clone);
+	if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
+		*clone = skb_clone_sk(skb);
+		if (!(*clone))
+			return false;
+
+		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
 		return true;
 	}
 
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 1b90570b257b..6a1f854a8c33 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -436,7 +436,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
  * callback, where we will timestamp it synchronously.
  */
 bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
-			   struct sk_buff *skb, unsigned int type)
+			   struct sk_buff *skb, struct sk_buff **clone)
 {
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_port *sp = &priv->ports[port];
@@ -444,6 +444,10 @@ bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
 	if (!sp->hwts_tx_en)
 		return false;
 
+	*clone = skb_clone_sk(skb);
+	if (!(*clone))
+		return false;
+
 	return true;
 }
 
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
index 3daa33e98e77..ab80b73219cb 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.h
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -105,7 +105,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
 			   struct sk_buff *skb, unsigned int type);
 
 bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
-			   struct sk_buff *skb, unsigned int type);
+			   struct sk_buff *skb, struct sk_buff **clone);
 
 int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr);
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 1259b0f40684..c8415c324e27 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -734,7 +734,7 @@ struct dsa_switch_ops {
 	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
 				     struct ifreq *ifr);
 	bool	(*port_txtstamp)(struct dsa_switch *ds, int port,
-				 struct sk_buff *clone, unsigned int type);
+				 struct sk_buff *skb, struct sk_buff **clone);
 	bool	(*port_rxtstamp)(struct dsa_switch *ds, int port,
 				 struct sk_buff *skb, unsigned int type);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9300cb66e500..5b746a903ef4 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -19,7 +19,6 @@
 #include <linux/if_bridge.h>
 #include <linux/if_hsr.h>
 #include <linux/netpoll.h>
-#include <linux/ptp_classify.h>
 
 #include "dsa_priv.h"
 
@@ -555,26 +554,19 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
 				 struct sk_buff *skb)
 {
 	struct dsa_switch *ds = p->dp->ds;
-	struct sk_buff *clone;
-	unsigned int type;
+	struct sk_buff *clone = NULL;
 
-	type = ptp_classify_raw(skb);
-	if (type == PTP_CLASS_NONE)
+	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
 		return;
 
 	if (!ds->ops->port_txtstamp)
 		return;
 
-	clone = skb_clone_sk(skb);
-	if (!clone)
+	if (!ds->ops->port_txtstamp(ds, p->dp->index, skb, &clone))
 		return;
 
-	if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type)) {
+	if (clone)
 		DSA_SKB_CB(skb)->clone = clone;
-		return;
-	}
-
-	kfree_skb(clone);
 }
 
 netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev)
@@ -628,9 +620,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	DSA_SKB_CB(skb)->clone = NULL;
 
-	/* Identify PTP protocol packets, clone them, and pass them to the
-	 * switch driver
-	 */
+	/* Handle tx timestamp request if has */
 	dsa_skb_tx_timestamp(p, skb);
 
 	if (dsa_realloc_skb(skb, dev)) {
-- 
2.25.1

Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Vladimir Oltean]

On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote:
> Optimization could be done on dsa_skb_tx_timestamp(), and dsa device
> drivers should adapt to it.
> 
> - Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in
>   port_txtstamp, so that most skbs not requiring tx timestamp just return.

Agree that this is a trivial performance optimization with no downside
that we should be making.

> - No longer to identify PTP packets, and limit tx timestamping only for PTP
>   packets. If device driver likes, let device driver do.

Agree that DSA has a way too heavy hand in imposing upon the driver
which packets should be timestampable and which ones shouldn't.

For example, I have a latency measurement tool called isochron which is
based on hardware timestamping of non-PTP packets (in order to not
disturb the PTP state machines):
https://github.com/vladimiroltean/tsn-scripts

I can't use it on DSA interfaces, for rather artificial reasons.

> - It is a waste to clone skb directly in dsa_skb_tx_timestamp().
>   For one-step timestamping, a clone is not needed. For any failure of
>   port_txtstamp (this may usually happen), the skb clone has to be freed.
>   So put skb cloning into port_txtstamp where it really needs.

Mostly agree. For two-step timestamping, it is an operation which all
drivers need to do, so it is in the common potion. If we want to support
one-step, we need to avoid cloning the PTP packets.

> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
>  Documentation/networking/timestamping.rst     |  7 +++++--
>  .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 20 ++++++++++++------
>  .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  2 +-
>  drivers/net/dsa/mv88e6xxx/hwtstamp.c          | 21 +++++++++++++------
>  drivers/net/dsa/mv88e6xxx/hwtstamp.h          |  6 +++---
>  drivers/net/dsa/ocelot/felix.c                | 11 ++++++----
>  drivers/net/dsa/sja1105/sja1105_ptp.c         |  6 +++++-
>  drivers/net/dsa/sja1105/sja1105_ptp.h         |  2 +-
>  include/net/dsa.h                             |  2 +-
>  net/dsa/slave.c                               | 20 +++++-------------
>  10 files changed, 57 insertions(+), 40 deletions(-)
> 
> diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst
> index f682e88fa87e..7f04a699a5d1 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -635,8 +635,8 @@ in generic code: a BPF classifier (``ptp_classify_raw``) is used to identify
>  PTP event messages (any other packets, including PTP general messages, are not
>  timestamped), and provides two hooks to drivers:
>  
> -- ``.port_txtstamp()``: The driver is passed a clone of the timestampable skb
> -  to be transmitted, before actually transmitting it. Typically, a switch will
> +- ``.port_txtstamp()``: A clone of the timestampable skb to be transmitted
> +  is needed, before actually transmitting it. Typically, a switch will
>    have a PTP TX timestamp register (or sometimes a FIFO) where the timestamp
>    becomes available. There may be an IRQ that is raised upon this timestamp's
>    availability, or the driver might have to poll after invoking
> @@ -645,6 +645,9 @@ timestamped), and provides two hooks to drivers:
>    later use (when the timestamp becomes available). Each skb is annotated with
>    a pointer to its clone, in ``DSA_SKB_CB(skb)->clone``, to ease the driver's
>    job of keeping track of which clone belongs to which skb.
> +  But one-step timestamping request is handled differently with above two-step
> +  timestamping. The skb clone is no longer needed since hardware will insert
> +  TX time information on packet during egress.

Bonus points for updating the documentation, but I don't quite like the
end result. Please feel free to restructure more, in order to have a
clearer and more coherent explanation.

Also, this paragraph from right above is no longer true:

	In code, DSA provides for most of the infrastructure for timestamping already,
	in generic code: a BPF classifier (``ptp_classify_raw``) is used to identify
	PTP event messages (any other packets, including PTP general messages, are not
	timestamped), and provides two hooks to drivers:

It's nothing like that anymore. It's more of a passthrough now with your
changes, the BPF classifier is not run by the DSA core but optionally by
individual taggers.

Here is my attempt of rewriting this documentation paragraph, feel free
to take which parts you consider relevant:

-----------------------------[cut here]-----------------------------

In the generic layer, DSA provides the following infrastructure for PTP
timestamping:

- ``.port_txtstamp()``: a hook called prior to the transmission of
  packets with a hardware TX timestamping request from user space.
  This is required for two-step timestamping, since the hardware
  timestamp becomes available after the actual MAC transmission, so the
  driver must be prepared to correlate the timestamp with the original
  packet so that it can re-enqueue the packet back into the socket's
  error queue. To save the packet for when the timestamp becomes
  available, the driver can call ``skb_clone_sk`` and save the resulting
  clone in ``DSA_SKB_CB(skb)->clone``. Typically, a switch will have a
  PTP TX timestamp register (or sometimes a FIFO) where the timestamp
  becomes available. In case of a FIFO, the hardware might store
  key-value pairs of PTP sequence ID/message type/domain number and the
  actual timestamp. To perform the correlation correctly between the
  packets in a queue waiting for timestamping and the actual timestamps,
  drivers can use a BPF classifier (``ptp_classify_raw``) to identify
  the PTP transport type, and ``ptp_parse_header`` to interpret the PTP
  header fields. There may be an IRQ that is raised upon this
  timestamp's availability, or the driver might have to poll after
  invoking ``dev_queue_xmit()`` towards the host interface.
  One-step TX timestamping do not require packet cloning, since there is
  no follow-up message required by the PTP protocol (because the
  TX timestamp is embedded into the packet by the MAC), and therefore
  user space does not expect the packet annotated with the TX timestamp
  to be re-enqueued into its socket's error queue.

- ``.port_rxtstamp()``: On RX, the BPF classifier is run by DSA to
  identify PTP event messages (any other packets, including PTP general
  messages, are not timestamped). The original (and only) timestampable
  skb is provided to the driver, for it to annotate it with a timestamp,
  if that is immediately available, or defer to later. On reception,
  timestamps might either be available in-band (through metadata in the
  DSA header, or attached in other ways to the packet), or out-of-band
  (through another RX timestamping FIFO). Deferral on RX is typically
  necessary when retrieving the timestamp needs a sleepable context. In
  that case, it is the responsibility of the DSA driver to call
  ``netif_rx_ni()`` on the freshly timestamped skb.

-----------------------------[cut here]-----------------------------

>  
>  - ``.port_rxtstamp()``: The original (and only) timestampable skb is provided
>    to the driver, for it to annotate it with a timestamp, if that is immediately
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> index 69dd9a2e8bb6..2ff4b7c08b72 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> @@ -374,31 +374,39 @@ long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
>  }
>  
>  bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
> -			     struct sk_buff *clone, unsigned int type)
> +			     struct sk_buff *skb, struct sk_buff **clone)
>  {
>  	struct hellcreek *hellcreek = ds->priv;
>  	struct hellcreek_port_hwtstamp *ps;
>  	struct ptp_header *hdr;
> +	unsigned int type;
>  
>  	ps = &hellcreek->ports[port].port_hwtstamp;
>  
> -	/* Check if the driver is expected to do HW timestamping */
> -	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> +	type = ptp_classify_raw(skb);
> +	if (type == PTP_CLASS_NONE)
>  		return false;
>  
>  	/* Make sure the message is a PTP message that needs to be timestamped
>  	 * and the interaction with the HW timestamping is enabled. If not, stop
>  	 * here
>  	 */
> -	hdr = hellcreek_should_tstamp(hellcreek, port, clone, type);
> +	hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
>  	if (!hdr)
>  		return false;
>  
> +	*clone = skb_clone_sk(skb);
> +	if (!(*clone))
> +		return false;
> +
>  	if (test_and_set_bit_lock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
> -				  &ps->state))
> +				  &ps->state)) {
> +		kfree_skb(*clone);
> +		*clone = NULL;
>  		return false;
> +	}
>  
> -	ps->tx_skb = clone;
> +	ps->tx_skb = *clone;
>  
>  	/* store the number of ticks occurred since system start-up till this
>  	 * moment
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> index c0745ffa1ebb..58cc96642076 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> @@ -45,7 +45,7 @@ int hellcreek_port_hwtstamp_get(struct dsa_switch *ds, int port,
>  bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
>  			     struct sk_buff *clone, unsigned int type);
>  bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
> -			     struct sk_buff *clone, unsigned int type);
> +			     struct sk_buff *skb, struct sk_buff **clone);
>  
>  int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
>  			  struct ethtool_ts_info *info);
> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> index 094d17a1d037..280a95962861 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> @@ -469,24 +469,33 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
>  }
>  
>  bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> -			     struct sk_buff *clone, unsigned int type)
> +			     struct sk_buff *skb, struct sk_buff **clone)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
>  	struct ptp_header *hdr;
> +	unsigned int type;
>  
> -	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> -		return false;
> +	type = ptp_classify_raw(skb);
> +	if (type == PTP_CLASS_NONE)
> +		return false
>  
> -	hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
> +	hdr = mv88e6xxx_should_tstamp(chip, port, skb, type);
>  	if (!hdr)
>  		return false;
>  
> +	*clone = skb_clone_sk(skb);
> +	if (!(*clone))
> +		return false;
> +
>  	if (test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
> -				  &ps->state))
> +				  &ps->state)) {
> +		kfree_skb(*clone);
> +		*clone = NULL;
>  		return false;
> +	}
>  
> -	ps->tx_skb = clone;
> +	ps->tx_skb = *clone;
>  	ps->tx_tstamp_start = jiffies;
>  	ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
>  
> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> index 9da9f197ba02..da2b253334d0 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> @@ -118,7 +118,7 @@ int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds, int port,
>  bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
>  			     struct sk_buff *clone, unsigned int type);
>  bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> -			     struct sk_buff *clone, unsigned int type);
> +			     struct sk_buff *skb, struct sk_buff **clone);
>  
>  int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
>  			  struct ethtool_ts_info *info);
> @@ -152,8 +152,8 @@ static inline bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
>  }
>  
>  static inline bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> -					   struct sk_buff *clone,
> -					   unsigned int type)
> +					   struct sk_buff *skb,
> +					   struct sk_buff **clone)
>  {
>  	return false;
>  }
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 6b5442be0230..cdec2f5e271c 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -1396,14 +1396,17 @@ static bool felix_rxtstamp(struct dsa_switch *ds, int port,
>  }
>  
>  static bool felix_txtstamp(struct dsa_switch *ds, int port,
> -			   struct sk_buff *clone, unsigned int type)
> +			   struct sk_buff *skb, struct sk_buff **clone)
>  {
>  	struct ocelot *ocelot = ds->priv;
>  	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  
> -	if (ocelot->ptp && (skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
> -	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> -		ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> +	if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> +		*clone = skb_clone_sk(skb);
> +		if (!(*clone))
> +			return false;
> +
> +		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
>  		return true;
>  	}
>  
> diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
> index 1b90570b257b..6a1f854a8c33 100644
> --- a/drivers/net/dsa/sja1105/sja1105_ptp.c
> +++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
> @@ -436,7 +436,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
>   * callback, where we will timestamp it synchronously.
>   */
>  bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
> -			   struct sk_buff *skb, unsigned int type)
> +			   struct sk_buff *skb, struct sk_buff **clone)
>  {
>  	struct sja1105_private *priv = ds->priv;
>  	struct sja1105_port *sp = &priv->ports[port];
> @@ -444,6 +444,10 @@ bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
>  	if (!sp->hwts_tx_en)
>  		return false;
>  
> +	*clone = skb_clone_sk(skb);
> +	if (!(*clone))
> +		return false;
> +
>  	return true;
>  }
>  
> diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
> index 3daa33e98e77..ab80b73219cb 100644
> --- a/drivers/net/dsa/sja1105/sja1105_ptp.h
> +++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
> @@ -105,7 +105,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
>  			   struct sk_buff *skb, unsigned int type);
>  
>  bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
> -			   struct sk_buff *skb, unsigned int type);
> +			   struct sk_buff *skb, struct sk_buff **clone);
>  
>  int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr);
>  
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 1259b0f40684..c8415c324e27 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -734,7 +734,7 @@ struct dsa_switch_ops {
>  	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
>  				     struct ifreq *ifr);
>  	bool	(*port_txtstamp)(struct dsa_switch *ds, int port,
> -				 struct sk_buff *clone, unsigned int type);
> +				 struct sk_buff *skb, struct sk_buff **clone);

How about not passing "clone" back to DSA as an argument by reference,
but instead require the driver to populate DSA_SKB_CB(skb)->clone if it
needs to do so?

Also, how about changing the return type to void? Returning true or
false makes no difference.

>  	bool	(*port_rxtstamp)(struct dsa_switch *ds, int port,
>  				 struct sk_buff *skb, unsigned int type);
>  
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 9300cb66e500..5b746a903ef4 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -19,7 +19,6 @@
>  #include <linux/if_bridge.h>
>  #include <linux/if_hsr.h>
>  #include <linux/netpoll.h>
> -#include <linux/ptp_classify.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -555,26 +554,19 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
>  				 struct sk_buff *skb)
>  {
>  	struct dsa_switch *ds = p->dp->ds;
> -	struct sk_buff *clone;
> -	unsigned int type;
> +	struct sk_buff *clone = NULL;
>  
> -	type = ptp_classify_raw(skb);
> -	if (type == PTP_CLASS_NONE)
> +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>  		return;
>  
>  	if (!ds->ops->port_txtstamp)
>  		return;
>  
> -	clone = skb_clone_sk(skb);
> -	if (!clone)
> +	if (!ds->ops->port_txtstamp(ds, p->dp->index, skb, &clone))
>  		return;
>  
> -	if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type)) {
> +	if (clone)
>  		DSA_SKB_CB(skb)->clone = clone;
> -		return;
> -	}
> -
> -	kfree_skb(clone);
>  }
>  
>  netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev)
> @@ -628,9 +620,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	DSA_SKB_CB(skb)->clone = NULL;
>  
> -	/* Identify PTP protocol packets, clone them, and pass them to the
> -	 * switch driver
> -	 */
> +	/* Handle tx timestamp request if has */

s/if has/if any/

>  	dsa_skb_tx_timestamp(p, skb);
>  
>  	if (dsa_realloc_skb(skb, dev)) {
> -- 
> 2.25.1
> 

Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Richard Cochran]

On Sun, Apr 18, 2021 at 12:18:42PM +0300, Vladimir Oltean wrote:
> 
> How about not passing "clone" back to DSA as an argument by reference,
> but instead require the driver to populate DSA_SKB_CB(skb)->clone if it
> needs to do so?
> 
> Also, how about changing the return type to void? Returning true or
> false makes no difference.

+1

Thanks,
Richard

RE: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Y.b. Lu]

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: 2021年4月18日 23:11
> To: Vladimir Oltean <olteanv@gmail.com>
> Cc: Y.b. Lu <yangbo.lu@nxp.com>; netdev@vger.kernel.org; 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 1/3] net: dsa: optimize tx timestamp request handling
> 
> On Sun, Apr 18, 2021 at 12:18:42PM +0300, Vladimir Oltean wrote:
> >
> > How about not passing "clone" back to DSA as an argument by reference,
> > but instead require the driver to populate DSA_SKB_CB(skb)->clone if
> > it needs to do so?
> >
> > Also, how about changing the return type to void? Returning true or
> > false makes no difference.

Thank you. That's good idea.
And how about letting driver store the skb clone pointer, or not? I copied my comments in patch #3 here,

" 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."

> 
> +1
> 
> Thanks,
> Richard

RE: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Y.b. Lu]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: 2021年4月18日 17:19
> 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 1/3] net: dsa: optimize tx timestamp request handling
> 
> On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote:
> > Optimization could be done on dsa_skb_tx_timestamp(), and dsa device
> > drivers should adapt to it.
> >
> > - Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in
> >   port_txtstamp, so that most skbs not requiring tx timestamp just return.
> 
> Agree that this is a trivial performance optimization with no downside that we
> should be making.
> 
> > - No longer to identify PTP packets, and limit tx timestamping only for PTP
> >   packets. If device driver likes, let device driver do.
> 
> Agree that DSA has a way too heavy hand in imposing upon the driver which
> packets should be timestampable and which ones shouldn't.
> 
> For example, I have a latency measurement tool called isochron which is based
> on hardware timestamping of non-PTP packets (in order to not disturb the PTP
> state machines):
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fvladimiroltean%2Ftsn-scripts&amp;data=04%7C01%7Cyangbo.lu%40
> nxp.com%7C3772f63deb6f4491933208d9024af750%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637543343267914018%7CUnknown%7CTW
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C1000&amp;sdata=deOn03j6biPEUwppNkv%2F9if1u5HIdLEtzz
> AkWW0p1rc%3D&amp;reserved=0
> 
> I can't use it on DSA interfaces, for rather artificial reasons.
> 
> > - It is a waste to clone skb directly in dsa_skb_tx_timestamp().
> >   For one-step timestamping, a clone is not needed. For any failure of
> >   port_txtstamp (this may usually happen), the skb clone has to be freed.
> >   So put skb cloning into port_txtstamp where it really needs.
> 
> Mostly agree. For two-step timestamping, it is an operation which all drivers
> need to do, so it is in the common potion. If we want to support one-step, we
> need to avoid cloning the PTP packets.
> 

Thanks a lot for your ACK.

> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> >  Documentation/networking/timestamping.rst     |  7 +++++--
> >  .../net/dsa/hirschmann/hellcreek_hwtstamp.c   | 20 ++++++++++++------
> >  .../net/dsa/hirschmann/hellcreek_hwtstamp.h   |  2 +-
> >  drivers/net/dsa/mv88e6xxx/hwtstamp.c          | 21
> +++++++++++++------
> >  drivers/net/dsa/mv88e6xxx/hwtstamp.h          |  6 +++---
> >  drivers/net/dsa/ocelot/felix.c                | 11 ++++++----
> >  drivers/net/dsa/sja1105/sja1105_ptp.c         |  6 +++++-
> >  drivers/net/dsa/sja1105/sja1105_ptp.h         |  2 +-
> >  include/net/dsa.h                             |  2 +-
> >  net/dsa/slave.c                               | 20 +++++-------------
> >  10 files changed, 57 insertions(+), 40 deletions(-)
> >
> > diff --git a/Documentation/networking/timestamping.rst
> > b/Documentation/networking/timestamping.rst
> > index f682e88fa87e..7f04a699a5d1 100644
> > --- a/Documentation/networking/timestamping.rst
> > +++ b/Documentation/networking/timestamping.rst
> > @@ -635,8 +635,8 @@ in generic code: a BPF classifier
> > (``ptp_classify_raw``) is used to identify  PTP event messages (any
> > other packets, including PTP general messages, are not  timestamped), and
> provides two hooks to drivers:
> >
> > -- ``.port_txtstamp()``: The driver is passed a clone of the
> > timestampable skb
> > -  to be transmitted, before actually transmitting it. Typically, a
> > switch will
> > +- ``.port_txtstamp()``: A clone of the timestampable skb to be
> > +transmitted
> > +  is needed, before actually transmitting it. Typically, a switch
> > +will
> >    have a PTP TX timestamp register (or sometimes a FIFO) where the
> timestamp
> >    becomes available. There may be an IRQ that is raised upon this
> timestamp's
> >    availability, or the driver might have to poll after invoking @@
> > -645,6 +645,9 @@ timestamped), and provides two hooks to drivers:
> >    later use (when the timestamp becomes available). Each skb is annotated
> with
> >    a pointer to its clone, in ``DSA_SKB_CB(skb)->clone``, to ease the driver's
> >    job of keeping track of which clone belongs to which skb.
> > +  But one-step timestamping request is handled differently with above
> > + two-step  timestamping. The skb clone is no longer needed since
> > + hardware will insert  TX time information on packet during egress.
> 
> Bonus points for updating the documentation, but I don't quite like the end
> result. Please feel free to restructure more, in order to have a clearer and more
> coherent explanation.
> 
> Also, this paragraph from right above is no longer true:
> 
> 	In code, DSA provides for most of the infrastructure for timestamping
> already,
> 	in generic code: a BPF classifier (``ptp_classify_raw``) is used to identify
> 	PTP event messages (any other packets, including PTP general messages,
> are not
> 	timestamped), and provides two hooks to drivers:
> 
> It's nothing like that anymore. It's more of a passthrough now with your
> changes, the BPF classifier is not run by the DSA core but optionally by
> individual taggers.
> 
> Here is my attempt of rewriting this documentation paragraph, feel free to
> take which parts you consider relevant:
> 
> -----------------------------[cut here]-----------------------------
> 
> In the generic layer, DSA provides the following infrastructure for PTP
> timestamping:
> 
> - ``.port_txtstamp()``: a hook called prior to the transmission of
>   packets with a hardware TX timestamping request from user space.
>   This is required for two-step timestamping, since the hardware
>   timestamp becomes available after the actual MAC transmission, so the
>   driver must be prepared to correlate the timestamp with the original
>   packet so that it can re-enqueue the packet back into the socket's
>   error queue. To save the packet for when the timestamp becomes
>   available, the driver can call ``skb_clone_sk`` and save the resulting
>   clone in ``DSA_SKB_CB(skb)->clone``. Typically, a switch will have a
>   PTP TX timestamp register (or sometimes a FIFO) where the timestamp
>   becomes available. In case of a FIFO, the hardware might store
>   key-value pairs of PTP sequence ID/message type/domain number and the
>   actual timestamp. To perform the correlation correctly between the
>   packets in a queue waiting for timestamping and the actual timestamps,
>   drivers can use a BPF classifier (``ptp_classify_raw``) to identify
>   the PTP transport type, and ``ptp_parse_header`` to interpret the PTP
>   header fields. There may be an IRQ that is raised upon this
>   timestamp's availability, or the driver might have to poll after
>   invoking ``dev_queue_xmit()`` towards the host interface.
>   One-step TX timestamping do not require packet cloning, since there is
>   no follow-up message required by the PTP protocol (because the
>   TX timestamp is embedded into the packet by the MAC), and therefore
>   user space does not expect the packet annotated with the TX timestamp
>   to be re-enqueued into its socket's error queue.
> 
> - ``.port_rxtstamp()``: On RX, the BPF classifier is run by DSA to
>   identify PTP event messages (any other packets, including PTP general
>   messages, are not timestamped). The original (and only) timestampable
>   skb is provided to the driver, for it to annotate it with a timestamp,
>   if that is immediately available, or defer to later. On reception,
>   timestamps might either be available in-band (through metadata in the
>   DSA header, or attached in other ways to the packet), or out-of-band
>   (through another RX timestamping FIFO). Deferral on RX is typically
>   necessary when retrieving the timestamp needs a sleepable context. In
>   that case, it is the responsibility of the DSA driver to call
>   ``netif_rx_ni()`` on the freshly timestamped skb.
> 
> -----------------------------[cut here]-----------------------------
> 

That's really considerate and accuracy description. I will update with this. (Will adjust if we can drop dsa_skb_cb in dsa core.)
Thanks.

> >
> >  - ``.port_rxtstamp()``: The original (and only) timestampable skb is provided
> >    to the driver, for it to annotate it with a timestamp, if that is
> > immediately diff --git
> > a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> > b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> > index 69dd9a2e8bb6..2ff4b7c08b72 100644
> > --- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> > +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
> > @@ -374,31 +374,39 @@ long hellcreek_hwtstamp_work(struct
> > ptp_clock_info *ptp)  }
> >
> >  bool hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
> > -			     struct sk_buff *clone, unsigned int type)
> > +			     struct sk_buff *skb, struct sk_buff **clone)
> >  {
> >  	struct hellcreek *hellcreek = ds->priv;
> >  	struct hellcreek_port_hwtstamp *ps;
> >  	struct ptp_header *hdr;
> > +	unsigned int type;
> >
> >  	ps = &hellcreek->ports[port].port_hwtstamp;
> >
> > -	/* Check if the driver is expected to do HW timestamping */
> > -	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> > +	type = ptp_classify_raw(skb);
> > +	if (type == PTP_CLASS_NONE)
> >  		return false;
> >
> >  	/* Make sure the message is a PTP message that needs to be
> timestamped
> >  	 * and the interaction with the HW timestamping is enabled. If not, stop
> >  	 * here
> >  	 */
> > -	hdr = hellcreek_should_tstamp(hellcreek, port, clone, type);
> > +	hdr = hellcreek_should_tstamp(hellcreek, port, skb, type);
> >  	if (!hdr)
> >  		return false;
> >
> > +	*clone = skb_clone_sk(skb);
> > +	if (!(*clone))
> > +		return false;
> > +
> >  	if (test_and_set_bit_lock(HELLCREEK_HWTSTAMP_TX_IN_PROGRESS,
> > -				  &ps->state))
> > +				  &ps->state)) {
> > +		kfree_skb(*clone);
> > +		*clone = NULL;
> >  		return false;
> > +	}
> >
> > -	ps->tx_skb = clone;
> > +	ps->tx_skb = *clone;
> >
> >  	/* store the number of ticks occurred since system start-up till this
> >  	 * moment
> > diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> > b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> > index c0745ffa1ebb..58cc96642076 100644
> > --- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> > +++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h
> > @@ -45,7 +45,7 @@ int hellcreek_port_hwtstamp_get(struct dsa_switch
> > *ds, int port,  bool hellcreek_port_rxtstamp(struct dsa_switch *ds, int port,
> >  			     struct sk_buff *clone, unsigned int type);  bool
> > hellcreek_port_txtstamp(struct dsa_switch *ds, int port,
> > -			     struct sk_buff *clone, unsigned int type);
> > +			     struct sk_buff *skb, struct sk_buff **clone);
> >
> >  int hellcreek_get_ts_info(struct dsa_switch *ds, int port,
> >  			  struct ethtool_ts_info *info);
> > diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> > b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> > index 094d17a1d037..280a95962861 100644
> > --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> > +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> > @@ -469,24 +469,33 @@ long mv88e6xxx_hwtstamp_work(struct
> > ptp_clock_info *ptp)  }
> >
> >  bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> > -			     struct sk_buff *clone, unsigned int type)
> > +			     struct sk_buff *skb, struct sk_buff **clone)
> >  {
> >  	struct mv88e6xxx_chip *chip = ds->priv;
> >  	struct mv88e6xxx_port_hwtstamp *ps = &chip->port_hwtstamp[port];
> >  	struct ptp_header *hdr;
> > +	unsigned int type;
> >
> > -	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> > -		return false;
> > +	type = ptp_classify_raw(skb);
> > +	if (type == PTP_CLASS_NONE)
> > +		return false
> >
> > -	hdr = mv88e6xxx_should_tstamp(chip, port, clone, type);
> > +	hdr = mv88e6xxx_should_tstamp(chip, port, skb, type);
> >  	if (!hdr)
> >  		return false;
> >
> > +	*clone = skb_clone_sk(skb);
> > +	if (!(*clone))
> > +		return false;
> > +
> >  	if (test_and_set_bit_lock(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS,
> > -				  &ps->state))
> > +				  &ps->state)) {
> > +		kfree_skb(*clone);
> > +		*clone = NULL;
> >  		return false;
> > +	}
> >
> > -	ps->tx_skb = clone;
> > +	ps->tx_skb = *clone;
> >  	ps->tx_tstamp_start = jiffies;
> >  	ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> > b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> > index 9da9f197ba02..da2b253334d0 100644
> > --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> > +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
> > @@ -118,7 +118,7 @@ int mv88e6xxx_port_hwtstamp_get(struct
> dsa_switch
> > *ds, int port,  bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int
> port,
> >  			     struct sk_buff *clone, unsigned int type);  bool
> > mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> > -			     struct sk_buff *clone, unsigned int type);
> > +			     struct sk_buff *skb, struct sk_buff **clone);
> >
> >  int mv88e6xxx_get_ts_info(struct dsa_switch *ds, int port,
> >  			  struct ethtool_ts_info *info);
> > @@ -152,8 +152,8 @@ static inline bool mv88e6xxx_port_rxtstamp(struct
> > dsa_switch *ds, int port,  }
> >
> >  static inline bool mv88e6xxx_port_txtstamp(struct dsa_switch *ds, int port,
> > -					   struct sk_buff *clone,
> > -					   unsigned int type)
> > +					   struct sk_buff *skb,
> > +					   struct sk_buff **clone)
> >  {
> >  	return false;
> >  }
> > diff --git a/drivers/net/dsa/ocelot/felix.c
> > b/drivers/net/dsa/ocelot/felix.c index 6b5442be0230..cdec2f5e271c
> > 100644
> > --- a/drivers/net/dsa/ocelot/felix.c
> > +++ b/drivers/net/dsa/ocelot/felix.c
> > @@ -1396,14 +1396,17 @@ static bool felix_rxtstamp(struct dsa_switch
> > *ds, int port,  }
> >
> >  static bool felix_txtstamp(struct dsa_switch *ds, int port,
> > -			   struct sk_buff *clone, unsigned int type)
> > +			   struct sk_buff *skb, struct sk_buff **clone)
> >  {
> >  	struct ocelot *ocelot = ds->priv;
> >  	struct ocelot_port *ocelot_port = ocelot->ports[port];
> >
> > -	if (ocelot->ptp && (skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP)
> &&
> > -	    ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> > -		ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> > +	if (ocelot->ptp && ocelot_port->ptp_cmd ==
> IFH_REW_OP_TWO_STEP_PTP) {
> > +		*clone = skb_clone_sk(skb);
> > +		if (!(*clone))
> > +			return false;
> > +
> > +		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
> >  		return true;
> >  	}
> >
> > diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c
> > b/drivers/net/dsa/sja1105/sja1105_ptp.c
> > index 1b90570b257b..6a1f854a8c33 100644
> > --- a/drivers/net/dsa/sja1105/sja1105_ptp.c
> > +++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
> > @@ -436,7 +436,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds,
> int port,
> >   * callback, where we will timestamp it synchronously.
> >   */
> >  bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
> > -			   struct sk_buff *skb, unsigned int type)
> > +			   struct sk_buff *skb, struct sk_buff **clone)
> >  {
> >  	struct sja1105_private *priv = ds->priv;
> >  	struct sja1105_port *sp = &priv->ports[port]; @@ -444,6 +444,10 @@
> > bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
> >  	if (!sp->hwts_tx_en)
> >  		return false;
> >
> > +	*clone = skb_clone_sk(skb);
> > +	if (!(*clone))
> > +		return false;
> > +
> >  	return true;
> >  }
> >
> > diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h
> > b/drivers/net/dsa/sja1105/sja1105_ptp.h
> > index 3daa33e98e77..ab80b73219cb 100644
> > --- a/drivers/net/dsa/sja1105/sja1105_ptp.h
> > +++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
> > @@ -105,7 +105,7 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds,
> int port,
> >  			   struct sk_buff *skb, unsigned int type);
> >
> >  bool sja1105_port_txtstamp(struct dsa_switch *ds, int port,
> > -			   struct sk_buff *skb, unsigned int type);
> > +			   struct sk_buff *skb, struct sk_buff **clone);
> >
> >  int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct
> > ifreq *ifr);
> >
> > diff --git a/include/net/dsa.h b/include/net/dsa.h index
> > 1259b0f40684..c8415c324e27 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -734,7 +734,7 @@ struct dsa_switch_ops {
> >  	int	(*port_hwtstamp_set)(struct dsa_switch *ds, int port,
> >  				     struct ifreq *ifr);
> >  	bool	(*port_txtstamp)(struct dsa_switch *ds, int port,
> > -				 struct sk_buff *clone, unsigned int type);
> > +				 struct sk_buff *skb, struct sk_buff **clone);
> 
> How about not passing "clone" back to DSA as an argument by reference, but
> instead require the driver to populate DSA_SKB_CB(skb)->clone if it needs to do
> so?
> 
> Also, how about changing the return type to void? Returning true or false
> makes no difference.
> 
> >  	bool	(*port_rxtstamp)(struct dsa_switch *ds, int port,
> >  				 struct sk_buff *skb, unsigned int type);
> >
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c index
> > 9300cb66e500..5b746a903ef4 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -19,7 +19,6 @@
> >  #include <linux/if_bridge.h>
> >  #include <linux/if_hsr.h>
> >  #include <linux/netpoll.h>
> > -#include <linux/ptp_classify.h>
> >
> >  #include "dsa_priv.h"
> >
> > @@ -555,26 +554,19 @@ static void dsa_skb_tx_timestamp(struct
> dsa_slave_priv *p,
> >  				 struct sk_buff *skb)
> >  {
> >  	struct dsa_switch *ds = p->dp->ds;
> > -	struct sk_buff *clone;
> > -	unsigned int type;
> > +	struct sk_buff *clone = NULL;
> >
> > -	type = ptp_classify_raw(skb);
> > -	if (type == PTP_CLASS_NONE)
> > +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> >  		return;
> >
> >  	if (!ds->ops->port_txtstamp)
> >  		return;
> >
> > -	clone = skb_clone_sk(skb);
> > -	if (!clone)
> > +	if (!ds->ops->port_txtstamp(ds, p->dp->index, skb, &clone))
> >  		return;
> >
> > -	if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type)) {
> > +	if (clone)
> >  		DSA_SKB_CB(skb)->clone = clone;
> > -		return;
> > -	}
> > -
> > -	kfree_skb(clone);
> >  }
> >
> >  netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device
> > *dev) @@ -628,9 +620,7 @@ static netdev_tx_t dsa_slave_xmit(struct
> > sk_buff *skb, struct net_device *dev)
> >
> >  	DSA_SKB_CB(skb)->clone = NULL;
> >
> > -	/* Identify PTP protocol packets, clone them, and pass them to the
> > -	 * switch driver
> > -	 */
> > +	/* Handle tx timestamp request if has */
> 
> s/if has/if any/

Sorry. Will fix.

> 
> >  	dsa_skb_tx_timestamp(p, skb);
> >
> >  	if (dsa_realloc_skb(skb, dev)) {
> > --
> > 2.25.1
> >

Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Richard Cochran]

On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote:
> Optimization could be done on dsa_skb_tx_timestamp(), and dsa device
> drivers should adapt to it.
> 
> - Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in
>   port_txtstamp, so that most skbs not requiring tx timestamp just return.
> 
> - No longer to identify PTP packets, and limit tx timestamping only for PTP
>   packets. If device driver likes, let device driver do.
> 
> - It is a waste to clone skb directly in dsa_skb_tx_timestamp().
>   For one-step timestamping, a clone is not needed. For any failure of
>   port_txtstamp (this may usually happen), the skb clone has to be freed.
>   So put skb cloning into port_txtstamp where it really needs.

This patch changes three things ^^^ at once.  These are AFAICT
independent changes, and so please break this one patch into three for
easier review.

Thanks,
Richard

RE: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Y.b. Lu]

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: 2021年4月18日 23:06
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; 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 1/3] net: dsa: optimize tx timestamp request handling
> 
> On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote:
> > Optimization could be done on dsa_skb_tx_timestamp(), and dsa device
> > drivers should adapt to it.
> >
> > - Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in
> >   port_txtstamp, so that most skbs not requiring tx timestamp just return.
> >
> > - No longer to identify PTP packets, and limit tx timestamping only for PTP
> >   packets. If device driver likes, let device driver do.
> >
> > - It is a waste to clone skb directly in dsa_skb_tx_timestamp().
> >   For one-step timestamping, a clone is not needed. For any failure of
> >   port_txtstamp (this may usually happen), the skb clone has to be freed.
> >   So put skb cloning into port_txtstamp where it really needs.
> 
> This patch changes three things ^^^ at once.  These are AFAICT independent
> changes, and so please break this one patch into three for easier review.

Will split it in next version. Thank you.

> 
> Thanks,
> Richard

Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Richard Cochran]

On Fri, Apr 16, 2021 at 08:36:53PM +0800, Yangbo Lu wrote:
> @@ -628,9 +620,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	DSA_SKB_CB(skb)->clone = NULL;
>  
> -	/* Identify PTP protocol packets, clone them, and pass them to the
> -	 * switch driver
> -	 */
> +	/* Handle tx timestamp request if has */

"if has" what?

>  	dsa_skb_tx_timestamp(p, skb);
>  
>  	if (dsa_realloc_skb(skb, dev)) {
> -- 
> 2.25.1
> 

Thanks,
Richard

Re: [net-next 1/3] net: dsa: optimize tx timestamp request handling

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Fri Apr 16 2021, Yangbo Lu wrote:
> Optimization could be done on dsa_skb_tx_timestamp(), and dsa device
> drivers should adapt to it.
>
> - Check SKBTX_HW_TSTAMP request flag at the very beginning, instead of in
>   port_txtstamp, so that most skbs not requiring tx timestamp just return.
>
> - No longer to identify PTP packets, and limit tx timestamping only for PTP
>   packets. If device driver likes, let device driver do.
>
> - It is a waste to clone skb directly in dsa_skb_tx_timestamp().
>   For one-step timestamping, a clone is not needed. For any failure of
>   port_txtstamp (this may usually happen), the skb clone has to be freed.
>   So put skb cloning into port_txtstamp where it really needs.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

PTP still works.

Tested-by: Kurt Kanzenbach <kurt@linutronix.de> # hellcreek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[net-next 2/3] net: mscc: ocelot: convert to ocelot_port_txtstamp_request()

[Yangbo Lu]

Convert to a common ocelot_port_txtstamp_request() for TX timestamp
request handling.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         | 14 +++++---------
 drivers/net/ethernet/mscc/ocelot.c     | 24 +++++++++++++++++++++---
 drivers/net/ethernet/mscc/ocelot_net.c | 18 +++++++-----------
 include/soc/mscc/ocelot.h              |  5 +++--
 4 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index cdec2f5e271c..5f2cf0f31253 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1399,18 +1399,14 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
 			   struct sk_buff *skb, struct sk_buff **clone)
 {
 	struct ocelot *ocelot = ds->priv;
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
 
-	if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
-		*clone = skb_clone_sk(skb);
-		if (!(*clone))
-			return false;
+	if (!ocelot->ptp)
+		return false;
 
-		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
-		return true;
-	}
+	if (ocelot_port_txtstamp_request(ocelot, port, skb, clone))
+		return false;
 
-	return false;
+	return true;
 }
 
 static int felix_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 8d06ffaf318a..541d3b4076be 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -530,8 +530,8 @@ void ocelot_port_disable(struct ocelot *ocelot, int port)
 }
 EXPORT_SYMBOL(ocelot_port_disable);
 
-void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
-				  struct sk_buff *clone)
+static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
+					 struct sk_buff *clone)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 
@@ -545,7 +545,25 @@ void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
 
 	spin_unlock(&ocelot_port->ts_id_lock);
 }
-EXPORT_SYMBOL(ocelot_port_add_txtstamp_skb);
+
+int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
+				 struct sk_buff *skb,
+				 struct sk_buff **clone)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	u8 ptp_cmd = ocelot_port->ptp_cmd;
+
+	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);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_port_txtstamp_request);
 
 static void ocelot_get_hwtimestamp(struct ocelot *ocelot,
 				   struct timespec64 *ts)
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 36f32a4d9b0f..8293152a6dc1 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -507,19 +507,15 @@ static netdev_tx_t ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Check if timestamping is needed */
 	if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
-		rew_op = ocelot_port->ptp_cmd;
+		struct sk_buff *clone;
 
-		if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
-			struct sk_buff *clone;
-
-			clone = skb_clone_sk(skb);
-			if (!clone) {
-				kfree_skb(skb);
-				return NETDEV_TX_OK;
-			}
-
-			ocelot_port_add_txtstamp_skb(ocelot, port, clone);
+		if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone)) {
+			kfree_skb(skb);
+			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;
 		}
 	}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 68cdc7ceaf4d..9cdaf1d9199f 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -820,8 +820,9 @@ 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);
-void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
-				  struct sk_buff *clone);
+int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,
+				 struct sk_buff *skb,
+				 struct sk_buff **clone);
 void ocelot_get_txtstamp(struct ocelot *ocelot);
 void ocelot_port_set_maxlen(struct ocelot *ocelot, int port, size_t sdu);
 int ocelot_get_max_mtu(struct ocelot *ocelot, int port);
-- 
2.25.1

Re: [net-next 2/3] net: mscc: ocelot: convert to ocelot_port_txtstamp_request()

[Vladimir Oltean]

On Fri, Apr 16, 2021 at 08:36:54PM +0800, Yangbo Lu wrote:
> Convert to a common ocelot_port_txtstamp_request() for TX timestamp
> request handling.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
>  drivers/net/dsa/ocelot/felix.c         | 14 +++++---------
>  drivers/net/ethernet/mscc/ocelot.c     | 24 +++++++++++++++++++++---
>  drivers/net/ethernet/mscc/ocelot_net.c | 18 +++++++-----------
>  include/soc/mscc/ocelot.h              |  5 +++--
>  4 files changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index cdec2f5e271c..5f2cf0f31253 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -1399,18 +1399,14 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
>  			   struct sk_buff *skb, struct sk_buff **clone)
>  {
>  	struct ocelot *ocelot = ds->priv;
> -	struct ocelot_port *ocelot_port = ocelot->ports[port];
>  
> -	if (ocelot->ptp && ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> -		*clone = skb_clone_sk(skb);
> -		if (!(*clone))
> -			return false;
> +	if (!ocelot->ptp)
> +		return false;
>  
> -		ocelot_port_add_txtstamp_skb(ocelot, port, *clone);
> -		return true;
> -	}
> +	if (ocelot_port_txtstamp_request(ocelot, port, skb, clone))
> +		return false;
>  
> -	return false;
> +	return true;

Considering the changes you'll have to make to patch 1 (changing the
return value and populating DSA_SKB_CB(skb)->clone at the end of this
function:

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[Yangbo Lu]

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(-)

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));
-- 
2.25.1

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

[Vladimir Oltean]

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
> 

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

[Y.b. Lu]

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
> >

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

[Vladimir Oltean]

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.