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

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

On Mon, 11 Nov 2024 15:06:09 -0800
Jakub Kicinski <kuba@kernel.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