netdev - Re: [PATCH net-next] 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] [day] [month] [year] [list]

Message-ID: <20251030161824.0000568a@kmaincent-XPS-13-7390>
Date: Thu, 30 Oct 2025 16:18:24 +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 Thu, 30 Oct 2025 12:18:50 +0000
Vadim Fedorenko <vadim.fedorenko@...ux.dev> wrote:

> On 30/10/2025 10:30, Kory Maincent wrote:
> > 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?  
> 
> 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@...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  
> 
> 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.

The PHY are either attached to gbe_intf->ndev or gbe_dev->dummy_ndev.
Therefore I think it will already attached to the net_device pointer used by
hwtstamp NDOs.
Mmh indeed each slave can have a different PHY. Why is there several slave + phy
per netdev?
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/net/ethernet/ti/netcp_ethss.c#L3163
Shouldn't each netdev be associated to one PHY (or use the phy topology
support). This is kind of weird.

In either case if there is an issue here, it should be split in a second patch
as you are modifying the behavior of the driver.
Maybe you should already split the patch in two to separate the NDO conversion
from the module iteration logic design change.

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

Yeah, this driver seems a bit ugly to me also but maybe we are missing
something. 

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