From: Wei Fang <wei.fang@nxp.com>
This patch set adds PTP timestamping support to the NETC switch, so that
the switch ports can be used for IEEE 1588 PTP synchronization.
The NETC switch has no time registers of its own and instead shares the
PTP time of the NETC Timer, which is a separate PCIe function driven by
its own driver. The first 3 patches prepare the Timer driver for this
role: tidy up its 64-bit register access, drop an ineffective reset, and
export netc_timer_get_current_time() so the switch driver can read the
current PTP time across the driver boundary in a race-free way.
The switch redirects PTP frames to the CPU through the ingress port
filter table (IPFT). A few patches harden and simplify the existing IPFT
and host flood handling first, then enable the ingress port filtering
lookup by default so PTP filter rules take effect without extra software
tracking.
On top of that groundwork, the switch driver gains RX timestamping and
both flavours of TX timestamping. Timestamps are carried between the
hardware and the driver through dedicated switch tags: the ingress and
transmit timestamps ride in To_Host tags, and per-frame requests ride in
To_Port tags. Two-step TX matches a queued skb clone against the
timestamp response echoed back by the hardware. One-step TX defers the
Sync frame to a per-port work, where the driver reads a fresh PTP time
and writes it into the Sync frame's timestamp field, then programs the
single-step register and transmits the frame so the hardware can add the
residence time to the correction field on the wire.
Together these changes let standard user-space PTP stacks drive hardware
timestamping on the NETC switch ports.
---
v3:
1. Change the prototype of netc_timer_get_current_time(), and check the
pdev driver before accessing the driver data
2. Patch 5 is a new patch
3. Add netc_port_tstamp_timeout_work()
4. Check the return value of ntmp_ipft_delete_entry(), and in
netc_port_add_ipft_ptp_entry(), check whether the entry already exists
in IPFT before creating a new entry
5. Remove netc_port_purge_onestep_queue() from netc_port_disable()
6. Add netc_port_purge_onestep_queue() to netc_onestep_destroy_work()
7. Add try_module_get() to netc_port_onestep_alloc() and add module_put()
to netc_onestep_destroy_work()
8. Use consume_skb() instead of the err_free_skb label calls kfree_skb()
for NETC_TAG_TH_SUBTYPE2 frames
9. Add subtype check in netc_flow_dissect() and update the comments
10. Check onestep queue length before enqueue
11. Update commit messages
v2 link: https://lore.kernel.org/imx/20260808032146.2335723-1-wei.fang@oss.nxp.com/
v1 link: https://lore.kernel.org/imx/20260728104548.3301214-1-wei.fang@oss.nxp.com/
---
Wei Fang (8):
ptp: netc: use ioread64_lo_hi/iowrite64_lo_hi for 64-bit register
access
ptp: netc: remove unnecessary pcie_flr() call in probe
ptp: netc: export netc_timer_get_current_time() for cross-driver use
net: dsa: netc: use entry ID instead of pointer to track host flood
rule
net: dsa: netc: check return value of ntmp_ipft_delete_entry()
net: dsa: netc: enable ingress port filtering lookup by default
net: dsa: netc: add PTP two-step timestamping support
net: dsa: netc: add PTP one-step timestamping support
drivers/net/dsa/netc/Kconfig | 2 +
drivers/net/dsa/netc/Makefile | 3 +-
drivers/net/dsa/netc/netc_main.c | 252 +++++--
drivers/net/dsa/netc/netc_platform.c | 1 +
drivers/net/dsa/netc/netc_ptp.c | 904 ++++++++++++++++++++++++++
drivers/net/dsa/netc/netc_switch.h | 93 ++-
drivers/net/dsa/netc/netc_switch_hw.h | 5 +
drivers/ptp/ptp_netc.c | 125 ++--
include/linux/dsa/tag_netc.h | 43 ++
include/linux/fsl/netc_global.h | 10 +
net/dsa/tag_netc.c | 236 ++++++-
11 files changed, 1551 insertions(+), 123 deletions(-)
create mode 100644 drivers/net/dsa/netc/netc_ptp.c
--
2.34.1
From: Wei Fang <wei.fang@nxp.com>
Replace the open-coded 64-bit register read/write sequences with
ioread64_lo_hi() and iowrite64_lo_hi() helpers. Introduce two new macros
netc_timer_rd64() and netc_timer_wr64() that wrap these helpers and use
them throughout the driver. This reduces boilerplate and makes the intent
of each operation clearer.
The high-half register defines (NETC_TMR_*_H) are kept to document the
register map; they are not used directly since netc_timer_rd/wr64()
address the 64-bit register pair via the low-half offset, relying on the
hardware layout where H is always at L + 4.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/ptp/ptp_netc.c | 72 ++++++++++++------------------------------
1 file changed, 21 insertions(+), 51 deletions(-)
@@ -127,6 +127,17 @@ struct netc_timer {#define netc_timer_rd(p, o) netc_read((p)->base + (o))#define netc_timer_wr(p, o, v) netc_write((p)->base + (o), v)++/* The 64-bit timer registers consist of a low (L) and high (H) register pair.+*Hardwarerequiresastrictaccessorder:forwrites,TMR_xxx_Lmustbe+*writtenfirst,whichlatchesthevalueintoashadowregister;thewrite+*toTMR_xxx_Hthenatomicallytransfersbothshadowregistersintothelive+*counter.Forreads,TMR_xxx_Lmustbereadfirsttocaptureacoherent+*snapshot.iowrite64_lo_hi()andioread64_lo_hi()enforcethisL-before-H+*ordering.+*/+#define netc_timer_rd64(p, o) ioread64_lo_hi((p)->base + (o))+#define netc_timer_wr64(p, o, v) iowrite64_lo_hi(v, (p)->base + (o))#define ptp_to_netc_timer(ptp) container_of((ptp), struct netc_timer, caps)staticconstchar*consttimer_clk_src[]={
@@ -136,66 +147,28 @@ static const char *const timer_clk_src[] = {staticvoidnetc_timer_cnt_write(structnetc_timer*priv,u64ns){-u32tmr_cnt_h=upper_32_bits(ns);-u32tmr_cnt_l=lower_32_bits(ns);--/* Writes to the TMR_CNT_L register copies the written value-*intotheshadowTMR_CNT_Lregister.WritestotheTMR_CNT_H-*registercopiesthevalueswrittenintotheshadowTMR_CNT_H-*register.Contentsoftheshadowregistersarecopiedinto-*theTMR_CNT_LandTMR_CNT_Hregistersfollowingawriteinto-*theTMR_CNT_Hregister.SotheusermustwritestoTMR_CNT_L-*registerfirst.OtherH/Lregistersshouldhavethesame-*behavior.-*/-netc_timer_wr(priv,NETC_TMR_CNT_L,tmr_cnt_l);-netc_timer_wr(priv,NETC_TMR_CNT_H,tmr_cnt_h);+netc_timer_wr64(priv,NETC_TMR_CNT_L,ns);}staticu64netc_timer_offset_read(structnetc_timer*priv){-u32tmr_off_l,tmr_off_h;-u64offset;--tmr_off_l=netc_timer_rd(priv,NETC_TMR_OFF_L);-tmr_off_h=netc_timer_rd(priv,NETC_TMR_OFF_H);-offset=(((u64)tmr_off_h)<<32)|tmr_off_l;--returnoffset;+returnnetc_timer_rd64(priv,NETC_TMR_OFF_L);}staticvoidnetc_timer_offset_write(structnetc_timer*priv,u64offset){-u32tmr_off_h=upper_32_bits(offset);-u32tmr_off_l=lower_32_bits(offset);--netc_timer_wr(priv,NETC_TMR_OFF_L,tmr_off_l);-netc_timer_wr(priv,NETC_TMR_OFF_H,tmr_off_h);+netc_timer_wr64(priv,NETC_TMR_OFF_L,offset);}staticu64netc_timer_cur_time_read(structnetc_timer*priv){-u32time_h,time_l;-u64ns;--/* The user should read NETC_TMR_CUR_TIME_L first to-*getcorrectcurrenttime.-*/-time_l=netc_timer_rd(priv,NETC_TMR_CUR_TIME_L);-time_h=netc_timer_rd(priv,NETC_TMR_CUR_TIME_H);-ns=(u64)time_h<<32|time_l;--returnns;+returnnetc_timer_rd64(priv,NETC_TMR_CUR_TIME_L);}staticvoidnetc_timer_alarm_write(structnetc_timer*priv,u64alarm,intindex){-u32alarm_h=upper_32_bits(alarm);-u32alarm_l=lower_32_bits(alarm);--netc_timer_wr(priv,NETC_TMR_ALARM_L(index),alarm_l);-netc_timer_wr(priv,NETC_TMR_ALARM_H(index),alarm_h);+netc_timer_wr64(priv,NETC_TMR_ALARM_L(index),alarm);}staticu32netc_timer_get_integral_period(structnetc_timer*priv)
@@ -500,22 +473,19 @@ static void netc_timer_handle_etts_event(struct netc_timer *priv, int index,boolupdate_event){structptp_clock_eventevent;-u32etts_l=0,etts_h=0;+u64etts=0;-while(netc_timer_rd(priv,NETC_TMR_STAT)&TMR_STAT_ETS_VLD(index)){-etts_l=netc_timer_rd(priv,NETC_TMR_ETTS_L(index));-etts_h=netc_timer_rd(priv,NETC_TMR_ETTS_H(index));-}+while(netc_timer_rd(priv,NETC_TMR_STAT)&TMR_STAT_ETS_VLD(index))+etts=netc_timer_rd64(priv,NETC_TMR_ETTS_L(index));/* Invalid time stamp */-if(!etts_l&&!etts_h)+if(!etts)return;if(update_event){event.type=PTP_CLOCK_EXTTS;event.index=index;-event.timestamp=(u64)etts_h<<32;-event.timestamp|=etts_l;+event.timestamp=etts;ptp_clock_event(priv->clock,&event);}}
From: Wei Fang <wei.fang@nxp.com>
According to the NETC reference manual, function level reset is not
applicable to the timer as a supporting function. Remove the pcie_flr()
call from netc_timer_pci_probe() as it has no effect.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/ptp/ptp_netc.c | 1 -
1 file changed, 1 deletion(-)
From: Wei Fang <wei.fang@nxp.com>
The NETC Switch does not have its own time registers and must obtain
the current PTP time from the NETC Timer bound to it. Since the two
are separate PCIe functions with independent drivers, add
netc_timer_get_current_time() to the Timer driver and export it via
EXPORT_SYMBOL_GPL().
The function takes the Timer's pci_dev pointer, acquires the per-device
spinlock to protect against concurrent register access, and reads
TMR_CUR_TIME via netc_timer_cur_time_read(). It returns an error if the
Timer driver has not yet probed or has already been removed, allowing
the caller to handle the unavailable case gracefully.
Hold the device lock around pci_get_drvdata() and the register read to
serialize against concurrent driver unbind. The remove() callback runs
under the same device lock, so this guarantees that priv and the MMIO
mapping remain valid for the entire duration of the read.
Move spin_lock_init() from netc_timer_probe() to netc_timer_pci_probe(),
before pci_set_drvdata(), so that the spinlock is fully initialized
before the driver data becomes visible to other drivers.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/ptp/ptp_netc.c | 52 ++++++++++++++++++++++++++++++++-
include/linux/fsl/netc_global.h | 10 +++++++
2 files changed, 61 insertions(+), 1 deletion(-)
From: Wei Fang <wei.fang@nxp.com>
Replace the struct ipft_entry_data pointer in struct netc_port with a
plain u32 entry ID (ipft_hf_eid), using NTMP_NULL_ENTRY_ID as the
sentinel value. The ipft_entry_data allocation is now freed immediately
inside netc_port_add_host_flood_rule() after the hardware entry is
committed, so no heap memory survives beyond that function. As a result,
netc_free_host_flood_rules() is no longer needed and can be removed.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/net/dsa/netc/netc_main.c | 63 ++++++++++++------------------
drivers/net/dsa/netc/netc_switch.h | 6 ++-
2 files changed, 29 insertions(+), 40 deletions(-)
@@ -286,6 +286,12 @@ static int netc_init_all_ports(struct netc_switch *priv)dev_err(dev,"Failed to create MDIO bus\n");returnerr;}++/* The ipft_hf_eid is initialized to an invalid entry+*IDbecausethehostfloodrule(IPFTentry)hasnot+*beencreated.+*/+np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;}}
@@ -938,30 +944,12 @@ static void netc_destroy_all_lists(struct netc_switch *priv)mutex_destroy(&priv->vft_lock);}-staticvoidnetc_free_host_flood_rules(structnetc_switch*priv)-{-structdsa_port*dp;--dsa_switch_for_each_user_port(dp,priv->ds){-structnetc_port*np=priv->ports[dp->index];--/* No need to clear the hardware IPFT entry. Because PCIe-*FLRwillbeperformedwhentheswitchisre-registered,-*itwillresethardwarestate.Soonlyneedtofreethe-*memorytoavoidmemoryleak.-*/-kfree(np->host_flood);-np->host_flood=NULL;-}-}-staticvoidnetc_teardown(structdsa_switch*ds){structnetc_switch*priv=ds->priv;disable_delayed_work_sync(&priv->fdbt_ageing_work);netc_destroy_all_lists(priv);-netc_free_host_flood_rules(priv);netc_free_ntmp_user(priv);}
@@ -1759,37 +1747,36 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,cfge->cfg=cpu_to_le32(cfg);err=ntmp_ipft_add_entry(&priv->ntmp,host_flood);-if(err){-kfree(host_flood);-returnerr;-}+if(err)+gotofree_host_flood;np->uc=uc;np->mc=mc;-np->host_flood=host_flood;+np->ipft_hf_eid=host_flood->entry_id;/* Enable ingress port filter table lookup */netc_port_wr(np,NETC_PIPFCR,PIPFCR_EN);-return0;+free_host_flood:+kfree(host_flood);++returnerr;}-staticvoidnetc_port_remove_host_flood(structnetc_port*np,-structipft_entry_data*host_flood)+staticvoidnetc_port_remove_host_flood(structnetc_port*np,u32entry_id){structnetc_switch*priv=np->switch_priv;booldisable_host_flood=false;-if(!host_flood)+if(entry_id==NTMP_NULL_ENTRY_ID)return;-if(np->host_flood==host_flood)+if(np->ipft_hf_eid==entry_id)disable_host_flood=true;-ntmp_ipft_delete_entry(&priv->ntmp,host_flood->entry_id);-kfree(host_flood);+ntmp_ipft_delete_entry(&priv->ntmp,entry_id);if(disable_host_flood){-np->host_flood=NULL;+np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;np->uc=false;np->mc=false;netc_port_wr(np,NETC_PIPFCR,0);
@@ -1800,7 +1787,7 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,booluc,boolmc){structnetc_port*np=NETC_PORT(ds,port);-structipft_entry_data*old_host_flood;+u32old_entry_id;/* Do not add host flood rule to ingress port filter table when*theporthasjoinedabridge.Otherwise,theingressframes
@@ -1818,20 +1805,18 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,/* IPFT does not support in-place updates to the KEYE element,*weneedtoaddanewentryandthendeletetheoldone.So-*savetheoldentryfirst.+*savetheoldentryIDfirst.*/-old_host_flood=np->host_flood;-np->host_flood=NULL;+old_entry_id=np->ipft_hf_eid;if(netc_port_add_host_flood_rule(np,uc,mc)){-np->host_flood=old_host_flood;dev_err(ds->dev,"Failed to add host flood rule on port %d\n",port);return;}/* Remove the old host flood entry */-netc_port_remove_host_flood(np,old_host_flood);+netc_port_remove_host_flood(np,old_entry_id);}staticintnetc_single_vlan_aware_bridge(structdsa_switch*ds,
@@ -2020,7 +2005,7 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,netc_port_set_pvid(np,vlan_unaware_pvid);out:-netc_port_remove_host_flood(np,np->host_flood);+netc_port_remove_host_flood(np,np->ipft_hf_eid);if(atomic_inc_return(&priv->br_cnt)==1)schedule_delayed_work(&priv->fdbt_ageing_work,
@@ -84,7 +84,11 @@ struct netc_port {u16uc:1;u16mc:1;u16pvid;-structipft_entry_data*host_flood;+/* ipft_hf_eid applies only to user ports and should be initialized+*toNTMP_NULL_ENTRY_ID.Otherports(suchasCPUports)donot+*requireinitialization.+*/+u32ipft_hf_eid;};structnetc_switch_regs{
From: Wei Fang <wei.fang@nxp.com>
ntmp_ipft_delete_entry() can fail, e.g. when DMA memory allocation fails.
The driver previously ignored the return value, so a failed deletion
would leave the IPFT entry untracked and impossible to clean up later.
Convert netc_port_remove_host_flood() from void to int and propagate the
error to all callers. In netc_port_add_host_flood_rule(), delete the old
entry before adding the new one, and guard against adding a new entry
when the old one still exists. In netc_port_set_host_flood() and
netc_port_bridge_join(), handle deletion failures and add rollback paths
to restore the host flood entry on error.
Note that the rollback itself may also fail, in which case there is
nothing more the driver can do. This is an acceptable limitation because
such failures only occur under resource exhaustion conditions that are
already abnormal, and the host flood feature is not critical to basic
packet forwarding.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/net/dsa/netc/netc_main.c | 93 ++++++++++++++++++++++----------
1 file changed, 64 insertions(+), 29 deletions(-)
@@ -1707,15 +1707,23 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,u32cfg;interr;-if(!uc&&!mc){-/* Disable ingress port filter table lookup */-netc_port_wr(np,NETC_PIPFCR,0);-np->uc=false;-np->mc=false;+if(np->ipft_hf_eid!=NTMP_NULL_ENTRY_ID){+/* The old rule is the same as the new rule, return directly.+*Butiftheexistingrulediffersfromtherequestedone,+*returnanerror.+*/+if(np->uc==uc&&np->mc==mc)+return0;-return0;+dev_err(priv->dev,+"The old host flood rule has not been removed from port %u\n",+np->dp->index);+return-EINVAL;}+if(!uc&&!mc)+return0;+host_flood=kzalloc_obj(*host_flood);if(!host_flood)return-ENOMEM;
@@ -1762,32 +1770,33 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,returnerr;}-staticvoidnetc_port_remove_host_flood(structnetc_port*np,u32entry_id)+staticintnetc_port_remove_host_flood(structnetc_port*np){structnetc_switch*priv=np->switch_priv;-booldisable_host_flood=false;+u32entry_id=np->ipft_hf_eid;+interr;if(entry_id==NTMP_NULL_ENTRY_ID)-return;+return0;-if(np->ipft_hf_eid==entry_id)-disable_host_flood=true;+err=ntmp_ipft_delete_entry(&priv->ntmp,entry_id);+if(err)+returnerr;-ntmp_ipft_delete_entry(&priv->ntmp,entry_id);+np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;+np->uc=false;+np->mc=false;+netc_port_wr(np,NETC_PIPFCR,0);-if(disable_host_flood){-np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;-np->uc=false;-np->mc=false;-netc_port_wr(np,NETC_PIPFCR,0);-}+return0;}staticvoidnetc_port_set_host_flood(structdsa_switch*ds,intport,booluc,boolmc){structnetc_port*np=NETC_PORT(ds,port);-u32old_entry_id;+boolold_uc=np->uc;+boolold_mc=np->mc;/* Do not add host flood rule to ingress port filter table when*theporthasjoinedabridge.Otherwise,theingressframes
@@ -1795,7 +1804,12 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,*willberedirecteddirectlytotheCPUport.*/if(dsa_port_bridge_dev_get(np->dp)){-netc_port_remove_host_flood(np,np->ipft_hf_eid);+if(!netc_port_remove_host_flood(np))+return;++dev_err(ds->dev,+"Failed to delete host flood rule on bridge port %u\n",+port);return;}
@@ -1804,19 +1818,29 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,return;/* IPFT does not support in-place updates to the KEYE element,-*weneedtoaddanewentryandthendeletetheoldone.So-*savetheoldentryIDfirst.+*weneedtodeletetheoldoneandthenaddthenewrule.If+*thedeletionfails,returnimmediately.*/-old_entry_id=np->ipft_hf_eid;+if(netc_port_remove_host_flood(np)){+dev_err(ds->dev,+"Failed to delete old host flood rule on port %u\n",+port);++return;+}if(netc_port_add_host_flood_rule(np,uc,mc)){dev_err(ds->dev,"Failed to add host flood rule on port %d\n",port);-return;-}-/* Remove the old host flood entry */-netc_port_remove_host_flood(np,old_entry_id);+/* Try to restore the old flood rule, if the recovery fails,+*thereisnothingelsewecando;thisisalimitation.+*/+if(netc_port_add_host_flood_rule(np,old_uc,old_mc))+dev_err(ds->dev,+"Failed to restore host flood rule on port %d\n",+port);+}}staticintnetc_single_vlan_aware_bridge(structdsa_switch*ds,
@@ -1981,6 +2005,8 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,structnetc_port*np=NETC_PORT(ds,port);structnetc_switch*priv=ds->priv;u16vlan_unaware_pvid;+booluc=np->uc;+boolmc=np->mc;interr;if(!bridge.num){
@@ -1992,6 +2018,12 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,if(err)returnerr;+err=netc_port_remove_host_flood(np);+if(err){+NL_SET_ERR_MSG_MOD(extack,"Failed to delete host flood rule");+returnerr;+}+netc_port_set_mlo(np,MLO_NOT_OVERRIDE);if(br_vlan_enabled(bridge.dev))
@@ -2005,8 +2037,6 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,netc_port_set_pvid(np,vlan_unaware_pvid);out:-netc_port_remove_host_flood(np,np->ipft_hf_eid);-if(atomic_inc_return(&priv->br_cnt)==1)schedule_delayed_work(&priv->fdbt_ageing_work,READ_ONCE(priv->fdbt_ageing_delay));
@@ -2016,6 +2046,11 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port,disable_mlo:netc_port_set_mlo(np,MLO_DISABLE);+if(netc_port_add_host_flood_rule(np,uc,mc))+dev_err(ds->dev,+"Failed to restore host flood rule on port %u\n",+port);+returnerr;}
From: Wei Fang <wei.fang@nxp.com>
The ingress port filtering lookup function involves performing a lookup
against the ingress port filter table (IPFT). If the frame matches an
entry, subsequent frame processing functions will perform corresponding
operations based on the parameters specified in that entry. If no entry
matches the frame, the frame is allowed in, and passed to the next frame
processing function.
In the future, the IPFT will be used to filter PTP frames and support tc
flower. Tracking in software whether any IPFT entry exists for a port in
order to decide whether to enable the ingress port filtering lookup would
increase code complexity unnecessarily. Since enabling the lookup when
the IPFT is empty has no effect on RX frames, enable the ingress port
filtering lookup unconditionally during driver initialization to simplify
code logic.
The only concern is that when deleting an IPFT entry fails, the hardware
will continue filtering RX frames based on that entry. However, we have
no choice. We cannot disable ingress port filter lookup because this
would deactivate other IPFT entries, which is obviously not what we want.
Moreover, deletion failures are almost impossible under normal
circumstances, and we don't need to be bothered by this minor issue.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/net/dsa/netc/netc_main.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
@@ -555,6 +555,12 @@ static void netc_port_fixed_config(struct netc_port *np)netc_port_rmw(np,NETC_PCR,PCR_L2DOSE|PCR_L3DOSE,PCR_L2DOSE|PCR_L3DOSE);+/* Enable ingress port filter table lookup, if no match is found,+*theframeisallowedandpassedtothenextframeprocessing+*function.+*/+netc_port_wr(np,NETC_PIPFCR,PIPFCR_EN);+/* Set the quanta value of TX PAUSE frame */netc_mac_port_wr(np,NETC_PM_PAUSE_QUANTA(0),NETC_PAUSE_QUANTA);
@@ -1761,8 +1767,6 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,np->uc=uc;np->mc=mc;np->ipft_hf_eid=host_flood->entry_id;-/* Enable ingress port filter table lookup */-netc_port_wr(np,NETC_PIPFCR,PIPFCR_EN);free_host_flood:kfree(host_flood);
@@ -1786,7 +1790,6 @@ static int netc_port_remove_host_flood(struct netc_port *np)np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;np->uc=false;np->mc=false;-netc_port_wr(np,NETC_PIPFCR,0);return0;}
From: Wei Fang <wei.fang@nxp.com>
Add two-step TX timestamping and RX timestamping for the NETC switch.
For RX, install ingress port filter table (IPFT) rules that redirect PTP
frames to the CPU port. Support L2, L4 over IPv4 and L4 over IPv6, for
both event and general messages, selected through the hwtstamp rx_filter.
The hardware inserts a To_Host subtype 1 tag carrying the 64-bit ingress
timestamp after the source MAC address. The tagger extracts it into the
skb control buffer, and netc_port_rxtstamp() copies it into
skb_hwtstamps().
For two-step TX, clone the skb and record a 4-bit timestamp request ID
in the clone, then queue it on a per-port list. netc_xmit() carries the
same ID in the To_Port subtype 2 tag it inserts into the transmitted
frame. After the hardware sends the frame out, it generates a To_Host
subtype 2 response frame whose tag echoes back that ID together with the
64-bit transmit timestamp. The tagger dispatches the ID and timestamp to
the switch driver through the twostep_tstamp_handler callback registered
in netc_tagger_data. The driver matches the ID to the queued clone and
hands the clone back to the stack with the transmit timestamp via
skb_complete_tx_timestamp(), then frees the response skb. Non-PTP frames
keep using the To_Port subtype 0 tag on the xmit fast path.
The request IDs themselves are managed per port: each transmit takes the
first ID not currently in flight, and an ID becomes available again once
its clone leaves the queue. A per-port delayed work, tstamp_timeout_work,
drops any clone that stays unmatched for 5 seconds, which is far longer
than the hardware's normal response latency. A queued clone is thus
completed by the response handler, or dropped by tstamp_timeout_work on
timeout.
The two-step response frame carries no payload; its total length is
only 26 bytes (12 bytes of DMAC and SMAC plus a 14-byte switch tag).
eth_type_trans() pulls ETH_HLEN bytes, advancing skb->data past the
DMAC, SMAC, and the TPID that opens the switch tag, leaving skb->len
at 12. Since the tag pointer is at (skb->data - 2), the pskb_may_pull()
check must use (NETC_TAG_MAX_LEN - 2) rather than NETC_TAG_MAX_LEN.
Otherwise pskb_may_pull() drops the response frame and breaks PTP
synchronization.
Add the To_Port subtype 2 and To_Host subtype 1/2 tag structures, extend
netc_xmit() to select the tag based on ptp_flag in the skb control buffer,
and add netc_connect()/netc_disconnect() to manage the per-switch
netc_tagger_data allocation. Grab the PTP timer's pci_dev in netc_setup()
so get_ts_info() can report its PHC index, and release it in the teardown
and error paths.
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/net/dsa/netc/Kconfig | 1 +
drivers/net/dsa/netc/Makefile | 3 +-
drivers/net/dsa/netc/netc_main.c | 71 +++-
drivers/net/dsa/netc/netc_platform.c | 1 +
drivers/net/dsa/netc/netc_ptp.c | 471 +++++++++++++++++++++++++++
drivers/net/dsa/netc/netc_switch.h | 42 +++
include/linux/dsa/tag_netc.h | 21 ++
net/dsa/tag_netc.c | 168 +++++++++-
8 files changed, 763 insertions(+), 15 deletions(-)
create mode 100644 drivers/net/dsa/netc/netc_ptp.c
@@ -65,6 +65,20 @@ netc_get_tag_protocol(struct dsa_switch *ds, int port,returnDSA_TAG_PROTO_NETC;}+staticintnetc_connect_tag_protocol(structdsa_switch*ds,+enumdsa_tag_protocolproto)+{+structnetc_tagger_data*tagger_data;++if(proto!=DSA_TAG_PROTO_NETC)+return-EPROTONOSUPPORT;++tagger_data=ds->tagger_data;+tagger_data->twostep_tstamp_handler=netc_port_twostep_tstamp_handler;++return0;+}+staticvoidnetc_port_rmw(structnetc_port*np,u32reg,u32mask,u32val){
@@ -292,6 +306,15 @@ static int netc_init_all_ports(struct netc_switch *priv)*beencreated.*/np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;++/* Only the user port needs to support PTP feature, so+*PTP-relatedresources,suchasskb_txtstamp_queue,+*tstamp_lock,etc.,areinitializedonlyfortheuser+*port.+*/+err=netc_port_ptp_init(np);+if(err)+returnerr;}}
@@ -881,6 +904,15 @@ static int netc_switch_bpt_default_config(struct netc_switch *priv)return0;}+staticstructpci_dev*netc_get_ptp_timer(structnetc_switch*priv)+{+structpci_bus*bus=priv->pdev->bus;+u32devfn=priv->info->tmr_devfn;++returnpci_get_domain_bus_and_slot(pci_domain_nr(bus),+bus->number,devfn);+}+staticintnetc_setup(structdsa_switch*ds){structnetc_switch*priv=ds->priv;
@@ -893,13 +925,23 @@ static int netc_setup(struct dsa_switch *ds)netc_get_switch_capabilities(priv);+/* The PTP timer sits on the same PCI bus as the switch. PCI creates+*everyfunction'spci_devduringbusenumeration,beforeanydriver+*probes,sowecangrabthetimer'spci_devhereevenifthetimer+*driverhasnotprobedyet.+*/+priv->tmr_dev=netc_get_ptp_timer(priv);+if(!priv->tmr_dev)+dev_info(priv->dev,+"PTP timer PCI device not found\n");+err=netc_init_all_ports(priv);if(err)-returnerr;+gotoput_ptp_timer;err=netc_init_ntmp_user(priv);if(err)-returnerr;+gotoput_ptp_timer;INIT_HLIST_HEAD(&priv->fdb_list);mutex_init(&priv->fdbt_lock);
@@ -938,6 +980,8 @@ static int netc_setup(struct dsa_switch *ds)mutex_destroy(&priv->fdbt_lock);mutex_destroy(&priv->vft_lock);netc_free_ntmp_user(priv);+put_ptp_timer:+pci_dev_put(priv->tmr_dev);returnerr;}
@@ -0,0 +1,471 @@+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)+/*+*NXPNETCswitchdriver+*Copyright2025-2026NXP+*/++#include<linux/ptp_classify.h>+#include<linux/ptp_clock_kernel.h>++#include"netc_switch.h"++#define NETC_NUM_TS_REQ_ID 16+#define NETC_TXTSTAMP_TIMEOUT (5 * HZ)++staticvoidnetc_port_tstamp_timeout_work(structwork_struct*work)+{+structnetc_port*np=container_of(work,structnetc_port,+tstamp_timeout_work.work);+structsk_buff_headfree_list;+structsk_buff*skb,*skb_tmp;++__skb_queue_head_init(&free_list);++spin_lock_bh(&np->tstamp_lock);+skb_queue_walk_safe(&np->skb_txtstamp_queue,skb,skb_tmp){+if(time_before(jiffies,NETC_SKB_CB(skb)->ptp_tx_time++NETC_TXTSTAMP_TIMEOUT))+continue;++dev_dbg_ratelimited(np->switch_priv->dev,+"Port %d ts_req_id %u which seems lost\n",+np->dp->index,NETC_SKB_CB(skb)->ts_req_id);++__skb_unlink(skb,&np->skb_txtstamp_queue);+__skb_queue_tail(&free_list,skb);+}++/* Reschedule if there are still pending clones that have not+*timedoutyet.+*/+if(!skb_queue_empty(&np->skb_txtstamp_queue))+schedule_delayed_work(&np->tstamp_timeout_work,+NETC_TXTSTAMP_TIMEOUT);++spin_unlock_bh(&np->tstamp_lock);+__skb_queue_purge(&free_list);+}++intnetc_port_ptp_init(structnetc_port*np)+{+/* Initialize to invalid entry IDs */+for(inti=0;i<NETC_PTP_MAX;i++)+np->ptp_ipft_eid[i]=NTMP_NULL_ENTRY_ID;++spin_lock_init(&np->tstamp_lock);+__skb_queue_head_init(&np->skb_txtstamp_queue);+INIT_DELAYED_WORK(&np->tstamp_timeout_work,+netc_port_tstamp_timeout_work);++return0;+}++voidnetc_port_purge_txtstamp_queue(structnetc_port*np)+{+structsk_buff_headfree_list;++__skb_queue_head_init(&free_list);++spin_lock_bh(&np->tstamp_lock);+skb_queue_splice_init(&np->skb_txtstamp_queue,&free_list);+spin_unlock_bh(&np->tstamp_lock);++__skb_queue_purge(&free_list);+}++staticintnetc_get_phc_index(structnetc_switch*priv)+{+if(!priv->tmr_dev)+return-1;++returnptp_clock_index_by_dev(&priv->tmr_dev->dev);+}++intnetc_get_ts_info(structdsa_switch*ds,intport,+structkernel_ethtool_ts_info*info)+{+structnetc_switch*priv=ds->priv;++info->phc_index=netc_get_phc_index(priv);+if(info->phc_index<0)+return0;++info->so_timestamping|=SOF_TIMESTAMPING_TX_HARDWARE|+SOF_TIMESTAMPING_RX_HARDWARE|+SOF_TIMESTAMPING_RAW_HARDWARE;++info->tx_types=BIT(HWTSTAMP_TX_OFF)|BIT(HWTSTAMP_TX_ON);++info->rx_filters=BIT(HWTSTAMP_FILTER_NONE)|+BIT(HWTSTAMP_FILTER_PTP_V2_EVENT)|+BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT)|+BIT(HWTSTAMP_FILTER_PTP_V2_L4_EVENT);++return0;+}++staticvoidnetc_port_del_ptp_filter(structnetc_port*np)+{+structnetc_switch*priv=np->switch_priv;+u32entry_id;+inti;++for(i=0;i<NETC_PTP_MAX;i++){+entry_id=np->ptp_ipft_eid[i];+if(entry_id!=NTMP_NULL_ENTRY_ID){+if(!ntmp_ipft_delete_entry(&priv->ntmp,entry_id)){+np->ptp_ipft_eid[i]=NTMP_NULL_ENTRY_ID;+continue;+}++dev_err(priv->dev,+"Deleting PTP entry 0x%x (type %d) on port %d failed\n",+entry_id,i,np->dp->index);+}+}+}++staticintnetc_build_ptp_ipft_keye(structipft_keye_data*keye,intport,+enumnetc_ptp_typetype)+{+u16src_port,frm_attr_flags;++keye->precedence=cpu_to_le16(NETC_IPFT_PTP_PRECEDENCE);+src_port=FIELD_PREP(IPFT_SRC_PORT,port);+src_port|=IPFT_SRC_PORT_MASK;+keye->src_port=cpu_to_le16(src_port);++switch(type){+caseNETC_PTP_L2:+keye->ethertype=htons(ETH_P_1588);+keye->ethertype_mask=htons(0xffff);+break;+caseNETC_PTP_L4_IPV4_EVENT:+caseNETC_PTP_L4_IPV4_GENERAL:+caseNETC_PTP_L4_IPV6_EVENT:+caseNETC_PTP_L4_IPV6_GENERAL:+frm_attr_flags=IPFT_FAF_IP_HDR|FIELD_PREP(IPFT_FAF_L4_CODE,+IPFT_FAF_UDP_HDR);+if(type==NETC_PTP_L4_IPV6_EVENT||+type==NETC_PTP_L4_IPV6_GENERAL)+frm_attr_flags|=IPFT_FAF_IP_VER6;++keye->frm_attr_flags=cpu_to_le16(frm_attr_flags);++/* Set IP version bit in flags_mask to match IPv4 or IPv6+*packets+*/+frm_attr_flags|=IPFT_FAF_IP_VER6;+keye->frm_attr_flags_mask=cpu_to_le16(frm_attr_flags);+keye->ip_protocol=IPPROTO_UDP;+keye->ip_protocol_mask=0xff;++if(type==NETC_PTP_L4_IPV4_EVENT||+type==NETC_PTP_L4_IPV6_EVENT)+keye->l4_dst_port=htons(PTP_EV_PORT);+else+keye->l4_dst_port=htons(PTP_GEN_PORT);++keye->l4_dst_port_mask=htons(0xffff);+break;+default:+return-ERANGE;+}++return0;+}++staticintnetc_port_add_ipft_ptp_entry(structnetc_port*np,+enumnetc_ptp_typetype)+{+structnetc_switch*priv=np->switch_priv;+structipft_entry_data*entry;+structipft_keye_data*keye;+u32cfg;+interr;++/* The previously configured PTP entry may not have been successfully+*deleted,sothereisnoneedtoconfigureitagain.+*/+if(np->ptp_ipft_eid[type]!=NTMP_NULL_ENTRY_ID)+return0;++entry=kzalloc_obj(*entry);+if(!entry)+return-ENOMEM;++keye=&entry->keye;+err=netc_build_ptp_ipft_keye(keye,np->dp->index,type);+if(err)+gotofree_entry;++cfg=FIELD_PREP(IPFT_FLTFA,IPFT_FLTFA_REDIRECT);+cfg|=FIELD_PREP(IPFT_HR,NETC_HR_PTP_TRAP);+cfg|=IPFT_TIMECAPE|IPFT_RRT;+entry->cfge.cfg=cpu_to_le32(cfg);++err=ntmp_ipft_add_entry(&priv->ntmp,entry);+if(err)+gotofree_entry;++np->ptp_ipft_eid[type]=entry->entry_id;++free_entry:+kfree(entry);++returnerr;+}++staticintnetc_port_add_l2_ptp_filter(structnetc_port*np)+{+returnnetc_port_add_ipft_ptp_entry(np,NETC_PTP_L2);+}++staticintnetc_port_add_l4_ptp_filter(structnetc_port*np)+{+interr;++err=netc_port_add_ipft_ptp_entry(np,NETC_PTP_L4_IPV4_EVENT);+if(err)+returnerr;++err=netc_port_add_ipft_ptp_entry(np,NETC_PTP_L4_IPV4_GENERAL);+if(err)+gotodel_ptp_filter;++err=netc_port_add_ipft_ptp_entry(np,NETC_PTP_L4_IPV6_EVENT);+if(err)+gotodel_ptp_filter;++err=netc_port_add_ipft_ptp_entry(np,NETC_PTP_L4_IPV6_GENERAL);+if(err)+gotodel_ptp_filter;++return0;++del_ptp_filter:+netc_port_del_ptp_filter(np);++returnerr;+}++staticintnetc_port_add_l2_l4_ptp_filter(structnetc_port*np)+{+interr;++err=netc_port_add_l2_ptp_filter(np);+if(err)+returnerr;++err=netc_port_add_l4_ptp_filter(np);+if(err)+gotodel_ptp_filter;++return0;++del_ptp_filter:+netc_port_del_ptp_filter(np);++returnerr;+}++staticintnetc_port_set_ptp_filter(structnetc_port*np,intrx_filter)+{+interr=0;++netc_port_del_ptp_filter(np);+np->ptp_rx_filter=HWTSTAMP_FILTER_NONE;++switch(rx_filter){+caseHWTSTAMP_FILTER_NONE:+break;+caseHWTSTAMP_FILTER_PTP_V2_L2_EVENT:+err=netc_port_add_l2_ptp_filter(np);+break;+caseHWTSTAMP_FILTER_PTP_V2_L4_EVENT:+err=netc_port_add_l4_ptp_filter(np);+break;+caseHWTSTAMP_FILTER_PTP_V2_EVENT:+err=netc_port_add_l2_l4_ptp_filter(np);+break;+default:+err=-ERANGE;+}++if(err)+returnerr;++np->ptp_rx_filter=rx_filter;++return0;+}++intnetc_port_hwtstamp_set(structdsa_switch*ds,intport,+structkernel_hwtstamp_config*config,+structnetlink_ext_ack*extack)+{+structnetc_port*np=NETC_PORT(ds,port);+structnetc_switch*priv=ds->priv;+intrx_filter,err;++if((config->tx_type!=HWTSTAMP_TX_OFF||+config->rx_filter!=HWTSTAMP_FILTER_NONE)&&+!priv->tmr_dev)+return-EOPNOTSUPP;++switch(config->tx_type){+caseHWTSTAMP_TX_ON:+caseHWTSTAMP_TX_OFF:+break;+default:+return-ERANGE;+}++switch(config->rx_filter){+caseHWTSTAMP_FILTER_NONE:+rx_filter=HWTSTAMP_FILTER_NONE;+break;+caseHWTSTAMP_FILTER_PTP_V2_L4_EVENT:+caseHWTSTAMP_FILTER_PTP_V2_L4_SYNC:+caseHWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:+rx_filter=HWTSTAMP_FILTER_PTP_V2_L4_EVENT;+break;+caseHWTSTAMP_FILTER_PTP_V2_L2_EVENT:+caseHWTSTAMP_FILTER_PTP_V2_L2_SYNC:+caseHWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:+rx_filter=HWTSTAMP_FILTER_PTP_V2_L2_EVENT;+break;+caseHWTSTAMP_FILTER_PTP_V2_EVENT:+caseHWTSTAMP_FILTER_PTP_V2_SYNC:+caseHWTSTAMP_FILTER_PTP_V2_DELAY_REQ:+rx_filter=HWTSTAMP_FILTER_PTP_V2_EVENT;+break;+default:+return-ERANGE;+}++err=netc_port_set_ptp_filter(np,rx_filter);+if(err){+NL_SET_ERR_MSG_MOD(extack,"Failed to set PTP filter");+returnerr;+}++WRITE_ONCE(np->ptp_tx_type,config->tx_type);++config->rx_filter=rx_filter;++return0;+}++intnetc_port_hwtstamp_get(structdsa_switch*ds,intport,+structkernel_hwtstamp_config*config)+{+structnetc_port*np=NETC_PORT(ds,port);++config->tx_type=READ_ONCE(np->ptp_tx_type);+config->rx_filter=np->ptp_rx_filter;++return0;+}++staticvoidnetc_port_txtstamp_twostep(structnetc_port*np,+structsk_buff*nskb)+{+DECLARE_BITMAP(ts_req_id_bitmap,NETC_NUM_TS_REQ_ID);+structsk_buff*clone=skb_clone_sk(nskb);+structsk_buff*skb,*skb_tmp;+unsignedlongts_req_id;++if(unlikely(!clone))+return;++bitmap_zero(ts_req_id_bitmap,NETC_NUM_TS_REQ_ID);+spin_lock_bh(&np->tstamp_lock);++skb_queue_walk_safe(&np->skb_txtstamp_queue,skb,skb_tmp)+__set_bit(NETC_SKB_CB(skb)->ts_req_id,ts_req_id_bitmap);++ts_req_id=find_first_zero_bit(ts_req_id_bitmap,NETC_NUM_TS_REQ_ID);+if(ts_req_id==NETC_NUM_TS_REQ_ID){+dev_dbg_ratelimited(np->switch_priv->dev,+"Port %d has no available ts_req_id\n",+np->dp->index);+spin_unlock_bh(&np->tstamp_lock);+kfree_skb(clone);+return;+}++NETC_SKB_CB(nskb)->ptp_flag=NETC_PTP_FLAG_TWOSTEP;+NETC_SKB_CB(nskb)->ts_req_id=ts_req_id;+NETC_SKB_CB(clone)->ts_req_id=ts_req_id;+NETC_SKB_CB(clone)->ptp_tx_time=jiffies;+skb_shinfo(clone)->tx_flags|=SKBTX_IN_PROGRESS;+__skb_queue_tail(&np->skb_txtstamp_queue,clone);+if(!delayed_work_pending(&np->tstamp_timeout_work))+schedule_delayed_work(&np->tstamp_timeout_work,+NETC_TXTSTAMP_TIMEOUT);++spin_unlock_bh(&np->tstamp_lock);+}++voidnetc_port_twostep_tstamp_handler(structdsa_switch*ds,intport,+u8ts_req_id,u64ts)+{+structsk_buff*skb,*skb_tmp,*skb_match=NULL;+structnetc_port*np=NETC_PORT(ds,port);+structskb_shared_hwtstampshwtstamps;+structnetc_switch*priv=ds->priv;++spin_lock_bh(&np->tstamp_lock);+skb_queue_walk_safe(&np->skb_txtstamp_queue,skb,skb_tmp){+if(NETC_SKB_CB(skb)->ts_req_id!=ts_req_id)+continue;++__skb_unlink(skb,&np->skb_txtstamp_queue);+skb_match=skb;+break;+}+spin_unlock_bh(&np->tstamp_lock);++if(!skb_match){+dev_dbg_ratelimited(priv->dev,+"Port %d ts_req_id %u which seems lost\n",+port,ts_req_id);+return;+}++hwtstamps.hwtstamp=ns_to_ktime(ts);+skb_complete_tx_timestamp(skb_match,&hwtstamps);+}++boolnetc_port_rxtstamp(structdsa_switch*ds,intport,structsk_buff*skb,+unsignedinttype)+{+structskb_shared_hwtstamps*hwtstamps=skb_hwtstamps(skb);+u64ts=NETC_SKB_CB(skb)->tstamp;++/* ts == 0 indicates the hardware did not capture the RX timestamp+*oftheframe.+*/+if(!ts)+returnfalse;++hwtstamps->hwtstamp=ns_to_ktime(ts);++returnfalse;+}++voidnetc_port_txtstamp(structdsa_switch*ds,intport,structsk_buff*skb)+{+structnetc_port*np=NETC_PORT(ds,port);+inttx_type=READ_ONCE(np->ptp_tx_type);+u32ptp_class;++NETC_SKB_CB(skb)->ptp_flag=0;+ptp_class=ptp_classify_raw(skb);+if(ptp_class==PTP_CLASS_NONE)+return;++if(tx_type==HWTSTAMP_TX_ON)+netc_port_txtstamp_twostep(np,skb);+}
@@ -53,10 +53,16 @@#define NETC_FDBT_AGEING_DELAY (3 * HZ)#define NETC_FDBT_AGEING_THRESH 100+/* PTP frames have a higher priority, so a higher priority is defined to+*prioritizematching(Thehigherthevalue,thehigherthepriority).+*/+#define NETC_IPFT_PTP_PRECEDENCE 0xf000+structnetc_switch;structnetc_switch_info{u32num_ports;+u32tmr_devfn;void(*phylink_get_caps)(intport,structphylink_config*config);};
@@ -97,15 +118,60 @@ static void netc_fill_tp_tag_subtype0(struct sk_buff *skb,NETC_TAG_TP_SUBTYPE0_LEN);}-/* Currently only support To_Port tag, subtype 0 */+staticvoidnetc_fill_tp_tag_subtype2(structsk_buff*skb,+structnet_device*ndev)+{+u8ts_req_id=NETC_SKB_CB(skb)->ts_req_id;+structnetc_tag_tp_subtype2*tag;++tag=netc_fill_common_tp_tag(skb,ndev,NETC_TAG_TP_SUBTYPE2,+NETC_TAG_TP_SUBTYPE2_LEN);+tag->ts_req_id=FIELD_PREP(NETC_TAG_TS_REQ_ID,ts_req_id);+}+staticstructsk_buff*netc_xmit(structsk_buff*skb,structnet_device*ndev){-netc_fill_tp_tag_subtype0(skb,ndev);+u8ptp_flag=NETC_SKB_CB(skb)->ptp_flag;++/* Fast path: the overwhelming majority of frames are not PTP frames */+if(likely(!ptp_flag)){+netc_fill_tp_tag_subtype0(skb,ndev);+returnskb;+}elseif(ptp_flag==NETC_PTP_FLAG_TWOSTEP){+netc_fill_tp_tag_subtype2(skb,ndev);+}else{+kfree_skb(skb);+returnNULL;+}returnskb;}+staticvoidnetc_rx_tstamp_process(structnetc_tag_th_subtype1*tag,+structsk_buff*skb)+{+u64ts=get_unaligned_be64(&tag->timestamp);++NETC_SKB_CB(skb)->tstamp=ts;+}++staticvoidnetc_twostep_tstamp_process(structnetc_tag_th_subtype2*tag,+structsk_buff*skb)+{+u8ts_req_id=FIELD_GET(NETC_TAG_TS_REQ_ID,tag->hr_tsreq_id);+structdsa_port*dp=dsa_user_to_port(skb->dev);+u64ts=get_unaligned_be64(&tag->timestamp);+structnetc_tagger_data*tagger_data;+structdsa_switch*ds=dp->ds;++tagger_data=ds->tagger_data;+if(unlikely(!tagger_data->twostep_tstamp_handler))+return;++tagger_data->twostep_tstamp_handler(ds,dp->index,ts_req_id,ts);+}+staticintnetc_get_rx_tag_len(inttype,intsubtype){/* Only NETC_TAG_TO_HOST and NETC_TAG_FORWARD are expected in RX,
@@ -156,17 +233,50 @@ static struct sk_buff *netc_rcv(struct sk_buff *skb,if(!skb->dev)gotoerr_free_skb;+/* skb->cb may be used to store hardware RX timestamp, so it must+*beclearedbeforeprocessingtoavoiddatapollutionfromthe+*previouslayer.+*/+NETC_SKB_CB(skb)->tstamp=0;+type=FIELD_GET(NETC_TAG_TYPE,tag_cmn->type);subtype=FIELD_GET(NETC_TAG_SUBTYPE,tag_cmn->type);if(type==NETC_TAG_FORWARD){dsa_default_offload_fwd_mark(skb);}elseif(type==NETC_TAG_TO_HOST){-/* Currently only subtype0 supported */-if(subtype!=NETC_TAG_TH_SUBTYPE0)+switch(subtype){+caseNETC_TAG_TH_SUBTYPE0:+break;+caseNETC_TAG_TH_SUBTYPE1:+/* To_Host Subtype 1 tag is 14 bytes, ensure it and the+*EtherTypebehinditarefullypresentinthelinear+*areabeforenetc_rcv()callsdsa_strip_etype_header()+*tostripthetag.+*/+if(unlikely(!pskb_may_pull(skb,+NETC_TAG_TH_SUBTYPE1_LEN)))+gotoerr_free_skb;++tag=dsa_etype_header_pos_rx(skb);+netc_rx_tstamp_process(tag,skb);+break;+caseNETC_TAG_TH_SUBTYPE2:+/* This skb is a hardware-generated response to a+*two-steptransmittimestamprequest.Thetag+*drivermustfreetheskbafterprocessing.+*/+netc_twostep_tstamp_process(tag,skb);+consume_skb(skb);+returnNULL;+default:+dev_warn_ratelimited(&ndev->dev,+"Unsupported To_Host subtype: %d\n",+subtype);gotoerr_free_skb;+}}else{dev_warn_ratelimited(&ndev->dev,-"Unexpected tag type %d\n",type);+"Unexpected tag type %d\n",type);gotoerr_free_skb;}
@@ -190,16 +300,48 @@ static void netc_flow_dissect(const struct sk_buff *skb, __be16 *proto,inttype=FIELD_GET(NETC_TAG_TYPE,tag_cmn->type);inttag_len=netc_get_rx_tag_len(type,subtype);-/* The RX minimum frame length of the NETC switch port is 64 bytes,-*andtheframeisreceivedbytheENETCdriver.Fromthehardware-*perspective,thereceivebufferofRXBDisatleast128bytes,-*sotheswitchtagheaderisguaranteedtobeinthelinearregion-*oftheskb.+/* The CPU port of the switch is connected to the ENETC, so the frame+*isreceivedbytheENETCdriver.Fromthehardwareperspective,the+*receivebufferofRXBDisatleast128bytes,sotheswitchtag+*headerisguaranteedtobeinthelinearregionoftheskb.+*+*WhenthesubtypeoftheframeisNETC_TAG_TH_SUBTYPE2,itindicates+*theframeisahardwaregeneratedtimestampresponse,whichisonly+*26bytes(DMAC+SMAC+tag),sotheframehasnopayloadafterthe+*tag.Therefore,thereisnoneedtoparsetheprotocolandoffset.+*Forothertypesoftheframes,theyareallreceivedfromtheswitch+*ports,andtheRXminimumframelengthoftheportis64bytes,+*framesshorterthan64byteswillbediscardedbythehardwareand+*willnotbereceivedbythesoftware.*/+if(subtype==NETC_TAG_TH_SUBTYPE2)+return;+*offset=tag_len;*proto=((__be16*)skb->data)[(tag_len/2)-1];}+staticintnetc_connect(structdsa_switch*ds)+{+structnetc_tagger_data*tagger_data;++tagger_data=kzalloc_obj(*tagger_data);+if(!tagger_data)+return-ENOMEM;++ds->tagger_data=tagger_data;++return0;+}++staticvoidnetc_disconnect(structdsa_switch*ds)+{+structnetc_tagger_data*tagger_data=ds->tagger_data;++kfree(tagger_data);+ds->tagger_data=NULL;+}+staticconststructdsa_device_opsnetc_netdev_ops={.name=NETC_NAME,.proto=DSA_TAG_PROTO_NETC,
@@ -207,6 +349,8 @@ static const struct dsa_device_ops netc_netdev_ops = {.rcv=netc_rcv,.needed_headroom=NETC_TAG_MAX_LEN,.flow_dissect=netc_flow_dissect,+.connect=netc_connect,+.disconnect=netc_disconnect,};MODULE_DESCRIPTION("DSA tag driver for NXP NETC switch family");
From: Wei Fang <wei.fang@nxp.com>
The NETC switch supports one-step TX timestamping for PTP Sync frames.
When transmitting a Sync frame, the driver reads the current PTP time
and writes it into the frame's originTimestamp field, and inserts a
To_Port subtype 1 tag carrying the low 30 bits of that same PTP time.
As the frame leaves the wire, the MAC captures the SFD transmit time
and derives the residence time from the timestamp in the tag. It then
reads the correction field at the offset given by
PM_SINGLE_STEP[OFFSET], adds the residence time to it, and writes the
result back.
PM_SINGLE_STEP is a per-port register that can describe only one
in-flight frame at a time, and programming it requires reading the
current PTP time, which may sleep. Both constraints rule out handling
one-step Sync on the xmit path.
Instead, defer transmission to a per-port process-context work. The
xmit path classifies the frame in netc_port_txtstamp(): a genuine
one-step Sync (twoStepFlag cleared) has its PTP header offsets cached
in the skb control block; frames that cannot be handled as one-step
fall back to the two-step path or are sent as normal frames. netc_xmit()
hands the classified frame to the switch driver via the
onestep_sync_enqueue tagger callback, which queues it and kicks the work
if no frame is currently in flight.
The work dequeues one frame at a time, reads a fresh PTP time, programs
PM_SINGLE_STEP, updates the originTimestamp field, and transmits the
frame directly to the conduit via the onestep_sync_xmit tagger callback,
bypassing dsa_user_xmit() to avoid double-counting TX stats. Only one
frame is in flight at a time: the frame carries a TX-completion
destructor that reschedules the work when the conduit frees the skb,
keeping PM_SINGLE_STEP always matched to the frame being transmitted.
The one-step context is reference-counted and its lifetime is decoupled
from the devm-allocated netc_port. In-flight skbs hold a reference via
their destructor, so the context outlives port disable until the conduit
frees the last in-flight skb. Port disable clears onestep->active under
work_lock; a work that runs afterwards observes active cleared will
purge the onestep->queue and return without touching the freed port
resources.
Assisted-by: Wchat:claude-opus-4-8
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
drivers/net/dsa/netc/Kconfig | 1 +
drivers/net/dsa/netc/netc_main.c | 60 +++-
drivers/net/dsa/netc/netc_ptp.c | 439 +++++++++++++++++++++++++-
drivers/net/dsa/netc/netc_switch.h | 45 +++
drivers/net/dsa/netc/netc_switch_hw.h | 5 +
include/linux/dsa/tag_netc.h | 22 ++
net/dsa/tag_netc.c | 70 +++-
7 files changed, 633 insertions(+), 9 deletions(-)
@@ -292,13 +308,13 @@ static int netc_init_all_ports(struct netc_switch *priv)err=netc_port_get_info_from_dt(np,dp->dn,dev);if(err)-returnerr;+gotofree_user_ports;if(dsa_port_is_user(dp)){err=netc_port_create_mdio_bus(np,dp->dn);if(err){dev_err(dev,"Failed to create MDIO bus\n");-returnerr;+gotofree_user_ports;}/* The ipft_hf_eid is initialized to an invalid entry
@@ -314,11 +330,16 @@ static int netc_init_all_ports(struct netc_switch *priv)*/err=netc_port_ptp_init(np);if(err)-returnerr;+gotofree_user_ports;}}return0;++free_user_ports:+netc_free_user_ports(priv);++returnerr;}staticvoidnetc_init_ntmp_tbl_versions(structnetc_switch*priv)
@@ -941,7 +962,7 @@ static int netc_setup(struct dsa_switch *ds)err=netc_init_ntmp_user(priv);if(err)-gotoput_ptp_timer;+gotofree_user_ports;INIT_HLIST_HEAD(&priv->fdb_list);mutex_init(&priv->fdbt_lock);
@@ -980,6 +1001,8 @@ static int netc_setup(struct dsa_switch *ds)mutex_destroy(&priv->fdbt_lock);mutex_destroy(&priv->vft_lock);netc_free_ntmp_user(priv);+free_user_ports:+netc_free_user_ports(priv);put_ptp_timer:pci_dev_put(priv->tmr_dev);
@@ -1560,6 +1596,7 @@ static int netc_port_enable(struct dsa_switch *ds, int port,structphy_device*phy){structnetc_port*np=NETC_PORT(ds,port);+structnetc_onestep*onestep=np->onestep;interr;if(np->enable)
@@ -1572,6 +1609,12 @@ static int netc_port_enable(struct dsa_switch *ds, int port,returnerr;}+if(onestep){+mutex_lock(&onestep->work_lock);+onestep->active=true;+mutex_unlock(&onestep->work_lock);+}+np->enable=true;return0;
@@ -1580,6 +1623,7 @@ static int netc_port_enable(struct dsa_switch *ds, int port,staticvoidnetc_port_disable(structdsa_switch*ds,intport){structnetc_port*np=NETC_PORT(ds,port);+structnetc_onestep*onestep=np->onestep;/* When .port_disable() is called, .port_enable() may not have been*called.Inthiscase,boththeprepare_countandenable_countof
@@ -4,13 +4,307 @@*Copyright2025-2026NXP*/+#include<linux/kref.h>#include<linux/ptp_classify.h>#include<linux/ptp_clock_kernel.h>+#include<linux/slab.h>#include"netc_switch.h"#define NETC_NUM_TS_REQ_ID 16#define NETC_TXTSTAMP_TIMEOUT (5 * HZ)+#define NETC_MAX_STEP_OFFSET 0x1ff+#define NETC_ONESTEP_QTH 1024++staticvoidnetc_port_set_onestep_control(structnetc_port*np,+boolcsum_update,intoffset)+{+u32val;++val=PM_SINGLE_STEP_EN|FIELD_PREP(PM_SINGLE_STEP_OFFSET,offset);+if(csum_update)+val|=PM_SINGLE_STEP_CH;+netc_mac_port_wr(np,NETC_PM_SINGLE_STEP(0),val);+}++staticvoidnetc_port_purge_onestep_queue(structnetc_onestep*onestep,+boolclear_flight)+{+structsk_buff_headfree_list;++__skb_queue_head_init(&free_list);++spin_lock_bh(&onestep->queue_lock);+skb_queue_splice_init(&onestep->queue,&free_list);+if(clear_flight)+onestep->in_flight=false;+spin_unlock_bh(&onestep->queue_lock);++__skb_queue_purge(&free_list);+}++staticvoidnetc_onestep_destroy_work(structwork_struct*work)+{+structnetc_onestep*onestep=container_of(work,structnetc_onestep,+destroy_work);++/* refcnt reaching zero does not by itself mean onestep->work has+*stopped:thelastin-flightskbdestructorcallsschedule_work(&work)+**before*thenetc_onestep_put()thatdropsthefinalreference,soat+*themomentrefcnthitszeroonestep->workmaystillbependingor+*runningonanotherCPU.destroy_workandworkaredistinctwork_structs+*andcanrunconcurrently,socancel_work_sync()isrequiredtodrain+*onestep->workbeforemutex_destroy()/kfree()below,otherwisea+*still-runningworkwouldtouchfreedmemory.Nonewschedule_work(&work)+*canoccurafterthispointbecausenoreferencesremain,sothis+*cancelisfinal.+*/+cancel_work_sync(&onestep->work);+netc_port_purge_onestep_queue(onestep,true);+mutex_destroy(&onestep->work_lock);+kfree(onestep);+module_put(THIS_MODULE);+}++staticvoidnetc_onestep_release(structkref*ref)+{+structnetc_onestep*onestep=container_of(ref,structnetc_onestep,+refcnt);++/* This may be called from the skb destructor in softirq context+*(napi_consume_skb()),wherecancel_work_sync()mustnotbeused.+*Deferthefinalteardowntoprocesscontext.+*/+schedule_work(&onestep->destroy_work);+}++staticvoidnetc_onestep_get(structnetc_onestep*onestep)+{+kref_get(&onestep->refcnt);+}++voidnetc_onestep_put(structnetc_onestep*onestep)+{+kref_put(&onestep->refcnt,netc_onestep_release);+}++staticvoidnetc_onestep_skb_destructor(structsk_buff*skb)+{+structnetc_onestep*onestep=skb_shinfo(skb)->destructor_arg;++/* skb has been transmitted by hardware. Schedule work to send the next+*queuedone-stepSyncpacket,thenreleasethisskb'sreferenceonthe+*context.Iftheporthasalreadybeentorndownandthisisthelast+*reference,thecontextisfreedvianetc_onestep_release().+*/+schedule_work(&onestep->work);+netc_onestep_put(onestep);+}++staticvoidnetc_port_program_onestep(structnetc_port*np,+structnetc_onestep*onestep,+structsk_buff*skb,+u64tstamp)+{+u16correction_offset=NETC_SKB_CB(skb)->correction_offset;+u16tstamp_offset=NETC_SKB_CB(skb)->timestamp_offset;+u8*hdr=skb_mac_header(skb);+boolcsum_update=false;+__be32new_sec_l,new_ns;+__be16new_sec_h;+u64sec;+u32ns;++NETC_SKB_CB(skb)->tstamp=tstamp;+NETC_SKB_CB(skb)->ptp_flag=NETC_PTP_FLAG_ONESTEP;++/* Update originTimestamp field of Sync packet+*-48bitssecondsfield+*-32bitsnanosecondsfield+*/+sec=div_u64_rem(tstamp,NSEC_PER_SEC,&ns);+new_sec_h=htons((sec>>32)&0xffff);+new_sec_l=htonl(sec&0xffffffff);+new_ns=htonl(ns);++if(NETC_SKB_CB(skb)->is_udp){+__be32old_sec_l,old_ns;+structudphdr*uh;+__be16old_sec_h;++if(skb->ip_summed==CHECKSUM_PARTIAL){+csum_update=true;+gotoupdate_timestamp;+}++if(unlikely(!skb_transport_header_was_set(skb)))+uh=(structudphdr*)(hdr+tstamp_offset-+sizeof(structptp_header)-+sizeof(structudphdr));+else+uh=udp_hdr(skb);++/* For IPv4, a UDP checksum of zero on the wire means "no+*checksum". For IPv6, its UDP checksum is mandatory and+*neverzero.+*/+if(!uh->check)+gotoupdate_timestamp;++old_sec_h=__get_unaligned_t(__be16,hdr+tstamp_offset);+old_sec_l=__get_unaligned_t(__be32,hdr+tstamp_offset+2);+old_ns=__get_unaligned_t(__be32,hdr+tstamp_offset+6);+inet_proto_csum_replace2(&uh->check,skb,old_sec_h,+new_sec_h,false);+inet_proto_csum_replace4(&uh->check,skb,old_sec_l,+new_sec_l,false);+inet_proto_csum_replace4(&uh->check,skb,old_ns,+new_ns,false);+csum_update=true;+}++update_timestamp:+__put_unaligned_t(__be16,new_sec_h,hdr+tstamp_offset);+__put_unaligned_t(__be32,new_sec_l,hdr+tstamp_offset+2);+__put_unaligned_t(__be32,new_ns,hdr+tstamp_offset+6);++netc_port_set_onestep_control(np,csum_update,correction_offset);++/* Orphan the skb to release the socket send buffer quota immediately.+*Thisissafebecausesock_wfree()doesnotaccessskb->dataorany+*framecontent.Afterskb_orphan(),weinstallourowndestructorso+*thatwhentheconduitdriverfreestheskbafterTXcompletion,we+*getnotifiedtosendthenextqueuedSyncpacket.+*/+skb_orphan(skb);+netc_onestep_get(onestep);/* in-flight reference */+skb_shinfo(skb)->destructor_arg=onestep;+skb->destructor=netc_onestep_skb_destructor;+}++staticintnetc_get_phc_time(structnetc_switch*priv,u64*ns)+{+if(unlikely(!priv->tmr_dev))+return-ENODEV;++returnnetc_timer_get_current_time(priv->tmr_dev,ns);+}++staticvoidnetc_onestep_work(structwork_struct*work)+{+structnetc_onestep*onestep=container_of(work,structnetc_onestep,+work);+structnetc_tagger_data*tagger_data;+structnetc_switch*priv;+structnetc_port*np;+structsk_buff*skb;+u64tstamp;++/* Serialize the whole hardware access against port disable. work_lock+*isamutex(thisrunsinprocesscontextandnetc_get_phc_time()may+*sleep).Iftheporthasbeendisabled,bailoutimmediately;npand+*privareonlydereferencedafterthe@activecheckpasses,sothey+*arealwaysvalidhere.+*/+mutex_lock(&onestep->work_lock);+if(unlikely(!onestep->active)){+netc_port_purge_onestep_queue(onestep,true);+gotounlock_work;+}++/* Send only one queued Sync per run. The shared SINGLE_STEP register+*mustmatchtheframecurrentlybeingtransmitted,sothenextframe+*isprogrammedonlyafterthisonecompletesTX,whenitsskb+*destructorreschedulesthiswork.Dequeueunderonestep->queue_lock,+*andifthequeuehasdrained,releasethein-flightslotsoalater+*framefromthexmitpathkickstheworkagain.+*/+spin_lock_bh(&onestep->queue_lock);+skb=__skb_dequeue(&onestep->queue);+if(!skb){+onestep->in_flight=false;+spin_unlock_bh(&onestep->queue_lock);+gotounlock_work;+}+spin_unlock_bh(&onestep->queue_lock);++np=onestep->np;+priv=np->switch_priv;+if(unlikely(netc_get_phc_time(priv,&tstamp))){+/* The PTP timer is not available, so there is no correct+*timestamptoprogram.Dropthisframeandre-kicktoprocess+*theremainingqueuedframes.+*+*netc_port_program_onestep()hasnotrunforthisskbyet,so+*netc_onestep_skb_destructor()isnotinstalledonit.Freeing+*itthereforedoesnotreschedulethework,sotheworkmustbe+*rescheduledexplicitlytokeepdrainingthequeue.+*/+dev_dbg_ratelimited(priv->dev,+"Port %d PTP timer unavailable, drop Sync\n",+np->dp->index);+kfree_skb(skb);+schedule_work(&onestep->work);+gotounlock_work;+}++/* Reuse the offsets cached at enqueue time; only the timestamp is+*readfreshsoitreflectstheactualTXmoment.+*/+netc_port_program_onestep(np,onestep,skb,tstamp);++/* Tag and hand the frame directly to the conduit via the tagger,+*bypassingdsa_user_xmit()sotheTXstatsarenotcountedtwice.+*Andthereisnoneedtocheckiftagger_dataisNULL,because+*dsa_tree_teardown_ports()executesbefore+*dsa_switch_teardown_tag_protocol(),sotagger_datacannotbe+*NULLwhenonestep->activeisset.+*/+tagger_data=priv->ds->tagger_data;+tagger_data->onestep_sync_xmit(skb,np->dp->user);++unlock_work:+mutex_unlock(&onestep->work_lock);+}++staticintnetc_port_onestep_alloc(structnetc_port*np)+{+structnetc_onestep*onestep;+interr;++/* Hold a module reference until the last in-flight one-step Sync skb+*isfreedbytheconduit.Withoutthis,themodulecouldbeunloaded+*beforenetc_onestep_skb_destructor()returns,causingapanic.+*Releasedinnetc_onestep_destroy_work()afterallcleanupisdone.+*/+if(!try_module_get(THIS_MODULE)){+dev_err(np->switch_priv->dev,+"Failed to get the driver module\n");+return-ENODEV;+}++onestep=kzalloc_obj(*onestep);+if(!onestep){+err=-ENOMEM;+gotoput_module;+}++kref_init(&onestep->refcnt);/* port (owner) reference */+np->onestep=onestep;+onestep->np=np;+mutex_init(&onestep->work_lock);+spin_lock_init(&onestep->queue_lock);+__skb_queue_head_init(&onestep->queue);+INIT_WORK(&onestep->work,netc_onestep_work);+INIT_WORK(&onestep->destroy_work,netc_onestep_destroy_work);++return0;++put_module:+module_put(THIS_MODULE);++returnerr;+}staticvoidnetc_port_tstamp_timeout_work(structwork_struct*work){
@@ -57,7 +351,7 @@ int netc_port_ptp_init(struct netc_port *np)INIT_DELAYED_WORK(&np->tstamp_timeout_work,netc_port_tstamp_timeout_work);-return0;+returnnetc_port_onestep_alloc(np);}voidnetc_port_purge_txtstamp_queue(structnetc_port*np)
@@ -94,7 +388,8 @@ int netc_get_ts_info(struct dsa_switch *ds, int port,SOF_TIMESTAMPING_RX_HARDWARE|SOF_TIMESTAMPING_RAW_HARDWARE;-info->tx_types=BIT(HWTSTAMP_TX_OFF)|BIT(HWTSTAMP_TX_ON);+info->tx_types=BIT(HWTSTAMP_TX_OFF)|BIT(HWTSTAMP_TX_ON)|+BIT(HWTSTAMP_TX_ONESTEP_SYNC);info->rx_filters=BIT(HWTSTAMP_FILTER_NONE)|BIT(HWTSTAMP_FILTER_PTP_V2_EVENT)|
@@ -316,6 +611,7 @@ int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,switch(config->tx_type){caseHWTSTAMP_TX_ON:caseHWTSTAMP_TX_OFF:+caseHWTSTAMP_TX_ONESTEP_SYNC:break;default:return-ERANGE;
@@ -352,6 +648,9 @@ int netc_port_hwtstamp_set(struct dsa_switch *ds, int port,WRITE_ONCE(np->ptp_tx_type,config->tx_type);+if(config->tx_type!=HWTSTAMP_TX_ONESTEP_SYNC)+netc_port_purge_onestep_queue(np->onestep,false);+config->rx_filter=rx_filter;return0;
@@ -455,10 +754,100 @@ bool netc_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb,returnfalse;}+staticvoidnetc_port_prepare_onestep_sync(structnetc_port*np,+structsk_buff*skb,+u32ptp_class,bool*twostep)+{+structnetc_switch*priv=np->switch_priv;+u16correction_offset,tstamp_offset;+structptp_header*ptp_hdr;+u8msg_type,twostep_flag;+boolis_udp=false;+u32pkt_type;+u8*pkt_hdr;++if(unlikely(skb_linearize(skb))){+NETC_SKB_CB(skb)->ptp_flag=NETC_PTP_FLAG_DROP;+return;+}++ptp_hdr=ptp_parse_header(skb,ptp_class);+if(unlikely(!ptp_hdr)){+NETC_SKB_CB(skb)->ptp_flag=NETC_PTP_FLAG_DROP;+dev_dbg_ratelimited(priv->dev,+"Port %d failed to parse Sync header\n",+np->dp->index);+return;+}++msg_type=ptp_get_msgtype(ptp_hdr,ptp_class);+twostep_flag=ptp_hdr->flag_field[0]&0x2;++pkt_hdr=skb_mac_header(skb);+correction_offset=(u8*)&ptp_hdr->correction-pkt_hdr;+tstamp_offset=(u8*)ptp_hdr+sizeof(*ptp_hdr)-pkt_hdr;++/* Ensure that the entire originTimestamp field is present in the+*linearbufferoftheskbandthecorrection_offsetmustbewithin+*thehardwarecapability.+*/+if(unlikely(tstamp_offset+10>skb_headlen(skb)||+correction_offset>NETC_MAX_STEP_OFFSET)){+NETC_SKB_CB(skb)->ptp_flag=NETC_PTP_FLAG_DROP;+dev_dbg_ratelimited(priv->dev,+"Port %d PTP offset check error\n",+np->dp->index);+return;+}++/* Only a Sync frame with the twoStepFlag cleared can use one-step+*timestamping.Aframethatrequeststwo-step(orisnotaSync)+*carriesdifferenton-wirefields,sothisisarealclassification;+*reportitthrough*twostepsothecallerfallsbacktothetwo-step+*path.+*/+if(msg_type!=PTP_MSGTYPE_SYNC||twostep_flag!=0){+*twostep=true;+return;+}++/* This is a genuine one-step Sync frame. skb_shinfo()->destructor_arg+*islaterusedtopassthenp->onesteppointerto+*netc_onestep_skb_destructor()forTXcompletionnotification.+*MSG_ZEROCOPYalsousesdestructor_arg(viaskb_zcopy_init())to+*trackuser-spacepagereferences.Overwritingitinthatcasewould+*leaktheubuf_inforeferenceandpreventuserpagesfrombeing+*released.PTPapplicationsdonotuseMSG_ZEROCOPY,butguard+*againstitdefensively.+*/+if(skb_zcopy(skb)){+NETC_SKB_CB(skb)->ptp_flag=NETC_PTP_FLAG_DROP;+dev_dbg_ratelimited(priv->dev,+"Port %d one-step Sync not supported on zerocopy skb\n",+np->dp->index);+return;+}++pkt_type=ptp_class&PTP_CLASS_PMASK;+if(pkt_type==PTP_CLASS_IPV4||pkt_type==PTP_CLASS_IPV6)+is_udp=true;++/* Cache the parsing results so the tagger xmit path and the deferred+*workdonotneedtore-parsethePTPheader,andsothat+*netc_port_program_onestep()canderivetheseparametersfromthe+*skb.+*/+NETC_SKB_CB(skb)->correction_offset=correction_offset;+NETC_SKB_CB(skb)->timestamp_offset=tstamp_offset;+NETC_SKB_CB(skb)->is_udp=is_udp;+NETC_SKB_CB(skb)->ptp_flag=NETC_PTP_FLAG_ONESTEP;+}+voidnetc_port_txtstamp(structdsa_switch*ds,intport,structsk_buff*skb){structnetc_port*np=NETC_PORT(ds,port);inttx_type=READ_ONCE(np->ptp_tx_type);+booltwostep=false;u32ptp_class;NETC_SKB_CB(skb)->ptp_flag=0;
@@ -466,6 +855,50 @@ void netc_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)if(ptp_class==PTP_CLASS_NONE)return;-if(tx_type==HWTSTAMP_TX_ON)+if(tx_type==HWTSTAMP_TX_ONESTEP_SYNC)+netc_port_prepare_onestep_sync(np,skb,ptp_class,&twostep);++if(tx_type==HWTSTAMP_TX_ON||twostep)netc_port_txtstamp_twostep(np,skb);}++voidnetc_port_onestep_sync_enqueue(structdsa_switch*ds,intport,+structsk_buff*skb)+{+structnetc_port*np=NETC_PORT(ds,port);+structnetc_onestep*onestep=np->onestep;+boolkick=false;++/* This runs in the xmit path (softirq / BH-disabled), so it must not+*sleep:onlyqueuetheframehereandletnetc_onestep_work()+*programtheSINGLE_STEPregisterandtransmititfromprocess+*context.ThesharedSINGLE_STEPregistercandescribeonlyoneframe+*atatime,soatmostoneone-stepSyncmaybeinflight.Trackthat+*with@in_flightunderonestep->queue_lock.+*+*Enqueuetheframeand,onlyifnoframeiscurrentlyinflight,claim+*thein-flightslotandkickthework.Whenaframeisalreadyin+*flight,justqueue:itsskbdestructorwillkicktheworktosendthe+*nextoneonceitcompletesTX,sotheframesaretransmittedstrictly+*oneatatimeinorder.+*/+spin_lock_bh(&onestep->queue_lock);+if(unlikely(skb_queue_len(&onestep->queue)>=NETC_ONESTEP_QTH)){+spin_unlock_bh(&onestep->queue_lock);+kfree_skb(skb);+return;+}++__skb_queue_tail(&onestep->queue,skb);+if(!onestep->in_flight){+onestep->in_flight=true;+kick=true;+}+spin_unlock_bh(&onestep->queue_lock);++/* Ownership is transferred to the queue; netc_xmit() stops processing+*thisskb.Theworkwillprogramandtransmitit.+*/+if(kick)+schedule_work(&onestep->work);+}
@@ -87,6 +88,44 @@ enum netc_host_reason {NETC_HR_PTP_TRAP=9,};+/* One-step Sync serialization context.+*+*Itslifetimeisdecoupledfromthedevm-allocatednetc_port.Anin-flight+*one-stepSyncskbkeepsareferenceonthiscontextviaitsskbdestructor,+*sothecontextoutlivestheportteardownuntiltheconduitfreesthelast+*in-flightskbafterTXcompletion.Oncetheportisdisabled,@activeis+*clearedandtheworkstopstouchinganydevmmemory(netc_port/netc_switch)+*ortheunregisteredusernetdevorthetagger_data.+*/+structnetc_onestep{+structnetc_port*np;+structkrefrefcnt;+/* Process-context lock: serializes the deferred TX work against port+*teardown,sotheworknevertouchesthedevm-allocatednetc_port/+*netc_switchortheunregisteredusernetdevafterteardown.Held+*acrossnetc_get_phc_time(),whichmaysleep,henceamutex.+*/+structmutexwork_lock;+/* Serialize access to in_flight and queue */+spinlock_tqueue_lock;+boolactive;/* set when port is enabled, under @work_lock */+/* In-flight slot: true while one one-step Sync frame is programmed+*intothesharedSINGLE_STEPregisterandbeingtransmitted.Onlyone+*framemaybeinflightatatime,sothenextqueuedframeissent+*onlyafterthecurrentonecompletesTX(itsskbdestructorkicks+*thework).Accessedunderqueue_lock,fromboththesoftirqxmit+*pathandtheprocess-contextwork.+*/+boolin_flight;+/* Pending one-step Sync frames. Enqueued from the softirq xmit path and+*dequeuedbytheprocess-contextwork;thelistisserializedby+*queue_locktogetherwith@in_flight.+*/+structsk_buff_headqueue;+structwork_structwork;/* drains @queue */+structwork_structdestroy_work;/* frees the context in process ctx */+};+structnetc_port{void__iomem*iobase;structnetc_switch*switch_priv;
@@ -129,6 +150,42 @@ static void netc_fill_tp_tag_subtype2(struct sk_buff *skb,tag->ts_req_id=FIELD_PREP(NETC_TAG_TS_REQ_ID,ts_req_id);}+staticnetdev_tx_tnetc_onestep_sync_xmit(structsk_buff*skb,+structnet_device*dev)+{+/* This deferred one-step Sync frame already went through+*dsa_user_xmit()'sskb_ensure_writable_head_tail()beforeitwas+*queuedinnetc_xmit(),andnothinghascloneditorshrunkits+*headroomsince.Sotheheadroomisstillguaranteedandthe+*skbisstillwritable;onlythetagneedstobepushedbefore+*handingitdirectlytotheconduit,bypassingdsa_user_xmit()+*sothatdev_sw_netstats_tx_add()isnotinvokedasecondtime+*forthesameframe.+*/+netc_fill_tp_tag_subtype1(skb,dev);++returndsa_enqueue_skb(skb,dev);+}++staticvoidnetc_onestep_sync_enqueue(structsk_buff*skb,+structnet_device*ndev)+{+structdsa_port*dp=dsa_user_to_port(ndev);+structnetc_tagger_data*tagger_data;++tagger_data=dp->ds->tagger_data;+if(unlikely(!tagger_data->onestep_sync_enqueue)){+kfree_skb(skb);+return;+}++/* Hand the one-step Sync to the switch driver, which takes ownership+*andqueuesitfordeferredtransmissionfromitswork.Thetagger+*mustnottouchtheskbafterthispoint.+*/+tagger_data->onestep_sync_enqueue(dp->ds,dp->index,skb);+}+staticstructsk_buff*netc_xmit(structsk_buff*skb,structnet_device*ndev){
@@ -138,9 +195,19 @@ static struct sk_buff *netc_xmit(struct sk_buff *skb,if(likely(!ptp_flag)){netc_fill_tp_tag_subtype0(skb,ndev);returnskb;+}++if(ptp_flag==NETC_PTP_FLAG_ONESTEP){+/* The switch driver takes ownership of the one-step Sync and+*queuesitfordeferredTX;thedeferredworktagsitsubtype1+*andtransmitsitdirectlytotheconduit.ReturnNULLso+*dsa_user_xmit()stopsprocessingthisskb.+*/+netc_onestep_sync_enqueue(skb,ndev);+returnNULL;}elseif(ptp_flag==NETC_PTP_FLAG_TWOSTEP){netc_fill_tp_tag_subtype2(skb,ndev);-}else{+}else{/* NETC_PTP_FLAG_DROP */kfree_skb(skb);returnNULL;}
@@ -329,6 +396,7 @@ static int netc_connect(struct dsa_switch *ds)if(!tagger_data)return-ENOMEM;+tagger_data->onestep_sync_xmit=netc_onestep_sync_xmit;ds->tagger_data=tagger_data;return0;
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-01 15:09:51
On Mon, 31 Aug 2026 16:23:27 +0800 wei.fang@oss.nxp.com wrote:
Subject: [PATCH v3 net-next 0/8] net: dsa: netc: add PTP support for NETC switch
Please be advised that the limit of 15 patches applies across
the series. You posted 2 large series to net-next at once.
See "Limit patches outstanding on mailing list" in the docs.
If this happens again we'll be forced to start discarding your patches.
On Mon, 31 Aug 2026 16:23:27 +0800 wei.fang@oss.nxp.com wrote:
quoted
Subject: [PATCH v3 net-next 0/8] net: dsa: netc: add PTP support for NETC
switch
Please be advised that the limit of 15 patches applies across
the series. You posted 2 large series to net-next at once.
See "Limit patches outstanding on mailing list" in the docs.
If this happens again we'll be forced to start discarding your patches.
Oh, sorry, I was not aware of the rule has been updated. I will repost
this series when the ENETC VF series is applied to net-next tree, thanks.
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 1 · Low: 1
- [High] Using device_lock() in an exported API to protect against driver
unbind is an architectural anti-pattern that creates a circular lock…
- [Medium] The newly exported netc_timer_get_current_time() gates the
TMR_CUR_TIME MMIO read only on 'a netc_timer driver is bound' and…
- [Low] Inconsistent / semantically wrong errno contract for the new
exported API netc_timer_get_current_time().
--
ptp: netc: export netc_timer_get_current_time() for cross-driver use
This adds netc_timer_get_current_time() to the NETC Timer driver and
exports it so the NETC Switch driver, which is a separate PCIe function
with its own driver, can read TMR_CUR_TIME. The helper takes the device
lock to serialize against unbind and the per-device spinlock around the
register read, and spin_lock_init() moves earlier so the lock is
initialized before pci_set_drvdata().
@@ -1021,5 +1021,55 @@ static struct pci_driver netc_timer_driver = { }; module_pci_driver(netc_timer_driver);+/**+ * netc_timer_get_current_time - read the current PTP time from the NETC Timer+ * @pdev: PCI device of the NETC Timer+ * @ns: The current PTP clock time in nanoseconds, returned to the caller+ *+ * Reads the 64-bit current time register (TMR_CUR_TIME) from the NETC Timer+ * device associated with @pdev. Returns an error if the Timer driver has not+ * yet probed or has already been removed.+ *+ * Context: Process context only. Acquires the device mutex via device_lock(),+ * which may sleep. Must not be called from atomic context, softirq,+ * BH, or while holding a spinlock.+ *+ * Return: 0 on success, otherwise a negative error code.+ */+int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)+{+ struct device *dev = &pdev->dev;+ struct netc_timer *priv;+ unsigned long flags;+ int err = 0;++ /* Serialize against driver unbind: the remove() callback runs under+ * the device lock, so holding it here ensures that priv remains valid+ * for the entire duration of the register read.+ */+ device_lock(dev);++ if (pci_dev_driver(pdev) != &netc_timer_driver) {+ err = -EINVAL;+ goto unlock_device;+ }++ priv = pci_get_drvdata(pdev);+ if (!priv) {+ err = -ENOMEM;+ goto unlock_device;+ }
[Severity: High]
Taking the supplier's device lock from an exported cross-driver
helper worries me, both for the lock ordering it creates and for the
contexts it rules out.
On the ordering side, consider the consumer added later in this
series. netc_switch_remove() runs with the switch device's dev->mutex
held by the driver core; it calls dsa_unregister_switch(), which takes
rtnl_lock(), and netc_port_disable(), which takes
&onestep->work_lock. That gives dev->mutex -> rtnl_lock ->
work_lock. In the datapath, netc_onestep_work() holds
&onestep->work_lock and then calls netc_timer_get_current_time(),
which takes the timer's dev->mutex here. Since lockdep tracks the
dev->mutex of devices initialized through device_initialize() as one
lock class, that second chain is work_lock -> dev->mutex and closes
the cycle. A splat there disables lock debugging for the rest of the
boot, so it is not something we can leave to be discovered by
whoever unbinds the switch while Sync frames are in flight.
On the context side, the kernel-doc you added already spells out the
consequence: process context only, may sleep, no softirq, no BH, no
spinlock held. That is a fairly severe restriction for what is
fundamentally a single 64-bit register read on a timestamping path,
and it is imposed purely by the unbind protection rather than by the
hardware access itself.
The usual way to express "this consumer needs that supplier to stay
bound" is a device link. If the switch driver called
device_link_add() against the Timer's pci_dev at probe time, the
Timer could not be unbound underneath a live consumer, and
netc_timer_get_current_time() could drop device_lock() entirely and
keep only the spin_lock_irqsave(&priv->lock) around
netc_timer_cur_time_read(), making it callable from atomic context
too.
Is there a reason a device link would not work for the
Timer/Switch pair here (for example probe ordering between the two
PCIe functions)? If so, could you describe what else guarantees the
lock ordering above is safe?
[Severity: Low]
This isn't a bug, but the errno contract of the new exported interface
looks inconsistent.
The same logical state, "the NETC Timer is not available", is reported
three different ways:
- netc_timer_get_current_time() returns -EINVAL when no netc_timer
driver is bound
- netc_timer_get_current_time() returns -ENOMEM when
pci_get_drvdata() is NULL
- the !IS_REACHABLE(CONFIG_PTP_NETC_V4_TIMER) stub in
include/linux/fsl/netc_global.h returns -ENODEV
The -ENOMEM in particular does not match what happened, since this
function performs no allocation at all, and the kernel-doc above
describes the only failure condition as "the Timer driver has not yet
probed or has already been removed".
Would -ENODEV for both in-function checks, matching the header stub,
make the interface easier for callers to test?
[Severity: Medium]
Can this read return a fabricated time and still report success while
the Timer function is suspended?
The two checks above only establish that a netc_timer driver is bound
and that drvdata is non-NULL. Neither implies that the Timer PCI
function still decodes memory, that it is in D0, or that TMR_CTRL[TE]
is still set.
netc_timer_driver registers no PM callbacks:
drivers/ptp/ptp_netc.c:
static struct pci_driver netc_timer_driver = {
.name = KBUILD_MODNAME,
.id_table = netc_timer_id_table,
.probe = netc_timer_probe,
.remove = netc_timer_remove,
};
so during system suspend the PCI core takes the no-dev_pm_ops path:
pci_pm_suspend()
pci_pm_default_suspend()
pci_disable_enabled_device() /* clears PCI_COMMAND_MEMORY */
-> D3hot
The driver is not unbound in that window, so pci_dev_driver(pdev) ==
&netc_timer_driver and pci_get_drvdata(pdev) != NULL both still pass,
ioread64_lo_hi() reads all-ones, and the function returns 0. The
cross-driver consumer added later in this series treats a 0 return as a
valid time:
drivers/net/dsa/netc/netc_ptp.c:netc_onestep_work() {
...
if (unlikely(netc_get_phc_time(priv, &tstamp))) {
...
}
and programs the value into a transmitted one-step Sync frame and into
PM_SINGLE_STEP.
Does the same apply after resume? Nothing re-runs netc_timer_init(), so
TMR_CTRL (CK_SEL/TE/TCLK_PERIOD/COMP_MODE), TMR_PRSC, TMR_ADD and
TMR_CNT are never reprogrammed after the D3hot->D0 transition, while
the exported reader keeps reporting success. The driver's own comment
documents these as preconditions:
drivers/ptp/ptp_netc.c:netc_timer_init() {
/* Software must enable timer first and the clock selected must be
* active, otherwise, the registers which are in the timer clock
* domain are not accessible.
*/
tmr_ctrl = FIELD_PREP(TMR_CTRL_CK_SEL, priv->clk_select) |
TMR_CTRL_TE | TMR_CTRL_FS;
...
}
The lack of suspend/resume support in ptp_netc predates this patch, but
this patch turns that unchecked power and enable state into a
cross-driver contract. Would it make sense for the exported helper to
also check pci_dev->current_state == PCI_D0 (or TMR_CTRL[TE], or take a
PM reference), or for the driver to gain suspend/resume handlers that
re-run netc_timer_init()?
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 2 (1)
- [Low] struct netc_port is devm_kzalloc()'d, so the new `u32
ipft_hf_eid` starts at 0, but the patch only writes the…
- [Low] Removing netc_free_host_flood_rules() also removes the only in-
tree documentation of a deliberate design decision: 'No need to clear…
Pre-existing issues:
- [Low] netc_port_remove_host_flood() discards the return status of
ntmp_ipft_delete_entry() (drivers/net/dsa/netc/netc_main.c:1776) and…
--
net: dsa: netc: use entry ID instead of pointer to track host flood rule
This replaces the struct ipft_entry_data pointer in struct netc_port with a
u32 entry ID using NTMP_NULL_ENTRY_ID as the sentinel, frees the
ipft_entry_data allocation inside netc_port_add_host_flood_rule() once the
hardware entry is committed, and drops netc_free_host_flood_rules().
A few questions below.
@@ -286,6 +286,12 @@ static int netc_init_all_ports(struct netc_switch *priv)dev_err(dev,"Failed to create MDIO bus\n");returnerr;}++/* The ipft_hf_eid is initialized to an invalid entry+*IDbecausethehostfloodrule(IPFTentry)hasnot+*beencreated.+*/+np->ipft_hf_eid=NTMP_NULL_ENTRY_ID;}}
[Severity: Low]
Should the sentinel be written for every port rather than only inside the
dsa_port_is_user(dp) branch?
Each netc_port is devm_kzalloc()'d in the earlier loop of
netc_init_all_ports(), so ipft_hf_eid starts at 0, which is a legal
hardware IPFT entry ID rather than "no rule". The old field was a pointer,
where the zeroed value (NULL) was inherently the safe "no rule" marker.
netc_port_remove_host_flood() only recognizes the 0xffffffff sentinel:
if (entry_id == NTMP_NULL_ENTRY_ID)
return;
...
ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
so a port that never got the assignment would pass that guard and delete
hardware entry ID 0, which may belong to another port.
I could not find a path that reaches netc_port_remove_host_flood() or
netc_port_bridge_join() with a non-user port today - dsa_port_set_host_flood()
comes only from dsa_user_manage_host_flood(), and the bridge join/leave
callers all operate on user ports - so this looks latent rather than
reachable. Would moving the assignment into the devm_kzalloc() loop that
sets np->switch_priv and np->iobase make the representation fail-safe for
all ports?
quoted hunk
@@ -938,30 +944,12 @@ static void netc_destroy_all_lists(struct netc_switch *priv) mutex_destroy(&priv->vft_lock); }-static void netc_free_host_flood_rules(struct netc_switch *priv)-{- struct dsa_port *dp;-- dsa_switch_for_each_user_port(dp, priv->ds) {- struct netc_port *np = priv->ports[dp->index];-- /* No need to clear the hardware IPFT entry. Because PCIe- * FLR will be performed when the switch is re-registered,- * it will reset hardware state. So only need to free the- * memory to avoid memory leak.- */- kfree(np->host_flood);- np->host_flood = NULL;- }-}- static void netc_teardown(struct dsa_switch *ds) { struct netc_switch *priv = ds->priv; disable_delayed_work_sync(&priv->fdbt_ageing_work); netc_destroy_all_lists(priv);- netc_free_host_flood_rules(priv); netc_free_ntmp_user(priv); }
[Severity: Low]
Could the deleted rationale be preserved somewhere? Behaviour is unchanged
(netc_teardown() still leaves the host flood IPFT entries programmed in
hardware), but this hunk removes the only in-tree record of why that is
intentional:
/* No need to clear the hardware IPFT entry. Because PCIe
* FLR will be performed when the switch is re-registered,
* it will reset hardware state. ...
*/
Note that the FLR assumption holds for a PCI re-probe, where
netc_switch_pci_probe() calls pcie_flr(pdev), but a DSA teardown/setup cycle
on the same pci_dev does not perform FLR. Would it help to keep the comment
in netc_teardown() or next to the ipft_hf_eid field so the assumption can be
re-checked later?
quoted hunk
@@ -1759,37 +1747,36 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but can the
IPFT entry become unremovable here?
ntmp_ipft_delete_entry() can fail (DMA buffer allocation or command BD
error), and its status is discarded before np->ipft_hf_eid is reset to
NTMP_NULL_ENTRY_ID, so the armed entry (FLTFA=REDIRECT, HR=NETC_HR_HOST_FLOOD)
stays in hardware with no software handle left.
This appears to be addressed by the following patch in the series, which
makes netc_port_remove_host_flood() return int, checks the delete status
before clearing np->ipft_hf_eid/uc/mc, and handles the error at all call
sites.
quoted hunk
@@ -1818,20 +1805,18 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port, /* IPFT does not support in-place updates to the KEYE element, * we need to add a new entry and then delete the old one. So- * save the old entry first.+ * save the old entry ID first. */- old_host_flood = np->host_flood;- np->host_flood = NULL;+ old_entry_id = np->ipft_hf_eid; if (netc_port_add_host_flood_rule(np, uc, mc)) {- np->host_flood = old_host_flood; dev_err(ds->dev, "Failed to add host flood rule on port %d\n", port); return; } /* Remove the old host flood entry */- netc_port_remove_host_flood(np, old_host_flood);+ netc_port_remove_host_flood(np, old_entry_id); }
[Severity: Low]
Same pre-existing concern in this key-update flow, which this patch does not
change: because the new entry is added before the old one is deleted, a
failed delete leaves two live IPFT entries matching the same source port with
PIPFCR still enabled. The subsequent patch in the series inverts the order to
delete-then-add, which closes that window.
@@ -84,7 +84,11 @@ struct netc_port {u16uc:1;u16mc:1;u16pvid;-structipft_entry_data*host_flood;+/* ipft_hf_eid applies only to user ports and should be initialized+*toNTMP_NULL_ENTRY_ID.Otherports(suchasCPUports)donot+*requireinitialization.+*/+u32ipft_hf_eid;};
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 3
- [Medium] Commit message does not match the code in three ways.
- [Medium] netc_port_remove_host_flood()
(drivers/net/dsa/netc/netc_main.c:1773-1792) now treats every error…
- [Low] This user-visible bug fix carries no `Fixes:` tag (and no stable
Cc).
- [Low] Format-specifier/argument signedness mismatch in the three newly
added dev_err() calls: they print the DSA port index with `%u` while…
- [Low] netc_port_set_host_flood() is changed from make-before-break (add
new entry, then delete old) to break-before-make…
--
net: dsa: netc: check return value of ntmp_ipft_delete_entry()
This converts netc_port_remove_host_flood() to return int, propagates the
error from ntmp_ipft_delete_entry() to its callers, and reorders
netc_port_set_host_flood() to delete the old IPFT host flood entry before
adding the new one, with rollback paths in netc_port_set_host_flood() and
netc_port_bridge_join().
A few questions below, both about the changelog and the new error paths.
[Severity: Medium]
The changelog says:
"In netc_port_add_host_flood_rule(), delete the old
entry before adding the new one, and guard against adding a new entry
when the old one still exists."
Is the first half of that sentence accurate? The post-patch
netc_port_add_host_flood_rule() has no call to ntmp_ipft_delete_entry() at
all; the only delete call site in the driver is
netc_port_remove_host_flood(). The helper only gains the early bail-out
that returns 0 when an entry already exists with the same uc/mc and -EINVAL
otherwise. The delete-before-add reordering actually lives in
netc_port_set_host_flood() and netc_port_bridge_join().
Could the changelog also mention that the helper loses its "disable" side
effect? Before the patch, netc_port_add_host_flood_rule(np, false, false)
wrote NETC_PIPFCR = 0 and cleared np->uc/np->mc; now it just returns 0
without touching hardware. netc_port_bridge_leave() still calls
netc_port_add_host_flood_rule(np, uc, mc) with a possible (false, false) and
now relies on PIPFCR already being 0.
Lastly:
"such failures only occur under resource exhaustion conditions that are
already abnormal"
Does that hold? netc_xmit_ntmp_cmd() in
drivers/net/ethernet/freescale/enetc/ntmp.c also returns -EBUSY when no
control ring BD is free, -ETIMEDOUT from read_poll_timeout() (NETC_CBDR_TIMEOUT
is 1000 us), and -EIO on a command BD system bus error or a non-zero writeback
error status. A failed rollback can therefore reflect a persistent device or
control ring problem rather than transient memory pressure.
[Severity: Low]
Should this carry a Fixes: tag (and possibly a stable Cc)? The bug being
fixed, netc_port_remove_host_flood() ignoring the return value of
ntmp_ipft_delete_entry() and then dropping np->ipft_hf_eid so a live
hardware IPFT entry becomes untracked, was added by commit 46d6407692c8
("net: dsa: netc: add FDB, STP, MTU, port setup and host flooding
support"), which is already upstream, so released kernels are affected.
@@ -1707,15 +1707,23 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,u32cfg;interr;-if(!uc&&!mc){-/* Disable ingress port filter table lookup */-netc_port_wr(np,NETC_PIPFCR,0);-np->uc=false;-np->mc=false;+if(np->ipft_hf_eid!=NTMP_NULL_ENTRY_ID){+/* The old rule is the same as the new rule, return directly.+*Butiftheexistingrulediffersfromtherequestedone,+*returnanerror.+*/+if(np->uc==uc&&np->mc==mc)+return0;-return0;+dev_err(priv->dev,+"The old host flood rule has not been removed from port %u\n",+np->dp->index);+return-EINVAL;}+if(!uc&&!mc)+return0;+host_flood=kzalloc_obj(*host_flood);if(!host_flood)return-ENOMEM;
[Severity: Medium]
Can this new -EINVAL guard make a divergent np->ipft_hf_eid permanently
sticky? See the question on netc_port_remove_host_flood() below: once
np->ipft_hf_eid is left set after a delete that the hardware may in fact
have executed, every later netc_port_add_host_flood_rule() with different
uc/mc returns -EINVAL, including the restore done by
netc_port_bridge_leave().
[Severity: Medium]
Is every error from ntmp_ipft_delete_entry() safe to interpret as "the
entry is still programmed in hardware"? The NTMP command path does not
look transactional:
drivers/net/ethernet/freescale/enetc/ntmp.c:netc_xmit_ntmp_cmd() {
...
cbdr->next_to_use = i;
netc_write(cbdr->regs.pir, i);
err = read_poll_timeout(netc_read, val, (val & NETC_CBDRCIR_INDEX) == i,
NETC_CBDR_DELAY_US, NETC_CBDR_TIMEOUT, true,
cbdr->regs.cir);
if (unlikely(err))
return err;
...
}
The DELETE BD and the producer index are published to hardware before the
1 ms completion poll, so -ETIMEDOUT means "completion not observed", not
"command not executed"; -EIO from the writeback error status is similarly
indeterminate. In those cases np->ipft_hf_eid keeps an entry ID the
hardware may no longer own, and there is no other release path:
ntmp_ipft_delete_entry() has this single caller and nothing else clears
np->ipft_hf_eid (this is still the case at the end of the series).
Two follow-on effects:
netc_port_bridge_join() now returns the delete error, so if the retry keeps
failing (for example the hardware reports an error status for a DELETE of a
non-existent entry ID) the port can never be enslaved again from an ordinary
sequence such as "ip link set swpX promisc on" followed by "ip link set swpX
master br0". Before this patch the error was ignored and the software state
was resynchronised, so the port stayed usable.
IPFT entry IDs are hardware assigned from a table shared by all ports (later
patches in this series add per-port PTP entries in np->ptp_ipft_eid[]), so a
retry on a stale ID could delete an unrelated rule.
Whether the retry ends up as a permanent dead end or a wrong-entry deletion
depends on how the hardware answers a DELETE for a freed or reallocated
entry ID, which can't be determined from the tree, so this part is a
question rather than a claim.
quoted hunk
- if (disable_host_flood) {- np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;- np->uc = false;- np->mc = false;- netc_port_wr(np, NETC_PIPFCR, 0);- }+ return 0; } static void netc_port_set_host_flood(struct dsa_switch *ds, int port, bool uc, bool mc) { struct netc_port *np = NETC_PORT(ds, port);- u32 old_entry_id;+ bool old_uc = np->uc;+ bool old_mc = np->mc; /* Do not add host flood rule to ingress port filter table when * the port has joined a bridge. Otherwise, the ingress frames
@@ -1795,7 +1804,12 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port, * will be redirected directly to the CPU port. */ if (dsa_port_bridge_dev_get(np->dp)) {- netc_port_remove_host_flood(np, np->ipft_hf_eid);+ if (!netc_port_remove_host_flood(np))+ return;++ dev_err(ds->dev,+ "Failed to delete host flood rule on bridge port %u\n",+ port);
[Severity: Low]
Should this be %d? port is declared int in netc_port_set_host_flood(struct
dsa_switch *ds, int port, ...), matching the .port_set_host_flood prototype
in include/net/dsa.h. The pre-existing messages in the same function print
the same variable with %d, so the file now uses both conventions for the
same value. The new dev_err() in netc_port_add_host_flood_rule() correctly
uses %u because it prints np->dp->index, which is unsigned. The mismatch
is only diagnosed with -Wformat-signedness.
quoted hunk
return;
}
@@ -1804,19 +1818,29 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port, return; /* IPFT does not support in-place updates to the KEYE element,- * we need to add a new entry and then delete the old one. So- * save the old entry ID first.+ * we need to delete the old one and then add the new rule. If+ * the deletion fails, return immediately. */- old_entry_id = np->ipft_hf_eid;+ if (netc_port_remove_host_flood(np)) {+ dev_err(ds->dev,+ "Failed to delete old host flood rule on port %u\n",+ port);
[Severity: Low]
Same %u versus int port question here.
[Severity: Low]
This turns the update from make-before-break into break-before-make. At
this commit netc_port_remove_host_flood() also writes NETC_PIPFCR = 0, so
between the delete and the following add the port's ingress filter lookup
is disarmed and the host flood redirect to the CPU port is inactive.
If the following add fails (kzalloc_obj() returning -ENOMEM,
dma_alloc_coherent() inside ntmp_ipft_add_entry(), -EBUSY on the control
ring, IPFT full), the function returns before np->ipft_hf_eid is set and
before netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN), and the rollback add
re-enters the identical allocation path, so it is likely to fail for the
same reason. Since .port_set_host_flood returns void, the netdev/bridge
layer still believes promisc/allmulti host flooding is in effect.
The PIPFCR half of this is removed later in the same series by "net: dsa:
netc: enable ingress port filtering lookup by default", which drops both
PIPFCR writes from these helpers and enables PIPFCR_EN in
netc_port_fixed_config(), so at the end of the series only the narrower
window on the IPFT entry itself remains, which is the limitation documented
in the comment below.
quoted hunk
++ return;+ } if (netc_port_add_host_flood_rule(np, uc, mc)) { dev_err(ds->dev, "Failed to add host flood rule on port %d\n", port);- return;- }- /* Remove the old host flood entry */- netc_port_remove_host_flood(np, old_entry_id);+ /* Try to restore the old flood rule, if the recovery fails,+ * there is nothing else we can do; this is a limitation.+ */+ if (netc_port_add_host_flood_rule(np, old_uc, old_mc))+ dev_err(ds->dev,+ "Failed to restore host flood rule on port %d\n",+ port);+} static int netc_single_vlan_aware_bridge(struct dsa_switch *ds,
[ ... ]
quoted hunk
@@ -1992,6 +2018,12 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port, if (err) return err;+ err = netc_port_remove_host_flood(np);+ if (err) {+ NL_SET_ERR_MSG_MOD(extack, "Failed to delete host flood rule");+ return err;+ }+
[Severity: Medium]
This is the path that makes the stale np->ipft_hf_eid question above
user visible: if the deletion keeps failing, bridge enslavement of that
port fails every time, and nothing in the driver resets np->ipft_hf_eid.
Would it be reasonable to distinguish "hardware confirmed the entry is
still there" from "completion unknown" before deciding to keep the entry
ID?
netc_port_set_mlo(np, MLO_NOT_OVERRIDE);
if (br_vlan_enabled(bridge.dev))
[ ... ]
quoted hunk
@@ -2016,6 +2046,11 @@ static int netc_port_bridge_join(struct dsa_switch *ds, int port, disable_mlo: netc_port_set_mlo(np, MLO_DISABLE);+ if (netc_port_add_host_flood_rule(np, uc, mc))+ dev_err(ds->dev,+ "Failed to restore host flood rule on port %u\n",+ port);+
[Severity: Low]
%u again for the int port argument of netc_port_bridge_join();
netc_port_bridge_leave() prints the same value with %d.
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 3 · Low: 1
- [Medium] Failed IPFT deletion leaves PTP trap rules programmed in
hardware while the driver reports the new (possibly NONE) rx_filter as…
- [Medium] netc_get_ts_info() and netc_port_hwtstamp_set() use different
definitions of 'hardware timestamping available'.
- [Medium] The 4-bit two-step timestamp request ID is recycled purely on
the basis of 'the clone is no longer on np->skb_txtstamp_queue'.
- [Low] netc_flow_dissect() (net/dsa/tag_netc.c:295) decides to skip
protocol/offset parsing based on the tag's subtype nibble alone: 'if…
--
net: dsa: netc: add PTP two-step timestamping support
This adds RX timestamping by installing IPFT rules that redirect PTP frames
to the CPU port, plus two-step TX timestamping where a cloned skb is matched
against a hardware-generated response frame using a 4-bit request ID. The
tagger gains To_Port subtype 2 and To_Host subtype 1/2 handling and
netc_connect()/netc_disconnect() to manage the shared netc_tagger_data.
A few questions on the new PTP paths below.
[Severity: Medium]
Can a request ID be handed out again while its response is still outstanding
in hardware?
netc_port_tstamp_timeout_work() unlinks and frees the clone after 5 seconds,
and the allocator in netc_port_txtstamp_twostep() derives the free IDs purely
from what is currently on the queue:
skb_queue_walk_safe(&np->skb_txtstamp_queue, skb, skb_tmp)
__set_bit(NETC_SKB_CB(skb)->ts_req_id, ts_req_id_bitmap);
ts_req_id = find_first_zero_bit(ts_req_id_bitmap, NETC_NUM_TS_REQ_ID);
So once a clone has timed out, nothing records that a To_Host subtype 2
response for that ID may still arrive.
If egress of the original frame is stalled past the timeout (link down, or
sustained 802.3x PAUSE from the link partner), the late response reaches
netc_port_twostep_tstamp_handler(), which matches on port and ts_req_id only,
with no generation tag:
if (NETC_SKB_CB(skb)->ts_req_id != ts_req_id)
continue;
__skb_unlink(skb, &np->skb_txtstamp_queue);
Would the newly queued clone then be completed with a timestamp that is
seconds old, and the correct response for that clone later dropped as
unmatched? Would a generation counter, or holding a timed-out ID back from
reuse, avoid this?
quoted hunk
++ /* Reschedule if there are still pending clones that have not+ * timed out yet.+ */+ if (!skb_queue_empty(&np->skb_txtstamp_queue))+ schedule_delayed_work(&np->tstamp_timeout_work,+ NETC_TXTSTAMP_TIMEOUT);++ spin_unlock_bh(&np->tstamp_lock);+ __skb_queue_purge(&free_list);+}
[ ... ]
quoted hunk
+static int netc_get_phc_index(struct netc_switch *priv)+{+ if (!priv->tmr_dev)+ return -1;++ return ptp_clock_index_by_dev(&priv->tmr_dev->dev);+}++int netc_get_ts_info(struct dsa_switch *ds, int port,+ struct kernel_ethtool_ts_info *info)+{+ struct netc_switch *priv = ds->priv;++ info->phc_index = netc_get_phc_index(priv);+ if (info->phc_index < 0)+ return 0;++ info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |+ SOF_TIMESTAMPING_RX_HARDWARE |+ SOF_TIMESTAMPING_RAW_HARDWARE;++ info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);++ info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |+ BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |+ BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |+ BIT(HWTSTAMP_FILTER_PTP_V2_L4_EVENT);++ return 0;+}++static void netc_port_del_ptp_filter(struct netc_port *np)+{+ struct netc_switch *priv = np->switch_priv;+ u32 entry_id;+ int i;++ for (i = 0; i < NETC_PTP_MAX; i++) {+ entry_id = np->ptp_ipft_eid[i];+ if (entry_id != NTMP_NULL_ENTRY_ID) {+ if (!ntmp_ipft_delete_entry(&priv->ntmp, entry_id)) {+ np->ptp_ipft_eid[i] = NTMP_NULL_ENTRY_ID;+ continue;+ }++ dev_err(priv->dev,+ "Deleting PTP entry 0x%x (type %d) on port %d failed\n",+ entry_id, i, np->dp->index);+ }+ }+}
[ ... ]
quoted hunk
+static int netc_port_add_ipft_ptp_entry(struct netc_port *np,+ enum netc_ptp_type type)+{+ struct netc_switch *priv = np->switch_priv;+ struct ipft_entry_data *entry;+ struct ipft_keye_data *keye;+ u32 cfg;+ int err;++ /* The previously configured PTP entry may not have been successfully+ * deleted, so there is no need to configure it again.+ */+ if (np->ptp_ipft_eid[type] != NTMP_NULL_ENTRY_ID)+ return 0;
+static int netc_port_set_ptp_filter(struct netc_port *np, int rx_filter)+{+ int err = 0;++ netc_port_del_ptp_filter(np);+ np->ptp_rx_filter = HWTSTAMP_FILTER_NONE;++ switch (rx_filter) {+ case HWTSTAMP_FILTER_NONE:+ break;
[ ... ]
quoted hunk
+ if (err)+ return err;++ np->ptp_rx_filter = rx_filter;++ return 0;+}
[Severity: Medium]
What happens here when ntmp_ipft_delete_entry() fails?
netc_port_del_ptp_filter() is void, and on failure it deliberately keeps
np->ptp_ipft_eid[i] populated and only logs the error. netc_port_set_ptp_filter()
calls it unconditionally, and on the HWTSTAMP_FILTER_NONE path err is never
assigned, so np->ptp_rx_filter is committed and SIOCSHWTSTAMP returns success.
The surviving entries were installed with:
cfg = FIELD_PREP(IPFT_FLTFA, IPFT_FLTFA_REDIRECT);
cfg |= FIELD_PREP(IPFT_HR, NETC_HR_PTP_TRAP);
cfg |= IPFT_TIMECAPE | IPFT_RRT;
so would that port keep diverting all matching PTP frames to the CPU port
(they are no longer bridged) and keep capturing ingress timestamps, while
netc_port_hwtstamp_get() reports the filter as off?
The reverse case is in netc_port_add_ipft_ptp_entry():
if (np->ptp_ipft_eid[type] != NTMP_NULL_ENTRY_ID)
return 0;
If the hardware actually executed the delete but the command was reported as
failed (command ring timeout after execution), a later enable request becomes
a silent no-op. Would RX timestamping then stay dead while the driver reports
the filter as enabled?
Nothing retries the deletion, and netc_free_ports_resources() does not touch
np->ptp_ipft_eid[], so the divergence would persist until the driver is
unbound. Would it be reasonable to propagate the delete error out of
netc_port_del_ptp_filter() and fail the ioctl instead?
[Severity: Medium]
Should this gate use the same condition as netc_get_ts_info()?
get_ts_info() advertises no timestamping capability at all unless a PHC is
registered for tmr_dev:
info->phc_index = netc_get_phc_index(priv);
if (info->phc_index < 0)
return 0;
while the set path only tests that the timer's PCI function exists, and
netc_setup()'s own comment says that function is created during bus
enumeration "even if the timer driver has not probed yet".
Neither net_hwtstamp_validate() nor dsa_user_hwtstamp_set() cross-checks the
driver's ts_info, so HWTSTAMP_TX_ON with HWTSTAMP_FILTER_PTP_V2_EVENT succeeds
and installs the IPFT redirect entries while ethtool reports that the device
supports no hardware timestamping.
The same applies with CONFIG_PTP_1588_CLOCK=n, which the new "depends on
PTP_1588_CLOCK_OPTIONAL" now permits: ptp_clock_index_by_dev() is stubbed to
-1 and ptp_classify_raw() returns PTP_CLASS_NONE, so no timestamp can ever be
produced, yet the set path still returns 0.
On the same subject, netc_port_twostep_tstamp_handler() passes the echoed
value straight on:
hwtstamps.hwtstamp = ns_to_ktime(ts);
skb_complete_tx_timestamp(skb_match, &hwtstamps);
whereas the RX path treats zero as "not captured":
/* ts == 0 indicates the hardware did not capture the RX timestamp
* of the frame.
*/
if (!ts)
return false;
Would a zero or frozen counter value from a timer that is not running be
delivered to the socket error queue as a genuine hardware TX timestamp?
[ ... ]
@@ -190,16 +300,48 @@ static void netc_flow_dissect(const struct sk_buff *skb, __be16 *proto, int type = FIELD_GET(NETC_TAG_TYPE, tag_cmn->type); int tag_len = netc_get_rx_tag_len(type, subtype);- /* The RX minimum frame length of the NETC switch port is 64 bytes,- * and the frame is received by the ENETC driver. From the hardware- * perspective, the receive buffer of RX BD is at least 128 bytes,- * so the switch tag header is guaranteed to be in the linear region- * of the skb.+ /* The CPU port of the switch is connected to the ENETC, so the frame+ * is received by the ENETC driver. From the hardware perspective, the+ * receive buffer of RX BD is at least 128 bytes, so the switch tag+ * header is guaranteed to be in the linear region of the skb.+ *+ * When the subtype of the frame is NETC_TAG_TH_SUBTYPE2, it indicates+ * the frame is a hardware generated timestamp response, which is only+ * 26 bytes (DMAC + SMAC + tag), so the frame has no payload after the+ * tag. Therefore, there is no need to parse the protocol and offset.+ * For other types of the frames, they are all received from the switch+ * ports, and the RX minimum frame length of the port is 64 bytes,+ * frames shorter than 64 bytes will be discarded by the hardware and+ * will not be received by the software. */+ if (subtype == NETC_TAG_TH_SUBTYPE2)+ return;+
[Severity: Low]
Should this test also require type == NETC_TAG_TO_HOST?
Everywhere else in this file the subtype nibble is only meaningful under the
To_Host type. netc_get_rx_tag_len() returns NETC_TAG_FORWARD_LEN for any
subtype when the type is not To_Host:
if (type == NETC_TAG_TO_HOST) {
if (subtype == NETC_TAG_TH_SUBTYPE1)
return NETC_TAG_TH_SUBTYPE1_LEN;
else if (subtype == NETC_TAG_TH_SUBTYPE2)
return NETC_TAG_TH_SUBTYPE2_LEN;
else
return NETC_TAG_TH_SUBTYPE0_LEN;
}
return NETC_TAG_FORWARD_LEN;
and netc_rcv() only enters the subtype switch inside its
"else if (type == NETC_TAG_TO_HOST)" branch.
If a Forward-tagged frame (type nibble 0) ever carries a subtype nibble of 2,
netc_flow_dissect() returns before either output is written:
*offset = tag_len;
*proto = ((__be16 *)skb->data)[(tag_len / 2) - 1];
__skb_flow_dissect() would then continue with proto == ETH_P_XDSA at offset 0
and produce an all-zero flow key, so those frames all hash to the same bucket
for RPS/RFS/GRO with no diagnostic. Does the hardware guarantee the subtype
nibble is zero in Forward tags, or would
"type == NETC_TAG_TO_HOST && subtype == NETC_TAG_TH_SUBTYPE2" be the safer
guard?
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 2 · Medium: 2 · Low: 0 (1)
- [High] netc_port_onestep_alloc() takes try_module_get(THIS_MODULE) for
every user port at switch setup time and the matching module_put()…
- [High] The one-frame-at-a-time interlock protecting the shared per-port
PM_SINGLE_STEP register uses skb->destructor as the 'frame has…
- [Medium] netc_port_prepare_onestep_sync()
(drivers/net/dsa/netc/netc_ptp.c) performs its unsupported-case checks…
- [Medium] netc_port_prepare_onestep_sync() accepts whatever
ptp_classify_raw() returns, including PTP_CLASS_V1_IPV4/V1_IPV6, and…
Pre-existing issues:
- [Low] The three new drop sites (NETC_PTP_FLAG_DROP handling in
netc_xmit(), the NETC_ONESTEP_QTH queue-full drop in…
--
net: dsa: netc: add PTP one-step timestamping support
This adds one-step TX timestamping for PTP Sync frames on the NETC switch.
The xmit path classifies a Sync frame and hands it to a per-port work,
which reads the PTP time, programs PM_SINGLE_STEP, patches the
originTimestamp field and transmits the frame to the conduit via a new
tagger callback.
+static void netc_onestep_skb_destructor(struct sk_buff *skb)+{+ struct netc_onestep *onestep = skb_shinfo(skb)->destructor_arg;++ /* skb has been transmitted by hardware. Schedule work to send the next+ * queued one-step Sync packet, then release this skb's reference on the+ * context. If the port has already been torn down and this is the last+ * reference, the context is freed via netc_onestep_release().+ */+ schedule_work(&onestep->work);+ netc_onestep_put(onestep);+}
[Severity: High]
Is skb->destructor a reliable "the frame has egressed" signal for the
PM_SINGLE_STEP interlock?
netc_port_program_onestep() calls skb_orphan(skb), which leaves
skb->sk == NULL, and then installs this destructor. Any later orphaning
fires and clears the destructor before the frame is transmitted, for
example in net/sched/sch_fq.c:fq_classify():
if (!sk || sk_listener_or_tw(sk)) {
unsigned long hash = skb_get_hash(skb) & q->orphan_mask;
...
skb_orphan(skb);
Since skb->sk was already cleared by the driver, this triggers at enqueue
time for every one-step frame when the conduit uses fq, and sch_netem does
the same via skb_orphan_partial(). A qdisc drop frees the skb too. In
those cases the in-flight slot is released while the frame is still queued,
and the real TX completion can no longer kick the work.
Even in the normal case, conduit TX completion only means the frame was
handed to the switch; it still has to traverse the switch and be scheduled
on the user port MAC. Can netc_onestep_work() then reprogram OFFSET/CH for
frame N+1 while frame N is still waiting for egress, so the residence time
is patched at the wrong offset when consecutive frames differ in
encapsulation (L2 versus UDP, VLAN versus untagged)?
The commit message states the opposite invariant:
"keeping PM_SINGLE_STEP always matched to the frame being transmitted"
Is there anything that structurally guarantees this, given there is no
egress confirmation from the destination port?
+update_timestamp:+ __put_unaligned_t(__be16, new_sec_h, hdr + tstamp_offset);+ __put_unaligned_t(__be32, new_sec_l, hdr + tstamp_offset + 2);+ __put_unaligned_t(__be32, new_ns, hdr + tstamp_offset + 6);++ netc_port_set_onestep_control(np, csum_update, correction_offset);++ /* Orphan the skb to release the socket send buffer quota immediately.+ * This is safe because sock_wfree() does not access skb->data or any+ * frame content. After skb_orphan(), we install our own destructor so+ * that when the conduit driver frees the skb after TX completion, we+ * get notified to send the next queued Sync packet.+ */+ skb_orphan(skb);+ netc_onestep_get(onestep); /* in-flight reference */+ skb_shinfo(skb)->destructor_arg = onestep;+ skb->destructor = netc_onestep_skb_destructor;+}
+static int netc_port_onestep_alloc(struct netc_port *np)+{+ struct netc_onestep *onestep;+ int err;++ /* Hold a module reference until the last in-flight one-step Sync skb+ * is freed by the conduit. Without this, the module could be unloaded+ * before netc_onestep_skb_destructor() returns, causing a panic.+ * Released in netc_onestep_destroy_work() after all cleanup is done.+ */+ if (!try_module_get(THIS_MODULE)) {+ dev_err(np->switch_priv->dev,+ "Failed to get the driver module\n");+ return -ENODEV;+ }
[Severity: High]
Does this make the module permanently unloadable while the switch is bound?
The reference is taken once per user port from netc_setup() ->
netc_init_all_ports() -> netc_port_ptp_init() -> netc_port_onestep_alloc(),
not per in-flight skb, so it is held for the whole bound lifetime. The
matching module_put(THIS_MODULE) only runs from netc_onestep_destroy_work(),
which requires the owner kref to be dropped in netc_free_ports_resources(),
i.e. it requires the remove path:
rmmod -> delete_module() -> try_stop_module() -> try_release_module_ref()
That returns non-zero with references outstanding and delete_module() fails
with -EWOULDBLOCK, so mod->exit (pci_unregister_driver) never runs and the
kref is never dropped. Is there a path that can ever release these
references?
Separately, module_put(THIS_MODULE) is the last statement of
netc_onestep_destroy_work(), which lives in module text. If that were the
final reference, can free_module() unmap the text while this work function
is still returning and while the workqueue core still has
worker->current_func pointing into it?
The in-flight skb already holds a kref on the context, so is the module
self-reference adding anything beyond blocking rmmod?
[Severity: Medium]
Should this path require PTP_CLASS_V2?
ptp_classify_raw() masks the version nibble and can return
PTP_CLASS_V1_IPV4 / PTP_CLASS_V1_IPV6, and ptp_parse_header() only length
checks, so a legacy PTPv1 event message reaches here. For v1,
ptp_get_msgtype() deliberately reads hdr->control:
include/linux/ptp_classify.h:ptp_get_msgtype() {
if (unlikely(type & PTP_CLASS_V1))
/* msg type is located at the control field for ptp v1 */
msgtype = hdr->control;
...
}
For a PTPv1 Sync that field is 0, which equals PTP_MSGTYPE_SYNC, so the
message-type test passes. twostep_flag is then read from
ptp_hdr->flag_field[0], which in a PTPv1 header is a byte of the 16-byte
subdomain name, and for common subdomains that byte has bit 1 clear, so the
frame is accepted as a genuine one-step Sync.
netc_port_program_onestep() then writes 10 bytes at ptp_header+34 (not the
v1 originTimestamp), programs PM_SINGLE_STEP[OFFSET] with ptp_header+8
(inside the v1 subdomain), and fixes up the UDP checksum over those wrong
bytes. Can a PTPv1 Sync be corrupted on the wire this way?
quoted hunk
++ /* Ensure that the entire originTimestamp field is present in the+ * linear buffer of the skb and the correction_offset must be within+ * the hardware capability.+ */+ if (unlikely(tstamp_offset + 10 > skb_headlen(skb) ||+ correction_offset > NETC_MAX_STEP_OFFSET)) {+ NETC_SKB_CB(skb)->ptp_flag = NETC_PTP_FLAG_DROP;+ dev_dbg_ratelimited(priv->dev,+ "Port %d PTP offset check error\n",+ np->dp->index);+ return;+ }++ /* Only a Sync frame with the twoStepFlag cleared can use one-step+ * timestamping. A frame that requests two-step (or is not a Sync)+ * carries different on-wire fields, so this is a real classification;+ * report it through *twostep so the caller falls back to the two-step+ * path.+ */+ if (msg_type != PTP_MSGTYPE_SYNC || twostep_flag != 0) {+ *twostep = true;+ return;+ }
[Severity: Medium]
Should the Sync/twoStepFlag classification happen before the
unsupported-case checks?
As written, the order is skb_linearize() failure -> DROP,
ptp_parse_header() == NULL -> DROP, tstamp_offset + 10 > skb_headlen() ||
correction_offset > NETC_MAX_STEP_OFFSET -> DROP, and only then the
msg_type / twostep_flag test. So any PTP frame that trips one of the
earlier checks, including a Delay_Req that only wants a two-step timestamp,
gets NETC_PTP_FLAG_DROP and is freed by netc_xmit() instead of taking the
two-step fallback.
ptp_classify_raw() only inspects the first two bytes of the PTP payload,
while ptp_parse_header() needs 34 bytes and this code needs
tstamp_offset + 10 bytes in the linear area. Can a short frame sent to UDP
port 319 by any local sender therefore be silently discarded regardless of
its message type, with only a dev_dbg_ratelimited() trace? The same applies
to skb_linearize() failure under memory pressure.
For comparison, enetc_xmit() falls back to two-step when the parse fails and
still transmits the frame.
netc_onestep_work() adds a fourth drop site: when netc_get_phc_time() fails,
an already-accepted Sync is freed with kfree_skb().
The commit message states:
"frames that cannot be handled as one-step fall back to the two-step
path or are sent as normal frames"
Can that be reconciled with the four kfree_skb() sites, or should the
description be adjusted?
[ ... ]
[Severity: Low]
This isn't a bug introduced by this patch, but the new drop sites do not
touch any drop counter. The queue-full drop here, the
NETC_PTP_FLAG_DROP branch in netc_xmit(), and the PHC-failure drop in
netc_onestep_work() all free the skb after dsa_user_xmit() has already
accounted it:
net/dsa/user.c:dsa_user_xmit() {
dev_sw_netstats_tx_add(dev, 1, skb->len);
...
}
So tx_packets counts frames that never left the box while tx_dropped stays
zero. The accounting side is pre-existing framework behaviour (dsa_user_xmit()
itself drops on skb_ensure_writable_head_tail() failure without accounting),
but would it be worth bumping a drop counter at the new sites?
From: Jakub Kicinski <kuba@kernel.org> Date: 2026-09-05 00:52:36
On Mon, 31 Aug 2026 16:23:35 +0800 wei.fang@oss.nxp.com wrote:
The NETC switch supports one-step TX timestamping for PTP Sync frames.
When transmitting a Sync frame, the driver reads the current PTP time
and writes it into the frame's originTimestamp field, and inserts a
To_Port subtype 1 tag carrying the low 30 bits of that same PTP time.
As the frame leaves the wire, the MAC captures the SFD transmit time
and derives the residence time from the timestamp in the tag. It then
reads the correction field at the offset given by
PM_SINGLE_STEP[OFFSET], adds the residence time to it, and writes the
result back.
Please spend more time on this code, especially this patch.
This really looks like clumsy slop, some DSA expert will
have to review it before it gets merged.
Hi Wei,
thanks for your patch!
On Mon, Aug 31, 2026 at 10:24 AM [off-list ref] wrote:
From: Wei Fang <wei.fang@nxp.com>
The NETC switch supports one-step TX timestamping for PTP Sync frames.
When transmitting a Sync frame, the driver reads the current PTP time
and writes it into the frame's originTimestamp field, and inserts a
To_Port subtype 1 tag carrying the low 30 bits of that same PTP time.
As the frame leaves the wire, the MAC captures the SFD transmit time
and derives the residence time from the timestamp in the tag. It then
reads the correction field at the offset given by
PM_SINGLE_STEP[OFFSET], adds the residence time to it, and writes the
result back.
PM_SINGLE_STEP is a per-port register that can describe only one
in-flight frame at a time, and programming it requires reading the
current PTP time, which may sleep. Both constraints rule out handling
one-step Sync on the xmit path.
Instead, defer transmission to a per-port process-context work. The
xmit path classifies the frame in netc_port_txtstamp(): a genuine
one-step Sync (twoStepFlag cleared) has its PTP header offsets cached
in the skb control block; frames that cannot be handled as one-step
fall back to the two-step path or are sent as normal frames. netc_xmit()
hands the classified frame to the switch driver via the
onestep_sync_enqueue tagger callback, which queues it and kicks the work
if no frame is currently in flight.
The work dequeues one frame at a time, reads a fresh PTP time, programs
PM_SINGLE_STEP, updates the originTimestamp field, and transmits the
frame directly to the conduit via the onestep_sync_xmit tagger callback,
bypassing dsa_user_xmit() to avoid double-counting TX stats. Only one
frame is in flight at a time: the frame carries a TX-completion
destructor that reschedules the work when the conduit frees the skb,
keeping PM_SINGLE_STEP always matched to the frame being transmitted.
The one-step context is reference-counted and its lifetime is decoupled
from the devm-allocated netc_port. In-flight skbs hold a reference via
their destructor, so the context outlives port disable until the conduit
frees the last in-flight skb. Port disable clears onestep->active under
work_lock; a work that runs afterwards observes active cleared will
purge the onestep->queue and return without touching the freed port
resources.
Assisted-by: Wchat:claude-opus-4-8
Signed-off-by: Wei Fang <wei.fang@nxp.com>
You agent produces "dostoyevsky commits" with too much verbiage.
Tell you agent to "be terse and to the point" in your AGENTS.md.
quoted hunk
++ /* dsa_tree_teardown() calls dsa_tree_teardown_ports() before+ * dsa_tree_teardown_switches(), so netc_port_disable() is+ * executed before netc_teardown() and onestep->active is set+ * to false. In-flight one-step skbs still hold references via+ * the destructor; the context (and its work) is freed only+ * after the conduit frees the last in-flight skb. By then np+ * may be gone, but the work no longer dereferences np because+ * onestep->active has been cleared. And onestep->queue will be+ * cleared by netc_onestep_work()/netc_onestep_destroy_work().+ */+ netc_onestep_put(np->onestep);+ np->onestep = NULL;
This type of stuff is over-commenting and is considered AI slop.
Tell you agent to be very restrictive with inserting comments, essentially
it should only do that when you explicitly tell it to, such as when you
yourself feel it is really necessary or you got a review comment that
this is needed.
Yours,
Linus Walleij
[Severity: Medium]
Is every error from ntmp_ipft_delete_entry() safe to interpret as "the
entry is still programmed in hardware"? The NTMP command path does not
look transactional:
drivers/net/ethernet/freescale/enetc/ntmp.c:netc_xmit_ntmp_cmd() {
...
cbdr->next_to_use = i;
netc_write(cbdr->regs.pir, i);
err = read_poll_timeout(netc_read, val, (val & NETC_CBDRCIR_INDEX) == i,
NETC_CBDR_DELAY_US, NETC_CBDR_TIMEOUT, true,
cbdr->regs.cir);
if (unlikely(err))
return err;
...
}
The DELETE BD and the producer index are published to hardware before the
1 ms completion poll, so -ETIMEDOUT means "completion not observed", not
"command not executed"; -EIO from the writeback error status is similarly
indeterminate. In those cases np->ipft_hf_eid keeps an entry ID the
hardware may no longer own, and there is no other release path:
ntmp_ipft_delete_entry() has this single caller and nothing else clears
np->ipft_hf_eid (this is still the case at the end of the series).
I think this concern is a bit over-cautious.
For a driver-allocated, currently-valid entry ID,
ntmp_ipft_delete_entry() will not return -ETIMEDOUT/-EBUSY/-EIO(error_rr)
on any normal path:
- -ETIMEDOUT: the command BD ring is only accessed while it is enabled
(between ntmp_init_cbdr() and ntmp_free_cbdr()); nothing disables it
at runtime. With the ring enabled, 1 ms is far more than enough for a
single command, so the completion poll cannot time out.
- -EBUSY: commands are issued synchronously and serialized under
ring_lock (post one BD, poll, return), so in-flight BDs never exceed
one against a 256-entry ring. A full ring would require the hardware
to stop advancing CIR, which would already surface as -ETIMEDOUT.
- -EIO(error_rr): the writeback error status is only set for malformed
request data. Validated interfaces always build correct requests.
These three checks are defensive programming to help locate wrong
configuration or malformed request data when developing a new NTMP
interface; they do not fire for validated ones.
In short: with the CBDR enabled, commands serialized, and a valid,
driver-allocated entry ID, -ETIMEDOUT/-EBUSY/-EIO(error_rr) never occur
on the normal path -- they are defensive checks for new-interface
development. -EIO(SBE) and -ENOMEM are bus/memory-level catastrophic or
transient failures; there the command did not take effect, so keeping
ipft_hf_eid and returning the error is the intended behavior -- the
hardware entry is still present.