[PATCH RESEND 0/3] PtP driver for s390 clocks

[PATCH RESEND 0/3] PtP driver for s390 clocks

[Sven Schnelle]

Hi,

these patches add support for using the s390 physical and TOD clock as ptp
clock. To do so, the first patch adds a clock id to the s390 TOD clock. The
second patch adds sending a udev event when a ptp device is added, so that
userspace is able to generate stable device names for virtual ptp devices.
The last patch adds the PtP driver itself.

Sven Schnelle (3):
  s390/time: Add clocksource id to TOD clock
  ptp: Add clock name to uevent
  s390/time: Add PtP driver

 MAINTAINERS                     |   6 ++
 arch/s390/include/asm/timex.h   |   8 ++
 arch/s390/kernel/time.c         |   8 ++
 drivers/ptp/Kconfig             |  11 +++
 drivers/ptp/Makefile            |   1 +
 drivers/ptp/ptp_clock.c         |  11 ++-
 drivers/ptp/ptp_s390.c          | 127 ++++++++++++++++++++++++++++++++
 include/linux/clocksource_ids.h |   1 +
 8 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ptp/ptp_s390.c

-- 
2.43.0

[PATCH 1/3] s390/time: Add clocksource id to TOD clock

[Sven Schnelle]

To allow specifying the clock source in the upcoming PtP driver,
add a clocksource ID to the s390 TOD clock.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 arch/s390/kernel/time.c         | 1 +
 include/linux/clocksource_ids.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index b713effe0579..4214901c3ab0 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -255,6 +255,7 @@ static struct clocksource clocksource_tod = {
 	.shift		= 24,
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 	.vdso_clock_mode = VDSO_CLOCKMODE_TOD,
+	.id		= CSID_S390_TOD,
 };
 
 struct clocksource * __init clocksource_default_clock(void)
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index 2bb4d8c2f1b0..c4ef4ae2eded 100644
--- a/include/linux/clocksource_ids.h
+++ b/include/linux/clocksource_ids.h
@@ -6,6 +6,7 @@
 enum clocksource_ids {
 	CSID_GENERIC		= 0,
 	CSID_ARM_ARCH_COUNTER,
+	CSID_S390_TOD,
 	CSID_X86_TSC_EARLY,
 	CSID_X86_TSC,
 	CSID_X86_KVM_CLK,
-- 
2.43.0

[PATCH 2/3] ptp: Add clock name to uevent

[Sven Schnelle]

To allow users to have stable device names with the help of udev,
add the name to the udev event that is sent when a new PtP clock
is available. The key is called 'PTP_CLOCK_NAME'.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 drivers/ptp/ptp_clock.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index c56cd0f63909..15937acb79c6 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -25,9 +25,11 @@
 #define PTP_PPS_EVENT PPS_CAPTUREASSERT
 #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
 
+static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);
 const struct class ptp_class = {
 	.name = "ptp",
-	.dev_groups = ptp_groups
+	.dev_groups = ptp_groups,
+	.dev_uevent = ptp_udev_uevent
 };
 
 /* private globals */
@@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
 
 /* module operations */
 
+static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
+
+	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);
+}
+
 static void __exit ptp_exit(void)
 {
 	class_unregister(&ptp_class);
-- 
2.43.0

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Greg Kroah-Hartman]

On Tue, Oct 15, 2024 at 12:54:13PM +0200, Sven Schnelle wrote:
> To allow users to have stable device names with the help of udev,
> add the name to the udev event that is sent when a new PtP clock
> is available. The key is called 'PTP_CLOCK_NAME'.

Where are you documenting this new user/kernel api you are adding?

> 
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  drivers/ptp/ptp_clock.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index c56cd0f63909..15937acb79c6 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -25,9 +25,11 @@
>  #define PTP_PPS_EVENT PPS_CAPTUREASSERT
>  #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
>  
> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);
>  const struct class ptp_class = {
>  	.name = "ptp",
> -	.dev_groups = ptp_groups
> +	.dev_groups = ptp_groups,
> +	.dev_uevent = ptp_udev_uevent
>  };
>  
>  /* private globals */
> @@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
>  
>  /* module operations */
>  
> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
> +
> +	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);

Why is this needed?  Can't you get the name from the sysfs paths, the
symlink should be there already.

thanks,

