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

[Kory Maincent <kory.maincent@bootlin.com>]

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