Message-ID: <20251030113007.1acc78b6@kmaincent-XPS-13-7390>
Date: Thu, 30 Oct 2025 11:30:07 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next] ti: netcp: convert to ndo_hwtstamp callbacks

On Wed, 29 Oct 2025 20:09:22 +0000
Vadim Fedorenko <vadim.fedorenko@...ux.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@...ux.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

Powered by blists - more mailing lists