greg k-h

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Sven Schnelle]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, Oct 15, 2024 at 12:54:13PM +0200, Sven Schnelle wrote:
>> To allow users to have stable device names with the help of udev,
>> add the name to the udev event that is sent when a new PtP clock
>> is available. The key is called 'PTP_CLOCK_NAME'.
>
> Where are you documenting this new user/kernel api you are adding?
>
>> 
>> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
>> ---
>>  drivers/ptp/ptp_clock.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> index c56cd0f63909..15937acb79c6 100644
>> --- a/drivers/ptp/ptp_clock.c
>> +++ b/drivers/ptp/ptp_clock.c
>> @@ -25,9 +25,11 @@
>>  #define PTP_PPS_EVENT PPS_CAPTUREASSERT
>>  #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
>>  
>> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);
>>  const struct class ptp_class = {
>>  	.name = "ptp",
>> -	.dev_groups = ptp_groups
>> +	.dev_groups = ptp_groups,
>> +	.dev_uevent = ptp_udev_uevent
>>  };
>>  
>>  /* private globals */
>> @@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
>>  
>>  /* module operations */
>>  
>> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
>> +{
>> +	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
>> +
>> +	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);
>
> Why is this needed?  Can't you get the name from the sysfs paths, the
> symlink should be there already.

You mean the 'clock_name' attribute in sysfs? That would require to
write some script to iterate over all ptp devices and check the name,
or is there a way to match that in udev?

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Greg Kroah-Hartman]

On Tue, Oct 15, 2024 at 02:02:17PM +0200, Sven Schnelle wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, Oct 15, 2024 at 12:54:13PM +0200, Sven Schnelle wrote:
> >> To allow users to have stable device names with the help of udev,
> >> add the name to the udev event that is sent when a new PtP clock
> >> is available. The key is called 'PTP_CLOCK_NAME'.
> >
> > Where are you documenting this new user/kernel api you are adding?
> >
> >> 
> >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> >> ---
> >>  drivers/ptp/ptp_clock.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> >> index c56cd0f63909..15937acb79c6 100644
> >> --- a/drivers/ptp/ptp_clock.c
> >> +++ b/drivers/ptp/ptp_clock.c
> >> @@ -25,9 +25,11 @@
> >>  #define PTP_PPS_EVENT PPS_CAPTUREASSERT
> >>  #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
> >>  
> >> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);
> >>  const struct class ptp_class = {
> >>  	.name = "ptp",
> >> -	.dev_groups = ptp_groups
> >> +	.dev_groups = ptp_groups,
> >> +	.dev_uevent = ptp_udev_uevent
> >>  };
> >>  
> >>  /* private globals */
> >> @@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
> >>  
> >>  /* module operations */
> >>  
> >> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
> >> +{
> >> +	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
> >> +
> >> +	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);
> >
> > Why is this needed?  Can't you get the name from the sysfs paths, the
> > symlink should be there already.
> 
> You mean the 'clock_name' attribute in sysfs?

Great, yes, it's right there.

> That would require to
> write some script to iterate over all ptp devices and check the name,
> or is there a way to match that in udev?

Yes there is.  Please use that :)

thanks,

greg k-h

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Sven Schnelle]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, Oct 15, 2024 at 02:02:17PM +0200, Sven Schnelle wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Tue, Oct 15, 2024 at 12:54:13PM +0200, Sven Schnelle wrote:
>> >> To allow users to have stable device names with the help of udev,
>> >> add the name to the udev event that is sent when a new PtP clock
>> >> is available. The key is called 'PTP_CLOCK_NAME'.
>> >
>> > Where are you documenting this new user/kernel api you are adding?
>> >
>> >> 
>> >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
>> >> ---
>> >>  drivers/ptp/ptp_clock.c | 11 ++++++++++-
>> >>  1 file changed, 10 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> >> index c56cd0f63909..15937acb79c6 100644
>> >> --- a/drivers/ptp/ptp_clock.c
>> >> +++ b/drivers/ptp/ptp_clock.c
>> >> @@ -25,9 +25,11 @@
>> >>  #define PTP_PPS_EVENT PPS_CAPTUREASSERT
>> >>  #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
>> >>  
>> >> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);
>> >>  const struct class ptp_class = {
>> >>  	.name = "ptp",
>> >> -	.dev_groups = ptp_groups
>> >> +	.dev_groups = ptp_groups,
>> >> +	.dev_uevent = ptp_udev_uevent
>> >>  };
>> >>  
>> >>  /* private globals */
>> >> @@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
>> >>  
>> >>  /* module operations */
>> >>  
>> >> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
>> >> +{
>> >> +	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
>> >> +
>> >> +	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);
>> >
>> > Why is this needed?  Can't you get the name from the sysfs paths, the
>> > symlink should be there already.
>> 
>> You mean the 'clock_name' attribute in sysfs?
>
> Great, yes, it's right there.
>
>> That would require to
>> write some script to iterate over all ptp devices and check the name,
>> or is there a way to match that in udev?
>
> Yes there is.  Please use that :)

