| Message ID | 20210416123655.42783-4-yangbo.lu@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support ocelot PTP Sync one-step timestamping | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 194 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | Link |
Hi Yangbo,
I love your patch! Yet something to improve:
[auto build test ERROR on 392c36e5be1dee19ffce8c8ba8f07f90f5aa3f7c]
url: https://github.com/0day-ci/linux/commits/Yangbo-Lu/Support-ocelot-PTP-Sync-one-step-timestamping/20210416-203333
base: 392c36e5be1dee19ffce8c8ba8f07f90f5aa3f7c
config: i386-randconfig-r035-20210416 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/7e3d064908a2ecf6287c2eb40e0a36d86434f41c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yangbo-Lu/Support-ocelot-PTP-Sync-one-step-timestamping/20210416-203333
git checkout 7e3d064908a2ecf6287c2eb40e0a36d86434f41c
# save the attached .config to linux build tree
make W=1 W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "ocelot_ptp_rew_op" [net/dsa/tag_ocelot.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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 >
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 > >
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.
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));
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(-)