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

Kory Maincent posted 10 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Kory Maincent 1 year, 3 months ago
Prepare for future hardware timestamp selection by adding source and
corresponding pointers to ptp_clock structure.
Additionally, introduce helpers for registering specific phydev or netdev
PTP clocks, retrieving PTP clock information such as hwtstamp source or
phydev/netdev pointers, and obtaining the ptp_clock structure from the
phc index.
These helpers are added to a new ptp_clock_consumer.c file, built as
builtin. This is necessary because these helpers will be called by
ethtool or net timestamping, which are builtin code.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Change in v8:
- New patch.

Change in v10:
- Add get and put function to avoid unregistering a ptp clock object used.
- Fix kdoc issues.

Change in v11:
- Remove useless extern.

Change in v12:
- Add missing return description in the kdoc.

Change in v13:
- Remove a semicolon which bring errors while not building PTP driver.
- Remove few useless EXPORT_SYMBOL().
- Separate PTP consumer symbole which are builtin from PTP provider.

Change in v14:
- Add back missing EXPORT_SYMBOL().
---
 drivers/ptp/Makefile             |   5 ++
 drivers/ptp/ptp_clock.c          |  33 ++++++++++-
 drivers/ptp/ptp_clock_consumer.c | 100 +++++++++++++++++++++++++++++++
 drivers/ptp/ptp_private.h        |   7 +++
 include/linux/ptp_clock_kernel.h | 125 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 269 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 01b5cd91eb61..ab4990f56e5e 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -3,6 +3,11 @@
 # Makefile for PTP 1588 clock support.
 #
 
+ifdef CONFIG_PTP_1588_CLOCK
+# The ptp_clock consumer is built-in whenever PTP_1588_CLOCK is built-in
+# or module
+obj-y					:= ptp_clock_consumer.o
+endif
 ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o ptp_vclock.o
 ptp_kvm-$(CONFIG_X86)			:= ptp_kvm_x86.o ptp_kvm_common.o
 ptp_kvm-$(CONFIG_HAVE_ARM_SMCCC)	:= ptp_kvm_arm.o ptp_kvm_common.o
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index c56cd0f63909..593b5c906314 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -34,7 +34,6 @@ const struct class ptp_class = {
 
 static dev_t ptp_devt;
 
-static DEFINE_XARRAY_ALLOC(ptp_clocks_map);
 
 /* time stamp event queue operations */
 
@@ -512,6 +511,38 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
 }
 EXPORT_SYMBOL(ptp_cancel_worker_sync);
 
+struct ptp_clock *netdev_ptp_clock_register(struct ptp_clock_info *info,
+					    struct net_device *dev)
+{
+	struct ptp_clock *ptp;
+
+	ptp = ptp_clock_register(info, &dev->dev);
+	if (IS_ERR(ptp))
+		return ptp;
+
+	ptp->phc_source = HWTSTAMP_SOURCE_NETDEV;
+	ptp->netdev = dev;
+
+	return ptp;
+}
+EXPORT_SYMBOL(netdev_ptp_clock_register);
+
+struct ptp_clock *phydev_ptp_clock_register(struct ptp_clock_info *info,
+					    struct phy_device *phydev)
+{
+	struct ptp_clock *ptp;
+
+	ptp = ptp_clock_register(info, &phydev->mdio.dev);
+	if (IS_ERR(ptp))
+		return ptp;
+
+	ptp->phc_source = HWTSTAMP_SOURCE_PHYLIB;
+	ptp->phydev = phydev;
+
+	return ptp;
+}
+EXPORT_SYMBOL(phydev_ptp_clock_register);
+
 /* module operations */
 
 static void __exit ptp_exit(void)