Indeed. Sorry, will drop the patch.

[PATCH 3/3] s390/time: Add PtP driver

[Sven Schnelle]

Add a small PtP driver which allows user space to get
the values of the physical and tod clock. This allows
programs like chrony to use STP as clock source and
steer the kernel clock. The physical clock can be used
as a debugging aid to get the clock without any additional
offsets like STP steering or LPAR offset.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 MAINTAINERS                   |   6 ++
 arch/s390/include/asm/timex.h |   8 +++
 arch/s390/kernel/time.c       |   7 ++
 drivers/ptp/Kconfig           |  11 +++
 drivers/ptp/Makefile          |   1 +
 drivers/ptp/ptp_s390.c        | 127 ++++++++++++++++++++++++++++++++++
 6 files changed, 160 insertions(+)
 create mode 100644 drivers/ptp/ptp_s390.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7ad507f49324..94793c935e86 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20328,6 +20328,12 @@ F:	Documentation/arch/s390/pci.rst
 F:	arch/s390/pci/
 F:	drivers/pci/hotplug/s390_pci_hpc.c
 
+S390 PTP DRIVER
+M:	Sven Schnelle <svens@linux.ibm.com>
+L:	linux-s390@vger.kernel.org
+S:	Supported
+F:	drivers/ptp/ptp_s390.c
+
 S390 SCM DRIVER
 M:	Vineeth Vijayan <vneethv@linux.ibm.com>
 L:	linux-s390@vger.kernel.org
diff --git a/arch/s390/include/asm/timex.h b/arch/s390/include/asm/timex.h
index 640901f2fbc3..4fc88a1493f0 100644
--- a/arch/s390/include/asm/timex.h
+++ b/arch/s390/include/asm/timex.h
@@ -93,6 +93,7 @@ extern unsigned char ptff_function_mask[16];
 #define PTFF_QAF	0x00	/* query available functions */
 #define PTFF_QTO	0x01	/* query tod offset */
 #define PTFF_QSI	0x02	/* query steering information */
+#define PTFF_QPT	0x03	/* query physical clock */
 #define PTFF_QUI	0x04	/* query UTC information */
 #define PTFF_ATO	0x40	/* adjust tod offset */
 #define PTFF_STO	0x41	/* set tod offset */
@@ -250,6 +251,12 @@ static __always_inline unsigned long tod_to_ns(unsigned long todval)
 	return ((todval >> 9) * 125) + (((todval & 0x1ff) * 125) >> 9);
 }
 
