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

[PATCH net-next v2] 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.

Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
v1 -> v2:
- avoid changing logic and hiding errors. keep the call failing after
  the first error
---
 drivers/net/ethernet/ti/netcp.h       |  5 ++
 drivers/net/ethernet/ti/netcp_core.c  | 58 +++++++++++++++++++++
 drivers/net/ethernet/ti/netcp_ethss.c | 72 +++++++++++++++------------
 3 files changed, 103 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..cee2686a4893 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -1781,6 +1781,62 @@ 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;
+			goto out;
+		}
+		if (err == 0)
+			ret = err;
+	}
+
+out:
+	return (ret == 0) ? 0 : err;
+}
+
 static int netcp_ndo_ioctl(struct net_device *ndev,
 			   struct ifreq *req, int cmd)
 {
@@ -1952,6 +2008,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 v2] ti: netcp: convert to ndo_hwtstamp callbacks

[Kory Maincent]

On Mon,  3 Nov 2025 17:29:02 +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.
> 
> 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.
> 
> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> ---
> v1 -> v2:
> - avoid changing logic and hiding errors. keep the call failing after
>   the first error
> ---

...

> +
> +	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;
> +			goto out;

Why don't you use break.

> +		}
> +		if (err == 0)
> +			ret = err;
> +	}
> +
> +out:
> +	return (ret == 0) ? 0 : err;
> +}
> +

...

> -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);

Sorry to come back to this but the choice of using PHY or MAC timestamping is
done in the core. Putting this here may conflict with the core.
I know this driver has kind of a weird PHYs management through slave
description but we shouldn't let the MAC driver call the PHY hwtstamp ops. 
If there is indeed an issue due to the weird development of this driver, people
will write a patch specifically tackling this issue and maybe (by luck)
refactoring this driver.

Anyway, this was not in the driver before, so I think we should not make this
change in this patch.

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

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

[Vadim Fedorenko]

On 03/11/2025 20:52, Kory Maincent wrote:
> On Mon,  3 Nov 2025 17:29:02 +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.
>>
>> 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.
>>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> ---
>> v1 -> v2:
>> - avoid changing logic and hiding errors. keep the call failing after
>>    the first error
>> ---
> 
> ...
> 
>> +
>> +	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;
>> +			goto out;
> 
> Why don't you use break.

That's the original code, I tried to make as less changes as possible

> 
>> +		}
>> +		if (err == 0)
>> +			ret = err;
>> +	}
>> +
>> +out:
>> +	return (ret == 0) ? 0 : err;
>> +}
>> +
> 
> ...
> 
>> -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);
> 
> Sorry to come back to this but the choice of using PHY or MAC timestamping is
> done in the core. Putting this here may conflict with the core.
> I know this driver has kind of a weird PHYs management through slave
> description but we shouldn't let the MAC driver call the PHY hwtstamp ops.
> If there is indeed an issue due to the weird development of this driver, people
> will write a patch specifically tackling this issue and maybe (by luck)
> refactoring this driver.
> 
> Anyway, this was not in the driver before, so I think we should not make this
> change in this patch.

Well, that was actually in the original code:

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);

         return -EOPNOTSUPP;
}

SIOCGHWTSTAMP/SIOCSHWTSTAMP were sent to gbe functions only when there
was no support for hwtstamps on phy layer. The original flow of the call
is:

netcp_ndo_ioctl -> gbe_ioctl -> gbe_hwtstamp_*/phy_mii_ioctl

where netcp_ndo_ioctl operating over netdev while other function
operating with other objects, with phy taken from gbe_intf.

Checking on init part of phy devices, I found that the only phydev
allocated structure is stored in gbe_slave object, which is definitely
not accessible from the core. I haven't found any assignments to
net_device->phydev in neither netcp_core.c nor netcp_ethss.c.
Even though there are checks for some phy functions from netdev->phydev
in RX and TX paths, I'm not quite sure it works properly.

I decided to keep the original logic here with checking phy from
gbe_intf->slave.

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

[Kory Maincent]

On Tue, 4 Nov 2025 12:15:32 +0000
Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:

> On 03/11/2025 20:52, Kory Maincent wrote:
> > On Mon,  3 Nov 2025 17:29:02 +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.
> >>
> >> 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.
> >>
> >> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> >> ---
> >> v1 -> v2:
> >> - avoid changing logic and hiding errors. keep the call failing after
> >>    the first error
> >> ---  
> > 
> > ...
> >   
> >> +
> >> +	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;
> >> +			goto out;  
> > 
> > Why don't you use break.  
> 
> That's the original code, I tried to make as less changes as possible
> 
> >   
> >> +		}
> >> +		if (err == 0)
> >> +			ret = err;
> >> +	}
> >> +
> >> +out:
> >> +	return (ret == 0) ? 0 : err;
> >> +}
> >> +  
> > 
> > ...
> >   
> >> -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);  
> > 
> > Sorry to come back to this but the choice of using PHY or MAC timestamping
> > is done in the core. Putting this here may conflict with the core.
> > I know this driver has kind of a weird PHYs management through slave
> > description but we shouldn't let the MAC driver call the PHY hwtstamp ops.
> > If there is indeed an issue due to the weird development of this driver,
> > people will write a patch specifically tackling this issue and maybe (by
> > luck) refactoring this driver.
> > 
> > Anyway, this was not in the driver before, so I think we should not make
> > this change in this patch.  
> 
> Well, that was actually in the original code:

Oh indeed, sorry, I missed that.
 
> 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);
> 
>          return -EOPNOTSUPP;
> }
> 
> SIOCGHWTSTAMP/SIOCSHWTSTAMP were sent to gbe functions only when there
> was no support for hwtstamps on phy layer. The original flow of the call
> is:
> 
> netcp_ndo_ioctl -> gbe_ioctl -> gbe_hwtstamp_*/phy_mii_ioctl
> 
> where netcp_ndo_ioctl operating over netdev while other function
> operating with other objects, with phy taken from gbe_intf.
> 
> Checking on init part of phy devices, I found that the only phydev
> allocated structure is stored in gbe_slave object, which is definitely
> not accessible from the core. I haven't found any assignments to
> net_device->phydev in neither netcp_core.c nor netcp_ethss.c.
> Even though there are checks for some phy functions from netdev->phydev
> in RX and TX paths, I'm not quite sure it works properly.
> 
> I decided to keep the original logic here with checking phy from
> gbe_intf->slave.

Ok. I still think this may conflict when associated to a PHY that support
hwtstamp, but if you keep the old logic then it is ok to me. Someone will fix
it when the case appear. FYI you could use the phy_hwtstamp() helper
instead of phy->mii_ts->hwtstamp(). Relevant in case of v3.

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!

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

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

[Vadim Fedorenko]

On 04/11/2025 13:39, Kory Maincent wrote:
> On Tue, 4 Nov 2025 12:15:32 +0000
> Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:
> 
>> On 03/11/2025 20:52, Kory Maincent wrote:
>>> On Mon,  3 Nov 2025 17:29:02 +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.
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>>>> ---
>>>> v1 -> v2:
>>>> - avoid changing logic and hiding errors. keep the call failing after
>>>>     the first error
>>>> ---
>>>
>>> ...
>>>    
>>>> +
>>>> +	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;
>>>> +			goto out;
>>>
>>> Why don't you use break.
>>
>> That's the original code, I tried to make as less changes as possible
>>
>>>    
>>>> +		}
>>>> +		if (err == 0)
>>>> +			ret = err;
>>>> +	}
>>>> +
>>>> +out:
>>>> +	return (ret == 0) ? 0 : err;
>>>> +}
>>>> +
>>>
>>> ...
>>>    
>>>> -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);
>>>
>>> Sorry to come back to this but the choice of using PHY or MAC timestamping
>>> is done in the core. Putting this here may conflict with the core.
>>> I know this driver has kind of a weird PHYs management through slave
>>> description but we shouldn't let the MAC driver call the PHY hwtstamp ops.
>>> If there is indeed an issue due to the weird development of this driver,
>>> people will write a patch specifically tackling this issue and maybe (by
>>> luck) refactoring this driver.
>>>
>>> Anyway, this was not in the driver before, so I think we should not make
>>> this change in this patch.
>>
>> Well, that was actually in the original code:
> 
> Oh indeed, sorry, I missed that.
>   
>> 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);
>>
>>           return -EOPNOTSUPP;
>> }
>>
>> SIOCGHWTSTAMP/SIOCSHWTSTAMP were sent to gbe functions only when there
>> was no support for hwtstamps on phy layer. The original flow of the call
>> is:
>>
>> netcp_ndo_ioctl -> gbe_ioctl -> gbe_hwtstamp_*/phy_mii_ioctl
>>
>> where netcp_ndo_ioctl operating over netdev while other function
>> operating with other objects, with phy taken from gbe_intf.
>>
>> Checking on init part of phy devices, I found that the only phydev
>> allocated structure is stored in gbe_slave object, which is definitely
>> not accessible from the core. I haven't found any assignments to
>> net_device->phydev in neither netcp_core.c nor netcp_ethss.c.
>> Even though there are checks for some phy functions from netdev->phydev
>> in RX and TX paths, I'm not quite sure it works properly.
>>
>> I decided to keep the original logic here with checking phy from
>> gbe_intf->slave.
> 
> Ok. I still think this may conflict when associated to a PHY that support
> hwtstamp, but if you keep the old logic then it is ok to me. Someone will fix
> it when the case appear. FYI you could use the phy_hwtstamp() helper
> instead of phy->mii_ts->hwtstamp(). Relevant in case of v3.

Sure, good catch. I'll change it if v3 is needed

> 
> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
> 
> Thank you!
> 
> Regards,

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  3 Nov 2025 17:29:02 +0000 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] ti: netcp: convert to ndo_hwtstamp callbacks
    https://git.kernel.org/netdev/net-next/c/3f02b8272557

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html