[PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

[PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

[Vadim Fedorenko]

Convert TI NetCP driver to use ndo_hwtstamp_get()/ndo_hwtstamp_set()
callbacks. The logic is slightly changed, because I believe the original
logic was not really correct. Config reading part is using the very
first module to get the configuration instead of iterating over all of
them and keep the last one as the configuration is supposed to be identical
for all modules. HW timestamp config set path is now trying to configure
all modules, but in case of error from one module it adds extack
message. This way the configuration will be as synchronized as possible.

There are only 2 modules using netcp core infrastructure, and both use
the very same function to configure HW timestamping, so no actual
difference in behavior is expected.

Compile test only.

Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 drivers/net/ethernet/ti/netcp.h       |  5 ++
 drivers/net/ethernet/ti/netcp_core.c  | 56 +++++++++++++++++++++
 drivers/net/ethernet/ti/netcp_ethss.c | 72 +++++++++++++++------------
 3 files changed, 101 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp.h b/drivers/net/ethernet/ti/netcp.h
index 7007eb8bed36..b9cbd3b4a8a2 100644
--- a/drivers/net/ethernet/ti/netcp.h
+++ b/drivers/net/ethernet/ti/netcp.h
@@ -207,6 +207,11 @@ struct netcp_module {
 	int	(*del_vid)(void *intf_priv, int vid);
 	int	(*ioctl)(void *intf_priv, struct ifreq *req, int cmd);
 	int	(*set_rx_mode)(void *intf_priv, bool promisc);
+	int	(*hwtstamp_get)(void *intf_priv,
+				struct kernel_hwtstamp_config *cfg);
+	int	(*hwtstamp_set)(void *intf_priv,
+				struct kernel_hwtstamp_config *cfg,
+				struct netlink_ext_ack *extack);
 
 	/* used internally */
 	struct list_head	module_list;
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 857820657bac..8c7b78d1fe36 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1781,6 +1781,60 @@ static int netcp_ndo_stop(struct net_device *ndev)
 	return 0;
 }
 
+static int netcp_ndo_hwtstamp_get(struct net_device *ndev,
+				  struct kernel_hwtstamp_config *config)
+{
+	struct netcp_intf *netcp = netdev_priv(ndev);
+	struct netcp_intf_modpriv *intf_modpriv;
+	struct netcp_module *module;
+	int err = -EOPNOTSUPP;
+
+	if (!netif_running(ndev))
+		return -EINVAL;
+
+	for_each_module(netcp, intf_modpriv) {
+		module = intf_modpriv->netcp_module;
+		if (!module->hwtstamp_get)
+			continue;
+
+		err = module->hwtstamp_get(intf_modpriv->module_priv, config);
+		break;
+	}
+
+	return err;
+}
+
+static int netcp_ndo_hwtstamp_set(struct net_device *ndev,
+				  struct kernel_hwtstamp_config *config,
+				  struct netlink_ext_ack *extack)
+{
+	struct netcp_intf *netcp = netdev_priv(ndev);
+	struct netcp_intf_modpriv *intf_modpriv;
+	struct netcp_module *module;
+	int ret = -1, err = -EOPNOTSUPP;
+
+	if (!netif_running(ndev))
+		return -EINVAL;
+
+	for_each_module(netcp, intf_modpriv) {
+		module = intf_modpriv->netcp_module;
+		if (!module->hwtstamp_set)
+			continue;
+
+		err = module->hwtstamp_set(intf_modpriv->module_priv, config,
+					   extack);
+		if ((err < 0) && (err != -EOPNOTSUPP)) {
+			NL_SET_ERR_MSG_WEAK_MOD(extack,
+						"At least one module failed to setup HW timestamps");
+			ret = err;
+		}
+		if (err == 0)
+			ret = err;
+	}
+
+	return (ret == 0) ? 0 : err;
+}
+
 static int netcp_ndo_ioctl(struct net_device *ndev,
 			   struct ifreq *req, int cmd)
 {
@@ -1952,6 +2006,8 @@ static const struct net_device_ops netcp_netdev_ops = {
 	.ndo_tx_timeout		= netcp_ndo_tx_timeout,
 	.ndo_select_queue	= dev_pick_tx_zero,
 	.ndo_setup_tc		= netcp_setup_tc,
+	.ndo_hwtstamp_get	= netcp_ndo_hwtstamp_get,
+	.ndo_hwtstamp_set	= netcp_ndo_hwtstamp_set,
 };
 
 static int netcp_create_interface(struct netcp_device *netcp_device,
diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c
index 55a1a96cd834..0ae44112812c 100644
--- a/drivers/net/ethernet/ti/netcp_ethss.c
+++ b/drivers/net/ethernet/ti/netcp_ethss.c
@@ -2591,20 +2591,26 @@ static int gbe_rxtstamp(struct gbe_intf *gbe_intf, struct netcp_packet *p_info)
 	return 0;
 }
 
-static int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq *ifr)
+static int gbe_hwtstamp_get(void *intf_priv, struct kernel_hwtstamp_config *cfg)
 {
-	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
-	struct cpts *cpts = gbe_dev->cpts;
-	struct hwtstamp_config cfg;
+	struct gbe_intf *gbe_intf = intf_priv;
+	struct gbe_priv *gbe_dev;
+	struct phy_device *phy;
+
+	gbe_dev = gbe_intf->gbe_dev;
 
-	if (!cpts)
+	if (!gbe_dev->cpts)
+		return -EOPNOTSUPP;
+
+	phy = gbe_intf->slave->phy;
+	if (phy_has_hwtstamp(phy))
 		return -EOPNOTSUPP;
 
-	cfg.flags = 0;
-	cfg.tx_type = gbe_dev->tx_ts_enabled ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
-	cfg.rx_filter = gbe_dev->rx_ts_enabled;
+	cfg->flags = 0;
+	cfg->tx_type = gbe_dev->tx_ts_enabled ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+	cfg->rx_filter = gbe_dev->rx_ts_enabled;
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static void gbe_hwtstamp(struct gbe_intf *gbe_intf)
@@ -2637,19 +2643,23 @@ static void gbe_hwtstamp(struct gbe_intf *gbe_intf)
 	writel(ctl,    GBE_REG_ADDR(slave, port_regs, ts_ctl_ltype2));
 }
 
-static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
+static int gbe_hwtstamp_set(void *intf_priv, struct kernel_hwtstamp_config *cfg,
+			    struct netlink_ext_ack *extack)
 {
-	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
-	struct cpts *cpts = gbe_dev->cpts;
-	struct hwtstamp_config cfg;
+	struct gbe_intf *gbe_intf = intf_priv;
+	struct gbe_priv *gbe_dev;
+	struct phy_device *phy;
 
-	if (!cpts)
+	gbe_dev = gbe_intf->gbe_dev;
+
+	if (!gbe_dev->cpts)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
-		return -EFAULT;
+	phy = gbe_intf->slave->phy;
+	if (phy_has_hwtstamp(phy))
+		return phy->mii_ts->hwtstamp(phy->mii_ts, cfg, extack);
 
-	switch (cfg.tx_type) {
+	switch (cfg->tx_type) {
 	case HWTSTAMP_TX_OFF:
 		gbe_dev->tx_ts_enabled = 0;
 		break;
@@ -2660,7 +2670,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
-	switch (cfg.rx_filter) {
+	switch (cfg->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_NONE;
 		break;
@@ -2668,7 +2678,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
 	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
 		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
 	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
@@ -2680,7 +2690,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
 	case HWTSTAMP_FILTER_PTP_V2_SYNC:
 	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
 		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_PTP_V2_EVENT;
-		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
 		break;
 	default:
 		return -ERANGE;
@@ -2688,7 +2698,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
 
 	gbe_hwtstamp(gbe_intf);
 
-	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
+	return 0;
 }
 
 static void gbe_register_cpts(struct gbe_priv *gbe_dev)
@@ -2745,12 +2755,15 @@ static inline void gbe_unregister_cpts(struct gbe_priv *gbe_dev)
 {
 }
 
-static inline int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq *req)
+static inline int gbe_hwtstamp_get(struct gbe_intf *gbe_intf,
+				   struct kernel_hwtstamp_config *cfg)
 {
 	return -EOPNOTSUPP;
 }
 
-static inline int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *req)
+static inline int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
+				   struct kernel_hwtstamp_config *cfg,
+				   struct netlink_ext_ack *extack)
 {
 	return -EOPNOTSUPP;
 }
@@ -2816,15 +2829,6 @@ static int gbe_ioctl(void *intf_priv, struct ifreq *req, int cmd)
 	struct gbe_intf *gbe_intf = intf_priv;
 	struct phy_device *phy = gbe_intf->slave->phy;
 
-	if (!phy_has_hwtstamp(phy)) {
-		switch (cmd) {
-		case SIOCGHWTSTAMP:
-			return gbe_hwtstamp_get(gbe_intf, req);
-		case SIOCSHWTSTAMP:
-			return gbe_hwtstamp_set(gbe_intf, req);
-		}
-	}
-
 	if (phy)
 		return phy_mii_ioctl(phy, req, cmd);
 
@@ -3824,6 +3828,8 @@ static struct netcp_module gbe_module = {
 	.add_vid	= gbe_add_vid,
 	.del_vid	= gbe_del_vid,
 	.ioctl		= gbe_ioctl,
+	.hwtstamp_get	= gbe_hwtstamp_get,
+	.hwtstamp_set	= gbe_hwtstamp_set,
 };
 
 static struct netcp_module xgbe_module = {
@@ -3841,6 +3847,8 @@ static struct netcp_module xgbe_module = {
 	.add_vid	= gbe_add_vid,
 	.del_vid	= gbe_del_vid,
 	.ioctl		= gbe_ioctl,
+	.hwtstamp_get	= gbe_hwtstamp_get,
+	.hwtstamp_set	= gbe_hwtstamp_set,
 };
 
 static int __init keystone_gbe_init(void)
-- 
2.47.3

Re: [PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

[Kory Maincent]

On Wed, 29 Oct 2025 20:09:22 +0000
Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:

> Convert TI NetCP driver to use ndo_hwtstamp_get()/ndo_hwtstamp_set()
> callbacks. The logic is slightly changed, because I believe the original
> logic was not really correct. Config reading part is using the very
> first module to get the configuration instead of iterating over all of
> them and keep the last one as the configuration is supposed to be identical
> for all modules. HW timestamp config set path is now trying to configure
> all modules, but in case of error from one module it adds extack
> message. This way the configuration will be as synchronized as possible.

On the case the hwtstamp_set return the extack error the hwtstamp_get will
return something that might not be true as not all module will have the same
config. Is it acceptable?
 
> There are only 2 modules using netcp core infrastructure, and both use
> the very same function to configure HW timestamping, so no actual
> difference in behavior is expected.
> 
> Compile test only.
> 
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> b/drivers/net/ethernet/ti/netcp_ethss.c index 55a1a96cd834..0ae44112812c
> 100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c
> +++ b/drivers/net/ethernet/ti/netcp_ethss.c
> @@ -2591,20 +2591,26 @@ static int gbe_rxtstamp(struct gbe_intf *gbe_intf,
> struct netcp_packet *p_info) return 0;
>  }
>  
> -static int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq *ifr)
> +static int gbe_hwtstamp_get(void *intf_priv, struct kernel_hwtstamp_config
> *cfg) {
> -	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
> -	struct cpts *cpts = gbe_dev->cpts;
> -	struct hwtstamp_config cfg;
> +	struct gbe_intf *gbe_intf = intf_priv;
> +	struct gbe_priv *gbe_dev;
> +	struct phy_device *phy;
> +
> +	gbe_dev = gbe_intf->gbe_dev;
>  
> -	if (!cpts)
> +	if (!gbe_dev->cpts)
> +		return -EOPNOTSUPP;
> +
> +	phy = gbe_intf->slave->phy;
> +	if (phy_has_hwtstamp(phy))
>  		return -EOPNOTSUPP;

This condition should be removed.
The selection between PHY or MAC timestamping is now done in the net core:
https://elixir.bootlin.com/linux/v6.17.1/source/net/core/dev_ioctl.c#L244

>  
> -	cfg.flags = 0;
> -	cfg.tx_type = gbe_dev->tx_ts_enabled ? HWTSTAMP_TX_ON :
> HWTSTAMP_TX_OFF;
> -	cfg.rx_filter = gbe_dev->rx_ts_enabled;
> +	cfg->flags = 0;
> +	cfg->tx_type = gbe_dev->tx_ts_enabled ? HWTSTAMP_TX_ON :
> HWTSTAMP_TX_OFF;
> +	cfg->rx_filter = gbe_dev->rx_ts_enabled;
>  
> -	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +	return 0;
>  }
>  
>  static void gbe_hwtstamp(struct gbe_intf *gbe_intf)
> @@ -2637,19 +2643,23 @@ static void gbe_hwtstamp(struct gbe_intf *gbe_intf)
>  	writel(ctl,    GBE_REG_ADDR(slave, port_regs, ts_ctl_ltype2));
>  }
>  
> -static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
> +static int gbe_hwtstamp_set(void *intf_priv, struct kernel_hwtstamp_config
> *cfg,
> +			    struct netlink_ext_ack *extack)
>  {
> -	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
> -	struct cpts *cpts = gbe_dev->cpts;
> -	struct hwtstamp_config cfg;
> +	struct gbe_intf *gbe_intf = intf_priv;
> +	struct gbe_priv *gbe_dev;
> +	struct phy_device *phy;
>  
> -	if (!cpts)
> +	gbe_dev = gbe_intf->gbe_dev;
> +
> +	if (!gbe_dev->cpts)
>  		return -EOPNOTSUPP;
>  
> -	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> -		return -EFAULT;
> +	phy = gbe_intf->slave->phy;
> +	if (phy_has_hwtstamp(phy))
> +		return phy->mii_ts->hwtstamp(phy->mii_ts, cfg, extack);

Same.

>  
> -	switch (cfg.tx_type) {
> +	switch (cfg->tx_type) {
>  	case HWTSTAMP_TX_OFF:
>  		gbe_dev->tx_ts_enabled = 0;
>  		break;
> @@ -2660,7 +2670,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
> struct ifreq *ifr) return -ERANGE;
>  	}
>  
> -	switch (cfg.rx_filter) {
> +	switch (cfg->rx_filter) {
>  	case HWTSTAMP_FILTER_NONE:
>  		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_NONE;
>  		break;
> @@ -2668,7 +2678,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
> struct ifreq *ifr) case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
>  	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
>  		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
> -		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
> +		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
>  		break;
>  	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
>  	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> @@ -2680,7 +2690,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
> struct ifreq *ifr) case HWTSTAMP_FILTER_PTP_V2_SYNC:
>  	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>  		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_PTP_V2_EVENT;
> -		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>  		break;
>  	default:
>  		return -ERANGE;
> @@ -2688,7 +2698,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
> struct ifreq *ifr) 
>  	gbe_hwtstamp(gbe_intf);
>  
> -	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +	return 0;
>  }
>  
>  static void gbe_register_cpts(struct gbe_priv *gbe_dev)
> @@ -2745,12 +2755,15 @@ static inline void gbe_unregister_cpts(struct
> gbe_priv *gbe_dev) {
>  }
>  
> -static inline int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq
> *req) +static inline int gbe_hwtstamp_get(struct gbe_intf *gbe_intf,
> +				   struct kernel_hwtstamp_config *cfg)
>  {
>  	return -EOPNOTSUPP;
>  }
>  
> -static inline int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq
> *req) +static inline int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
> +				   struct kernel_hwtstamp_config *cfg,
> +				   struct netlink_ext_ack *extack)
>  {
>  	return -EOPNOTSUPP;
>  }
> @@ -2816,15 +2829,6 @@ static int gbe_ioctl(void *intf_priv, struct ifreq
> *req, int cmd) struct gbe_intf *gbe_intf = intf_priv;
>  	struct phy_device *phy = gbe_intf->slave->phy;
>  
> -	if (!phy_has_hwtstamp(phy)) {
> -		switch (cmd) {
> -		case SIOCGHWTSTAMP:
> -			return gbe_hwtstamp_get(gbe_intf, req);
> -		case SIOCSHWTSTAMP:
> -			return gbe_hwtstamp_set(gbe_intf, req);
> -		}
> -	}
> -
>  	if (phy)
>  		return phy_mii_ioctl(phy, req, cmd);
>  
> @@ -3824,6 +3828,8 @@ static struct netcp_module gbe_module = {
>  	.add_vid	= gbe_add_vid,
>  	.del_vid	= gbe_del_vid,
>  	.ioctl		= gbe_ioctl,
> +	.hwtstamp_get	= gbe_hwtstamp_get,
> +	.hwtstamp_set	= gbe_hwtstamp_set,
>  };
>  
>  static struct netcp_module xgbe_module = {
> @@ -3841,6 +3847,8 @@ static struct netcp_module xgbe_module = {
>  	.add_vid	= gbe_add_vid,
>  	.del_vid	= gbe_del_vid,
>  	.ioctl		= gbe_ioctl,
> +	.hwtstamp_get	= gbe_hwtstamp_get,
> +	.hwtstamp_set	= gbe_hwtstamp_set,
>  };
>  
>  static int __init keystone_gbe_init(void)



-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

[Vadim Fedorenko]

On 30/10/2025 10:30, Kory Maincent wrote:
> On Wed, 29 Oct 2025 20:09:22 +0000
> Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:
> 
>> Convert TI NetCP driver to use ndo_hwtstamp_get()/ndo_hwtstamp_set()
>> callbacks. The logic is slightly changed, because I believe the original
>> logic was not really correct. Config reading part is using the very
>> first module to get the configuration instead of iterating over all of
>> them and keep the last one as the configuration is supposed to be identical
>> for all modules. HW timestamp config set path is now trying to configure
>> all modules, but in case of error from one module it adds extack
>> message. This way the configuration will be as synchronized as possible.
> 
> On the case the hwtstamp_set return the extack error the hwtstamp_get will
> return something that might not be true as not all module will have the same
> config. Is it acceptable?

Well, technically you are right. But this logic was broken from the very
beginning. And as I also mentioned, both modules use the same set
function with the same error path, which means in current situation if
the first call is successful, the second one will also succeed. And
that's why it's acceptable

>> There are only 2 modules using netcp core infrastructure, and both use
>> the very same function to configure HW timestamping, so no actual
>> difference in behavior is expected.
>>
>> Compile test only.
>>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> b/drivers/net/ethernet/ti/netcp_ethss.c index 55a1a96cd834..0ae44112812c
>> 100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c
>> +++ b/drivers/net/ethernet/ti/netcp_ethss.c
>> @@ -2591,20 +2591,26 @@ static int gbe_rxtstamp(struct gbe_intf *gbe_intf,
>> struct netcp_packet *p_info) return 0;
>>   }
>>   
>> -static int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq *ifr)
>> +static int gbe_hwtstamp_get(void *intf_priv, struct kernel_hwtstamp_config
>> *cfg) {
>> -	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
>> -	struct cpts *cpts = gbe_dev->cpts;
>> -	struct hwtstamp_config cfg;
>> +	struct gbe_intf *gbe_intf = intf_priv;
>> +	struct gbe_priv *gbe_dev;
>> +	struct phy_device *phy;
>> +
>> +	gbe_dev = gbe_intf->gbe_dev;
>>   
>> -	if (!cpts)
>> +	if (!gbe_dev->cpts)
>> +		return -EOPNOTSUPP;
>> +
>> +	phy = gbe_intf->slave->phy;
>> +	if (phy_has_hwtstamp(phy))
>>   		return -EOPNOTSUPP;
> 
> This condition should be removed.
> The selection between PHY or MAC timestamping is now done in the net core:
> https://elixir.bootlin.com/linux/v6.17.1/source/net/core/dev_ioctl.c#L244

Yeah, but the problem here is that phy device is not really attached to 
netdev, but rather to some private structure, which is not accessible by
the core, only driver can work with it, according to the original code.

But I might be missing something obvious here, if someone is at least a
bit aware of this code and can shed some light and confirm that phydev
is correctly set and attached to actual netdev, I'm happy to remove this
ugly part.

> 
>>   
>> -	cfg.flags = 0;
>> -	cfg.tx_type = gbe_dev->tx_ts_enabled ? HWTSTAMP_TX_ON :
>> HWTSTAMP_TX_OFF;
>> -	cfg.rx_filter = gbe_dev->rx_ts_enabled;
>> +	cfg->flags = 0;
>> +	cfg->tx_type = gbe_dev->tx_ts_enabled ? HWTSTAMP_TX_ON :
>> HWTSTAMP_TX_OFF;
>> +	cfg->rx_filter = gbe_dev->rx_ts_enabled;
>>   
>> -	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
>> +	return 0;
>>   }
>>   
>>   static void gbe_hwtstamp(struct gbe_intf *gbe_intf)
>> @@ -2637,19 +2643,23 @@ static void gbe_hwtstamp(struct gbe_intf *gbe_intf)
>>   	writel(ctl,    GBE_REG_ADDR(slave, port_regs, ts_ctl_ltype2));
>>   }
>>   
>> -static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq *ifr)
>> +static int gbe_hwtstamp_set(void *intf_priv, struct kernel_hwtstamp_config
>> *cfg,
>> +			    struct netlink_ext_ack *extack)
>>   {
>> -	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
>> -	struct cpts *cpts = gbe_dev->cpts;
>> -	struct hwtstamp_config cfg;
>> +	struct gbe_intf *gbe_intf = intf_priv;
>> +	struct gbe_priv *gbe_dev;
>> +	struct phy_device *phy;
>>   
>> -	if (!cpts)
>> +	gbe_dev = gbe_intf->gbe_dev;
>> +
>> +	if (!gbe_dev->cpts)
>>   		return -EOPNOTSUPP;
>>   
>> -	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
>> -		return -EFAULT;
>> +	phy = gbe_intf->slave->phy;
>> +	if (phy_has_hwtstamp(phy))
>> +		return phy->mii_ts->hwtstamp(phy->mii_ts, cfg, extack);
> 
> Same.
> 
>>   
>> -	switch (cfg.tx_type) {
>> +	switch (cfg->tx_type) {
>>   	case HWTSTAMP_TX_OFF:
>>   		gbe_dev->tx_ts_enabled = 0;
>>   		break;
>> @@ -2660,7 +2670,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
>> struct ifreq *ifr) return -ERANGE;
>>   	}
>>   
>> -	switch (cfg.rx_filter) {
>> +	switch (cfg->rx_filter) {
>>   	case HWTSTAMP_FILTER_NONE:
>>   		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_NONE;
>>   		break;
>> @@ -2668,7 +2678,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
>> struct ifreq *ifr) case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
>>   	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
>>   		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
>> -		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
>> +		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V1_L4_EVENT;
>>   		break;
>>   	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
>>   	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
>> @@ -2680,7 +2690,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
>> struct ifreq *ifr) case HWTSTAMP_FILTER_PTP_V2_SYNC:
>>   	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
>>   		gbe_dev->rx_ts_enabled = HWTSTAMP_FILTER_PTP_V2_EVENT;
>> -		cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>> +		cfg->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
>>   		break;
>>   	default:
>>   		return -ERANGE;
>> @@ -2688,7 +2698,7 @@ static int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
>> struct ifreq *ifr)
>>   	gbe_hwtstamp(gbe_intf);
>>   
>> -	return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
>> +	return 0;
>>   }
>>   
>>   static void gbe_register_cpts(struct gbe_priv *gbe_dev)
>> @@ -2745,12 +2755,15 @@ static inline void gbe_unregister_cpts(struct
>> gbe_priv *gbe_dev) {
>>   }
>>   
>> -static inline int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq
>> *req) +static inline int gbe_hwtstamp_get(struct gbe_intf *gbe_intf,
>> +				   struct kernel_hwtstamp_config *cfg)
>>   {
>>   	return -EOPNOTSUPP;
>>   }
>>   
>> -static inline int gbe_hwtstamp_set(struct gbe_intf *gbe_intf, struct ifreq
>> *req) +static inline int gbe_hwtstamp_set(struct gbe_intf *gbe_intf,
>> +				   struct kernel_hwtstamp_config *cfg,
>> +				   struct netlink_ext_ack *extack)
>>   {
>>   	return -EOPNOTSUPP;
>>   }
>> @@ -2816,15 +2829,6 @@ static int gbe_ioctl(void *intf_priv, struct ifreq
>> *req, int cmd) struct gbe_intf *gbe_intf = intf_priv;
>>   	struct phy_device *phy = gbe_intf->slave->phy;
>>   
>> -	if (!phy_has_hwtstamp(phy)) {
>> -		switch (cmd) {
>> -		case SIOCGHWTSTAMP:
>> -			return gbe_hwtstamp_get(gbe_intf, req);
>> -		case SIOCSHWTSTAMP:
>> -			return gbe_hwtstamp_set(gbe_intf, req);
>> -		}
>> -	}
>> -
>>   	if (phy)
>>   		return phy_mii_ioctl(phy, req, cmd);
>>   
>> @@ -3824,6 +3828,8 @@ static struct netcp_module gbe_module = {
>>   	.add_vid	= gbe_add_vid,
>>   	.del_vid	= gbe_del_vid,
>>   	.ioctl		= gbe_ioctl,
>> +	.hwtstamp_get	= gbe_hwtstamp_get,
>> +	.hwtstamp_set	= gbe_hwtstamp_set,
>>   };
>>   
>>   static struct netcp_module xgbe_module = {
>> @@ -3841,6 +3847,8 @@ static struct netcp_module xgbe_module = {
>>   	.add_vid	= gbe_add_vid,
>>   	.del_vid	= gbe_del_vid,
>>   	.ioctl		= gbe_ioctl,
>> +	.hwtstamp_get	= gbe_hwtstamp_get,
>> +	.hwtstamp_set	= gbe_hwtstamp_set,
>>   };
>>   
>>   static int __init keystone_gbe_init(void)
> 
> 
> 

Re: [PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

[Kory Maincent]

On Thu, 30 Oct 2025 12:18:50 +0000
Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:

> On 30/10/2025 10:30, Kory Maincent wrote:
> > On Wed, 29 Oct 2025 20:09:22 +0000
> > Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:
> >   
> >> Convert TI NetCP driver to use ndo_hwtstamp_get()/ndo_hwtstamp_set()
> >> callbacks. The logic is slightly changed, because I believe the original
> >> logic was not really correct. Config reading part is using the very
> >> first module to get the configuration instead of iterating over all of
> >> them and keep the last one as the configuration is supposed to be identical
> >> for all modules. HW timestamp config set path is now trying to configure
> >> all modules, but in case of error from one module it adds extack
> >> message. This way the configuration will be as synchronized as possible.  
> > 
> > On the case the hwtstamp_set return the extack error the hwtstamp_get will
> > return something that might not be true as not all module will have the same
> > config. Is it acceptable?  
> 
> Well, technically you are right. But this logic was broken from the very
> beginning. And as I also mentioned, both modules use the same set
> function with the same error path, which means in current situation if
> the first call is successful, the second one will also succeed. And
> that's why it's acceptable
> 
> >> There are only 2 modules using netcp core infrastructure, and both use
> >> the very same function to configure HW timestamping, so no actual
> >> difference in behavior is expected.
> >>
> >> Compile test only.
> >>
> >> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> >> b/drivers/net/ethernet/ti/netcp_ethss.c index 55a1a96cd834..0ae44112812c
> >> 100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c
> >> +++ b/drivers/net/ethernet/ti/netcp_ethss.c
> >> @@ -2591,20 +2591,26 @@ static int gbe_rxtstamp(struct gbe_intf *gbe_intf,
> >> struct netcp_packet *p_info) return 0;
> >>   }
> >>   
> >> -static int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq *ifr)
> >> +static int gbe_hwtstamp_get(void *intf_priv, struct kernel_hwtstamp_config
> >> *cfg) {
> >> -	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
> >> -	struct cpts *cpts = gbe_dev->cpts;
> >> -	struct hwtstamp_config cfg;
> >> +	struct gbe_intf *gbe_intf = intf_priv;
> >> +	struct gbe_priv *gbe_dev;
> >> +	struct phy_device *phy;
> >> +
> >> +	gbe_dev = gbe_intf->gbe_dev;
> >>   
> >> -	if (!cpts)
> >> +	if (!gbe_dev->cpts)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	phy = gbe_intf->slave->phy;
> >> +	if (phy_has_hwtstamp(phy))
> >>   		return -EOPNOTSUPP;  
> > 
> > This condition should be removed.
> > The selection between PHY or MAC timestamping is now done in the net core:
> > https://elixir.bootlin.com/linux/v6.17.1/source/net/core/dev_ioctl.c#L244  
> 
> Yeah, but the problem here is that phy device is not really attached to 
> netdev, but rather to some private structure, which is not accessible by
> the core, only driver can work with it, according to the original code.

The PHY are either attached to gbe_intf->ndev or gbe_dev->dummy_ndev.
Therefore I think it will already attached to the net_device pointer used by
hwtstamp NDOs.
Mmh indeed each slave can have a different PHY. Why is there several slave + phy
per netdev?
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/net/ethernet/ti/netcp_ethss.c#L3163
Shouldn't each netdev be associated to one PHY (or use the phy topology
support). This is kind of weird.