+static __always_inline unsigned long eitod_to_ns(union tod_clock *clk)
+{
+	clk->eitod -= TOD_UNIX_EPOCH;
+	return ((clk->eitod >> 9) * 125) + (((clk->eitod & 0x1ff) * 125) >> 9);
+}
+
 /**
  * tod_after - compare two 64 bit TOD values
  * @a: first 64 bit TOD timestamp
@@ -278,4 +285,5 @@ static inline int tod_after_eq(unsigned long a, unsigned long b)
 	return a >= b;
 }
 
+bool stp_enabled(void);
 #endif
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 4214901c3ab0..47b20235953c 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -469,6 +469,13 @@ static void __init stp_reset(void)
 	}
 }
 
+bool stp_enabled(void)
+{
+	return test_bit(CLOCK_SYNC_HAS_STP, &clock_sync_flags) &&
+		stp_online;
+}
+EXPORT_SYMBOL(stp_enabled);
+
 static void stp_timeout(struct timer_list *unused)
 {
 	queue_work(time_sync_wq, &stp_work);
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 604541dcb320..907330413ef8 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -224,4 +224,15 @@ config PTP_DFL_TOD
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp_dfl_tod.
 
+config PTP_S390
+	tristate "S390 PTP driver"
+	depends on PTP_1588_CLOCK
+	default y
+	help
+	  This driver adds support for S390 time steering via the PtP
+	  interface. This works by adding a in-kernel clock delta value,
+	  which is always added to time values used in the kernel. The PtP
+	  driver provides the raw clock value without the delta to
+	  userspace. That way userspace programs like chrony could steer
+	  the kernel clock.
 endmenu
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 68bf02078053..4dd9f35eb0cf 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_MOCK)	+= ptp_mock.o
 obj-$(CONFIG_PTP_1588_CLOCK_VMW)	+= ptp_vmw.o
 obj-$(CONFIG_PTP_1588_CLOCK_OCP)	+= ptp_ocp.o
 obj-$(CONFIG_PTP_DFL_TOD)		+= ptp_dfl_tod.o
+obj-$(CONFIG_PTP_S390)			+= ptp_s390.o
diff --git a/drivers/ptp/ptp_s390.c b/drivers/ptp/ptp_s390.c
new file mode 100644
index 000000000000..4edccebcf2b5
--- /dev/null
+++ b/drivers/ptp/ptp_s390.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * s390 PTP clock driver
+ *
+ */
+
+#include "ptp_private.h"
+#include <linux/time.h>
+static struct ptp_clock *ptp_stcke_clock, *ptp_qpt_clock;
+
+static int ptp_s390_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ptp_s390_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	return -EOPNOTSUPP;
+}
+
+static struct timespec64 eitod_to_timespec64(union tod_clock *clk)
+{
+	return ns_to_timespec64(eitod_to_ns(clk));
+}
+
+static struct timespec64 tod_to_timespec64(unsigned long tod)
+{
+	return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
+}
+
+static int ptp_s390_stcke_gettime(struct ptp_clock_info *ptp,
+				  struct timespec64 *ts)
+{
+	union tod_clock tod;
+
+	if (!stp_enabled())
+		return -EOPNOTSUPP;
+
+	store_tod_clock_ext_cc(&tod);
+	*ts = eitod_to_timespec64(&tod);
+	return 0;
+}
+
+static int ptp_s390_qpt_gettime(struct ptp_clock_info *ptp,
+				struct timespec64 *ts)
+{
+	unsigned long tod;
+
+	ptff(&tod, sizeof(tod), PTFF_QPT);
+	*ts = tod_to_timespec64(tod);
+	return 0;
+}
+
+static int ptp_s390_settime(struct ptp_clock_info *ptp,
+			    const struct timespec64 *ts)
+{
+	return -EOPNOTSUPP;
+}
+
+static int s390_arch_ptp_get_crosststamp(ktime_t *device_time,
+					 struct system_counterval_t *system_counter,
+					 void *ctx)
+{
+	union tod_clock clk;
+
+	store_tod_clock_ext_cc(&clk);
+	*device_time = ns_to_ktime(tod_to_ns(clk.tod - TOD_UNIX_EPOCH));
+	system_counter->cycles = clk.tod;
+	system_counter->cs_id = CSID_S390_TOD;
+	return 0;
+}
+
+static int ptp_s390_getcrosststamp(struct ptp_clock_info *ptp,
+				   struct system_device_crosststamp *xtstamp)
+{
+	if (!stp_enabled())
+		return -EOPNOTSUPP;
+	return get_device_system_crosststamp(s390_arch_ptp_get_crosststamp, NULL, NULL, xtstamp);
+}
+
+static struct ptp_clock_info ptp_s390_stcke_info = {
+	.owner		= THIS_MODULE,
+	.name		= "IBM Z STCKE Clock",
+	.max_adj	= 0,
+	.adjfine	= ptp_s390_adjfine,
+	.adjtime	= ptp_s390_adjtime,
+	.gettime64	= ptp_s390_stcke_gettime,
+	.settime64	= ptp_s390_settime,
+	.getcrosststamp = ptp_s390_getcrosststamp,
+};
+
+static struct ptp_clock_info ptp_s390_qpt_info = {
+	.owner		= THIS_MODULE,
+	.name		= "IBM Z Physical Clock",
+	.max_adj	= 0,
+	.adjfine	= ptp_s390_adjfine,
+	.adjtime	= ptp_s390_adjtime,
+	.gettime64	= ptp_s390_qpt_gettime,
+	.settime64	= ptp_s390_settime,
+};
+
+static __init int ptp_s390_init(void)
+{
+	ptp_stcke_clock = ptp_clock_register(&ptp_s390_stcke_info, NULL);
+	if (IS_ERR(ptp_stcke_clock))
+		return PTR_ERR(ptp_stcke_clock);
+
+	ptp_qpt_clock = ptp_clock_register(&ptp_s390_qpt_info, NULL);
+	if (IS_ERR(ptp_qpt_clock)) {
+		ptp_clock_unregister(ptp_stcke_clock);
+		return PTR_ERR(ptp_qpt_clock);
+	}
+	return 0;
+}
+
+static __exit void ptp_s390_exit(void)
+{
+	ptp_clock_unregister(ptp_qpt_clock);
+	ptp_clock_unregister(ptp_stcke_clock);
+}
+
+module_init(ptp_s390_init);
+module_exit(ptp_s390_exit);
+
+MODULE_AUTHOR("Sven Schnelle <svens@linux.ibm.com>");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 3/3] s390/time: Add PtP driver

