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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

[-- 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