netdev - Re: [PATCH net-next v2] ti: netcp: convert to ndo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <afbddc5a-c051-4e45-9d4f-79d4543f6529@linux.dev>
Date: Tue, 4 Nov 2025 12:15:32 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Kory Maincent <kory.maincent@...tlin.com>
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 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:

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.