[kernel test robot]

Hi Sven,

kernel test robot noticed the following build errors:

[auto build test ERROR on s390/features]
[also build test ERROR on linus/master v6.12-rc3 next-20241015]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sven-Schnelle/s390-time-Add-clocksource-id-to-TOD-clock/20241015-185651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
patch link:    https://lore.kernel.org/r/20241015105414.2825635-4-svens%40linux.ibm.com
patch subject: [PATCH 3/3] s390/time: Add PtP driver
config: x86_64-buildonly-randconfig-003-20241016 (https://download.01.org/0day-ci/archive/20241016/202410161414.jC5t2eWE-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241016/202410161414.jC5t2eWE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410161414.jC5t2eWE-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/ptp/ptp_s390.c:21:52: warning: declaration of 'union tod_clock' will not be visible outside of this function [-Wvisibility]
      21 | static struct timespec64 eitod_to_timespec64(union tod_clock *clk)
         |                                                    ^
>> drivers/ptp/ptp_s390.c:23:26: error: call to undeclared function 'eitod_to_ns'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      23 |         return ns_to_timespec64(eitod_to_ns(clk));
         |                                 ^
>> drivers/ptp/ptp_s390.c:28:26: error: call to undeclared function 'tod_to_ns'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      28 |         return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
         |                                 ^
>> drivers/ptp/ptp_s390.c:28:42: error: use of undeclared identifier 'TOD_UNIX_EPOCH'
      28 |         return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
         |                                                 ^
>> drivers/ptp/ptp_s390.c:34:18: error: variable has incomplete type 'union tod_clock'
      34 |         union tod_clock tod;
         |                         ^
   drivers/ptp/ptp_s390.c:34:8: note: forward declaration of 'union tod_clock'
      34 |         union tod_clock tod;
         |               ^
>> drivers/ptp/ptp_s390.c:36:7: error: call to undeclared function 'stp_enabled'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      36 |         if (!stp_enabled())
         |              ^
>> drivers/ptp/ptp_s390.c:39:2: error: call to undeclared function 'store_tod_clock_ext_cc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      39 |         store_tod_clock_ext_cc(&tod);
         |         ^
>> drivers/ptp/ptp_s390.c:49:2: error: call to undeclared function 'ptff'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      49 |         ptff(&tod, sizeof(tod), PTFF_QPT);
         |         ^
>> drivers/ptp/ptp_s390.c:49:26: error: use of undeclared identifier 'PTFF_QPT'
      49 |         ptff(&tod, sizeof(tod), PTFF_QPT);
         |                                 ^
   drivers/ptp/ptp_s390.c:64:18: error: variable has incomplete type 'union tod_clock'
      64 |         union tod_clock clk;
         |                         ^
   drivers/ptp/ptp_s390.c:64:8: note: forward declaration of 'union tod_clock'
      64 |         union tod_clock clk;
         |               ^
   drivers/ptp/ptp_s390.c:66:2: error: call to undeclared function 'store_tod_clock_ext_cc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      66 |         store_tod_clock_ext_cc(&clk);
         |         ^
   drivers/ptp/ptp_s390.c:67:29: error: call to undeclared function 'tod_to_ns'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      67 |         *device_time = ns_to_ktime(tod_to_ns(clk.tod - TOD_UNIX_EPOCH));
         |                                    ^
   drivers/ptp/ptp_s390.c:67:49: error: use of undeclared identifier 'TOD_UNIX_EPOCH'
      67 |         *device_time = ns_to_ktime(tod_to_ns(clk.tod - TOD_UNIX_EPOCH));
         |                                                        ^
   drivers/ptp/ptp_s390.c:76:7: error: call to undeclared function 'stp_enabled'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      76 |         if (!stp_enabled())
         |              ^
   1 warning and 13 errors generated.


vim +/eitod_to_ns +23 drivers/ptp/ptp_s390.c

    20	
  > 21	static struct timespec64 eitod_to_timespec64(union tod_clock *clk)
    22	{
  > 23		return ns_to_timespec64(eitod_to_ns(clk));
    24	}
    25	
    26	static struct timespec64 tod_to_timespec64(unsigned long tod)
    27	{
  > 28		return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
    29	}
    30	
    31	static int ptp_s390_stcke_gettime(struct ptp_clock_info *ptp,
    32					  struct timespec64 *ts)
    33	{
  > 34		union tod_clock tod;
    35	
  > 36		if (!stp_enabled())
    37			return -EOPNOTSUPP;
    38	
  > 39		store_tod_clock_ext_cc(&tod);
    40		*ts = eitod_to_timespec64(&tod);
    41		return 0;
    42	}
    43	
    44	static int ptp_s390_qpt_gettime(struct ptp_clock_info *ptp,
    45					struct timespec64 *ts)
    46	{
    47		unsigned long tod;
    48	
  > 49		ptff(&tod, sizeof(tod), PTFF_QPT);
    50		*ts = tod_to_timespec64(tod);
    51		return 0;
    52	}
    53	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] s390/time: Add PtP driver

