From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:40:59
This is an assorted collection of fixes for issues seen on the NXP
LS1028A switch.
- PTP packet drops due to switch congestion result in catastrophic
damage to the driver's state
- loops are not blocked by STP if using the ocelot-8021q tagger
- driver uses the wrong CPU port when two of them are defined in DT
- module autoloading is broken* with both tagging protocol drivers
(ocelot and ocelot-8021q)
*I did notice that a similar fix but for a different driver did get
applied to "net-next" instead of "net" despite my deliberate targeting
of the branch that goes towards "stable". I don't know why, it is an
issue that is really bothering some people.
https://patchwork.kernel.org/project/netdevbpf/cover/20210922143726.2431036-1-vladimir.oltean@nxp.com/
Changes in v2:
- Stop printing that we aren't going to take TX timestamps if we don't
have TX timestamping anyway, and we are just carrying PTP frames for a
cascaded DSA switch.
- Shorten the deferred xmit kthread name so that it fits the 16
character limit (TASK_COMM_LEN)
Vladimir Oltean (10):
net: mscc: ocelot: make use of all 63 PTP timestamp identifiers
net: mscc: ocelot: avoid overflowing the PTP timestamp FIFO
net: mscc: ocelot: warn when a PTP IRQ is raised for an unknown skb
net: mscc: ocelot: deny TX timestamping of non-PTP packets
net: mscc: ocelot: cross-check the sequence id from the timestamp FIFO
with the skb PTP header
net: dsa: tag_ocelot: break circular dependency with ocelot switch lib
driver
net: dsa: tag_ocelot_8021q: break circular dependency with ocelot
switch lib
net: dsa: felix: purge skb from TX timestamping queue if it cannot be
sent
net: dsa: tag_ocelot_8021q: fix inability to inject STP BPDUs into
BLOCKING ports
net: dsa: felix: break at first CPU port during init and teardown
drivers/net/dsa/ocelot/felix.c | 149 +++++++++++++++++++++++--
drivers/net/dsa/ocelot/felix.h | 1 +
drivers/net/ethernet/mscc/ocelot.c | 103 +++++++++++------
drivers/net/ethernet/mscc/ocelot_net.c | 1 +
include/linux/dsa/ocelot.h | 49 ++++++++
include/soc/mscc/ocelot.h | 55 +--------
include/soc/mscc/ocelot_ptp.h | 3 +
net/dsa/Kconfig | 4 -
net/dsa/tag_ocelot.c | 1 -
net/dsa/tag_ocelot_8021q.c | 40 ++++---
10 files changed, 291 insertions(+), 115 deletions(-)
--
2.25.1
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:06
PTP packets with 2-step TX timestamp requests are matched to packets
based on the egress port number and a 6-bit timestamp identifier.
All PTP timestamps are held in a common FIFO that is 128 entry deep.
This patch ensures that back-to-back timestamping requests cannot exceed
the hardware FIFO capacity. If that happens, simply send the packets
without requesting a TX timestamp to be taken (in the case of felix,
since the DSA API has a void return code in ds->ops->port_txtstamp) or
drop them (in the case of ocelot).
I've moved the ts_id_lock from a per-port basis to a per-switch basis,
because we need separate accounting for both numbers of PTP frames in
flight. And since we need locking to inc/dec the per-switch counter,
that also offers protection for the per-port counter and hence there is
no reason to have a per-port counter anymore.
Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/dsa/ocelot/felix.c | 6 ++++-
drivers/net/ethernet/mscc/ocelot.c | 37 ++++++++++++++++++++++++------
include/soc/mscc/ocelot.h | 5 +++-
include/soc/mscc/ocelot_ptp.h | 1 +
4 files changed, 40 insertions(+), 9 deletions(-)
@@ -569,22 +569,36 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,}EXPORT_SYMBOL_GPL(ocelot_phylink_mac_link_up);-staticvoidocelot_port_add_txtstamp_skb(structocelot*ocelot,intport,-structsk_buff*clone)+staticintocelot_port_add_txtstamp_skb(structocelot*ocelot,intport,+structsk_buff*clone){structocelot_port*ocelot_port=ocelot->ports[port];+unsignedlongflags;++spin_lock_irqsave(&ocelot->ts_id_lock,flags);-spin_lock(&ocelot_port->ts_id_lock);+if(ocelot_port->ptp_skbs_in_flight==OCELOT_MAX_PTP_ID||+ocelot->ptp_skbs_in_flight==OCELOT_PTP_FIFO_SIZE){+spin_unlock_irqrestore(&ocelot->ts_id_lock,flags);+return-EBUSY;+}skb_shinfo(clone)->tx_flags|=SKBTX_IN_PROGRESS;/* Store timestamp ID in OCELOT_SKB_CB(clone)->ts_id */OCELOT_SKB_CB(clone)->ts_id=ocelot_port->ts_id;+ocelot_port->ts_id++;if(ocelot_port->ts_id==OCELOT_MAX_PTP_ID)ocelot_port->ts_id=0;++ocelot_port->ptp_skbs_in_flight++;+ocelot->ptp_skbs_in_flight++;+skb_queue_tail(&ocelot_port->tx_skbs,clone);-spin_unlock(&ocelot_port->ts_id_lock);+spin_unlock_irqrestore(&ocelot->ts_id_lock,flags);++return0;}u32ocelot_ptp_rew_op(structsk_buff*skb)
@@ -633,6 +647,7 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,{structocelot_port*ocelot_port=ocelot->ports[port];u8ptp_cmd=ocelot_port->ptp_cmd;+interr;/* Store ptp_cmd in OCELOT_SKB_CB(skb)->ptp_cmd */if(ptp_cmd==IFH_REW_OP_ORIGIN_PTP){
@@ -650,7 +665,10 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,if(!(*clone))return-ENOMEM;-ocelot_port_add_txtstamp_skb(ocelot,port,*clone);+err=ocelot_port_add_txtstamp_skb(ocelot,port,*clone);+if(err)+returnerr;+OCELOT_SKB_CB(skb)->ptp_cmd=ptp_cmd;}
@@ -709,9 +727,14 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)id=SYS_PTP_STATUS_PTP_MESS_ID_X(val);txport=SYS_PTP_STATUS_PTP_MESS_TXPORT_X(val);-/* Retrieve its associated skb */port=ocelot->ports[txport];+spin_lock(&ocelot->ts_id_lock);+port->ptp_skbs_in_flight--;+ocelot->ptp_skbs_in_flight--;+spin_unlock(&ocelot->ts_id_lock);++/* Retrieve its associated skb */spin_lock_irqsave(&port->tx_skbs.lock,flags);skb_queue_walk_safe(&port->tx_skbs,skb,skb_tmp){
@@ -603,10 +603,10 @@ struct ocelot_port {/* The VLAN ID that will be transmitted as untagged, on egress */structocelot_vlannative_vlan;+unsignedintptp_skbs_in_flight;u8ptp_cmd;structsk_buff_headtx_skbs;u8ts_id;-spinlock_tts_id_lock;phy_interface_tphy_mode;
@@ -680,6 +680,9 @@ struct ocelot {structptp_clock*ptp_clock;structptp_clock_infoptp_info;structhwtstamp_confighwtstamp_config;+unsignedintptp_skbs_in_flight;+/* Protects the 2-step TX timestamp ID logic */+spinlock_tts_id_lock;/* Protects the PTP interface state */structmutexptp_lock;/* Protects the PTP clock */
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:10
As explained here:
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
DSA tagging protocol drivers cannot depend on symbols exported by switch
drivers, because this creates a circular dependency that breaks module
autoloading.
The tag_ocelot.c file depends on the ocelot_ptp_rew_op() function
exported by the common ocelot switch lib. This function looks at
OCELOT_SKB_CB(skb) and computes how to populate the REW_OP field of the
DSA tag, for PTP timestamping (the command: one-step/two-step, and the
TX timestamp identifier).
None of that requires deep insight into the driver, it is quite
stateless, as it only depends upon the skb->cb. So let's make it a
static inline function and put it in include/linux/dsa/ocelot.h, a
file that despite its name is used by the ocelot switch driver for
populating the injection header too - since commit 40d3f295b5fe ("net:
mscc: ocelot: use common tag parsing code with DSA").
With that function declared as static inline, its body is expanded
inside each call site, so the dependency is broken and the DSA tagger
can be built without the switch library, upon which the felix driver
depends.
Fixes: 39e5308b3250 ("net: mscc: ocelot: support PTP Sync one-step timestamping")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/ethernet/mscc/ocelot.c | 17 ------------
drivers/net/ethernet/mscc/ocelot_net.c | 1 +
include/linux/dsa/ocelot.h | 37 ++++++++++++++++++++++++++
include/soc/mscc/ocelot.h | 24 -----------------
net/dsa/Kconfig | 2 --
net/dsa/tag_ocelot.c | 1 -
net/dsa/tag_ocelot_8021q.c | 1 +
7 files changed, 39 insertions(+), 44 deletions(-)
@@ -215,4 +235,21 @@ static inline void ocelot_ifh_set_vid(void *injection, u64 vid)packing(injection,&vid,11,0,OCELOT_TAG_LEN,PACK,0);}+/* Determine the PTP REW_OP to use for injecting the given skb */+staticinlineu32ocelot_ptp_rew_op(structsk_buff*skb)+{+structsk_buff*clone=OCELOT_SKB_CB(skb)->clone;+u8ptp_cmd=OCELOT_SKB_CB(skb)->ptp_cmd;+u32rew_op=0;++if(ptp_cmd==IFH_REW_OP_TWO_STEP_PTP&&clone){+rew_op=ptp_cmd;+rew_op|=OCELOT_SKB_CB(clone)->ts_id<<3;+}elseif(ptp_cmd==IFH_REW_OP_ORIGIN_PTP){+rew_op=ptp_cmd;+}++returnrew_op;+}+#endif
@@ -101,8 +101,6 @@ config NET_DSA_TAG_RTL4_AconfigNET_DSA_TAG_OCELOTtristate"Tag driver for Ocelot family of switches, using NPI port"-depends onMSCC_OCELOT_SWITCH_LIB||\-(MSCC_OCELOT_SWITCH_LIB=n&&COMPILE_TEST)selectPACKINGhelpSayYorMifyouwanttoenableNPItaggingfortheOcelotswitches
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:10
At present, there is a problem when user space bombards a port with PTP
event frames which have TX timestamping requests (or when a tc-taprio
offload is installed on a port, which delays the TX timestamps by a
significant amount of time). The driver will happily roll over the 2-bit
timestamp ID and this will cause incorrect matches between an skb and
the TX timestamp collected from the FIFO.
The Ocelot switches have a 6-bit PTP timestamp identifier, and the value
63 is reserved, so that leaves identifiers 0-62 to be used.
The timestamp identifiers are selected by the REW_OP packet field, and
are actually shared between CPU-injected frames and frames which match a
VCAP IS2 rule that modifies the REW_OP. The hardware supports
partitioning between the two uses of the REW_OP field through the
PTP_ID_LOW and PTP_ID_HIGH registers, and by default reserves the PTP
IDs 0-3 for CPU-injected traffic and the rest for VCAP IS2.
The driver does not use VCAP IS2 to set REW_OP for 2-step timestamping,
and it also writes 0xffffffff to both PTP_ID_HIGH and PTP_ID_LOW in
ocelot_init_timestamp() which makes all timestamp identifiers available
to CPU injection.
Therefore, we can make use of all 63 timestamp identifiers, which should
allow more timestampable packets to be in flight on each port. This is
only part of the solution, more issues will be addressed in future changes.
Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/ethernet/mscc/ocelot.c | 4 +++-
include/soc/mscc/ocelot_ptp.h | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
@@ -579,7 +579,9 @@ static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,skb_shinfo(clone)->tx_flags|=SKBTX_IN_PROGRESS;/* Store timestamp ID in OCELOT_SKB_CB(clone)->ts_id */OCELOT_SKB_CB(clone)->ts_id=ocelot_port->ts_id;-ocelot_port->ts_id=(ocelot_port->ts_id+1)%4;+ocelot_port->ts_id++;+if(ocelot_port->ts_id==OCELOT_MAX_PTP_ID)+ocelot_port->ts_id=0;skb_queue_tail(&ocelot_port->tx_skbs,clone);spin_unlock(&ocelot_port->ts_id_lock);
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:12
Michael reported that when using the "ocelot-8021q" tagging protocol,
the switch driver module must be manually loaded before the tagging
protocol can be loaded/is available.
This appears to be the same problem described here:
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
where due to the fact that DSA tagging protocols make use of symbols
exported by the switch drivers, circular dependencies appear and this
breaks module autoloading.
The ocelot_8021q driver needs the ocelot_can_inject() and
ocelot_port_inject_frame() functions from the switch library. Previously
the wrong approach was taken to solve that dependency: shims were
provided for the case where the ocelot switch library was compiled out,
but that turns out to be insufficient, because the dependency when the
switch lib _is_ compiled is problematic too.
We cannot declare ocelot_can_inject() and ocelot_port_inject_frame() as
static inline functions, because these access I/O functions like
__ocelot_write_ix() which is called by ocelot_write_rix(). Making those
static inline basically means exposing the whole guts of the ocelot
switch library, not ideal...
We already have one tagging protocol driver which calls into the switch
driver during xmit but not using any exported symbol: sja1105_defer_xmit.
We can do the same thing here: create a kthread worker and one work item
per skb, and let the switch driver itself do the register accesses to
send the skb, and then consume it.
Fixes: 0a6f17c6ae21 ("net: dsa: tag_ocelot_8021q: add support for PTP timestamping")
Reported-by: Michael Walle <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: shorten kthread name to fit the 16 character limit
drivers/net/dsa/ocelot/felix.c | 96 ++++++++++++++++++++++++++++++++--
drivers/net/dsa/ocelot/felix.h | 1 +
include/linux/dsa/ocelot.h | 12 +++++
include/soc/mscc/ocelot.h | 27 ----------
net/dsa/Kconfig | 2 -
net/dsa/tag_ocelot_8021q.c | 38 +++++++++-----
6 files changed, 130 insertions(+), 46 deletions(-)
@@ -1074,6 +1074,73 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)return0;}+#define work_to_xmit_work(w) \+container_of((w),structfelix_deferred_xmit_work,work)++staticvoidfelix_port_deferred_xmit(structkthread_work*work)+{+structfelix_deferred_xmit_work*xmit_work=work_to_xmit_work(work);+structdsa_switch*ds=xmit_work->dp->ds;+structsk_buff*skb=xmit_work->skb;+u32rew_op=ocelot_ptp_rew_op(skb);+structocelot*ocelot=ds->priv;+intport=xmit_work->dp->index;+intretries=10;++do{+if(ocelot_can_inject(ocelot,0))+break;++cpu_relax();+}while(--retries);++if(!retries){+dev_err(ocelot->dev,"port %d failed to inject skb\n",+port);+kfree_skb(skb);+return;+}++ocelot_port_inject_frame(ocelot,port,0,rew_op,skb);++consume_skb(skb);+kfree(xmit_work);+}++staticintfelix_port_setup_tagger_data(structdsa_switch*ds,intport)+{+structdsa_port*dp=dsa_to_port(ds,port);+structocelot*ocelot=ds->priv;+structfelix*felix=ocelot_to_felix(ocelot);+structfelix_port*felix_port;++if(!dsa_port_is_user(dp))+return0;++felix_port=kzalloc(sizeof(*felix_port),GFP_KERNEL);+if(!felix_port)+return-ENOMEM;++felix_port->xmit_worker=felix->xmit_worker;+felix_port->xmit_work_fn=felix_port_deferred_xmit;++dp->priv=felix_port;++return0;+}++staticvoidfelix_port_teardown_tagger_data(structdsa_switch*ds,intport)+{+structdsa_port*dp=dsa_to_port(ds,port);+structfelix_port*felix_port=dp->priv;++if(!felix_port)+return;++dp->priv=NULL;+kfree(felix_port);+}+/* Hardware initialization done here so that we can allocate structures with*devmwithoutfearofdsa_register_switchreturning-EPROBE_DEFERandcausing*ustoallocatestructurestwice(leakmemory)andmapPCImemorytwice
@@ -1102,6 +1169,12 @@ static int felix_setup(struct dsa_switch *ds)}}+felix->xmit_worker=kthread_create_worker(0,"felix_xmit");+if(IS_ERR(felix->xmit_worker)){+err=PTR_ERR(felix->xmit_worker);+gotoout_deinit_timestamp;+}+for(port=0;port<ds->num_ports;port++){if(dsa_is_unused_port(ds,port))continue;
@@ -1112,6 +1185,14 @@ static int felix_setup(struct dsa_switch *ds)*bitsofvlantag.*/felix_port_qos_map_init(ocelot,port);++err=felix_port_setup_tagger_data(ds,port);+if(err){+dev_err(ds->dev,+"port %d failed to set up tagger data: %pe\n",+port,ERR_PTR(err));+gotoout_deinit_ports;+}}err=ocelot_devlink_sb_register(ocelot);
@@ -1138,9 +1219,13 @@ static int felix_setup(struct dsa_switch *ds)if(dsa_is_unused_port(ds,port))continue;+felix_port_teardown_tagger_data(ds,port);ocelot_deinit_port(ocelot,port);}+kthread_destroy_worker(felix->xmit_worker);++out_deinit_timestamp:ocelot_deinit_timestamp(ocelot);ocelot_deinit(ocelot);
@@ -112,8 +112,6 @@ config NET_DSA_TAG_OCELOTconfigNET_DSA_TAG_OCELOT_8021Qtristate"Tag driver for Ocelot family of switches, using VLAN"-depends onMSCC_OCELOT_SWITCH_LIB||\-(MSCC_OCELOT_SWITCH_LIB=n&&COMPILE_TEST)helpSayYorMifyouwanttoenablesupportfortaggingframeswithacustomVLAN-basedheader.Framesthatrequiretimestamping,suchas
@@ -10,10 +10,31 @@*/#include<linux/dsa/8021q.h>#include<linux/dsa/ocelot.h>-#include<soc/mscc/ocelot.h>-#include<soc/mscc/ocelot_ptp.h>#include"dsa_priv.h"+staticstructsk_buff*ocelot_defer_xmit(structdsa_port*dp,+structsk_buff*skb)+{+structfelix_deferred_xmit_work*xmit_work;+structfelix_port*felix_port=dp->priv;++xmit_work=kzalloc(sizeof(*xmit_work),GFP_ATOMIC);+if(!xmit_work)+returnNULL;++/* Calls felix_port_deferred_xmit in felix.c */+kthread_init_work(&xmit_work->work,felix_port->xmit_work_fn);+/* Increase refcount so the kfree_skb in dsa_slave_xmit+*won'treallyfreethepacket.+*/+xmit_work->dp=dp;+xmit_work->skb=skb_get(skb);++kthread_queue_work(felix_port->xmit_worker,&xmit_work->work);++returnNULL;+}+staticstructsk_buff*ocelot_xmit(structsk_buff*skb,structnet_device*netdev){
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:13
At present, when a PTP packet which requires TX timestamping gets
dropped under congestion by the switch, things go downhill very fast.
The driver keeps a clone of that skb in a queue of packets awaiting TX
timestamp interrupts, but interrupts will never be raised for the
dropped packets.
Moreover, matching timestamped packets to timestamps is done by a 2-bit
timestamp ID, and this can wrap around and we can match on the wrong skb.
Since with the default NPI-based tagging protocol, we get no notification
about packet drops, the best we can do is eventually recover from the
drop of a PTP frame: its skb will be dead memory until another skb which
was assigned the same timestamp ID happens to find it.
However, with the ocelot-8021q tagger which injects packets using the
manual register interface, it appears that we can check for more
information, such as:
- whether the input queue has reached the high watermark or not
- whether the injection group's FIFO can accept additional data or not
so we know that a PTP frame is likely to get dropped before actually
sending it, and drop it ourselves (because DSA uses NETIF_F_LLTX, so it
can't return NETDEV_TX_BUSY to ask the qdisc to requeue the packet).
But when we do that, we can also remove the skb from the timestamping
queue, because there surely won't be any timestamp that matches it.
Fixes: 0a6f17c6ae21 ("net: dsa: tag_ocelot_8021q: add support for PTP timestamping")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/dsa/ocelot/felix.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
@@ -1074,6 +1074,33 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports)return0;}+staticvoidocelot_port_purge_txtstamp_skb(structocelot*ocelot,intport,+structsk_buff*skb)+{+structocelot_port*ocelot_port=ocelot->ports[port];+structsk_buff*clone=OCELOT_SKB_CB(skb)->clone;+structsk_buff*skb_match=NULL,*skb_tmp;+unsignedlongflags;++if(!clone)+return;++spin_lock_irqsave(&ocelot_port->tx_skbs.lock,flags);++skb_queue_walk_safe(&ocelot_port->tx_skbs,skb,skb_tmp){+if(skb!=clone)+continue;+__skb_unlink(skb,&ocelot_port->tx_skbs);+skb_match=skb;+break;+}++spin_unlock_irqrestore(&ocelot_port->tx_skbs.lock,flags);++WARN_ONCE(!skb_match,+"Could not find skb clone in TX timestamping list\n");+}+#define work_to_xmit_work(w) \container_of((w),structfelix_deferred_xmit_work,work)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:14
When skb_match is NULL, it means we received a PTP IRQ for a timestamp
ID that the kernel has no idea about, since there is no skb in the
timestamping queue with that timestamp ID.
This is a grave error and not something to just "continue" over.
So print a big warning in case this happens.
Also, move the check above ocelot_get_hwtimestamp(), there is no point
in reading the full 64-bit current PTP time if we're not going to do
anything with it anyway for this skb.
Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/ethernet/mscc/ocelot.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -747,12 +747,12 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)spin_unlock_irqrestore(&port->tx_skbs.lock,flags);+if(WARN_ON(!skb_match))+continue;+/* Get the h/w timestamp */ocelot_get_hwtimestamp(ocelot,&ts);-if(unlikely(!skb_match))-continue;-/* Set the timestamp into the skb */memset(&shhwtstamps,0,sizeof(shhwtstamps));shhwtstamps.hwtstamp=ktime_set(ts.tv_sec,ts.tv_nsec);
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:16
It appears that Ocelot switches cannot timestamp non-PTP frames,
I tested this using the isochron program at:
https://github.com/vladimiroltean/tsn-scripts
with the result that the driver increments the ocelot_port->ts_id
counter as expected, puts it in the REW_OP, but the hardware seems to
not timestamp these packets at all, since no IRQ is emitted.
Therefore check whether we are sending PTP frames, and refuse to
populate REW_OP otherwise.
Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: return early if TX timestamping is not enabled
(ocelot_port->ptp_cmd is 0)
drivers/net/ethernet/mscc/ocelot.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
@@ -647,11 +643,20 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,{structocelot_port*ocelot_port=ocelot->ports[port];u8ptp_cmd=ocelot_port->ptp_cmd;+unsignedintptp_class;interr;+/* Don't do anything if PTP timestamping not enabled */+if(!ptp_cmd)+return0;++ptp_class=ptp_classify_raw(skb);+if(ptp_class==PTP_CLASS_NONE)+return-EINVAL;+/* Store ptp_cmd in OCELOT_SKB_CB(skb)->ptp_cmd */if(ptp_cmd==IFH_REW_OP_ORIGIN_PTP){-if(ocelot_ptp_is_onestep_sync(skb)){+if(ocelot_ptp_is_onestep_sync(skb,ptp_class)){OCELOT_SKB_CB(skb)->ptp_cmd=ptp_cmd;return0;}
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:17
When setting up a bridge with stp_state 1, topology changes are not
detected and loops are not blocked. This is because the standard way of
transmitting a packet, based on VLAN IDs redirected by VCAP IS2 to the
right egress port, does not override the port STP state (in the case of
Ocelot switches, that's really the PGID_SRC masks).
To force a packet to be injected into a port that's BLOCKING, we must
send it as a control packet, which means in the case of this tagger to
send it using the manual register injection method. We already do this
for PTP frames, extend the logic to apply to any link-local MAC DA.
Fixes: 7c83a7c539ab ("net: dsa: add a second tagger for Ocelot switches based on tag_8021q")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
net/dsa/tag_ocelot_8021q.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:19
The NXP LS1028A switch has two Ethernet ports towards the CPU, but only
one of them is capable of acting as an NPI port at a time (inject and
extract packets using DSA tags).
However, using the alternative ocelot-8021q tagging protocol, it should
be possible to use both CPU ports symmetrically, but for that we need to
mark both ports in the device tree as DSA masters.
In the process of doing that, it can be seen that traffic to/from the
network stack gets broken, and this is because the Felix driver iterates
through all DSA CPU ports and configures them as NPI ports. But since
there can only be a single NPI port, we effectively end up in a
situation where DSA thinks the default CPU port is the first one, but
the hardware port configured to be an NPI is the last one.
I would like to treat this as a bug, because if the updated device trees
are going to start circulating, it would be really good for existing
kernels to support them, too.
Fixes: adb3dccf090b ("net: dsa: felix: convert to the new .change_tag_protocol DSA API")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/dsa/ocelot/felix.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
@@ -284,12 +284,15 @@ static int felix_setup_mmio_filtering(struct felix *felix)}for(port=0;port<ocelot->num_phys_ports;port++){-if(dsa_is_user_port(ds,port))-user_ports|=BIT(port);-if(dsa_is_cpu_port(ds,port))-cpu_ports|=BIT(port);+if(dsa_is_cpu_port(ds,port)){+cpu=port;+break;+}}+if(cpu<0)+return-EINVAL;+tagging_rule->key_type=OCELOT_VCAP_KEY_ETYPE;*(__be16*)tagging_rule->key.etype.etype.value=htons(ETH_P_1588);*(__be16*)tagging_rule->key.etype.etype.mask=htons(0xffff);
@@ -325,7 +328,7 @@ static int felix_setup_mmio_filtering(struct felix *felix)*theCPUportmodule*/redirect_rule->action.mask_mode=OCELOT_MASK_MODE_REDIRECT;-redirect_rule->action.port_mask=cpu_ports;+redirect_rule->action.port_mask=BIT(cpu);}else{/* Trap PTP packets only to the CPU port module (which is*redirectedtotheNPIport)
@@ -1235,6 +1238,7 @@ static int felix_setup(struct dsa_switch *ds)*there'snorealpointincheckingforerrors.*/felix_set_tag_protocol(ds,port,felix->tag_proto);+break;}ds->mtu_enforcement_ingress=true;
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-12 11:41:20
The sad reality is that when a PTP frame with a TX timestamping request
is transmitted, it isn't guaranteed that it will make it all the way to
the wire (due to congestion inside the switch), and that a timestamp
will be taken by the hardware and placed in the timestamp FIFO where an
IRQ will be raised for it.
The implication is that if enough PTP frames are silently dropped by the
hardware such that the timestamp ID has rolled over, it is possible to
match a timestamp to an old skb.
Furthermore, nobody will match on the real skb corresponding to this
timestamp, since we stupidly matched on a previous one that was stale in
the queue, and stopped there.
So PTP timestamping will be broken and there will be no way to recover.
It looks like the hardware parses the sequenceID from the PTP header,
and also provides that metadata for each timestamp. The driver currently
ignores this, but it shouldn't.
As an extra resiliency measure, do the following:
- check whether the PTP sequenceID also matches between the skb and the
timestamp, treat the skb as stale otherwise and free it
- if we see a stale skb, don't stop there and try to match an skb one
more time, chances are there's one more skb in the queue with the same
timestamp ID, otherwise we wouldn't have ever found the stale one (it
is by timestamp ID that we matched it).
While this does not prevent PTP packet drops, it at least prevents
the catastrophic consequences of incorrect timestamp matching.
Since we already call ptp_classify_raw in the TX path, save the result
in the skb->cb of the clone, and just use that result in the interrupt
code path.
Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none
drivers/net/ethernet/mscc/ocelot.c | 24 +++++++++++++++++++++++-
include/soc/mscc/ocelot.h | 1 +
2 files changed, 24 insertions(+), 1 deletion(-)
@@ -675,6 +675,7 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port,returnerr;OCELOT_SKB_CB(skb)->ptp_cmd=ptp_cmd;+OCELOT_SKB_CB(*clone)->ptp_class=ptp_class;}return0;
@@ -731,6 +743,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)/* Retrieve the ts ID and Tx port */id=SYS_PTP_STATUS_PTP_MESS_ID_X(val);txport=SYS_PTP_STATUS_PTP_MESS_TXPORT_X(val);+seqid=SYS_PTP_STATUS_PTP_MESS_SEQ_ID(val);port=ocelot->ports[txport];
@@ -755,6 +769,14 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)if(WARN_ON(!skb_match))continue;+if(!ocelot_validate_ptp_skb(skb_match,seqid)){+dev_err_ratelimited(ocelot->dev,+"port %d received stale TX timestamp for seqid %d, discarding\n",+txport,seqid);+dev_kfree_skb_any(skb);+gototry_again;+}+/* Get the h/w timestamp */ocelot_get_hwtimestamp(ocelot,&ts);
The NXP LS1028A switch has two Ethernet ports towards the CPU, but only
one of them is capable of acting as an NPI port at a time (inject and
extract packets using DSA tags).
However, using the alternative ocelot-8021q tagging protocol, it should
be possible to use both CPU ports symmetrically, but for that we need to
mark both ports in the device tree as DSA masters.
In the process of doing that, it can be seen that traffic to/from the
network stack gets broken, and this is because the Felix driver iterates
through all DSA CPU ports and configures them as NPI ports. But since
there can only be a single NPI port, we effectively end up in a
situation where DSA thinks the default CPU port is the first one, but
the hardware port configured to be an NPI is the last one.
I would like to treat this as a bug, because if the updated device trees
are going to start circulating, it would be really good for existing
kernels to support them, too.
Fixes: adb3dccf090b ("net: dsa: felix: convert to the new .change_tag_protocol DSA API")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
As explained here:
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
DSA tagging protocol drivers cannot depend on symbols exported by switch
drivers, because this creates a circular dependency that breaks module
autoloading.
The tag_ocelot.c file depends on the ocelot_ptp_rew_op() function
exported by the common ocelot switch lib. This function looks at
OCELOT_SKB_CB(skb) and computes how to populate the REW_OP field of the
DSA tag, for PTP timestamping (the command: one-step/two-step, and the
TX timestamp identifier).
None of that requires deep insight into the driver, it is quite
stateless, as it only depends upon the skb->cb. So let's make it a
static inline function and put it in include/linux/dsa/ocelot.h, a
file that despite its name is used by the ocelot switch driver for
populating the injection header too - since commit 40d3f295b5fe ("net:
mscc: ocelot: use common tag parsing code with DSA").
With that function declared as static inline, its body is expanded
inside each call site, so the dependency is broken and the DSA tagger
can be built without the switch library, upon which the felix driver
depends.
Fixes: 39e5308b3250 ("net: mscc: ocelot: support PTP Sync one-step timestamping")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
When skb_match is NULL, it means we received a PTP IRQ for a timestamp
ID that the kernel has no idea about, since there is no skb in the
timestamping queue with that timestamp ID.
This is a grave error and not something to just "continue" over.
So print a big warning in case this happens.
Also, move the check above ocelot_get_hwtimestamp(), there is no point
in reading the full 64-bit current PTP time if we're not going to do
anything with it anyway for this skb.
Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
When setting up a bridge with stp_state 1, topology changes are not
detected and loops are not blocked. This is because the standard way of
transmitting a packet, based on VLAN IDs redirected by VCAP IS2 to the
right egress port, does not override the port STP state (in the case of
Ocelot switches, that's really the PGID_SRC masks).
To force a packet to be injected into a port that's BLOCKING, we must
send it as a control packet, which means in the case of this tagger to
send it using the manual register injection method. We already do this
for PTP frames, extend the logic to apply to any link-local MAC DA.
Fixes: 7c83a7c539ab ("net: dsa: add a second tagger for Ocelot switches based on tag_8021q")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Hello:
This series was applied to netdev/net.git (master)
by Jakub Kicinski [off-list ref]:
On Tue, 12 Oct 2021 14:40:34 +0300 you wrote:
This is an assorted collection of fixes for issues seen on the NXP
LS1028A switch.
- PTP packet drops due to switch congestion result in catastrophic
damage to the driver's state
- loops are not blocked by STP if using the ocelot-8021q tagger
- driver uses the wrong CPU port when two of them are defined in DT
- module autoloading is broken* with both tagging protocol drivers
(ocelot and ocelot-8021q)
[...]