netdev - Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information

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: <20241112111232.1637f814@kmaincent-XPS-13-7390>
Date: Tue, 12 Nov 2024 11:12:32 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>, Broadcom internal
 kernel review list <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn
 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Richard
 Cochran <richardcochran@...il.com>, Radu Pirea
 <radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, Andy
 Gospodarek <andy@...yhouse.net>, Nicolas Ferre
 <nicolas.ferre@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jonathan Corbet
 <corbet@....net>, Horatiu Vultur <horatiu.vultur@...rochip.com>,
 UNGLinuxDriver@...rochip.com, Simon Horman <horms@...nel.org>, Vladimir
 Oltean <vladimir.oltean@....com>, donald.hunter@...il.com,
 danieller@...dia.com, ecree.xilinx@...il.com, Andrew Lunn
 <andrew+netdev@...n.ch>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-doc@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>,
 Willem de Bruijn <willemb@...gle.com>, Shannon Nelson
 <shannon.nelson@....com>, Alexandra Winter <wintera@...ux.ibm.com>, Jacob
 Keller <jacob.e.keller@...el.com>
Subject: Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to
 register specific PTP clock or get information

On Mon, 11 Nov 2024 15:06:09 -0800
Jakub Kicinski <kuba@...nel.org> wrote:

> On Wed, 30 Oct 2024 14:54:45 +0100 Kory Maincent wrote:
> > @@ -41,6 +43,11 @@ struct ptp_clock {
> >  	struct ptp_clock_info *info;
> >  	dev_t devid;
> >  	int index; /* index into clocks.map */
> > +	enum hwtstamp_source phc_source;
> > +	union { /* Pointer of the phc_source device */
> > +		struct net_device *netdev;
> > +		struct phy_device *phydev;
> > +	};  
> 
> Storing the info about the "user" (netdev, phydev) in the "provider"
> (PHC) feels too much like a layering violation. Why do you need this?

The things is that, the way to manage the phc depends on the "user".
ndo_hwtstamp_set for netdev and phy_hwtstamp_set for phydev.
https://elixir.bootlin.com/linux/v6.11.6/source/net/core/dev_ioctl.c#L323

Before PHC was managed by the driver "user" so there was no need for this
information as the core only gives the task to the single "user". This didn't
really works when there is more than one user possible on the net topology.

> In general I can't shake the feeling that we're trying to configure 
> the "default" PHC for a narrow use case, while the goal should be 
> to let the user pick the PHC per socket.

Indeed PHC per socket would be neat but it would need a lot more work and I am
even not sure how it should be done. Maybe with a new cmsg structure containing
the information of the PHC provider?
In any case the new ETHTOOL UAPI is ready to support multiple PHC at the same
time when it will be supported.
This patch series is something in the middle, being able to enable all the PHC
on a net topology but only one at a time.

> > +/**
> > + * netdev_ptp_clock_register() - Register a PTP hardware clock driver for
> > + *				 a net device
> > + *
> > + * @info: Structure describing the new clock.
> > + * @dev:  Pointer of the net device.  
> 
> > +/**
> > + * ptp_clock_from_netdev() - Does the PTP clock comes from netdev
> > + *
> > + * @ptp:  The clock obtained from net/phy_ptp_clock_register().
> > + *
> > + * Return: True if the PTP clock comes from netdev, false otherwise.  
> 
> > +/**
> > + * ptp_clock_netdev() - Obtain the net_device reference of PTP clock  
> 
> nit: pick one way to spell netdev ?

Yup indeed.

> > +	ret = ptp_clock_get(dev, ptp);
> > +	if (ret)
> > +		return ERR_PTR(ret);  
> 
> why do you take references on the ptp device?

Because we only select one PHC at a time on the net topology.
We need to avoid the selected PTP pointer being freed.

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