In either case if there is an issue here, it should be split in a second patch
as you are modifying the behavior of the driver.
Maybe you should already split the patch in two to separate the NDO conversion
from the module iteration logic design change.

> But I might be missing something obvious here, if someone is at least a
> bit aware of this code and can shed some light and confirm that phydev
> is correctly set and attached to actual netdev, I'm happy to remove this
> ugly part.

Yeah, this driver seems a bit ugly to me also but maybe we are missing
something. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

[Jakub Kicinski]

On Thu, 30 Oct 2025 16:18:24 +0100 Kory Maincent wrote:
> > But I might be missing something obvious here, if someone is at least a
> > bit aware of this code and can shed some light and confirm that phydev
> > is correctly set and attached to actual netdev, I'm happy to remove this
> > ugly part.  
> 
> Yeah, this driver seems a bit ugly to me also but maybe we are missing
> something. 

Since this is a TI device it may be one of those two-port things they
have for HSR/PRP(?). It's a single netdev but it actually has two ports
and PHY to forward frames along the ring. Hence two "modules"? I could
be wrong.

In any case - in a large refactoring effort like removing the hwts
ioctl we should try to avoid non-obvious cleanups. Unless the code is
actively maintained and someone is offering to help or at least test.
We still have a few drivers and a bunch of PHY plumbing to go thru..

So I'd copy what the original code was doing, add the goto in
netcp_ndo_hwtstamp_set() after the first failure..

This:

+		err = module->hwtstamp_set(intf_modpriv->module_priv, config,
+					   extack);
+		if ((err < 0) && (err != -EOPNOTSUPP)) {
+			NL_SET_ERR_MSG_WEAK_MOD(extack,
+						"At least one module failed to setup HW timestamps");
+			ret = err;
+		}
+		if (err == 0)
+			ret = err;

will leave ret at 0 if _any_ configuration succeeded which is worse.
User needs to know about the failure. We could keep going in the loop.
But hiding errors is a no-no.