diff --git a/drivers/ptp/ptp_clock_consumer.c b/drivers/ptp/ptp_clock_consumer.c
new file mode 100644
index 000000000000..58b0c8948fc8
--- /dev/null
+++ b/drivers/ptp/ptp_clock_consumer.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * PTP 1588 clock support
+ *
+ * Copyright (C) 2010 OMICRON electronics GmbH
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/posix-clock.h>
+#include <linux/pps_kernel.h>
+#include <linux/slab.h>
+#include <linux/syscalls.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <linux/xarray.h>
+#include <uapi/linux/sched/types.h>
+
+#include "ptp_private.h"
+
+DEFINE_XARRAY_ALLOC(ptp_clocks_map);
+EXPORT_SYMBOL(ptp_clocks_map);
+
+bool ptp_clock_from_phylib(struct ptp_clock *ptp)
+{
+	return ptp->phc_source == HWTSTAMP_SOURCE_PHYLIB;
+}
+
+bool ptp_clock_from_netdev(struct ptp_clock *ptp)
+{
+	return ptp->phc_source == HWTSTAMP_SOURCE_NETDEV;
+}
+
+struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
+{
+	if (ptp->phc_source != HWTSTAMP_SOURCE_NETDEV)
+		return NULL;
+
+	return ptp->netdev;
+}
+
+struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp)
+{
+	if (ptp->phc_source != HWTSTAMP_SOURCE_PHYLIB)
+		return NULL;
+
+	return ptp->phydev;
+}
+EXPORT_SYMBOL(ptp_clock_phydev);
+
+int ptp_clock_get(struct device *dev, struct ptp_clock *ptp)
+{
+	struct device_link *link;
+
+	if (!ptp)
+		return 0;
+
+	if (!try_module_get(ptp->info->owner))
+		return -EPROBE_DEFER;
+
+	get_device(&ptp->dev);
+
+	link = device_link_add(dev, &ptp->dev, DL_FLAG_STATELESS);
+	if (!link)
+		dev_warn(dev, "failed to create device link to %s\n",
+			 dev_name(&ptp->dev));
+
+	return 0;
+}
+
+struct ptp_clock *ptp_clock_get_by_index(struct device *dev, int index)
+{
+	struct ptp_clock *ptp;
+	int ret;
+
+	if (index < 0)
+		return NULL;
+
+	ptp = xa_load(&ptp_clocks_map, (unsigned long)index);
+	if (IS_ERR_OR_NULL(ptp))
+		return ptp;
+
+	ret = ptp_clock_get(dev, ptp);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return ptp;
+}
+
+void ptp_clock_put(struct device *dev, struct ptp_clock *ptp)
+{
+	if (!ptp)
+		return;
+
+	device_link_remove(dev, &ptp->dev);
+	put_device(&ptp->dev);
+	module_put(ptp->info->owner);
+}
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 18934e28469e..6a306e6e34c2 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -24,6 +24,8 @@
 #define PTP_DEFAULT_MAX_VCLOCKS 20
 #define PTP_MAX_CHANNELS 2048
 
+extern struct xarray ptp_clocks_map;
+
 struct timestamp_event_queue {
 	struct ptp_extts_event buf[PTP_MAX_TIMESTAMPS];
 	int head;
@@ -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;
+	};
 	struct pps_device *pps_source;
 	long dialed_frequency; /* remembers the frequency adjustment */
 	struct list_head tsevqs; /* timestamp fifo list */
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index c892d22ce0a7..87d8f42b2cc1 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -9,7 +9,9 @@
 #define _PTP_CLOCK_KERNEL_H_
 
 #include <linux/device.h>
+#include <linux/netdevice.h>
 #include <linux/pps_kernel.h>
+#include <linux/phy.h>
 #include <linux/ptp_clock.h>
 #include <linux/timecounter.h>
 #include <linux/skbuff.h>
