[PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[Vladimir Oltean]

This driver seems susceptible to a form of the bug explained in commit
c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
and in Documentation/networking/timestamping.rst section "Other caveats
for MAC drivers", specifically it timestamps any skb which has
SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.

Evaluate the proper TX timestamping condition only once on the TX
path (in tsnep_netdev_xmit_frame()) and pass it down to
tsnep_xmit_frame_ring() and tsnep_tx_activate() through a bool variable.

Also evaluate it again in the TX confirmation path, in tsnep_tx_poll(),
since I don't know whether TSNEP_DESC_EXTENDED_WRITEBACK_FLAG is a
confounding condition and may be set for other reasons than hardware
timestamping too.

Fixes: 403f69bbdbad ("tsnep: Add TSN endpoint Ethernet MAC driver")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/engleder/tsnep_main.c | 25 ++++++++++++++--------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 625245b0845c..00eb570e026e 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -377,7 +377,7 @@ static void tsnep_tx_disable(struct tsnep_tx *tx, struct napi_struct *napi)
 }
 
 static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
-			      bool last)
+			      bool last, bool do_tstamp)
 {
 	struct tsnep_tx_entry *entry = &tx->entry[index];
 
@@ -386,8 +386,7 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
 	if (entry->skb) {
 		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
 		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
-		if ((entry->type & TSNEP_TX_TYPE_SKB) &&
-		    (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS))
+		if (do_tstamp)
 			entry->properties |= TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;
 
 		/* toggle user flag to prevent false acknowledge
@@ -556,7 +555,8 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
 }
 
 static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
-					 struct tsnep_tx *tx)
+					 struct tsnep_tx *tx,
+					 bool do_tstamp)
 {
 	int count = 1;
 	struct tsnep_tx_entry *entry;
@@ -591,12 +591,12 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
 	}
 	length = retval;
 
-	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
+	if (do_tstamp)
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 
 	for (i = 0; i < count; i++)
 		tsnep_tx_activate(tx, (tx->write + i) & TSNEP_RING_MASK, length,
-				  i == count - 1);
+				  i == count - 1, do_tstamp);
 	tx->write = (tx->write + count) & TSNEP_RING_MASK;
 
 	skb_tx_timestamp(skb);
@@ -704,7 +704,7 @@ static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
 
 	for (i = 0; i < count; i++)
 		tsnep_tx_activate(tx, (tx->write + i) & TSNEP_RING_MASK, length,
-				  i == count - 1);
+				  i == count - 1, false);
 	tx->write = (tx->write + count) & TSNEP_RING_MASK;
 
 	/* descriptor properties shall be valid before hardware is notified */
@@ -775,7 +775,7 @@ static void tsnep_xdp_xmit_frame_ring_zc(struct xdp_desc *xdpd,
 
 	length = tsnep_xdp_tx_map_zc(xdpd, tx);
 
-	tsnep_tx_activate(tx, tx->write, length, true);
+	tsnep_tx_activate(tx, tx->write, length, true, false);
 	tx->write = (tx->write + 1) & TSNEP_RING_MASK;
 }
 
@@ -845,6 +845,7 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
 		length = tsnep_tx_unmap(tx, tx->read, count);
 
 		if ((entry->type & TSNEP_TX_TYPE_SKB) &&
+		    (tx->adapter->hwtstamp_config.tx_type == HWTSTAMP_TX_ON) &&
 		    (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS) &&
 		    (__le32_to_cpu(entry->desc_wb->properties) &
 		     TSNEP_DESC_EXTENDED_WRITEBACK_FLAG)) {
@@ -2153,11 +2154,17 @@ static netdev_tx_t tsnep_netdev_xmit_frame(struct sk_buff *skb,
 {
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
 	u16 queue_mapping = skb_get_queue_mapping(skb);
+	bool do_tstamp = false;
 
 	if (queue_mapping >= adapter->num_tx_queues)
 		queue_mapping = 0;
 
-	return tsnep_xmit_frame_ring(skb, &adapter->tx[queue_mapping]);
+	if (adapter->hwtstamp_config.tx_type == HWTSTAMP_TX_ON &&
+	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
+		do_tstamp = true;
+
+	return tsnep_xmit_frame_ring(skb, &adapter->tx[queue_mapping],
+				     do_tstamp);
 }
 
 static int tsnep_netdev_ioctl(struct net_device *netdev, struct ifreq *ifr,
-- 
2.43.0

Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[Gerhard Engleder]

On 12.05.25 15:24, Vladimir Oltean wrote:
> This driver seems susceptible to a form of the bug explained in commit
> c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
> and in Documentation/networking/timestamping.rst section "Other caveats
> for MAC drivers", specifically it timestamps any skb which has
> SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.

Is it necessary in general to check adapter->hwtstamp_config.tx_type for
HWTSTAMP_TX_ON or only to fix this bug? In 
Documentation/networking/timestamping.rst
section "Hardware Timestamping Implementation: Device Drivers" only the
check of (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) is mentioned.

> Evaluate the proper TX timestamping condition only once on the TX
> path (in tsnep_netdev_xmit_frame()) and pass it down to
> tsnep_xmit_frame_ring() and tsnep_tx_activate() through a bool variable.

What about also storing the TX timestamping condition in TX entry and
evaluating it in tsnep_tx_poll() instead of checking
adapter->hwtstamp_config.tx_type again? This way the timestamping
decision is only done in tsnep_netdev_xmit_frame() and tsnep_tx_poll()
cannot decide differently e.g. if hardware timestamping was deactivated
in between. Also this means that SKBTX_IN_PROGRESS is only set but
never evaluated by tsnep, which should fix this bug AFAIU.

> Also evaluate it again in the TX confirmation path, in tsnep_tx_poll(),
> since I don't know whether TSNEP_DESC_EXTENDED_WRITEBACK_FLAG is a
> confounding condition and may be set for other reasons than hardware
> timestamping too.

Yes, there is also some DMA statistic included besides timestamping so
it can be set for other reasons too in the future.

I can take over this patch and test it when I understand more clearly
what needs to be done.

Gerhard

Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[Vladimir Oltean]

On Mon, May 12, 2025 at 10:07:52PM +0200, Gerhard Engleder wrote:
> On 12.05.25 15:24, Vladimir Oltean wrote:
> > This driver seems susceptible to a form of the bug explained in commit
> > c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
> > and in Documentation/networking/timestamping.rst section "Other caveats
> > for MAC drivers", specifically it timestamps any skb which has
> > SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.
> 
> Is it necessary in general to check adapter->hwtstamp_config.tx_type for
> HWTSTAMP_TX_ON or only to fix this bug?

I'll start with the problem description and work my way towards an answer.

The socket UAPI doesn't support presenting multiple TX hardware timestamps
for the same packet, or better said, user space has no way of distinguishing
the source of a hardware timestamp other than assuming it comes from the
PHC reported by the ethtool get_ts_info() of the driver. If the kernel
delivers multiple hardware timestamps to the socket error queue for the
same packet, nothing good comes out of that, so we expect there to be
filtering somewhere to avoid that.

The way DSA switches are hooked up is by presenting themselves as a
collection of N MAC interfaces (possibly with hardware timestamping
capabilities of their own) stacked on top of the host port like VLANs,
with their ndo_start_xmit() doing some packet processing and ultimately
calling the ndo_start_xmit() of the tsnep driver.

DSA is supposed to work with any MAC driver as host port as long as it
is fairly well behaved, and as a MAC driver you don't really get to
choose if you support it or not. One of the expectations is that the
host port driver should only provide hardware TX timestamps only if it
is the active TX timestamping layer for the packet. That is one level of
filtering.

DSA will prevent adapter->hwtstamp_config.tx_type from ever being
changed from the default value by blocking driver calls to
SIOCSHWTSTAMP, and that is another level of filtering. See
dev_set_hwtstamp() -> dsa_conduit_hwtstamp_validate() -> ... ->
__dsa_conduit_hwtstamp_validate().

Another case where the packet visits your ndo_start_xmit() but the
timestamping duty is "not for you" is with PHY timestamping. To user
space, it looks the same (for better or for worse), so SKBTX_HW_TSTAMP
is set by the usual place in __sock_tx_timestamp(), and can be found set
during ndo_start_xmit().

I didn't mention this because as opposed to DSA, PHY timestamping needs
explicit MAC driver cooperation, and the tsnep driver does not currently
fulfill the requirements for supporting PHY timestamping - so no user
visible bug exists there.
(1) the control path operations, SIOCSHWTSTAMP and SIOCGHWTSTAMP, are
    always processed by tsnep_ptp_ioctl() and never passed on to
    phy_mii_ioctl() -> phydev->mii_ts->hwtstamp(). So it never gives
    timestamping PHY drivers a chance to set themselves up.
(2) the data path hook is in skb_tx_timestamp(), which tsnep calls from
    tsnep_xmit_frame_ring(). That part is fine, but in the future it may
    result in one of the drivers from drivers/net/phy/ setting
    SKBTX_IN_PROGRESS even when tsnep_xmit_frame_ring() didn't do that.

(1) is something that in the new API based on ndo_hwtstamp_set() changes
radically. After converting to the new API, the core, in dev_set_hwtstamp(),
decides now whether to pass the SIOCSHWTSTAMP call to the MAC driver or
to the PHY driver, it is no longer the MAC driver's choice. So it
becomes more like DSA, you need to comply with a basic set of principles
and then you may support stacked timestamping layers without even
knowing it. It boils down to the same thing, only provide a timestamp
if you're the active layer.

> In Documentation/networking/timestamping.rst section "Hardware
> Timestamping Implementation: Device Drivers" only the check of
> (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) is mentioned.

Yeah, a more complete picture is built later on in the document, in the
section immediately following the one you quoted.

> > Evaluate the proper TX timestamping condition only once on the TX
> > path (in tsnep_netdev_xmit_frame()) and pass it down to
> > tsnep_xmit_frame_ring() and tsnep_tx_activate() through a bool variable.
> 
> What about also storing the TX timestamping condition in TX entry and
> evaluating it in tsnep_tx_poll() instead of checking
> adapter->hwtstamp_config.tx_type again? This way the timestamping
> decision is only done in tsnep_netdev_xmit_frame() and tsnep_tx_poll()
> cannot decide differently e.g. if hardware timestamping was deactivated
> in between.

That would be a lot better indeed. I didn't want to make a clumsy
attempt at that.

> Also this means that SKBTX_IN_PROGRESS is only set but never evaluated
> by tsnep, which should fix this bug AFAIU.

SKBTX_IN_PROGRESS is part of the problem (and in case of gianfar was the
only problem), but tsnep has the additional problem of not evaluating
adapter->hwtstamp_config.tx_type even once, in the TX path. I've explained
above the results I expect.

> > Also evaluate it again in the TX confirmation path, in tsnep_tx_poll(),
> > since I don't know whether TSNEP_DESC_EXTENDED_WRITEBACK_FLAG is a
> > confounding condition and may be set for other reasons than hardware
> > timestamping too.
> 
> Yes, there is also some DMA statistic included besides timestamping so
> it can be set for other reasons too in the future.
> 
> I can take over this patch and test it when I understand more clearly
> what needs to be done.
> 
> Gerhard

It would be great if you could take over this patch. After the net ->
net-next merge I can then submit the ndo_hwtstamp_get()/ndo_hwtstamp_set()
conversion patch for tsnep, the one which initially prompted me to look
into how this driver uses the provided configuration.

Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[Gerhard Engleder]

On 12.05.25 23:09, Vladimir Oltean wrote:
> On Mon, May 12, 2025 at 10:07:52PM +0200, Gerhard Engleder wrote:
>> On 12.05.25 15:24, Vladimir Oltean wrote:
>>> This driver seems susceptible to a form of the bug explained in commit
>>> c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
>>> and in Documentation/networking/timestamping.rst section "Other caveats
>>> for MAC drivers", specifically it timestamps any skb which has
>>> SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.
>>
>> Is it necessary in general to check adapter->hwtstamp_config.tx_type for
>> HWTSTAMP_TX_ON or only to fix this bug?
> 
> I'll start with the problem description and work my way towards an answer.

(...)

>> I can take over this patch and test it when I understand more clearly
>> what needs to be done.
>>
>> Gerhard
> 
> It would be great if you could take over this patch. After the net ->
> net-next merge I can then submit the ndo_hwtstamp_get()/ndo_hwtstamp_set()
> conversion patch for tsnep, the one which initially prompted me to look
> into how this driver uses the provided configuration.

I will post a new patch version in the next days. You can send me the
ndo_hwtstamp_get()/ndo_hwtstamp_set() conversion patch for testing. I
have it on my list, but nothing done so far, so I feel responsible
for that too.

Gerhard

Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[Vladimir Oltean]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Tue, May 13, 2025 at 08:34:17PM +0200, Gerhard Engleder wrote:
> On 12.05.25 23:09, Vladimir Oltean wrote:
> > On Mon, May 12, 2025 at 10:07:52PM +0200, Gerhard Engleder wrote:
> > > On 12.05.25 15:24, Vladimir Oltean wrote:
> > > > This driver seems susceptible to a form of the bug explained in commit
> > > > c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
> > > > and in Documentation/networking/timestamping.rst section "Other caveats
> > > > for MAC drivers", specifically it timestamps any skb which has
> > > > SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.
> > > 
> > > Is it necessary in general to check adapter->hwtstamp_config.tx_type for
> > > HWTSTAMP_TX_ON or only to fix this bug?
> > 
> > I'll start with the problem description and work my way towards an answer.
> 
> (...)
> 
> > > I can take over this patch and test it when I understand more clearly
> > > what needs to be done.
> > > 
> > > Gerhard
> > 
> > It would be great if you could take over this patch. After the net ->
> > net-next merge I can then submit the ndo_hwtstamp_get()/ndo_hwtstamp_set()
> > conversion patch for tsnep, the one which initially prompted me to look
> > into how this driver uses the provided configuration.
> 
> I will post a new patch version in the next days. You can send me the
> ndo_hwtstamp_get()/ndo_hwtstamp_set() conversion patch for testing. I
> have it on my list, but nothing done so far, so I feel responsible
> for that too.
> 
> Gerhard

See attached. It applies on top of this patch ("do_tstamp" is present in
the context).

Thanks!

[-- Attachment #2: 0001-net-tsnep-convert-to-ndo_hwtstamp_get-and-ndo_hwtsta.patch --]
[-- Type: text/x-diff, Size: 7149 bytes --]

From 0539bea821d67c25a3d628c12ffd1c458cd38bb6 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Tue, 4 Jul 2023 21:09:50 +0300
Subject: [PATCH net-next] net: tsnep: convert to ndo_hwtstamp_get() and
 ndo_hwtstamp_set()

New timestamping API was introduced in commit 66f7223039c0 ("net: add
NDOs for configuring hardware timestamping") from kernel v6.6.

It is time to convert the tsnep driver to the new API, so that
timestamping configuration can be removed from the ndo_eth_ioctl()
path completely.

I don't think the driver needs the interface to be down in order for
timestamping to be changed. The netif_running() restriction is there
in tsnep_netdev_ioctl() - I haven't migrated it to the new API. There
is no interaction with hardware registers for either operation, just a
concurrency with the data path which should be fine.

After removing the PHY timestamping logic from tsnep_netdev_ioctl(),
the rest is almost equivalent to phy_do_ioctl_running(), except for the
return code on the !netif_running() condition: -EINVAL vs -ENODEV.
I'm making the conversion to phy_do_ioctl_running() anyway, on the
premise that a return code standardized tree-wide is less complex.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/engleder/tsnep.h      |  8 +-
 drivers/net/ethernet/engleder/tsnep_main.c | 14 +---
 drivers/net/ethernet/engleder/tsnep_ptp.c  | 90 +++++++++++-----------
 3 files changed, 53 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
index f188fba021a6..452ab982afbe 100644
--- a/drivers/net/ethernet/engleder/tsnep.h
+++ b/drivers/net/ethernet/engleder/tsnep.h
@@ -176,7 +176,7 @@ struct tsnep_adapter {
 	struct tsnep_gcl gcl[2];
 	int next_gcl;
 
-	struct hwtstamp_config hwtstamp_config;
+	struct kernel_hwtstamp_config hwtstamp_config;
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_clock_info;
 	/* ptp clock lock */
@@ -203,7 +203,11 @@ extern const struct ethtool_ops tsnep_ethtool_ops;
 
 int tsnep_ptp_init(struct tsnep_adapter *adapter);
 void tsnep_ptp_cleanup(struct tsnep_adapter *adapter);
-int tsnep_ptp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd);
+int tsnep_hwtstamp_get(struct net_device *netdev,
+		       struct kernel_hwtstamp_config *config);
+int tsnep_hwtstamp_set(struct net_device *netdev,
+		       struct kernel_hwtstamp_config *config,
+		       struct netlink_ext_ack *extack);
 
 int tsnep_tc_init(struct tsnep_adapter *adapter);
 void tsnep_tc_cleanup(struct tsnep_adapter *adapter);
diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 00eb570e026e..b880efddaaea 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c
@@ -2167,16 +2167,6 @@ static netdev_tx_t tsnep_netdev_xmit_frame(struct sk_buff *skb,
 				     do_tstamp);
 }
 
-static int tsnep_netdev_ioctl(struct net_device *netdev, struct ifreq *ifr,
-			      int cmd)
-{
-	if (!netif_running(netdev))
-		return -EINVAL;
-	if (cmd == SIOCSHWTSTAMP || cmd == SIOCGHWTSTAMP)
-		return tsnep_ptp_ioctl(netdev, ifr, cmd);
-	return phy_mii_ioctl(netdev->phydev, ifr, cmd);
-}
-
 static void tsnep_netdev_set_multicast(struct net_device *netdev)
 {
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
@@ -2383,7 +2373,7 @@ static const struct net_device_ops tsnep_netdev_ops = {
 	.ndo_open = tsnep_netdev_open,
 	.ndo_stop = tsnep_netdev_close,
 	.ndo_start_xmit = tsnep_netdev_xmit_frame,
-	.ndo_eth_ioctl = tsnep_netdev_ioctl,
+	.ndo_eth_ioctl = phy_do_ioctl_running,
 	.ndo_set_rx_mode = tsnep_netdev_set_multicast,
 	.ndo_get_stats64 = tsnep_netdev_get_stats64,
 	.ndo_set_mac_address = tsnep_netdev_set_mac_address,
@@ -2393,6 +2383,8 @@ static const struct net_device_ops tsnep_netdev_ops = {
 	.ndo_bpf = tsnep_netdev_bpf,
 	.ndo_xdp_xmit = tsnep_netdev_xdp_xmit,
 	.ndo_xsk_wakeup = tsnep_netdev_xsk_wakeup,
+	.ndo_hwtstamp_get = tsnep_hwtstamp_get,
+	.ndo_hwtstamp_set = tsnep_hwtstamp_set,
 };
 
 static int tsnep_mac_init(struct tsnep_adapter *adapter)
diff --git a/drivers/net/ethernet/engleder/tsnep_ptp.c b/drivers/net/ethernet/engleder/tsnep_ptp.c
index 54fbf0126815..f2d001f58017 100644
--- a/drivers/net/ethernet/engleder/tsnep_ptp.c
+++ b/drivers/net/ethernet/engleder/tsnep_ptp.c
@@ -19,56 +19,54 @@ void tsnep_get_system_time(struct tsnep_adapter *adapter, u64 *time)
 	*time = (((u64)high) << 32) | ((u64)low);
 }
 
-int tsnep_ptp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
+int tsnep_hwtstamp_get(struct net_device *netdev,
+		       struct kernel_hwtstamp_config *config)
 {
 	struct tsnep_adapter *adapter = netdev_priv(netdev);
-	struct hwtstamp_config config;
-
-	if (!ifr)
-		return -EINVAL;
-
-	if (cmd == SIOCSHWTSTAMP) {
-		if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-			return -EFAULT;
-
-		switch (config.tx_type) {
-		case HWTSTAMP_TX_OFF:
-		case HWTSTAMP_TX_ON:
-			break;
-		default:
-			return -ERANGE;
-		}
-
-		switch (config.rx_filter) {
-		case HWTSTAMP_FILTER_NONE:
-			break;
-		case HWTSTAMP_FILTER_ALL:
-		case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
-		case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
-		case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
-		case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
-		case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
-		case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
-		case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
-		case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
-		case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
-		case HWTSTAMP_FILTER_PTP_V2_EVENT:
-		case HWTSTAMP_FILTER_PTP_V2_SYNC:
-		case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
-		case HWTSTAMP_FILTER_NTP_ALL:
-			config.rx_filter = HWTSTAMP_FILTER_ALL;
-			break;
-		default:
-			return -ERANGE;
-		}
-
-		memcpy(&adapter->hwtstamp_config, &config,
-		       sizeof(adapter->hwtstamp_config));
+
+	*config = adapter->hwtstamp_config;
+
+	return 0;
+}
+
+int tsnep_hwtstamp_set(struct net_device *netdev,
+		       struct kernel_hwtstamp_config *config,
+		       struct netlink_ext_ack *extack)
+{
+	struct tsnep_adapter *adapter = netdev_priv(netdev);
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+	case HWTSTAMP_TX_ON:
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		break;
+	case HWTSTAMP_FILTER_ALL:
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+	case HWTSTAMP_FILTER_NTP_ALL:
+		config->rx_filter = HWTSTAMP_FILTER_ALL;
+		break;
+	default:
+		return -ERANGE;
 	}
 
-	if (copy_to_user(ifr->ifr_data, &adapter->hwtstamp_config,
-			 sizeof(adapter->hwtstamp_config)))
-		return -EFAULT;
+	adapter->hwtstamp_config = *config;
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

[Gerhard Engleder]

On 13.05.25 22:06, Vladimir Oltean wrote:
> On Tue, May 13, 2025 at 08:34:17PM +0200, Gerhard Engleder wrote:
>> On 12.05.25 23:09, Vladimir Oltean wrote:
>>> On Mon, May 12, 2025 at 10:07:52PM +0200, Gerhard Engleder wrote:
>>>> On 12.05.25 15:24, Vladimir Oltean wrote:
>>>>> This driver seems susceptible to a form of the bug explained in commit
>>>>> c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
>>>>> and in Documentation/networking/timestamping.rst section "Other caveats
>>>>> for MAC drivers", specifically it timestamps any skb which has
>>>>> SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.
>>>>
>>>> Is it necessary in general to check adapter->hwtstamp_config.tx_type for
>>>> HWTSTAMP_TX_ON or only to fix this bug?
>>>
>>> I'll start with the problem description and work my way towards an answer.
>>
>> (...)
>>
>>>> I can take over this patch and test it when I understand more clearly
>>>> what needs to be done.
>>>>
>>>> Gerhard
>>>
>>> It would be great if you could take over this patch. After the net ->
>>> net-next merge I can then submit the ndo_hwtstamp_get()/ndo_hwtstamp_set()
>>> conversion patch for tsnep, the one which initially prompted me to look
>>> into how this driver uses the provided configuration.
>>
>> I will post a new patch version in the next days. You can send me the
>> ndo_hwtstamp_get()/ndo_hwtstamp_set() conversion patch for testing. I
>> have it on my list, but nothing done so far, so I feel responsible
>> for that too.
>>
>> Gerhard
> 
> See attached. It applies on top of this patch ("do_tstamp" is present in
> the context).

I tested the ndo_hwtstamp_get()/ndo_hwtstamp_set() conversion patch
successfully on top of the timestamping fix patch. Thank you for the
patch!

Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>
Tested-by: Gerhard Engleder <gerhard@engleder-embedded.com>