[kernel test robot]

Hi Sven,

kernel test robot noticed the following build errors:

[auto build test ERROR on s390/features]
[also build test ERROR on linus/master v6.12-rc3 next-20241015]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sven-Schnelle/s390-time-Add-clocksource-id-to-TOD-clock/20241015-185651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
patch link:    https://lore.kernel.org/r/20241015105414.2825635-4-svens%40linux.ibm.com
patch subject: [PATCH 3/3] s390/time: Add PtP driver
config: um-x86_64_defconfig (https://download.01.org/0day-ci/archive/20241016/202410161404.V66COAYS-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241016/202410161404.V66COAYS-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410161404.V66COAYS-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/ptp/ptp_s390.c:7:
   In file included from drivers/ptp/ptp_private.h:16:
   In file included from include/linux/ptp_clock_kernel.h:15:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/ptp/ptp_s390.c:7:
   In file included from drivers/ptp/ptp_private.h:16:
   In file included from include/linux/ptp_clock_kernel.h:15:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/ptp/ptp_s390.c:7:
   In file included from drivers/ptp/ptp_private.h:16:
   In file included from include/linux/ptp_clock_kernel.h:15:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   drivers/ptp/ptp_s390.c:21:52: warning: declaration of 'union tod_clock' will not be visible outside of this function [-Wvisibility]
   static struct timespec64 eitod_to_timespec64(union tod_clock *clk)
                                                      ^
>> drivers/ptp/ptp_s390.c:23:26: error: call to undeclared function 'eitod_to_ns'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           return ns_to_timespec64(eitod_to_ns(clk));
                                   ^
>> drivers/ptp/ptp_s390.c:28:26: error: call to undeclared function 'tod_to_ns'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
                                   ^
   drivers/ptp/ptp_s390.c:28:42: error: use of undeclared identifier 'TOD_UNIX_EPOCH'
           return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
                                                   ^
   drivers/ptp/ptp_s390.c:34:18: error: variable has incomplete type 'union tod_clock'
           union tod_clock tod;
                           ^
   drivers/ptp/ptp_s390.c:34:8: note: forward declaration of 'union tod_clock'
           union tod_clock tod;
                 ^
>> drivers/ptp/ptp_s390.c:36:7: error: call to undeclared function 'stp_enabled'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           if (!stp_enabled())
                ^
   drivers/ptp/ptp_s390.c:36:7: note: did you mean 'cpu_enabled'?
   include/linux/cpumask.h:1163:29: note: 'cpu_enabled' declared here
   static __always_inline bool cpu_enabled(unsigned int cpu)
                               ^
>> drivers/ptp/ptp_s390.c:39:2: error: call to undeclared function 'store_tod_clock_ext_cc'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           store_tod_clock_ext_cc(&tod);
           ^
>> drivers/ptp/ptp_s390.c:49:2: error: call to undeclared function 'ptff'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           ptff(&tod, sizeof(tod), PTFF_QPT);
           ^
   drivers/ptp/ptp_s390.c:49:26: error: use of undeclared identifier 'PTFF_QPT'
           ptff(&tod, sizeof(tod), PTFF_QPT);
                                   ^
   drivers/ptp/ptp_s390.c:64:18: error: variable has incomplete type 'union tod_clock'
           union tod_clock clk;
                           ^
   drivers/ptp/ptp_s390.c:64:8: note: forward declaration of 'union tod_clock'
           union tod_clock clk;
                 ^
   drivers/ptp/ptp_s390.c:66:2: error: call to undeclared function 'store_tod_clock_ext_cc'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           store_tod_clock_ext_cc(&clk);
           ^
   drivers/ptp/ptp_s390.c:67:29: error: call to undeclared function 'tod_to_ns'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           *device_time = ns_to_ktime(tod_to_ns(clk.tod - TOD_UNIX_EPOCH));
                                      ^
   drivers/ptp/ptp_s390.c:67:49: error: use of undeclared identifier 'TOD_UNIX_EPOCH'
           *device_time = ns_to_ktime(tod_to_ns(clk.tod - TOD_UNIX_EPOCH));
                                                          ^
   drivers/ptp/ptp_s390.c:76:7: error: call to undeclared function 'stp_enabled'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           if (!stp_enabled())
                ^
   13 warnings and 13 errors generated.


vim +/eitod_to_ns +23 drivers/ptp/ptp_s390.c

    20	
    21	static struct timespec64 eitod_to_timespec64(union tod_clock *clk)
    22	{
  > 23		return ns_to_timespec64(eitod_to_ns(clk));
    24	}
    25	
    26	static struct timespec64 tod_to_timespec64(unsigned long tod)
    27	{
  > 28		return ns_to_timespec64(tod_to_ns(tod - TOD_UNIX_EPOCH));
    29	}
    30	
    31	static int ptp_s390_stcke_gettime(struct ptp_clock_info *ptp,
    32					  struct timespec64 *ts)
    33	{
    34		union tod_clock tod;
    35	
  > 36		if (!stp_enabled())
    37			return -EOPNOTSUPP;
    38	
  > 39		store_tod_clock_ext_cc(&tod);
    40		*ts = eitod_to_timespec64(&tod);
    41		return 0;
    42	}
    43	
    44	static int ptp_s390_qpt_gettime(struct ptp_clock_info *ptp,
    45					struct timespec64 *ts)
    46	{
    47		unsigned long tod;
    48	
  > 49		ptff(&tod, sizeof(tod), PTFF_QPT);
    50		*ts = tod_to_timespec64(tod);
    51		return 0;
    52	}
    53	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] s390/time: Add PtP driver

