Message-ID: <20211109111213.6vo5swdhxjvgmyjt@skbuf>
Date:   Tue, 9 Nov 2021 13:12:13 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Martin Kaistra <martin.kaistra@...utronix.de>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 6/7] net: dsa: b53: Add logic for TX timestamping

On Tue, Nov 09, 2021 at 10:50:08AM +0100, Martin Kaistra wrote:
> In order to get the switch to generate a timestamp for a transmitted
> packet, we need to set the TS bit in the BRCM tag. The switch will then
> create a status frame, which gets send back to the cpu.
> In b53_port_txtstamp() we put the skb into a waiting position.
> 
> When a status frame is received, we extract the timestamp and put the time
> according to our timecounter into the waiting skb. When
> TX_TSTAMP_TIMEOUT is reached and we have no means to correctly get back
> a full timestamp, we cancel the process.
> 
> As the status frame doesn't contain a reference to the original packet,
> only one packet with timestamp request can be sent at a time.
> 
> Signed-off-by: Martin Kaistra <martin.kaistra@...utronix.de>
> ---
>  drivers/net/dsa/b53/b53_common.c |  1 +
>  drivers/net/dsa/b53/b53_ptp.c    | 56 ++++++++++++++++++++++++++++++++
>  drivers/net/dsa/b53/b53_ptp.h    |  8 +++++
>  net/dsa/tag_brcm.c               | 46 ++++++++++++++++++++++++++
>  4 files changed, 111 insertions(+)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index a9408f9cd414..56a9de89b38b 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2301,6 +2301,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
>  	.port_change_mtu	= b53_change_mtu,
>  	.get_ts_info		= b53_get_ts_info,
>  	.port_rxtstamp		= b53_port_rxtstamp,
> +	.port_txtstamp		= b53_port_txtstamp,
>  };
>  
>  struct b53_chip_data {
> diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c
> index f8dd8d484d93..5567135ba8b9 100644
> --- a/drivers/net/dsa/b53/b53_ptp.c
> +++ b/drivers/net/dsa/b53/b53_ptp.c
> @@ -100,14 +100,65 @@ static long b53_hwtstamp_work(struct ptp_clock_info *ptp)
>  {
>  	struct b53_device *dev =
>  		container_of(ptp, struct b53_device, ptp_clock_info);
> +	struct dsa_switch *ds = dev->ds;
> +	int i;
>  
>  	mutex_lock(&dev->ptp_mutex);
>  	timecounter_read(&dev->tc);
>  	mutex_unlock(&dev->ptp_mutex);
>  
> +	for (i = 0; i < ds->num_ports; i++) {
> +		struct b53_port_hwtstamp *ps;
> +
> +		if (!dsa_is_user_port(ds, i))
> +			continue;

dsa_switch_for_each_user_port and replace i with dp->index.
This makes for a more efficient iteration through the port list.

> +
> +		ps = &dev->ports[i].port_hwtstamp;
> +
> +		if (test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state) &&
> +		    time_is_before_jiffies(ps->tx_tstamp_start +
> +					   TX_TSTAMP_TIMEOUT)) {
> +			dev_err(dev->dev,
> +				"Timeout while waiting for Tx timestamp!\n");
> +			dev_kfree_skb_any(ps->tx_skb);
> +			ps->tx_skb = NULL;
> +			clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS,
> +					 &ps->state);
> +		}
> +	}
> +
>  	return B53_PTP_OVERFLOW_PERIOD;
>  }
>  
> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
> +{
> +	struct b53_device *dev = ds->priv;
> +	struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp;
> +	struct sk_buff *clone;
> +	unsigned int type;
> +
> +	type = ptp_classify_raw(skb);
> +
> +	if (type != PTP_CLASS_V2_L2)
> +		return;
> +
> +	if (!test_bit(B53_HWTSTAMP_ENABLED, &ps->state))
> +		return;
> +
> +	clone = skb_clone_sk(skb);
> +	if (!clone)
> +		return;
> +
> +	if (test_and_set_bit_lock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state)) {

Is it ok if you simply don't timestamp a second skb which may be sent
while the first one is in flight, I wonder? What PTP profiles have you
tested with? At just one PTP packet at a time, the switch isn't giving
you a lot. Is it a hardware limitation?

> +		kfree_skb(clone);
> +		return;
> +	}
> +
> +	ps->tx_skb = clone;
> +	ps->tx_tstamp_start = jiffies;
> +}
> +EXPORT_SYMBOL(b53_port_txtstamp);
> +
>  bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
>  		       unsigned int type)
>  {
> @@ -136,6 +187,8 @@ EXPORT_SYMBOL(b53_port_rxtstamp);
>  
>  int b53_ptp_init(struct b53_device *dev)
>  {
> +	struct dsa_port *dp;
> +
>  	mutex_init(&dev->ptp_mutex);
>  
>  	/* Enable BroadSync HD for all ports */
> @@ -191,6 +244,9 @@ int b53_ptp_init(struct b53_device *dev)
>  
>  	ptp_schedule_worker(dev->ptp_clock, 0);
>  
> +	dsa_switch_for_each_port(dp, dev->ds)
> +		dp->priv = &dev->ports[dp->index].port_hwtstamp;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(b53_ptp_init);
> diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h
> index 3b3437870c55..f888f0a2022a 100644
> --- a/drivers/net/dsa/b53/b53_ptp.h
> +++ b/drivers/net/dsa/b53/b53_ptp.h
> @@ -10,6 +10,7 @@
>  #include "b53_priv.h"
>  
>  #define SKB_PTP_TYPE(__skb) (*(unsigned int *)((__skb)->cb))
> +#define TX_TSTAMP_TIMEOUT msecs_to_jiffies(40)
>  
>  #ifdef CONFIG_B53_PTP
>  int b53_ptp_init(struct b53_device *dev);
> @@ -18,6 +19,8 @@ int b53_get_ts_info(struct dsa_switch *ds, int port,
>  		    struct ethtool_ts_info *info);
>  bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,
>  		       unsigned int type);
> +void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
> +
>  #else /* !CONFIG_B53_PTP */
>  
>  static inline int b53_ptp_init(struct b53_device *dev)
> @@ -41,5 +44,10 @@ static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port,
>  	return false;
>  }
>  
> +static inline void b53_port_txtstamp(struct dsa_switch *ds, int port,
> +				     struct sk_buff *skb)
> +{
> +}
> +
>  #endif
>  #endif
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index d611c1073deb..a44ac81fa097 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -8,6 +8,7 @@
>  #include <linux/dsa/brcm.h>
>  #include <linux/etherdevice.h>
>  #include <linux/list.h>
> +#include <linux/ptp_classify.h>
>  #include <linux/slab.h>
>  #include <linux/dsa/b53.h>
>  
> @@ -76,6 +77,7 @@
>  #define BRCM_EG_TC_SHIFT	5
>  #define BRCM_EG_TC_MASK		0x7
>  #define BRCM_EG_PID_MASK	0x1f
> +#define BRCM_EG_T_R		0x20
>  
>  #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM) || \
>  	IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
> @@ -85,6 +87,8 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
>  					unsigned int offset)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	unsigned int type = ptp_classify_raw(skb);
> +	struct b53_port_hwtstamp *ps = dp->priv;
>  	u16 queue = skb_get_queue_mapping(skb);
>  	u8 *brcm_tag;
>  
> @@ -112,7 +116,13 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
>  	 */
>  	brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT) |
>  		       ((queue & BRCM_IG_TC_MASK) << BRCM_IG_TC_SHIFT);
> +
>  	brcm_tag[1] = 0;
> +
> +	if (type == PTP_CLASS_V2_L2 &&
> +	    test_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
> +		brcm_tag[1] = 1 << BRCM_IG_TS_SHIFT;
> +
>  	brcm_tag[2] = 0;
>  	if (dp->index == 8)
>  		brcm_tag[2] = BRCM_IG_DSTMAP2_MASK;
> @@ -126,6 +136,33 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
>  	return skb;
>  }
>  
> +static int set_txtstamp(struct dsa_port *dp,
> +			int port,
> +			u64 ns)
> +{
> +	struct b53_device *b53_dev = dp->ds->priv;
> +	struct b53_port_hwtstamp *ps = dp->priv;
> +	struct skb_shared_hwtstamps shhwtstamps;
> +	struct sk_buff *tmp_skb;
> +
> +	if (!ps->tx_skb)
> +		return 0;
> +
> +	mutex_lock(&b53_dev->ptp_mutex);
> +	ns = timecounter_cyc2time(&b53_dev->tc, ns);
> +	mutex_unlock(&b53_dev->ptp_mutex);

This is called from brcm_tag_rcv_ll which runs in softirq context.
You can't take a sleeping mutex, sorry.
Please test your patches with CONFIG_DEBUG_ATOMIC_SLEEP and friends
(lockdep etc).

> +
> +	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +	shhwtstamps.hwtstamp = ns_to_ktime(ns);
> +	tmp_skb = ps->tx_skb;
> +	ps->tx_skb = NULL;
> +
> +	clear_bit_unlock(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state);
> +	skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
> +
> +	return 0;
> +}
> +
>  /* Frames with this tag have one of these two layouts:
>   * -----------------------------------
>   * | MAC DA | MAC SA | 4b tag | Type | DSA_TAG_PROTO_BRCM
> @@ -143,6 +180,7 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
>  				       unsigned int offset,
>  				       int *tag_len)
>  {
> +	struct dsa_port *dp;
>  	int source_port;
>  	u8 *brcm_tag;
>  	u32 tstamp;
> @@ -177,6 +215,14 @@ static struct sk_buff *brcm_tag_rcv_ll(struct sk_buff *skb,
>  	if (!skb->dev)
>  		return NULL;
>  
> +	/* Check whether this is a status frame */
> +	if (unlikely(*tag_len == 8 && brcm_tag[3] & BRCM_EG_T_R)) {
> +		dp = dsa_slave_to_port(skb->dev);
> +
> +		set_txtstamp(dp, source_port, tstamp);
> +		return NULL;
> +	}
> +
>  	/* Remove Broadcom tag and update checksum */
>  	skb_pull_rcsum(skb, *tag_len);
>  
> -- 
> 2.20.1
> 

Powered by blists - more mailing lists