Re: [PATCH net 04/10] net: mscc: ocelot: deny TX timestamping of non-PTP packets
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: 2021-10-11 23:30:38
On Tue, Oct 12, 2021 at 12:26:10AM +0300, Vladimir Oltean wrote:
quoted hunk ↗ jump to hunk
It appears that Ocelot switches cannot timestamp non-PTP frames, I tested this using the isochron program at: https://github.com/vladimiroltean/tsn-scripts with the result that the driver increments the ocelot_port->ts_id counter as expected, puts it in the REW_OP, but the hardware seems to not timestamp these packets at all, since no IRQ is emitted. Therefore check whether we are sending PTP frames, and refuse to populate REW_OP otherwise. Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/ethernet/mscc/ocelot.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c index 190a5900615b..4a667df9b447 100644 --- a/drivers/net/ethernet/mscc/ocelot.c +++ b/drivers/net/ethernet/mscc/ocelot.c@@ -618,16 +618,12 @@ u32 ocelot_ptp_rew_op(struct sk_buff *skb) } EXPORT_SYMBOL(ocelot_ptp_rew_op); -static bool ocelot_ptp_is_onestep_sync(struct sk_buff *skb) +static bool ocelot_ptp_is_onestep_sync(struct sk_buff *skb, + unsigned int ptp_class) { struct ptp_header *hdr; - unsigned int ptp_class; u8 msgtype, twostep; - ptp_class = ptp_classify_raw(skb); - if (ptp_class == PTP_CLASS_NONE) - return false; - hdr = ptp_parse_header(skb, ptp_class); if (!hdr) return false;@@ -647,11 +643,16 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port, { struct ocelot_port *ocelot_port = ocelot->ports[port]; u8 ptp_cmd = ocelot_port->ptp_cmd; + unsigned int ptp_class; int err; + ptp_class = ptp_classify_raw(skb); + if (ptp_class == PTP_CLASS_NONE) + return -EINVAL; +
I am actually introducing an issue here, sorry that I caught it just now. The ocelot_port_txtstamp_request() function can actually fall through without doing anything, for example when PTP timestamping is not enabled (felix_hwtstamp_set was not called, and ocelot_port->ptp_cmd is 0). We may still carry PTP frames that must be timestamped by somebody else (a PHY, a downstream DSA switch etc), and ds->ops->port_txtstamp() will still get called, because skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP is true, DSA doesn't have the insight on who has PTP timestamping enabled and who doesn't. When attached to a downstream DSA switch who is also the one performing TX timestamping, we are processing an skb here which has SKBTX_HW_TSTAMP set, yet is DSA-tagged, so the call to ptp_classify_raw() fails to find the PTP header. So we print that we are "delivering skb without TX timestamp" (yay us), whereas before this change, we silently did nothing anyway because ocelot_port->ptp_cmd was zero, so this is slightly annoying. I will wait for more feedback on the series then repost tomorrow.
/* Store ptp_cmd in OCELOT_SKB_CB(skb)->ptp_cmd */
if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) {
- if (ocelot_ptp_is_onestep_sync(skb)) {
+ if (ocelot_ptp_is_onestep_sync(skb, ptp_class)) {
OCELOT_SKB_CB(skb)->ptp_cmd = ptp_cmd;
return 0;
}
--
2.25.1