[Heiko Carstens]

On Tue, Oct 15, 2024 at 12:54:14PM +0200, Sven Schnelle wrote:
> Add a small PtP driver which allows user space to get
> the values of the physical and tod clock. This allows
> programs like chrony to use STP as clock source and
> steer the kernel clock. The physical clock can be used
> as a debugging aid to get the clock without any additional
> offsets like STP steering or LPAR offset.
> 
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>

...

> +static __always_inline unsigned long eitod_to_ns(union tod_clock *clk)
> +{
> +	clk->eitod -= TOD_UNIX_EPOCH;
> +	return ((clk->eitod >> 9) * 125) + (((clk->eitod & 0x1ff) * 125) >> 9);
> +}

This is quite odd ;). This helper modifies the input union, which may
be very surprising to callers. It subtracts TOD_UNIX_EPOCH, which may
also be surprising, especially since we don't do that for tod_to_ns().

In addition the input value contains 72 bits, while the output value
is truncated to the lower 64 bits.

From my point of view this should look like

static __always_inline u128 eitod_to_ns(u128 todval)
{
	return (todval * 125) >> 9;
}

This way there are no surpring semantics (at least to me), and this
is more or less similar to tod_to_ns(). Since there won't be any
overflow with the multiplication it is also possible to simplify the
calculation.

Then it is up to the caller to subtract TOD_UNIX_EPOCH from the input
value before calling this helper, and the caller can also do
truncation in whatever way wanted.

> +bool stp_enabled(void);
>  #endif

Besides that there is an empty line missing before the endif: why is
this in timex.h and not in stp.h where it naturally would belong to?

> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 4214901c3ab0..47b20235953c 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -469,6 +469,13 @@ static void __init stp_reset(void)
>  	}
>  }
>  
> +bool stp_enabled(void)
> +{
> +	return test_bit(CLOCK_SYNC_HAS_STP, &clock_sync_flags) &&
> +		stp_online;
> +}

Make this one long line, please.

> +config PTP_S390
> +	tristate "S390 PTP driver"
> +	depends on PTP_1588_CLOCK
> +	default y

Why default y?

