Message-ID: <20251104143901.5f030fa9@kmaincent-XPS-13-7390>
Date: Tue, 4 Nov 2025 14:39:01 +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 v2] ti: netcp: convert to ndo_hwtstamp
 callbacks

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

> On 03/11/2025 20:52, Kory Maincent wrote:
> > On Mon,  3 Nov 2025 17:29:02 +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.
> >>
> >> 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@...ux.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@...tlin.com>

Thank you!

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

Powered by blists - more mailing lists