@@ -342,6 +344,106 @@ extern void ptp_clock_event(struct ptp_clock *ptp,
 
 extern int ptp_clock_index(struct ptp_clock *ptp);
 
+/**
+ * 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.
+ *
+ * Return: Pointer of the PTP clock, error pointer otherwise.
+ */
+
+struct ptp_clock *
+netdev_ptp_clock_register(struct ptp_clock_info *info,
+			  struct net_device *dev);
+
+/**
+ * phydev_ptp_clock_register() - Register a PTP hardware clock driver for
+ *				 a phy device
+ *
+ * @info:   Structure describing the new clock.
+ * @phydev:  Pointer of the phy device.
+ *
+ * Return: Pointer of the PTP clock, error pointer otherwise.
+ */
+
+struct ptp_clock *
+phydev_ptp_clock_register(struct ptp_clock_info *info,
+			  struct phy_device *phydev);
+
+/**
+ * ptp_clock_from_phylib() - Does the PTP clock comes from phylib
+ *
+ * @ptp:  The clock obtained from net/phy_ptp_clock_register().
+ *
+ * Return: True if the PTP clock comes from phylib, false otherwise.
+ */
+
+bool ptp_clock_from_phylib(struct ptp_clock *ptp);
+
+/**
+ * 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.
+ */
+
+bool ptp_clock_from_netdev(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_netdev() - Obtain the net_device reference of PTP clock
+ *
+ * @ptp:  The clock obtained from netdev_ptp_clock_register().
+ *
+ * Return: Pointer of the net device, NULL otherwise.
+ */
+
+struct net_device *ptp_clock_netdev(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_phydev() - Obtain the phy_device reference of a PTP clock
+ *
+ * @ptp:  The clock obtained from phydev_ptp_clock_register().
+ *
+ * Return: Pointer of the phy device, NULL otherwise.
+ */
+
+struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_get() - Increment refcount of the PTP clock
+ *
+ * @dev:  The device which get the PTP clock.
+ * @ptp:  Pointer of a PTP clock.
+ *
+ * Return: 0 in case of success, error otherwise.
+ */
+
+int ptp_clock_get(struct device *dev, struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_get_by_index() - Obtain the PTP clock reference from a given
+ *			      PHC index
+ *
+ * @dev:  The device which get the PTP clock.
+ * @index:  The device index of a PTP clock.
+ *
+ * Return: Pointer of the PTP clock, error pointer otherwise.
+ */
+
+struct ptp_clock *ptp_clock_get_by_index(struct device *dev, int index);
+
+/**
+ * ptp_clock_put() - decrement refcount of the PTP clock
+ *
+ * @dev:  The device which get the PTP clock.
+ * @ptp:  Pointer of a PTP clock.
+ */
+
+void ptp_clock_put(struct device *dev, struct ptp_clock *ptp);
+
 /**
  * ptp_find_pin() - obtain the pin index of a given auxiliary function
  *
@@ -407,6 +509,29 @@ static inline void ptp_clock_event(struct ptp_clock *ptp,
 { }
 static inline int ptp_clock_index(struct ptp_clock *ptp)
 { return -1; }
+static inline struct ptp_clock *
+netdev_ptp_clock_register(struct ptp_clock_info *info,
+			  struct net_device *dev)
+{ return NULL; }
+static inline struct ptp_clock *
+phydev_ptp_clock_register(struct ptp_clock_info *info,
+			  struct phy_device *phydev)
+{ return NULL; }
+static inline bool ptp_clock_from_phylib(struct ptp_clock *ptp)
+{ return false; }
+static inline bool ptp_clock_from_netdev(struct ptp_clock *ptp)
+{ return false; }
+static inline struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
+{ return NULL; }
+static inline struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp)
+{ return NULL; }
+static inline int ptp_clock_get(struct device *dev, struct ptp_clock *ptp)
+{ return -ENODEV; }
+static inline void ptp_clock_put(struct device *dev, struct ptp_clock *ptp)
+{ }
+static inline struct ptp_clock *ptp_clock_get_by_index(struct device *dev,
+						       int index)
+{ return NULL; }
 static inline int ptp_find_pin(struct ptp_clock *ptp,
 			       enum ptp_pin_function func, unsigned int chan)
 { return -1; }

-- 
2.34.1
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Jakub Kicinski 1 year, 2 months ago
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?

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.

> +/**
> + * 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 ?

> +	ret = ptp_clock_get(dev, ptp);
> +	if (ret)
> +		return ERR_PTR(ret);

why do you take references on the ptp device?
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Kory Maincent 1 year, 2 months ago
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
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Jakub Kicinski 1 year, 2 months ago
On Tue, 12 Nov 2024 11:12:32 +0100 Kory Maincent wrote:
> > 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.

I don't understand. I'm complaining storing netdev state in 
struct ptp_clock. It's perfectly fine to add the extra info to netdev
and PHY topology maintained by the core.

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

I understand, I don't want to push you towards implementing all that.
But if we keep that in mind as the north star we should try to align
this default / temporary solution. If the socket API takes a PHC ID
as an input, the configuration we take in should also be maintained
as "int default_phc", not pointers to things.

IOW I'm struggling to connect the dots how the code you're adding now
will be built _upon_ rather than _on the side_ of when socket PHC
selection is in place.
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Kory Maincent 1 year, 2 months ago
On Tue, 12 Nov 2024 18:22:26 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 12 Nov 2024 11:12:32 +0100 Kory Maincent wrote:
> > > 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.  
> 
> I don't understand. I'm complaining storing netdev state in 
> struct ptp_clock. It's perfectly fine to add the extra info to netdev
> and PHY topology maintained by the core.

Oh okay. Didn't get it the first time. I could move the PHC source with the
phydev pointer in netdev core to avoid this.

> > > 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.  
> 
> I understand, I don't want to push you towards implementing all that.
> But if we keep that in mind as the north star we should try to align
> this default / temporary solution. If the socket API takes a PHC ID
> as an input, the configuration we take in should also be maintained
> as "int default_phc", not pointers to things.

In fact I could remove the hwtstamp pointer from netdevice and add only the
hwtstamp source with the phydev rcu pointer.
We will need the phydev pointer as we don't know to which phy device belong the
PTP. Or we could also use the phyindex instead of the phydev pointer and use
phy_link_topo_get_phy() in the hot path.

> IOW I'm struggling to connect the dots how the code you're adding now
> will be built _upon_ rather than _on the side_ of when socket PHC
> selection is in place.

I see what you mean! It is not something easy to think of as I don't really
know how it would be implemented.
Do you think adding simply the PHC source and the phydev pointer or index would
fit? 

This could be removed from netdev core when we move to socket PHC as it
won't be necessary to save the current PHC.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Jakub Kicinski 1 year, 2 months ago
On Wed, 13 Nov 2024 11:38:08 +0100 Kory Maincent wrote:
> > IOW I'm struggling to connect the dots how the code you're adding now
> > will be built _upon_ rather than _on the side_ of when socket PHC
> > selection is in place.  
> 
> I see what you mean! It is not something easy to think of as I don't really
> know how it would be implemented.
> Do you think adding simply the PHC source and the phydev pointer or index would
> fit? 

In net_device? Yes, I think so.

> This could be removed from netdev core when we move to socket PHC as it
> won't be necessary to save the current PHC.
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Kory Maincent 1 year, 2 months ago
On Wed, 13 Nov 2024 16:39:25 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 13 Nov 2024 11:38:08 +0100 Kory Maincent wrote:
> > > IOW I'm struggling to connect the dots how the code you're adding now
> > > will be built _upon_ rather than _on the side_ of when socket PHC
> > > selection is in place.    
> > 
> > I see what you mean! It is not something easy to think of as I don't really
> > know how it would be implemented.
> > Do you think adding simply the PHC source and the phydev pointer or index
> > would fit?   
> 
> In net_device? Yes, I think so.
 
Also as the "user" is not described in the ptp_clock structure the only way to
find it is to roll through all the PTP of the concerned net device topology.
This find ptp loop will not be in the hotpath but only when getting the tsinfo
of a PHC or changing the current PHC. Is it ok for you?

I am at v20 so I ask for confirmation before changing the full patch series! ;)

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Jakub Kicinski 1 year, 2 months ago
On Thu, 14 Nov 2024 11:46:10 +0100 Kory Maincent wrote:
> > > I see what you mean! It is not something easy to think of as I don't really
> > > know how it would be implemented.
> > > Do you think adding simply the PHC source and the phydev pointer or index
> > > would fit?     
> > 
> > In net_device? Yes, I think so.  
>  
> Also as the "user" is not described in the ptp_clock structure the only way to
> find it is to roll through all the PTP of the concerned net device topology.
> This find ptp loop will not be in the hotpath but only when getting the tsinfo
> of a PHC or changing the current PHC. Is it ok for you?

I think so :) We need to be able to figure out if it's the MAC PHC
quickly, because MAC timestamping can be high rate. But IIUC PHY
timestamping will usually involve async work and slow buses, so
walking all PHYs of a netdev should be fine. Especially that 99%
of the time there will only be one. Hope I understood the question..

> I am at v20 so I ask for confirmation before changing the full patch series! ;)
Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information
Posted by Kory Maincent 1 year, 2 months ago
On Thu, 14 Nov 2024 17:39:06 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 14 Nov 2024 11:46:10 +0100 Kory Maincent wrote:
>  [...]  
> > > 
> > > In net_device? Yes, I think so.    
> >  
> > Also as the "user" is not described in the ptp_clock structure the only way
> > to find it is to roll through all the PTP of the concerned net device
> > topology. This find ptp loop will not be in the hotpath but only when
> > getting the tsinfo of a PHC or changing the current PHC. Is it ok for you?  
> 
> I think so :) We need to be able to figure out if it's the MAC PHC
> quickly, because MAC timestamping can be high rate. But IIUC PHY
> timestamping will usually involve async work and slow buses, so
> walking all PHYs of a netdev should be fine. Especially that 99%
> of the time there will only be one. Hope I understood the question..

Yes, thanks for the confirmation. 

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