> +static int ptp_s390_stcke_gettime(struct ptp_clock_info *ptp,
> +				  struct timespec64 *ts)
> +{
> +	union tod_clock tod;
> +
> +	if (!stp_enabled())
> +		return -EOPNOTSUPP;
> +
> +	store_tod_clock_ext_cc(&tod);

Why store_tod_clock_ext_cc()? This doesn't check the condition code,
but generates dead instructions (ipm+srl). store_tod_clock_ext() is
probably what should be used here?

> +static int s390_arch_ptp_get_crosststamp(ktime_t *device_time,
> +					 struct system_counterval_t *system_counter,
> +					 void *ctx)
> +{
> +	union tod_clock clk;
> +
> +	store_tod_clock_ext_cc(&clk);

Same here.

> +static struct ptp_clock_info ptp_s390_stcke_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IBM Z STCKE Clock",

Please use "s390..." instead of "IBM Z...", which is the architecture
name within the kernel.

> +static struct ptp_clock_info ptp_s390_qpt_info = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IBM Z Physical Clock",

Same.

[PATCH 0/3] PtP driver for s390 clocks

[Sven Schnelle]

Hi,

these patches add support for using the s390 physical and TOD clock as ptp
clock. To do so, the first patch adds a clock id to the s390 TOD clock. The
second patch adds sending a udev event when a ptp device is added, so that
userspace is able to generate stable device names for virtual ptp devices.
The last patch adds the PtP driver itself.

Sven Schnelle (3):
  s390/time: Add clocksource id to TOD clock
  ptp: Add clock name to uevent
  s390/time: Add PtP driver

 MAINTAINERS                     |   6 ++
 arch/s390/include/asm/timex.h   |   8 ++
 arch/s390/kernel/time.c         |   8 ++
 drivers/ptp/Kconfig             |  11 +++
 drivers/ptp/Makefile            |   1 +
 drivers/ptp/ptp_clock.c         |  11 ++-
 drivers/ptp/ptp_s390.c          | 127 ++++++++++++++++++++++++++++++++
 include/linux/clocksource_ids.h |   1 +
 8 files changed, 172 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ptp/ptp_s390.c

-- 
2.43.0

[PATCH 2/3] ptp: Add clock name to uevent

[Sven Schnelle]

To allow users to have stable device names with the help of udev,
add the name to the udev event that is sent when a new PtP clock
is available. The key is called 'PTP_CLOCK_NAME'.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 drivers/ptp/ptp_clock.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index c56cd0f63909..15937acb79c6 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -25,9 +25,11 @@
 #define PTP_PPS_EVENT PPS_CAPTUREASSERT
 #define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
 
+static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);
 const struct class ptp_class = {
 	.name = "ptp",
-	.dev_groups = ptp_groups
+	.dev_groups = ptp_groups,
+	.dev_uevent = ptp_udev_uevent
 };
 
 /* private globals */
@@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
 
 /* module operations */
 
+static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
+
+	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);
+}
+
 static void __exit ptp_exit(void)
 {
 	class_unregister(&ptp_class);
-- 
2.43.0

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Andrew Lunn]

>  
> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env);

No forward declarations please. Put the code in the correct order.

>  const struct class ptp_class = {
>  	.name = "ptp",
> -	.dev_groups = ptp_groups
> +	.dev_groups = ptp_groups,
> +	.dev_uevent = ptp_udev_uevent
>  };
>  
>  /* private globals */
> @@ -514,6 +516,13 @@ EXPORT_SYMBOL(ptp_cancel_worker_sync);
>  
>  /* module operations */
>  
> +static int ptp_udev_uevent(const struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev);
> +
> +	return add_uevent_var(env, "PTP_CLOCK_NAME=%s", ptp->info->name);
> +}

https://elixir.bootlin.com/linux/v6.11.3/source/include/linux/ptp_clock_kernel.h#L60

 * @name:      A short "friendly name" to identify the clock and to
 *             help distinguish PHY based devices from MAC based ones.
 *             The string is not meant to be a unique id.

If the name is not unique, you probably should not be using it for
udev naming.

	Andrew

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Richard Cochran]

On Tue, Oct 15, 2024 at 02:43:28PM +0200, Andrew Lunn wrote:
>  * @name:      A short "friendly name" to identify the clock and to
>  *             help distinguish PHY based devices from MAC based ones.
>  *             The string is not meant to be a unique id.
> 
> If the name is not unique, you probably should not be using it for
> udev naming.

+1

Maybe the name is unique for s390, but it will not be in general.

Thanks,
Richard

Re: [PATCH 2/3] ptp: Add clock name to uevent

[Sven Schnelle]

Richard Cochran <richardcochran@gmail.com> writes:

> On Tue, Oct 15, 2024 at 02:43:28PM +0200, Andrew Lunn wrote:
>>  * @name:      A short "friendly name" to identify the clock and to
>>  *             help distinguish PHY based devices from MAC based ones.
>>  *             The string is not meant to be a unique id.
>> 
>> If the name is not unique, you probably should not be using it for
>> udev naming.
>
> +1
>
> Maybe the name is unique for s390, but it will not be in general.

As already written to Greg, i will drop this Patch. The name is unique,
i was just not aware that the clock_name attribute is present in sysfs.