Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752213AbdI1RkQ (ORCPT ); Thu, 28 Sep 2017 13:40:16 -0400 Received: from mail-qt0-f196.google.com ([209.85.216.196]:45465 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751339AbdI1RkO (ORCPT ); Thu, 28 Sep 2017 13:40:14 -0400 X-Google-Smtp-Source: AOwi7QAf6T2EqVTtVaiEWsIuhJa1HnQpLc0waS1rF8A0eYpjO2PYaRoqFdrhrZf++hZ5qbbV92JYqA== Subject: Re: [PATCH net-next RFC 6/9] net: dsa: forward timestamping callbacks to switch drivers To: Brandon Streiff , netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org, "David S. Miller" , Andrew Lunn , Vivien Didelot , Richard Cochran , Erik Hons References: <1506612341-18061-1-git-send-email-brandon.streiff@ni.com> <1506612341-18061-7-git-send-email-brandon.streiff@ni.com> From: Florian Fainelli Message-ID: Date: Thu, 28 Sep 2017 10:40:05 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1506612341-18061-7-git-send-email-brandon.streiff@ni.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6833 Lines: 204 On 09/28/2017 08:25 AM, Brandon Streiff wrote: > Forward the rx/tx timestamp machinery from the dsa infrastructure to the > switch driver. > > On the rx side, defer delivery of skbs until we have an rx timestamp. > This mimicks the behavior of skb_defer_rx_timestamp. The implementation > does have to thread through the tagging protocol handlers because > it is where that we know which switch and port the skb goes to. > > On the tx side, identify PTP packets, clone them, and pass them to the > underlying switch driver before we transmit. This mimicks the behavior > of skb_tx_timestamp. > > Signed-off-by: Brandon Streiff > --- > include/net/dsa.h | 13 +++++++++++-- > net/dsa/dsa.c | 39 ++++++++++++++++++++++++++++++++++++++- > net/dsa/slave.c | 25 +++++++++++++++++++++++++ > net/dsa/tag_brcm.c | 6 +++++- > net/dsa/tag_dsa.c | 6 +++++- > net/dsa/tag_edsa.c | 6 +++++- > net/dsa/tag_ksz.c | 6 +++++- > net/dsa/tag_lan9303.c | 6 +++++- > net/dsa/tag_mtk.c | 6 +++++- > net/dsa/tag_qca.c | 6 +++++- > net/dsa/tag_trailer.c | 6 +++++- > 11 files changed, 114 insertions(+), 11 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 1163af1..4daf7f7 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -101,11 +101,14 @@ struct dsa_platform_data { > }; > > struct packet_type; > +struct dsa_switch; > > struct dsa_device_ops { > struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev); > struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev, > - struct packet_type *pt); > + struct packet_type *pt, > + struct dsa_switch **src_dev, > + int *src_port); > int (*flow_dissect)(const struct sk_buff *skb, __be16 *proto, > int *offset); > }; > @@ -134,7 +137,9 @@ struct dsa_switch_tree { > /* Copy of tag_ops->rcv for faster access in hot path */ > struct sk_buff * (*rcv)(struct sk_buff *skb, > struct net_device *dev, > - struct packet_type *pt); > + struct packet_type *pt, > + struct dsa_switch **src_dev, > + int *src_port); > > /* > * The switch port to which the CPU is attached. > @@ -449,6 +454,10 @@ struct dsa_switch_ops { > struct ifreq *ifr); > int (*port_hwtstamp_set)(struct dsa_switch *ds, int port, > struct ifreq *ifr); > + void (*port_txtstamp)(struct dsa_switch *ds, int port, > + struct sk_buff *clone, unsigned int type); > + bool (*port_rxtstamp)(struct dsa_switch *ds, int port, > + struct sk_buff *skb, unsigned int type); > }; > > struct dsa_switch_driver { > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index 81c852e..42e7286 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -157,6 +158,37 @@ struct net_device *dsa_dev_to_net_device(struct device *dev) > } > EXPORT_SYMBOL_GPL(dsa_dev_to_net_device); > > +/* Determine if we should defer delivery of skb until we have a rx timestamp. > + * > + * Called from dsa_switch_rcv. For now, this will only work if tagging is > + * enabled on the switch. Normally the MAC driver would retrieve the hardware > + * timestamp when it reads the packet out of the hardware. However in a DSA > + * switch, the DSA driver owning the interface to which the packet is > + * delivered is never notified unless we do so here. > + */ > +static bool dsa_skb_defer_rx_timestamp(struct dsa_switch *ds, int port, > + struct sk_buff *skb) You should not need the port information here because it's already implied from skb->dev which points to the DSA slave network device, see below. > +{ > + unsigned int type; > + > + if (skb_headroom(skb) < ETH_HLEN) > + return false; Are you positive this is necessary? Because we called dst->rcv() we have called eth_type_trans() which already made sure about that > + > + __skb_push(skb, ETH_HLEN); > + > + type = ptp_classify_raw(skb); > + > + __skb_pull(skb, ETH_HLEN); > + > + if (type == PTP_CLASS_NONE) > + return false; > + > + if (likely(ds->ops->port_rxtstamp)) > + return ds->ops->port_rxtstamp(ds, port, skb, type); > + > + return false; > +} Can we also have a fast-path bypass in case time stamping is not supported by the switch so we don't have to even try to classify this packet only to realize we don't have a port_rxtsamp() operation later? You can either gate this with a compile-time option, or use e.g: a static key or something like an early test? > + > static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, > struct packet_type *pt, struct net_device *unused) > { > @@ -164,6 +196,8 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, > struct sk_buff *nskb = NULL; > struct pcpu_sw_netstats *s; > struct dsa_slave_priv *p; > + struct dsa_switch *ds = NULL; > + int source_port; > > if (unlikely(dst == NULL)) { > kfree_skb(skb); > @@ -174,7 +208,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, > if (!skb) > return 0; > > - nskb = dst->rcv(skb, dev, pt); > + nskb = dst->rcv(skb, dev, pt, &ds, &source_port); I don't think this is necessary, what dst->rcv() does is actually properly assign skb->dev to the correct dsa slave network device, which has the information about the port number already in its private context. > if (!nskb) { > kfree_skb(skb); > return 0; > @@ -192,6 +226,9 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, > s->rx_bytes += skb->len; > u64_stats_update_end(&s->syncp); > > + if (dsa_skb_defer_rx_timestamp(ds, source_port, skb)) > + return 0; Can we just propagate an integer return value from dsa_skb_defer_rx_timestamp()? > + > netif_receive_skb(skb); > > return 0; > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 2cf6a83..a278335 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > > #include "dsa_priv.h" > > @@ -407,6 +408,25 @@ static inline netdev_tx_t dsa_slave_netpoll_send_skb(struct net_device *dev, > return NETDEV_TX_OK; > } > > +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; > + > + type = ptp_classify_raw(skb); > + if (type == PTP_CLASS_NONE) > + return; If we don't have a port_txtstamp option, is there even value in classifying this packet? -- Florian