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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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